Move data files into their own subdirectory & implement backwards compatibility
Speiger opened this issue ยท 11 comments
I am running Lootr (lootr-1.16.5-0.1.11.36) on our server and it looks like that it creates 1 file per loot chest that had been opened.
This is kinda excessive, to a point where you spam files.
We are already at +250 files created by lootr itself.
my suggestion would be that you merge them all into 1 file.
While this might result into a memory leak, it is at least a traceable thing.
But having +250 files is excessive.
My comment regarding this being the way it has worked was in reference to your initial concern about the number of files. You didn't mention anything about the current system having a memory leak until just now, so I was taking in the aesthetic sense.
I don't have an IDE available at the minute to confirm but I'll see if I can verify that the current system causes memory leaks.
That said, I'm unsure about switching to other systems mentioned without looking into them further and examining the implications for the fabric port.
My comment regarding this being the way it has worked was in reference to your initial concern about the number of files. You didn't mention anything about the current system having a memory leak until just now, so I was taking in the aesthetic sense.
Yeah it is only memory leak if you dont have frequent restarts because WorldSavedData is NEVER unloaded once it is loaded, until the world is fully unloaded. Which in servers its a full restart.
Have enough explorers and you have a lot of single loaded files that take even more ram then 1 file would. (Hashmaps are mean like that)
I don't have an IDE available at the minute to confirm but I'll see if I can verify that the current system causes memory leaks.
This is hard to verify unless you want to verify the non unloading of loaded WorldSavedData instances.
That said, I'm unsure about switching to other systems mentioned without looking into them further and examining the implications for the fabric port.
I am not sure but I think fabric should have chunkload/unload/save events. (looking up)
Nope it doesn't have the data events that forge has on its own.
Though I doubt it is hard to replicate the code in fabric with mixins.
Gotta say "love" fabric...
Used memory is not leaked memory.
I'm happy to look into moving them into subdirectories.
Used memory is not leaked memory.
Useless memory that is not used at all is leaked memory.
If the chunks are unloaded after a player leaves then the data about the chests should also unload.
To give you a example:
Structures were stored globally until 1.14 guess what after a 1-2k radius generation (chunk radius) happened.
You had 5GB of Mineshafts in ram loaded all the time even so you didn't need them.
In 1.14 and newer PointsOfInterests are loaded always even if they are not needed (same way like world saved data)
And you can only generate your world only so far before the game crashes because it runs out of memory.
"Used Memory is not leaked Memory" is correct to some degree but "used" is only valid as it is being used.
If it sits around idle doing nothing while everything else around it unloads makes it a invalid statement or a memory leak.
I'm happy to look into moving them into subdirectories.
Thats a start but in the long term it would be a good idea to implement a proper system that is capable of unloading unnecessary data.
So these can be pushed into a subdirectory by overriding the WorldSavedData::save
method:
@Override
public void save (File file) {
file.getParentFile().mkdirs();
super.save(file);
}
This would still require some piece of intermediate code to support backwards compatibility, which I'm not able to sit down and do at this current point in time.
(edit): However, given my attempts to stop letting anxiety and stress control my life, I realize there isn't anything that actually stops me from doing this for 1.16.5. A super-simple method would be to just move any Lootr-related files from the data
folder into the relevant subfolder... so many options that I haven't considered!
As you said, merging them could create a memory leak and is more likely to result in a file that is susceptible to corruption.
This is the way the mod has functioned for multiple years without only one previous complaint. I don't currently have the time or energy to put into restructuring the mod to make this happen (edit): due to a lot of real-life stress that I'm currently undergoing, as it would require adjustment to retain backwards compatibility with previous versions.
(edit 2): Apologies if this reply was brusque, I've been suffering from a lot of break-through anxiety recently and my default responses aren't particularly good right now. I think you have a valid point, although I'm not comfortable merging them all into the same file due to past experiences, primarily with HQM.
I'm definitely happy with the idea of doing a subdirectory though. Whether I get that done depends on mental energy, etc.
No worry about the answer, everyone has stress.
About the memory leak: A subfolder might help, but it doesn't solve the memory leak because if each chest has their own file that means open enough chests during a running server and you have the same memory leak.
Honestly there is 3-4 ways you could do it in a sane matter, where it is manageable by the server owner.
- Store all in 1 file: If each data entry is large enough this will result in a memory leak. But honestly if you expect 100k entries and you are less then 100MB of ram usage (by mathing out everything) you are fine. Which is 1KB of data.
- Store it as per player: That allows you to basically have player specfic files and only when players use some chest data gets loaded, in some cases this might work in others with loads of players also not a good idea, but likely only like 50 files at worst.
- Store it in the player: Honestly playerdata is a thing and you could store it in there. Though it would be always loaded so that has to be kept in mind. Basically first method but only if players are logged in the data is loaded for that specfic player that means the data can also unload unlike the others.
- Store it in the chunk: As long as you compress your data (I can give you my inventory compression code that can turn a double chest of dirt from 3KB to 200 bytes) you could technically store it in the chunks themselves. And it loads/unloads as you need it. So you actually have like data being dynamically loaded/unloaded and there is ways to load/unload data from chunks. And you don't even have to store it directly in the chunk, which means no compression required, you can just use the events from the chunks to do such a thing.
Also if you want backwards compat you could just implement a command that converts the old files to the new system.
If you figure out something.
I hope that helps you.
Edit: I would highly suggest you use the "Chunk" method even if you store it in a folder where a lot of files are especially since that is technically the best solution. And it can be actually migrated on world load if you only store it indirectly in chunks. No user input required.
This is the way the mod has functioned for multiple years without only one previous complaint.
I wanted to answer this separately.
First of all please don't take offense to this answer to this specific quote.
It is meant to show you how dangerous that answer can be.
That answer was the worst one you could have picked. If something worked for years without "issue" doesn't mean it isn't a issue.
It was just minor enough that nobody complained about it or checked into the data folder.
The data folder is pretty hidden and usually not used by users a lot unless they know what they do.
So it is pretty much hidden.
And if we go by "it always worked that way and it was fine nobody had issues", then i would like you to look at any "security risk" article that has came out this year.
There are plenty out there.
Yes your problem isn't a security risk but I just wanted to point out how dangerous that mentality can be.
So next time please don't use such a answer unless it is a really weird bug that clearly has some fishy going on.
Anyhow I hope life gets better for you, i know from first hand that stress can cause bad reactions from people.
This is done for Lootr for 1.18 and later. I'll be finalizing the code for 1.16.5 once I get some more feedback on this change.