Use ConcurrentWeakHashMap for RubyClass.subclasses.

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.
jruby · Apr 12, 2018 · 3203007 · 3203007
1 parent 79e9b26
commit 3203007
Show file tree

Hide file tree

Showing 2 changed files with 1,473 additions and 17 deletions.
diff --git a/core/src/main/java/org/jruby/RubyClass.java b/core/src/main/java/org/jruby/RubyClass.java
@@ -91,6 +91,7 @@
 import org.jruby.util.ClassDefiningClassLoader;
 import org.jruby.util.CodegenUtils;
 import org.jruby.util.JavaNameMangler;
+import org.jruby.util.collections.ConcurrentWeakHashMap;
 import org.jruby.util.collections.WeakHashSet;
 import org.jruby.util.log.Logger;
 import org.jruby.util.log.LoggerFactory;
@@ -1098,7 +1099,7 @@ public final Collection<RubyClass> subclasses() {
     }
 
     public Collection<RubyClass> subclasses(boolean includeDescendants) {
-        Set<RubyClass> subclasses = this.subclasses;
+        Map<RubyClass, Object> subclasses = this.subclasses;
         if (subclasses != null) {
             Collection<RubyClass> mine = new ArrayList<>();
             subclassesInner(mine, includeDescendants);
@@ -1109,11 +1110,12 @@ public Collection<RubyClass> subclasses(boolean includeDescendants) {
     }
 
     private void subclassesInner(Collection<RubyClass> mine, boolean includeDescendants) {
-        Set<RubyClass> subclasses = this.subclasses;
+        Map<RubyClass, Object> subclasses = this.subclasses;
         if (subclasses != null) {
-            mine.addAll(subclasses);
+            Set<RubyClass> keys = subclasses.keySet();
+            mine.addAll(keys);
             if (includeDescendants) {
-                for (RubyClass klass: subclasses) {
+                for (RubyClass klass: keys) {
                     klass.subclassesInner(mine, includeDescendants);
                 }
             }
@@ -1130,18 +1132,18 @@ private void subclassesInner(Collection<RubyClass> mine, boolean includeDescenda
      * @param subclass The subclass to add
      */
     public void addSubclass(RubyClass subclass) {
-        Set<RubyClass> subclasses = this.subclasses;
+        Map<RubyClass, Object> subclasses = this.subclasses;
         if (subclasses == null) {
             // check again
             synchronized (this) {
                 subclasses = this.subclasses;
                 if (subclasses == null) {
-                    this.subclasses = subclasses = Collections.synchronizedSet(new WeakHashSet<RubyClass>(4));
+                    this.subclasses = subclasses = new ConcurrentWeakHashMap<>(4, 0.75f, 1);
                 }
             }
         }
 
-        subclasses.add(subclass);
+        subclasses.put(subclass, NEVER);
     }
 
     /**
@@ -1150,7 +1152,7 @@ public void addSubclass(RubyClass subclass) {
      * @param subclass The subclass to remove
      */
     public void removeSubclass(RubyClass subclass) {
-        Set<RubyClass> subclasses = this.subclasses;
+        Map<RubyClass, Object> subclasses = this.subclasses;
         if (subclasses == null) return;
 
         subclasses.remove(subclass);
@@ -1163,20 +1165,20 @@ public void removeSubclass(RubyClass subclass) {
      * @param newSubclass The subclass to replace it with
      */
     public void replaceSubclass(RubyClass subclass, RubyClass newSubclass) {
-        Set<RubyClass> subclasses = this.subclasses;
+        Map<RubyClass, Object> subclasses = this.subclasses;
         if (subclasses == null) return;
 
         subclasses.remove(subclass);
-        subclasses.add(newSubclass);
+        subclasses.put(newSubclass, NEVER);
     }
 
     @Override
     public void becomeSynchronized() {
         // make this class and all subclasses sync
         super.becomeSynchronized();
-        Set<RubyClass> subclasses = this.subclasses;
+        Map<RubyClass, Object> subclasses = this.subclasses;
         if (subclasses != null) {
-            for (RubyClass subclass : subclasses) subclass.becomeSynchronized();
+            for (RubyClass subclass : subclasses.keySet()) subclass.becomeSynchronized();
         }
     }
 
@@ -1196,9 +1198,9 @@ public void becomeSynchronized() {
     public void invalidateCacheDescendants() {
         super.invalidateCacheDescendants();
 
-        Set<RubyClass> subclasses = this.subclasses;
+        Map<RubyClass, Object> subclasses = this.subclasses;
         if (subclasses != null) {
-            for (RubyClass subclass : subclasses) subclass.invalidateCacheDescendants();
+            for (RubyClass subclass : subclasses.keySet()) subclass.invalidateCacheDescendants();
         }
     }
 
@@ -1209,12 +1211,12 @@ public void addInvalidatorsAndFlush(List<Invalidator> invalidators) {
         // if we're not at boot time, don't bother fully clearing caches
         if (!runtime.isBootingCore()) cachedMethods.clear();
 
-        Set<RubyClass> subclasses = this.subclasses;
+        Map<RubyClass, Object> subclasses = this.subclasses;
         // no subclasses, don't bother with lock and iteration
         if (subclasses == null || subclasses.isEmpty()) return;
 
         // cascade into subclasses
-        for (RubyClass subclass : subclasses) subclass.addInvalidatorsAndFlush(invalidators);
+        for (RubyClass subclass : subclasses.keySet()) subclass.addInvalidatorsAndFlush(invalidators);
     }
 
     public final Ruby getClassRuntime() {
@@ -2327,7 +2329,7 @@ public IRubyObject invoke(ThreadContext context, IRubyObject self, String name,
     protected final Ruby runtime;
     private ObjectAllocator allocator; // the default allocator
     protected ObjectMarshal marshal;
-    private volatile Set<RubyClass> subclasses;
+    private volatile Map<RubyClass, Object> subclasses;
     public static final int CS_IDX_INITIALIZE = 0;
     public enum CS_NAMES {
         INITIALIZE("initialize");