Don't create a new config for each slime world#122
Don't create a new config for each slime world#122jtJava wants to merge 6 commits intoInfernalSuite:developfrom
Conversation
RealBauHD
left a comment
There was a problem hiding this comment.
A better name on how the speed up was made like "reuse config" etc. would be nice
| + Iterable<ServerLevel> levels = net.minecraft.server.dedicated.DedicatedServer.getServer().getAllLevels(); | ||
| + Iterator<ServerLevel> levelIterator = levels.iterator(); | ||
| + if (levelIterator.hasNext()) { | ||
| + return levelIterator.next().paperConfig(); |
There was a problem hiding this comment.
I am not a fan of this solution, I think instead of reusing a random config from another world, we should cache a brand new with the default values or establish a new file with default values for slime worlds.
There was a problem hiding this comment.
You're right, I'm not sure where to cache the config files, or if they could even be generated outside of the constructor.
|
Adding to the comments above, this behavior should be configurable using SlimeProperties; I can imagine use-cases where this is inappropriate. Just a thought: couldn't we cache a "preset" config and clone it each time (so that you can still edit the config per-world), or is deep cloning of the configs not viable? |
…an prevent SpigotConfig from being saved in the first place.
I added a new SlimeProperty, I agree with you that a config should be cached, and ideally grabbed whenever a world is cloned. Not sure where to start on this part though, but I did move the patch to a slightly better location. I'm not familiar with patches, so unsure how to rename the patch without breaking something. |
kyngs
left a comment
There was a problem hiding this comment.
I'm sorry, but this is kinda a mess. ASP is full of scuffed hacks, but this is possibly too much. I guess I can implement it "properly" if you won't.
| SlimeLevelInstance server = createCustomWorld(slimeWorld, dimensionOverride); | ||
| + | ||
| + Optional<StringTag> tag = slimeWorld.getExtraData().getAsStringTag("clonedFrom"); | ||
| + if (tag.isEmpty()) { |
There was a problem hiding this comment.
Why apply the caching only for non-cloned worlds? I think we should do this for cloned worlds too. What's the reasoning behind this?
| + Optional<StringTag> tag = slimeWorld.getExtraData().getAsStringTag("clonedFrom"); | ||
| + if (tag.isEmpty()) { | ||
| + AdvancedSlimePaper.instance().getCachedPaperConfigs().put(slimeWorld.getName(), server.paperConfig()); | ||
| + AdvancedSlimePaper.instance().getCachedSpigotConfigs().put(slimeWorld.getName(), server.spigotConfig); |
There was a problem hiding this comment.
I'm afraid this actually doesn't clone the configs -> if one were to edit the config for a single world, it would get applied to all.
| + } else { | ||
| + this.spigotConfig = new org.spigotmc.SpigotWorldConfig(((net.minecraft.world.level.storage.PrimaryLevelData) worlddatamutable).getLevelName()); // Spigot | ||
| + this.paperConfig = paperWorldConfigCreator.apply(this.spigotConfig); // Paper - create paper world config | ||
| + } |
There was a problem hiding this comment.
The cloned worlds should have a separate independent (cloned) config from the original ones. This also doesn't solve the original issue -> we're still generating a new config for each (new) slime world.
I’m not actually done yet, give me another week or two and if it’s not up to par then you can properly do it on your own timeframe |
7d00f4e to
167c327
Compare
The package name has been changed by ComputerNerd The NBT library has been changed by ComputerNerd Updated to the newest paper and updated all patches Unused events and exceptions have been removed from the API Loaders have been separated into different modules. The loader package continues to exist as a BOM package of the default loaders included in the plugin. Delete the temporary storage access temp directory after the world unloads Don't write/read dimension data (raids, random_sequences, ...) to/from disk Don't write region files in specific scenarios to disk Utilize access transformers instead of patches Use a custom world config for all slime worlds, and don't read/write to disk on world load. Fixes Performance improvement for loading worlds #110 and replaces Don't create a new config for each slime world #122 Fix issue SlimeWorld read-only status always true when importing vanilla world #152 Return SlimeWorldInstance in API to make it easier to get the bukkit world after loading a world Delete NMSSlimeWorld, as it was unnecessary and had wierd behaviour (explained below) Fix a chunk pruner issue introduced by ComputerNerd in his initial 1.21.4 work Replace entity loading code with the vanilla PostLoadProcessor, so this burden is no longer on us Generate World UUIDs randomly instead of trying to read a file that's 100% nonexistent Properly log if there are exceptions in async save (that are not IO related) Fix a SlimeProperty issue that ComputerNerd introduced in his initial 1.21.4 work Fix biome slime property Fix poi warnings Don't place chunks in the chunkstorage prematurely
|
This has been implemented differently in the newest 1.21.4 builds from the development branch |
Meant to fix #110
Testing was done with a small 100x100x64 world, cloning done on main thread but pretty sure it can be offloaded.
Results: 2177 ms -> 1294 ms
Before:
https://spark.lucko.me/46jHRGoWHt
https://mclo.gs/eZfDIFC
After:
https://spark.lucko.me/DspcQn4jQL
https://mclo.gs/eDOs3Jc
Testing code:
