Repository navigation
fix: EventNodeImpl handle cache preventing event nodes from being garbage collected - #208
Open
DragonL0508 wants to merge 1 commit into
Open
DragonL0508 wants to merge 1 commit into
DragonL0508 wants to merge 1 commit into
Conversation
…bage collected EventNodeImpl caches its handles in a ClassValue. ClassValue stores each value inside the Class object it is computed for (a WeakHashMap keyed by the ClassValue's identity), and the value strongly references the key here: Handle is an inner class holding its node, and the node holds the ClassValue. Such an entry is never cleared, and loaded event classes are GC roots, so once a handle has been computed for a node, that node and everything its listeners capture stay in memory forever. A parent asks every child for a handle whenever it builds a consumer, so this happens to practically every node. Fairy's GUI creates a node for every opened Gui (and one per slot), so every opened menu leaked together with its slots and items. On a production server this accumulated to around three million StaticGuiSlot instances, 8-10 GB of a 16 GB heap, and Full GC pauses of up to 5 seconds. Removing the node from its parent (Gui#close) does not help, since the entries live in the event classes. Use a ConcurrentHashMap owned by the node instead, so the cache is collected together with it. invalidateEvent now skips types that never had a handle requested, which a ClassValue used to create on demand. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
EventNodeImplcaches its handles in aClassValue. AClassValuestores each value inside theClassit is computed for (aWeakHashMapkeyed by theClassValue's identity), and here the value strongly references the key:Handleis an inner class holding its node, and the node holds theClassValue. Such an entry is never cleared, and loaded event classes are GC roots, so once a handle has been computed for a node, that node and everything its listeners capture stay in memory forever. A parent asks every child for a handle whenever it builds a consumer, so this happens to practically every node.The GUI module creates a node for every opened
Gui(and one per slot), so every opened menu leaked together with its slots and items. On our production server this grew to about three millionStaticGuiSlotinstances, 8-10 GB of a 16 GB heap, with Full GC pauses of up to 5 seconds.Gui#closeremoving the node from its parent doesn't help, since the entries live in the event classes.The cache is now a
ConcurrentHashMapowned by the node, so it is collected together with it.invalidateEventskips types that never had a handle requested (aClassValueused to create one on demand). We have been running this change as a patched class in production.Tests:
EventNodeGarbageCollectionTestattaches a child with a listener, dispatches through the parent, removes the child and checks that it can be collected (retryingSystem.gc()up to 50 times). It fails before this change. Since it relies on the GC, feel free to@Disabledit if you'd rather not have it in CI, likecheckIfPrototypeCanBeGarbageCollected.🤖 Generated with Claude Code