[GR-15285] Intern strings when String#-@ is called.

PullRequest: truffleruby/807

Author: aardvark179

CHANGELOG.md

@@ -10,6 +10,10 @@ Bug fixes:
 * Fixed `rb_get_kwargs` to correctly handle optional and rest arguments.
 * Calling `Kernel#raise` with a raised exception will no longer set the cause of the exception to itself (#1682).
 
+Compatibility
+
+* `String#-@` now performs string deduplication 
+
 # 1.0 RC 17
 
 Bug fixes:
@@ -55,6 +59,7 @@ Compatibility:
 * `StringScanner` will now match a regexp beginning with `^` even when not scanning from the start of the string.
 * `Module#define_method` is now public like in MRI.
 * `Kernel#warn` now supports the `uplevel:` keyword argument.
+* `String#-@` now performs string deduplication (#1608) .
 
 # 1.0 RC 15, 5 April 2019
 

spec/ruby/core/string/uminus_spec.rb

@@ -42,6 +42,20 @@
       (-dynamic).should_not equal("this string is frozen".freeze)
       (-dynamic).should_not equal(-"this string is frozen".freeze)
     end
+
+    it "does not deduplicate tainted strings" do
+      dynamic = %w(this string is frozen).join(' ')
+      dynamic.taint
+      (-dynamic).should_not equal("this string is frozen".freeze)
+      (-dynamic).should_not equal(-"this string is frozen".freeze)
+    end
+
+    it "does not deduplicate strings with additional instance variables" do
+      dynamic = %w(this string is frozen).join(' ')
+      dynamic.instance_variable_set(:@foo, :bar)
+      (-dynamic).should_not equal("this string is frozen".freeze)
+      (-dynamic).should_not equal(-"this string is frozen".freeze)
+    end
   end
 
   ruby_version_is "2.6" do

spec/tags/core/string/uminus_tags.txt

@@ -46,6 +46,7 @@
 import org.truffleruby.core.rope.PathToRopeCache;
 import org.truffleruby.core.rope.Rope;
 import org.truffleruby.core.rope.RopeCache;
+import org.truffleruby.core.rope.RopeKey;
 import org.truffleruby.core.string.CoreStrings;
 import org.truffleruby.core.string.FrozenStringLiterals;
 import org.truffleruby.core.symbol.SymbolTable;
@@ -637,6 +638,10 @@ public DynamicObject getFrozenStringLiteral(Rope rope) {
         return frozenStringLiterals.getFrozenStringLiteral(rope);
     }
 
+    public DynamicObject getInternedString(DynamicObject string) {
+        return frozenStringLiterals.getFrozenStringLiteral(string);
+    }
+
     public Object getClassVariableDefinitionLock() {
         return classVariableDefinitionLock;
     }

src/main/java/org/truffleruby/RubyContext.java

@@ -38,6 +38,8 @@
 import java.util.Map.Entry;
 import java.util.concurrent.ConcurrentHashMap;
 
+import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
+
 import org.truffleruby.core.hash.ReHashable;
 
 /**
@@ -65,6 +67,7 @@ public Value get(Key key) {
      * Returns the value in the cache (existing or added).
      * Similar to a putIfAbsent() but always return the value in the cache.
      */
+    @TruffleBoundary
     public Value addInCacheIfAbsent(Key key, Value newValue) {
         removeStaleEntries();
 

src/main/java/org/truffleruby/collections/WeakValueCache.java

@@ -12,23 +12,36 @@
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 
+import org.truffleruby.Layouts;
 import org.truffleruby.RubyContext;
 import org.truffleruby.collections.ConcurrentOperations;
+import org.truffleruby.collections.WeakValueCache;
 import org.truffleruby.core.rope.Rope;
+import org.truffleruby.core.rope.RopeKey;
 
 import com.oracle.truffle.api.object.DynamicObject;
 
 public class FrozenStringLiterals {
 
     private final RubyContext context;
-    private final Map<Rope, DynamicObject> strings = new ConcurrentHashMap<>();
+    private final WeakValueCache<RopeKey, DynamicObject> values = new WeakValueCache<>();
 
     public FrozenStringLiterals(RubyContext context) {
         this.context = context;
     }
 
     public DynamicObject getFrozenStringLiteral(Rope rope) {
-        return ConcurrentOperations.getOrCompute(strings, rope, r -> StringOperations.createFrozenString(context, rope));
+        final RopeKey key = new RopeKey(rope, context.getHashing(values));
+        return values.addInCacheIfAbsent(key, StringOperations.createFrozenString(context, rope));
+    }
+
+    public DynamicObject getFrozenStringLiteral(DynamicObject string) {
+        assert Layouts.STRING.getFrozen(string) == true;
+
+        final Rope rope = Layouts.STRING.getRope(string);
+        final RopeKey key = new RopeKey(rope, context.getHashing(values));
+
+        return values.addInCacheIfAbsent(key, string);
     }
 
 }

src/main/java/org/truffleruby/core/string/FrozenStringLiterals.java

@@ -125,8 +125,10 @@
 import org.truffleruby.core.rope.RopeBuilder;
 import org.truffleruby.core.rope.RopeConstants;
 import org.truffleruby.core.rope.RopeGuards;
+import org.truffleruby.core.rope.RopeKey;
 import org.truffleruby.core.rope.RopeNodes;
 import org.truffleruby.core.rope.RopeNodes.RepeatNode;
+import org.truffleruby.core.rope.TruffleRopesNodes.FlattenRopeNode;
 import org.truffleruby.core.rope.RopeOperations;
 import org.truffleruby.core.rope.SubstringRope;
 import org.truffleruby.core.string.StringNodesFactory.ByteIndexFromCharIndexNodeGen;
@@ -4712,4 +4714,13 @@ protected Object emptyString(DynamicObject string) {
 
     }
 
+    @Primitive(name = "string_intern", needsSelf = false)
+    public abstract static class InternNode extends PrimitiveArrayArgumentsNode {
+
+        @Specialization
+        public DynamicObject internString(DynamicObject string) {
+            return getContext().getInternedString(string);
+        }
+    }
+
 }

src/main/java/org/truffleruby/core/string/StringNodes.java

@@ -1590,7 +1590,11 @@ def +@
   end
 
   def -@
-    frozen? ? self : dup.freeze
+    str = frozen? ? self : dup.freeze
+    unless str.tainted? || !(str.instance_variables).empty?
+      Truffle::Ropes.flatten_rope(str)
+      Truffle.invoke_primitive(:string_intern, str)
+    end
   end
 
   def encoding