Use ConcurrentWeakHashMap for RubyClass.subclasses#5137
Merged
Conversation
The naively-synchronized WeakHashSet does not guarantee safe iteration in the presence of modification, which triggers a ConcurrentModificationException as seen in #5115. By using a ConcurrentWeakHashMap (designed by Doug Lea et al, see http://www.java2s.com/Code/Java/Collections-Data-Structure/Ahashtablewithemweakkeysemfullconcurrencyofretrievalsandadjustableexpectedconcurrencyforupdates.htm) we can iterate and modify concurrently without errors. However this implementation is based the design of ConcurrentHashMap, which means the in-memory structure will likely be considerably larger than a simple WeakHashMap. I believe it is also based off the pre-Java 8 ConcurrentHashMap, which means any load factor above one thread greatly increases in-memory size, and a load factor of one limits the concurrent use of the collection.
Member
Author
|
It's hardy scientific, but using VisualVM to get a heap dump of the following script: a = Object
10000.times { a = Class.new(a) }
puts 'ok'
sleepShows a bit over a 2MB size difference, or about 200 bytes per class. This may increase with more subclasses in each collection, but it should be a much smaller increment than the base hit of the collection. Note that I'm forcing a load factor of 1, so we are getting no concurrency out of this collection...just thread-safe traversal+modification. Before: After: |
Member
Author
|
This is a pretty non-invasive patch, and the cost of 200 bytes per class doesn't seem too bad, so I vote that we go with it. |
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
1 participant
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.
The naively-synchronized WeakHashSet does not guarantee safe iteration in the presence of modification, which triggers a ConcurrentModificationException as seen in #5115.
By using a ConcurrentWeakHashMap (designed by Doug Lea et al) we can iterate and modify concurrently without errors.
However this implementation is based the design of ConcurrentHashMap, which means the in-memory structure will likely be considerably larger than a simple WeakHashMap. I believe it is also based off the pre-Java 8 ConcurrentHashMap, which means any load factor above one thread greatly increases in-memory size, and a load factor of one limits the concurrent use of the collection.