[Overhauled] Improve scrubber efficiency calculation - #866
Open
Meldexun wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR reimplements how scrubbers calculate their efficiency to fix issues, improve performance, and reduce unnecessary complexity.
Memory Leak: We noticed a memory leak in RLCraft Dregora 1.1.2b which uses NuclearCraft 2.19a. This was the initial motivation to look into the whole thing.
The problem is that
RadiationEnvironmentHandler.removeTileis only called inTileEntity.invalidatewhich is only called when the tile entity gets removed directly (for example by breaking the block) but not when the chunk is unloaded and also not when the world is unloaded. This causes server world instances to be strongly referenced byRadiationEnvironmentHandler.ENVIRONMENT_MAPforever.Looking into the
RadiationEnvironmentHandlerimplementation I noticed that it's very convoluted. From my understanding the only goal of theRadiationEnvironmentHandleris to efficiently compute theMap<BlockPos, Integer>occlusion maps of the scrubbers.I simplified the whole thing by just using a
Object2IntMap<BlockPos>for every dimension stored in aWeakHashMap<World, ...>.On load a scrubber increments the counter at every position that it affects. When a scrubber is unloaded (
onChunkUnload) or removed (invalidate) the counters are decreased again and when reaching zero the entries are removed.These per-world occlusion maps can then simply be used directly in
RadiationEnvironmentHandler.checkRadiationEnvironmentInfo.This prevents memory leaks because it never holds a strong reference to any world object.
I expect performance to be similar to the old implementation because
Object2IntOpenHashMapcheckRadiationEnvironmentInfonow always checks every affected position, while previously non-occlusive positions where not processed until they where re-added to the occlusion map bycheckRadiationEnvironmentInfo