Additional fixes for deuping strings (#4970).

* Use deduped as both key and value. Hard reference to key is the responsibility of callers of freezeAndDedupString. * Remove use of Java 8+ atomic Map.compute API calls. This means 9.1 will have a race, but that should only affect concurrent loads that attempt to deduplicate the same currently-not- deduplicated strings.
jruby · Jan 22, 2018 · 28db22d · 28db22d
1 parent fc97cd8
commit 28db22d
Showing 1 changed file with 26 additions and 2 deletions.
diff --git a/core/src/main/java/org/jruby/Ruby.java b/core/src/main/java/org/jruby/Ruby.java
@@ -195,6 +195,8 @@
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.function.BiFunction;
+import java.util.function.Function;
 import java.util.regex.Pattern;
 
 import static java.lang.invoke.MethodHandles.explicitCastArguments;
@@ -4577,14 +4579,36 @@ public RubyString freezeAndDedupString(RubyString string) {
         RubyString deduped;
 
         if (dedupedRef == null || (deduped = dedupedRef.get()) == null) {
+            // Never use incoming value as key
             deduped = string.strDup(this);
             deduped.setFrozen(true);
-            dedupMap.put(string, new WeakReference<RubyString>(deduped));
-        } else if (deduped.getEncoding() != string.getEncoding()) {
+
+            WeakReference<RubyString> weakref = new WeakReference<>(deduped);
+            WeakReference<RubyString> existing;
+            RubyString existingDeduped;
+
+            // Check if someone else beat us to it.
+            // NOTE: This still races because Map.compute* API is Java 8+.
+            while ((existing = dedupMap.putIfAbsent(deduped, weakref)) != null ) {
+
+                existingDeduped = existing.get();
+
+                if (existingDeduped != null) {
+                    deduped = existingDeduped;
+                    break;
+                }
+
+                // keep trying to put it if existing has been evacuated
+            }
+        }
+
+        if (deduped.getEncoding() != string.getEncoding()) {
             // if encodings don't match, new string loses; can't dedup
+            // FIXME: This may never happen, if we are properly considering encoding in RubyString.hashCode
             deduped = string.strDup(this);
             deduped.setFrozen(true);
         }
+
         return deduped;
     }