Optimize world save logic

[RealSilverMoon]

My GTNH Crucible server has suffered from periodic lag spikes for a long time. Spark shows this may mainly casued by the autosave's flush() call. In default it will run a global waitForFinish() and closes every cached region file every 900tick. This will result in significant pause on server-thread when the world is larger then 5GB. Skipping the flush on autosave is safe since the autosave still writes chunks to disk every 900 tick.
I also notice that the region-file cache size was hardcoded at 256 instead of reading crucible_chunkCacheSize. Region-file eviction is now O(1) and use config's cache size.

[EverNife]

Are you sure REGIONS_BY_FILE is already sorted by latest edition?
You change to, rahter than delete the last one used, delete the first of the list

Is that right?

[RealSilverMoon]

Are you sure REGIONS_BY_FILE is already sorted by latest edition? You change to, rahter than delete the last one used, delete the first of the list

Is that right?

REGIONS_BY_FILE map is an access-ordered LinkedHashMap:
public static final Map<File, RegionFile> REGIONS_BY_FILE = new LinkedHashMap<File, RegionFile>(CrucibleConfigs.configs.crucible_chunkCacheSize, 0.75f, true);
When the accessOrder=true, the iteration order is from LRU to MRU. Every get() or put() of an existing key moves that entry to the tail. So delete the head of the map just means delete the last one used.

[EverNife]

1- I agree, i think the problem about "REGIONS_BY_FILE.keySet().iterator().next()" is a good idea. O(1) is good. And i can't see many other places this map can be acessed.

2- The fact that now CrucibleConfigs.configs.crucible_chunkCacheSize is used for both the

new LinkedHashMap<File, RegionFile>(**initialCapacity**, loadFactor, accessOrder)

and the

REGIONS_BY_FILE.size() >= CrucibleConfigs.configs.crucible_chunkCacheSize

might not be a good idea.

3- the variable p_71267_1_
is meanted to decide if there will be logs on the save, and you overcharged it with the 'only flush when false', why have you decided that?

---

All things could have solved your problem, i don't know each have. Maybe just because you have spoped flusing your saves at auto-saving solved it?

Maybe the O(1) ?

Maybe the fact you reduced the cache to 256 files and now all loops are at maximum that?

[RealSilverMoon]

2- The fact that now CrucibleConfigs.configs.crucible_chunkCacheSize is used for both the

new LinkedHashMap<File, RegionFile>(**initialCapacity**, loadFactor, accessOrder)

and the

REGIONS_BY_FILE.size() >= CrucibleConfigs.configs.crucible_chunkCacheSize

might not be a good idea.

Initiating the map with crucible_chunkCacheSize could skip the resize step, as the ori code did. size() >= chunkCacheSize is the real cache control line, while it was hardcoded to 256. This made chunkCacheSize actually useless before.

3- the variable p_71267_1_ is meanted to decide if there will be logs on the save, and you overcharged it with the 'only flush when false', why have you decided that?

saveAllWorlds(boolean p_71267_1_ ) is called in only two places:
A. saveAllWorlds(true): periodic autosave, and B. saveAllWorlds(false): server shutdown or manual /save-all. So i think p_71267_1_ =true just means this is an autosave, which should skip flush().

All things could have solved your problem

256 is the default size of chunkCacheSize, i did no change to that except removing the hardcode. But tha't true, I'm not sure which one did solve the lag spikes (or maybe both?). I will run more tests later today.

[RealSilverMoon]

OK I'm back. Test world size: 16.67GB; Set autosave in bukkit.yml to 400tick. Recorded tick time on world-save-tick:
O(1) LRU: 4981.45ms
No Flush: 1546ms
Both disable: 5555.64ms
Both enable: 1055.69ms
(Manual use /save-all almost the same in four tests: 880ms-934ms)
Flush() made a great difference to world-save time! O(1) LRU do not as good as i thought. Maybe this is because i am the only player on the server so the chunk_cache is too small to trigger the recycle......

[juanmuscaria]

Thanks for digging into this, the RegionFileCache half is all correct.

The cache size never being read was an incomplete Paper patch implementation.
The lastModified scan was terrible in every way since both it touched filesystem boundry API but also could suffer from clock drift and was unreliable since id depended on the file actually being written to disk. All in all just bad stuff inherited from thermos.

Your version is correct, the map is built with accessOrder=true so the first key really is the LRU one.

Now the flush part.

This was going to be an entire analysis of "this is a bandaid over a symptom", which it still is, but the flush call was introduced by bukkit and/or thermos in specific which is not a thing on vanilla and forge and clearly causes issues, the flag you are gating it behind is silent save however so just nuke the flush call entirely instead of gating behind it, otherwise everything is correct and ready to merge after you remove it instead of leaving behind the silent flag. This is the correct fix until incremental save is backported which that alone would be its own can of worms.

[RealSilverMoon]

Done!