Eliminate leak of non-concrete subclass references #8591
Merged
+159
−148
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
|
Performance comparison with 9.4.10.0, which leaked but had a more efficient data structure for traversal (needed for 9.4.10.0: New impl: Notice that performance is not as fast as the (broken) linked list implementation, but compare to CRuby and JRuby 9.8.9.0 (before any of these changes): CRuby 3.4 JRuby is much faster than CRuby for JRuby 9.4.8.0 The new impl is again far faster than 9.4.8.0 for all singleton-heavy |
My implementation of a new subclasses structure in jruby#8462 was a bit too "clever" leading to a potential leak of subclass references. To avoid using a ReferenceQueue, I tied the "cleaning" of the subclasses list to encountering a percentage of empty subclass references during traversal. However if traversal only walks concrete subclass references (via Class#subclasses), empty non-concrete (singleton, included, prepended) subclass references could accumulate. This would only be cleared out with a full (not concrete-only) subclass walk, which only happened for a few internal uses of this list. The patch here reverts to the Map-based approach previously used with two changes: * Instead of a fully-concurrent or synchronized WeakHashMap, we use read/write locking to favor mostly-read operations like Class#subclasses and hierarchy-walking. * Concrete subclasses are tracked separately in a second map, to allow fast performance of the user-facing Class#subclasses. Both map fields are lazily initialized: * If the operation is for read and the field is null, they are initialized to Collections.EMPTY_MAP. * If the operation is for write and the field is null or Collections.EMPTY_MAP, they are initialized to a new read/write locking WeakHashMap.
headius
force-pushed
the
subclasses_leak
branch
from
ee6ce75 to
9d2f6bd
Compare
This commit introduces specialized collection types adapted for subclass traversal: * SubclassList and SubclassArray, extending ArrayList and RubyArray respectively but also implementing BiConsumer, so they can be passed as the lambda for traversal. * InvalidatorList, extending ArrayList and implementing BiConsumer and Consumer so it can be passed in place of the lambda versions. This reduces allocation for Class#subclasses to just the eventual Ruby Array and does similar for the internal (and rarely-used) RubyClass#subclasses(boolean) logic. Allocation for invalidator gathering is reduced to the invalidator list. All cases now avoid multiple levels of lambda allocation due to recursion and use of forEach internal iteration.
This makes a few changes to improve concurrent traversal of the subclasses collections. * Replace ReentrantReadWriteLock with StampedLock. * Avoid CAS operations once the fields have been fully initialized.
|
With all changes up to this point, here's the new numbers. We are consistently faster than CRuby on all benchmarks. Performance is comparable to the old ConcurrentWeakHashMap-based implementation but is slower to traverse under contention. Performance is slower than the (broken) linked list implementation but not by a significant degree. |
|
It is worth pointing out that the implementation at this point also reduces allocation of lambda instances for traversal for all subclass walking, including cache invalidation, method invalidator gathering, and the usual subclass list aggregation operations. This will improve performance of several core runtime operations. |
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.
My implementation of a new subclasses structure in #8462
was a bit too "clever" leading to a potential leak of subclass
references. To avoid using a ReferenceQueue, I tied the "cleaning"
of the subclasses list to encountering a percentage of empty
subclass references during traversal. However if traversal only
walks concrete subclass references (via Class#subclasses),
empty non-concrete (singleton, included, prepended) subclass
references could accumulate. This would only be cleared out with
a full (not concrete-only) subclass walk, which only happened for
a few internal uses of this list.
The patch here reverts to the Map-based approach previously
used with two changes:
use read/write locking to favor mostly-read operations like
Class#subclasses and hierarchy-walking.
allow fast performance of the user-facing Class#subclasses.
Both map fields are lazily initialized:
initialized to Collections.EMPTY_MAP.
Collections.EMPTY_MAP, they are initialized to a new
read/write locking WeakHashMap.