Astral sorcery loads all registered worlds on server startup; if no celestial gateways present worlds don't unload
zmaster587 opened this issue ยท 32 comments
It's come to my attention that when both Astral sorcery and Advanced Rocketry (or any other dimension adding mod) that all the worlds are loaded on startup when Astral Sorcery checks for the existance of celestial gateways. The issue is that even if the worlds are registered with forge such that they are not forcibly loaded, they still do not unload when there are no celestial gateways present in a dimension.
Additionally, if a player then visits that dimension and then leaves it, the dimension unloads as it should.
What I suspect is happening is that Astral Sorcery is loading all registered (even if not loaded) worlds on startup. Forge does not unload a dimension until the last chunk loaded is popped out of a queue to unload (in ChunkProviderServer.tick(), called from WorldServer.tick()). So if a world is loaded and zero chunk are loaded, the world will not unload. Using just enough dimensions in my test instance, I had verified that the worlds are loaded, but contain zero loaded chunks.
I'm not sure if it will affect the functionality of the gateways at all since the configs are already loaded in the onWorldLoad event, but I suggest possibly using getWorlds() instead of getStaticDimensionIDs() here
I have also created a custom build where the onServerStart method here
is commented out. When testing that build, only the dimensions in use by players/chunkloaders are loaded.
If the world state is loaded, but nothing actually IS, that's not remotely bad in my eyes.
And if you've got the capability for 2000 worlds loaded, wtf idiotic bullshit pack design are you using?
Admittedly I'm not super familar with the gateways, but I'm going to assume they function something like portals to travel between worlds and require a base station of some kind on both ends. So if I'm wrong there, I apologize.
Another solution would also just be to make sure one chunk is loaded in each dimension checked on load. Then there will only be a CPU usage/memory spike at the beginning rather than continuing through all gameplay. The easiest way to do that is if there are 0 cached coordinates, then call a block getter or tile getter for (0,0,0) to get a chunk to load, then of couse since nothing is keeping the chunk loaded, forge will unload it, then the world. If you don't want to change any logic and are worried about things breaking, this is probably the easiest working solution, even if not ideal.
A much more complex, but probably more elegant solution is to store the locations of the gateways totally seperately from the region data (though perhaps preferably in a seperate file in each dim directory). You can save them on world unload and load them on server start without needing to touch the worlds at all (make sure to save to a temp file first, so if anything goes wrong during serialization the data can be recovered). This may only cause a problem if somebody uses an external tool like MCEdit. But to be fair, if somebody is using MCedit, something else has likely gone terribly wrong already. If you want to protect against that, then just before teleporting the player to the other world, run a second check to make sure all is good, if not, then throw an error and abort teleportation and update the cache accordingly.
@LemADEC
While it's not a massive overhead issue since no chunks are loaded, there is still some performance overhead because the tick functions still are called on those worlds, as well as other processing. I've managed servers with fairly large packs myself and some of these packs people love to play have a ton of overhead as it is. While admittedly having a ton of worlds loaded is not as bad as some of the other stuff that happens in these packs. People still panic when they see 200 worlds loaded, that's how the issue came to my attention in the first place. So in addition to performance, there's also a PR component.
store the locations of the gateways totally seperately from the region data (though perhaps preferably in a seperate file in each dim directory)
i actually do that. however, to correctly get the dimension folder, you need the dim to be loaded... i could somewhat do around that but idk how stable that would end up being. So "external" saving is already a thing.
Idk... doing a check while teleporting seems odd... uh. huh. i'll see what i end up doing
In regards to the many servers thing: Having played GT:NH multiple times now, that pack had a ton of different worlds loaded at all times in 1.7.10 and none of the unloaded ones even reached .1ms per tick. so. there goes that, even back in 1.7.10
Ah, alright, I see the external saving as a thing now. I forgot getProvider also loads the world. You could have a central db with a known path and each world is a key to an NBT object. Then you'd not need the path to the world.
Doing a check while teleporting would not only ensure the reciving gateway exists anyway. But will also prevent players from getting stuck in the case of an external edit, a rollback, or a save failure on minecraft's part. It is redundant, but it does catch the remaining issues afaik.
1.8+ is unfortunately more CPU heavy than 1.7.10 due to some modifications Mojang did. Particularly a lot of extra class instantiation. Like I said it wasn't a huge problem. But on the hardware we used even on 1.7.10, I have found each world sitting closer to 1ms to 3ms per tick, though at least one chunk has been loaded in those instances. To achieve 20tps you need to keep the server running less than 50ms/tick. Even the 100us you provided is significant if you have a lot of worlds. 100 worlds = 10ms = 1/5 of available processing power even with the seemly better hardware you're working with.
Anyway, I have also confirmed with a custom build that adding the line "world.getBlockState(new BlockPos(0, 0, 0));" underneath
resolves the issue without any further modification neccessary.So if a world is loaded and zero chunk are loaded, the world will not unload.
that's a new one. didn't expect that :|
Imean. the ultimate problem with all of this is that i would like to prevent users teleporting into "nowhere" with no chance of getting back or straight into a wall or something like that....
i could just leave the check out. but then problems might arise that gateways are cached somewhere where they are not and thus creates some desync.
You want to verify the target location before sending a player there, as there's always something that will change the area without your mod detecting it. Checking at boot won't solve that. You need to actually check just before sending the player, delaying teleportation by a few ticks so the area can be loaded for you to check it.
there's always something that will change the area without your mod detecting it
i wouldn't be so sure about that. unless it's something like MCEdit, something that modifies it needs to at least load the chunk and physically change something - thus once the gateway gets replaced with something else, the logic in the chunk fires 1 method on the block getting replaced and there i remove the position from the gateway cache. So it would capture that.
You need to actually check just before sending the player
at that point it's already too late. Gameplay would suffer massively from that. What do you want me to do? Load the world & chunk once the player just looks at the corresponding "golden star" ? oh boy. what a way to singlehandedly ruin bigger servers.
OR alternatively i can just not check and "hope everything's all right anyway" - which it isn't but that would be one solution x)
Anyway, i'll wait for zmaster's response and then see how i fare with this.
You would be surprised what a server crash or other mods/plugins can do to your blocks. I'm just saying don't assume your event will always be triggered on adding (rolling back the map) or removing (dimension regeneration).
I meant you can cancel the teleportation when the player actually ask for it. At that point, you remove the entry from your database.
Since it's relative improbable for the target to be invalid, I would consider it a minor annoyance to have an 'obsolete' target show to the player. And you only scan when really needed, saving a lot of CPU time.
it is impossible to change a single blockstate in a chunk via code without calling that method. i'm fairly certain that anything that changes it ingame will call this method. It is not an event.
Sure, nobody will:
- directly modify the base data structure
- asm into vanilla/forge code
- modify the block in another thread
- crash the server saving the map or your data
- regenerate the chunk
- delete the dimension, that specific region file or even your data file/NBT
- etc.
directly modify the base data structure
why does one overwrite the chunk class to modify how blockstates are serialized and written into the ExtBlockStorage? There is neither a performance gain nor a "it provides more functionality" since it's literally just a write onto the storage. besides, if one was to do that, the method wouldn't fire at all anymore, causing wayyy more problems than just on my end. (A Example: a chest would no longer drop its contents when broken, the data would just be lost)
asm into vanilla/forge code
Same as above
modify the block in another thread
The method stays the same if you call it on another thread or not
crash the server saving the map or your data
Saving my WorldCacheData is protected against such problems, so a issue arising while saving wouldn't get the server to crash. If anything crashes concurrently while my data gets serialized: oh well. whatever i can do against that. If you know a way to track what's influencing overall behavior through multiple threads in a performant way, why are you not making thousands of dollars instead of hanging around here? Oh yea, i know..
regenerate the chunk
Removing said chunk directly from the level data is in that case literally what MCEdit does, hence i excluded that above if you would've read my statement correctly.
delete the dimension, that specific region file or even your data file/NBT
if you delete the world, you delete the cache alongside with it since it's in a subfolder of that world's folder. As for region file, see the one above. As for the cache file, i don't care? If someone wants to remove a world's data, they may do so. The gateway will no longer know that position then, at least the user doesn't get teleported into a wall since he can't teleport there to begin with. no issue arises.
Edit: i have seen enough people who are hysterically screaming around "But anything can happen in forge, you don't know anything" - sighs - i don't need another discussion about that.
As for reference: the code that's relevant for this can be found in net.minecraft.world.Chunk#setBlockState:
https://i.imgur.com/WMxhd69.png
I understand you worked hard trying to cover all the cases you could imagine.
The real question isn't whether you can control and cover all those cases.
The question is how probable is it, what's the impact and how do you recover from it.
Same goes in real life: you can't imagine all the possible things that'll be thrown at you, but you'll still have to deal with it.
Better question: does it really matter? Or is this just a goose chase that has no relevance. If no chunks load, is there really even a performance concern to be worried about? Anything else is either going to load chunks, or it won't care if the world is up or not.
The WorldTick event is attached to the world, not the chunks.
Mods will use this tick to do their stuff. For good or bad reasons, it'll take time to do so.
That doesn't change that that level of load on a wholly empty world is disconcerting. Probably looking into what's causing that would also help.
Having all world loaded effectively rapes tps on server which i dont really like.
https://i.imgur.com/wf2Sbjn.png
compared to
https://i.imgur.com/fIqwIAt.png
I got the same Problem. The problem increases with Advanced Rocketry exponentially because this mod creates around 50 dimension (planets, asteroides,usw.). Is there ah way to add a Blacklist for unwandet dimensions to deactivate the gateway nodes? (https://pastebin.com/J0Es6wUR)
[14:33:34] [Server thread/INFO]: Dim -11325 Deep Dark : Mean tick time: 3.256 ms. Mean TPS: 20.000
Dim -9999 ExtraUtils2_Quarry_Dim : Mean tick time: 3.386 ms. Mean TPS: 20.000
Dim -2 space : Mean tick time: 3.294 ms. Mean TPS: 20.000
Dim -1 Nether : Mean tick time: 3.449 ms. Mean TPS: 20.000
Dim 0 overworld : Mean tick time: 73.650 ms. Mean TPS: 13.578
Dim 1 the_end : Mean tick time: 3.524 ms. Mean TPS: 20.000
Dim 2 Storage Cell : Mean tick time: 3.449 ms. Mean TPS: 20.000
Dim 3 planet : Mean tick time: 3.574 ms. Mean TPS: 20.000
Dim 5 rftools_dimension : Mean tick time: 10.183 ms. Mean TPS: 20.000
Dim 7 twilight_forest : Mean tick time: 3.272 ms. Mean TPS: 20.000
Dim 22 planet : Mean tick time: 3.245 ms. Mean TPS: 20.000
Dim 23 planet : Mean tick time: 3.260 ms. Mean TPS: 20.000
Dim 24 planet : Mean tick time: 3.256 ms. Mean TPS: 20.000
Dim 25 planet : Mean tick time: 3.272 ms. Mean TPS: 20.000
Dim 26 planet : Mean tick time: 3.299 ms. Mean TPS: 20.000
Dim 32 planet : Mean tick time: 3.356 ms. Mean TPS: 20.000
Dim 33 underworld : Mean tick time: 3.352 ms. Mean TPS: 20.000
Dim 44 planet : Mean tick time: 3.382 ms. Mean TPS: 20.000
Dim 45 planet : Mean tick time: 3.522 ms. Mean TPS: 20.000
Dim 46 planet : Mean tick time: 4.479 ms. Mean TPS: 20.000
Dim 50 The Abyssal Wasteland : Mean tick time: 3.460 ms. Mean TPS: 20.000
Dim 53 The Dark Realm : Mean tick time: 3.260 ms. Mean TPS: 20.000
Dim 111 lostcities : Mean tick time: 3.289 ms. Mean TPS: 20.000
Dim 144 CompactMachines : Mean tick time: 3.744 ms. Mean TPS: 20.000
Overall : Mean tick time: 167.358 ms. Mean TPS: 5.975
That is the tps at the time of saving the snapshots
When i used JustEnoughDimensions to unload those worlds the tps went to
Dim -9999 ExtraUtils2_Quarry_Dim : Mean tick time: 3.915 ms. Mean TPS: 20.000
Dim -1 Nether : Mean tick time: 3.707 ms. Mean TPS: 20.000
Dim 0 overworld : Mean tick time: 35.705 ms. Mean TPS: 20.000
Dim 1 the_end : Mean tick time: 3.608 ms. Mean TPS: 20.000
Dim 2 Storage Cell : Mean tick time: 3.606 ms. Mean TPS: 20.000
Dim 5 rftools_dimension : Mean tick time: 6.602 ms. Mean TPS: 20.000
Dim 144 CompactMachines : Mean tick time: 3.995 ms. Mean TPS: 20.000
Overall : Mean tick time: 62.775 ms. Mean TPS: 15.930
Already yelling at Wizardry devs https://i.imgur.com/UUbOw0j.png
Even if Wizardry is causing added tick time on all worlds, empty or no chunks loaded or otherwise, that is still not the "problem" (it is it's own problem, independent of this one, that might have helped reveal/expose this problem)
I agree with @zmaster587 that .1 ms is still .1 ms. Every tiny bit adds up, especially when trying to keep TPS in a reasonable range for a public server for a large pack like ATM3. Even if Wizardry or no other mod was doing anything to those dimensions, the tick time can add up quickly. Adfvanced Rocketry, RFTools both can make dozens of not up to hundreds of dims just on their own, not counting mods that add a handful of their own dims for content.
My point is that empty, 0-chunks-loaded dims staying loaded should not be dismissed or downplayed as moot... Especially if the "solution" is as easy as loading a single chunk after your checks on server start to let forge cull the loaded dims.
I never said that there is no problem on my end. It just so happened to reveal this one as well. I didn't downplay the issue at all anywhere, i was curious what caused 3ms ticktime in a otherwise completely empty world. Turns out, it is relevant to ask that question as well.
Now the solution isn't too amazing, but functional.
A file gets created now that keeps track of dimensions that are or were involved in the gateway system. Upon starting up, before a world would be checked, the filter-file determines if the world needs to be checked at all. If that's not the case, it'll be skipped. If the id is found in the filter, the world will be loaded and checked like before. If no chunks were loaded as result, the world gets unloaded again, following the same checks that are done after unloading a chunk.
For more specifics on that, check the changes to the CelestialGatewaySystem class in the commit. 1f23749
@HellFirePvP are you also removing the dimension from that file if the dimension no longer contains a gateway?
Currently I'm using a plugin made by myself to fix the unloading of all dimensions that I deem non-vital, which so far seems to work fine. Haven't heard anyone about issues with a gateway or so.
@HellFirePvP The problem seems to have come back. It had worked a couple of times but now it refuses to work any more and i have tried multiple worlds after the 1.8.5 update. Even removing all configs files and even the world filters folder didn't help it.
(I'll be be testing a pack with just jei, TF, AR, JEI and report back if it is still occurring with those mods).