[GR-30852] Add RubyEncoding field to RubySymbol

eregon · eregon · commit 87fec1aee358 · 2021-07-16T11:09:59.000Z
PullRequest: truffleruby/2803
diff --git a/src/main/java/org/truffleruby/RubyLanguage.java b/src/main/java/org/truffleruby/RubyLanguage.java
@@ -36,6 +36,7 @@
 import org.truffleruby.core.array.RubyArray;
 import org.truffleruby.core.basicobject.RubyBasicObject;
 import org.truffleruby.core.binding.RubyBinding;
+import org.truffleruby.core.encoding.RubyEncoding;
 import org.truffleruby.core.encoding.RubyEncodingConverter;
 import org.truffleruby.core.exception.RubyException;
 import org.truffleruby.core.exception.RubyFrozenError;
@@ -314,8 +315,8 @@ public RubySymbol getSymbol(String string) {
 
 
     @TruffleBoundary
-    public RubySymbol getSymbol(Rope rope) {
-        return symbolTable.getSymbol(rope);
+    public RubySymbol getSymbol(Rope rope, RubyEncoding encoding) {
+        return symbolTable.getSymbol(rope, encoding);
     }
 
     public Assumption getTracingAssumption() {
diff --git a/src/main/java/org/truffleruby/cext/CExtNodes.java b/src/main/java/org/truffleruby/cext/CExtNodes.java
@@ -1635,7 +1635,9 @@ public abstract static class RbCheckSymbolCStrNode extends CoreMethodArrayArgume
         @Specialization(guards = "strings.isRubyString(string)")
         protected Object checkSymbolCStr(Object string,
                 @CachedLibrary(limit = "2") RubyStringLibrary strings) {
-            final RubySymbol sym = getLanguage().symbolTable.getSymbolIfExists(strings.getRope(string));
+            final RubySymbol sym = getLanguage().symbolTable.getSymbolIfExists(
+                    strings.getRope(string),
+                    strings.getEncoding(string));
             return sym == null ? nil : sym;
         }
     }
diff --git a/src/main/java/org/truffleruby/core/cast/StringToSymbolNode.java b/src/main/java/org/truffleruby/core/cast/StringToSymbolNode.java
@@ -24,12 +24,12 @@ public abstract class StringToSymbolNode extends RubyContextSourceNode {
 
     @Specialization
     protected RubySymbol doString(RubyString string) {
-        return getSymbol(string.rope);
+        return getSymbol(string.rope, string.encoding);
     }
 
     @Specialization
     protected RubySymbol doString(ImmutableRubyString string) {
-        return getSymbol(string.rope);
+        return getSymbol(string.rope, string.encoding);
     }
 
 }
diff --git a/src/main/java/org/truffleruby/core/cast/ToRubyEncodingNode.java b/src/main/java/org/truffleruby/core/cast/ToRubyEncodingNode.java
@@ -40,7 +40,7 @@ protected RubyEncoding immutableStringToEncoding(ImmutableRubyString value) {
 
     @Specialization
     protected RubyEncoding symbolToEncoding(RubySymbol value) {
-        return getContext().getEncodingManager().getRubyEncoding(value.getRope().getEncoding());
+        return value.encoding;
     }
 
     @Specialization
diff --git a/src/main/java/org/truffleruby/core/cast/ToSymbolNode.java b/src/main/java/org/truffleruby/core/cast/ToSymbolNode.java
@@ -13,6 +13,7 @@
 import com.oracle.truffle.api.dsl.CachedLanguage;
 import com.oracle.truffle.api.library.CachedLibrary;
 import org.truffleruby.RubyLanguage;
+import org.truffleruby.core.encoding.RubyEncoding;
 import org.truffleruby.core.rope.Rope;
 import org.truffleruby.core.rope.RopeNodes;
 import org.truffleruby.core.symbol.RubySymbol;
@@ -57,22 +58,24 @@ protected RubySymbol toSymbolJavaStringUncached(String str,
     @Specialization(
             guards = {
                     "strings.isRubyString(str)",
-                    "equals.execute(strings.getRope(str), cachedRope)" },
+                    "equals.execute(strings.getRope(str), cachedRope)",
+                    "strings.getEncoding(str) == cachedEncoding" },
             limit = "getCacheLimit()")
     protected RubySymbol toSymbolRubyString(Object str,
             @CachedLibrary(limit = "2") RubyStringLibrary strings,
             @Cached(value = "strings.getRope(str)") Rope cachedRope,
+            @Cached(value = "strings.getEncoding(str)") RubyEncoding cachedEncoding,
             @CachedLanguage RubyLanguage language,
             @Cached RopeNodes.EqualNode equals,
-            @Cached(value = "language.getSymbol(cachedRope)") RubySymbol rubySymbol) {
+            @Cached(value = "language.getSymbol(cachedRope, cachedEncoding)") RubySymbol rubySymbol) {
         return rubySymbol;
     }
 
     @Specialization(guards = "strings.isRubyString(str)", replaces = "toSymbolRubyString")
     protected RubySymbol toSymbolRubyStringUncached(Object str,
             @CachedLanguage RubyLanguage language,
             @CachedLibrary(limit = "2") RubyStringLibrary strings) {
-        return language.getSymbol(strings.getRope(str));
+        return language.getSymbol(strings.getRope(str), strings.getEncoding(str));
     }
 
     protected int getCacheLimit() {
diff --git a/src/main/java/org/truffleruby/core/encoding/EncodingNodes.java b/src/main/java/org/truffleruby/core/encoding/EncodingNodes.java
@@ -643,7 +643,7 @@ protected RubyEncoding encodingGetObjectEncodingImmutableString(ImmutableRubyStr
 
         @Specialization
         protected RubyEncoding encodingGetObjectEncodingSymbol(RubySymbol object) {
-            return getRubyEncodingNode.executeGetRubyEncoding(object.getRope().getEncoding());
+            return object.encoding;
         }
 
         @Specialization
diff --git a/src/main/java/org/truffleruby/core/format/printf/PrintfSimpleTreeBuilder.java b/src/main/java/org/truffleruby/core/format/printf/PrintfSimpleTreeBuilder.java
@@ -15,6 +15,7 @@
 import com.oracle.truffle.api.CompilerDirectives;
 import org.jcodings.specific.USASCIIEncoding;
 import org.truffleruby.RubyLanguage;
+import org.truffleruby.core.encoding.Encodings;
 import org.truffleruby.core.format.FormatNode;
 import org.truffleruby.core.format.LiteralFormatNode;
 import org.truffleruby.core.format.SharedTreeBuilder;
@@ -65,7 +66,7 @@ private void buildTree() {
                     final RubySymbol key = language.getSymbol(RopeOperations.create(
                             config.getNamesBytes(),
                             USASCIIEncoding.INSTANCE,
-                            CodeRange.CR_7BIT));
+                            CodeRange.CR_7BIT), Encodings.US_ASCII);
                     valueNode = ReadHashValueNodeGen.create(key, new SourceNode());
                 } else if (config.getAbsoluteArgumentIndex() != null) {
                     valueNode = ReadArgumentIndexValueNodeGen
diff --git a/src/main/java/org/truffleruby/core/regexp/RegexpNodes.java b/src/main/java/org/truffleruby/core/regexp/RegexpNodes.java
@@ -216,7 +216,7 @@ protected RubyArray regexpNames(RubyRegexp regexp) {
                 final byte[] bytes = Arrays.copyOfRange(e.name, e.nameP, e.nameEnd);
 
                 final Rope rope = RopeOperations.create(bytes, UTF8Encoding.INSTANCE, CodeRange.CR_UNKNOWN);
-                final RubySymbol name = getSymbol(rope);
+                final RubySymbol name = getSymbol(rope, Encodings.UTF_8);
 
                 final int[] backrefs = e.getBackRefs();
                 final RubyArray backrefsRubyArray = createArray(backrefs);
diff --git a/src/main/java/org/truffleruby/core/rope/RopeWithEncoding.java b/src/main/java/org/truffleruby/core/rope/RopeWithEncoding.java
@@ -11,7 +11,9 @@
 
 import org.truffleruby.core.encoding.RubyEncoding;
 
-public class RopeWithEncoding {
+import java.util.Objects;
+
+public final class RopeWithEncoding {
 
     private final Rope rope;
     private final RubyEncoding encoding;
@@ -29,4 +31,22 @@ public Rope getRope() {
     public RubyEncoding getEncoding() {
         return encoding;
     }
+
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) {
+            return true;
+        }
+        if (!(o instanceof RopeWithEncoding)) {
+            return false;
+        }
+        RopeWithEncoding that = (RopeWithEncoding) o;
+        return rope.equals(that.rope) && encoding == that.encoding;
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(rope, encoding);
+    }
+
 }
diff --git a/src/main/java/org/truffleruby/core/string/StringNodes.java b/src/main/java/org/truffleruby/core/string/StringNodes.java
@@ -2806,20 +2806,22 @@ public abstract static class ToSymNode extends CoreMethodArrayArgumentsNode {
         @Specialization(
                 guards = {
                         "!isBrokenCodeRange(strings.getRope(string), codeRangeNode)",
-                        "equalNode.execute(strings.getRope(string),cachedRope)" },
+                        "equalNode.execute(strings.getRope(string),cachedRope)",
+                        "strings.getEncoding(string) == cachedEncoding" },
                 limit = "getDefaultCacheLimit()")
         protected RubySymbol toSymCached(Object string,
                 @CachedLibrary(limit = "2") RubyStringLibrary strings,
                 @Cached("strings.getRope(string)") Rope cachedRope,
-                @Cached("getSymbol(cachedRope)") RubySymbol cachedSymbol,
+                @Cached("strings.getEncoding(string)") RubyEncoding cachedEncoding,
+                @Cached("getSymbol(cachedRope, cachedEncoding)") RubySymbol cachedSymbol,
                 @Cached RopeNodes.EqualNode equalNode) {
             return cachedSymbol;
         }
 
         @Specialization(guards = "!isBrokenCodeRange(strings.getRope(string), codeRangeNode)", replaces = "toSymCached")
         protected RubySymbol toSym(Object string,
                 @CachedLibrary(limit = "2") RubyStringLibrary strings) {
-            return getSymbol(strings.getRope(string));
+            return getSymbol(strings.getRope(string), strings.getEncoding(string));
         }
 
         @Specialization(guards = "isBrokenCodeRange(strings.getRope(string), codeRangeNode)")
diff --git a/src/main/java/org/truffleruby/core/symbol/CoreSymbols.java b/src/main/java/org/truffleruby/core/symbol/CoreSymbols.java
@@ -13,6 +13,7 @@
 import java.util.List;
 
 import org.jcodings.specific.USASCIIEncoding;
+import org.truffleruby.core.encoding.Encodings;
 import org.truffleruby.core.rope.LeafRope;
 import org.truffleruby.core.rope.RopeConstants;
 import org.truffleruby.core.rope.RopeOperations;
@@ -197,7 +198,7 @@ public RubySymbol createRubySymbol(String string, long id) {
             rope = RopeOperations.encodeAscii(string, USASCIIEncoding.INSTANCE);
         }
 
-        final RubySymbol symbol = new RubySymbol(string, rope, id);
+        final RubySymbol symbol = new RubySymbol(string, rope, Encodings.US_ASCII, id);
         CORE_SYMBOLS.add(symbol);
 
         if (id != RubySymbol.UNASSIGNED_ID) {
diff --git a/src/main/java/org/truffleruby/core/symbol/RubySymbol.java b/src/main/java/org/truffleruby/core/symbol/RubySymbol.java
@@ -14,6 +14,7 @@
 import org.truffleruby.RubyContext;
 import org.truffleruby.RubyLanguage;
 import org.truffleruby.core.Hashing;
+import org.truffleruby.core.encoding.RubyEncoding;
 import org.truffleruby.core.klass.RubyClass;
 import org.truffleruby.core.rope.LeafRope;
 import org.truffleruby.language.ImmutableRubyObject;
@@ -34,22 +35,25 @@ public final class RubySymbol extends ImmutableRubyObject implements TruffleObje
 
     private static final int CLASS_SALT = 92021474; // random number, stops hashes for similar values but different classes being the same, static because we want deterministic hashes
 
+    public final RubyEncoding encoding;
     private final String string;
     private final LeafRope rope;
     private final int javaStringHashCode;
     private final long id;
 
     private volatile RootCallTarget callTargetNoRefinements = null;
 
-    public RubySymbol(String string, LeafRope rope, long id) {
+    public RubySymbol(String string, LeafRope rope, RubyEncoding encoding, long id) {
+        assert rope.encoding == encoding.jcoding;
+        this.encoding = encoding;
         this.string = string;
         this.rope = rope;
         this.javaStringHashCode = string.hashCode();
         this.id = id;
     }
 
-    public RubySymbol(String string, LeafRope rope) {
-        this(string, rope, UNASSIGNED_ID);
+    public RubySymbol(String string, LeafRope rope, RubyEncoding encoding) {
+        this(string, rope, encoding, UNASSIGNED_ID);
     }
 
     public long getId() {
diff --git a/src/main/java/org/truffleruby/core/symbol/SymbolNodes.java b/src/main/java/org/truffleruby/core/symbol/SymbolNodes.java
@@ -22,8 +22,6 @@
 import org.truffleruby.collections.ConcurrentOperations;
 import org.truffleruby.core.CoreLibrary;
 import org.truffleruby.core.array.RubyArray;
-import org.truffleruby.core.encoding.EncodingNodes;
-import org.truffleruby.core.encoding.RubyEncoding;
 import org.truffleruby.core.klass.RubyClass;
 import org.truffleruby.core.module.RubyModule;
 import org.truffleruby.core.proc.ProcCallTargets;
@@ -276,10 +274,8 @@ public abstract static class ToSNode extends CoreMethodArrayArgumentsNode {
 
         @Specialization
         protected RubyString toS(RubySymbol symbol,
-                @Cached StringNodes.MakeStringNode makeStringNode,
-                @Cached EncodingNodes.GetRubyEncodingNode getRubyEncodingNode) {
-            final RubyEncoding rubyEncoding = getRubyEncodingNode.executeGetRubyEncoding(symbol.getRope().encoding);
-            return makeStringNode.fromRope(symbol.getRope(), rubyEncoding);
+                @Cached StringNodes.MakeStringNode makeStringNode) {
+            return makeStringNode.fromRope(symbol.getRope(), symbol.encoding);
         }
 
     }
diff --git a/src/main/java/org/truffleruby/core/symbol/SymbolTable.java b/src/main/java/org/truffleruby/core/symbol/SymbolTable.java
@@ -15,11 +15,14 @@
 import org.jcodings.specific.UTF8Encoding;
 import org.truffleruby.RubyContext;
 import org.truffleruby.collections.WeakValueCache;
+import org.truffleruby.core.encoding.Encodings;
+import org.truffleruby.core.encoding.RubyEncoding;
 import org.truffleruby.core.rope.LeafRope;
 import org.truffleruby.core.rope.NativeRope;
 import org.truffleruby.core.rope.Rope;
 import org.truffleruby.core.rope.RopeCache;
 import org.truffleruby.core.rope.RopeOperations;
+import org.truffleruby.core.rope.RopeWithEncoding;
 import org.truffleruby.core.string.StringOperations;
 import org.truffleruby.language.control.RaiseException;
 import org.truffleruby.parser.Identifiers;
@@ -35,9 +38,9 @@ public class SymbolTable {
     // However, this doesn't matter as the cache entries will be re-created when used.
     private final WeakValueCache<String, RubySymbol> stringToSymbolCache = new WeakValueCache<>();
 
-    // Weak map of RopeKey to Symbol to keep Symbols unique.
+    // Weak map of RopeWithEncoding to Symbol to keep Symbols unique.
     // As long as the Symbol is referenced, the entry will stay in the symbolMap.
-    private final WeakValueCache<Rope, RubySymbol> symbolMap = new WeakValueCache<>();
+    private final WeakValueCache<RopeWithEncoding, RubySymbol> symbolMap = new WeakValueCache<>();
 
     public SymbolTable(RopeCache ropeCache, CoreSymbols coreSymbols) {
         this.ropeCache = ropeCache;
@@ -47,15 +50,19 @@ public SymbolTable(RopeCache ropeCache, CoreSymbols coreSymbols) {
     private void addCoreSymbols(CoreSymbols coreSymbols) {
         for (RubySymbol symbol : coreSymbols.CORE_SYMBOLS) {
             final Rope rope = symbol.getRope();
-            assert rope == normalizeRopeForLookup(rope);
+            final RopeWithEncoding ropeWithEncoding = normalizeRopeForLookup(rope, symbol.encoding);
+            assert rope == ropeWithEncoding.getRope();
             assert rope == ropeCache.getRope(rope);
 
-            final RubySymbol existing = symbolMap.put(rope, symbol);
+            final RubySymbol existing = symbolMap.put(ropeWithEncoding, symbol);
             if (existing != null) {
                 throw new AssertionError("Duplicate Symbol in SymbolTable: " + existing);
             }
 
-            stringToSymbolCache.put(symbol.getString(), symbol);
+            final RubySymbol old = stringToSymbolCache.put(symbol.getString(), symbol);
+            if (old != null) {
+                throw new AssertionError("Duplicate Symbol in SymbolTable: " + old);
+            }
         }
     }
 
@@ -67,56 +74,60 @@ public RubySymbol getSymbol(String string) {
         }
 
         final LeafRope rope;
+        final RubyEncoding encoding;
         if (StringOperations.isAsciiOnly(string)) {
             rope = RopeOperations.encodeAscii(string, USASCIIEncoding.INSTANCE);
+            encoding = Encodings.US_ASCII;
         } else {
             rope = StringOperations.encodeRope(string, UTF8Encoding.INSTANCE);
+            encoding = Encodings.UTF_8;
         }
-        symbol = getSymbol(rope);
-
-        // Add it to the direct j.l.String to Symbol cache
+        symbol = getSymbol(rope, encoding);
 
+        // Add it to the direct java.lang.String to Symbol cache
         stringToSymbolCache.addInCacheIfAbsent(string, symbol);
 
         return symbol;
     }
 
     @TruffleBoundary
-    public RubySymbol getSymbol(Rope rope) {
-        final Rope normalizedRope = normalizeRopeForLookup(rope);
-        final RubySymbol symbol = symbolMap.get(normalizedRope);
+    public RubySymbol getSymbol(Rope rope, RubyEncoding encoding) {
+        final RopeWithEncoding ropeEncodingForLookup = normalizeRopeForLookup(rope, encoding);
+        final RubySymbol symbol = symbolMap.get(ropeEncodingForLookup);
         if (symbol != null) {
             return symbol;
         }
 
-        final LeafRope cachedRope = ropeCache.getRope(normalizedRope);
-        final RubySymbol newSymbol = createSymbol(cachedRope);
-        // Use a RopeKey with the cached Rope in symbolMap, since the Symbol refers to it and so we
+        final LeafRope cachedRope = ropeCache.getRope(ropeEncodingForLookup.getRope());
+        final RubyEncoding symbolEncoding = ropeEncodingForLookup.getEncoding();
+        final RubySymbol newSymbol = createSymbol(cachedRope, symbolEncoding);
+        // Use a RopeWithEncoding with the cached Rope in symbolMap, since the Symbol refers to it and so we
         // do not keep rope alive unnecessarily.
-        return symbolMap.addInCacheIfAbsent(cachedRope, newSymbol);
+        return symbolMap.addInCacheIfAbsent(new RopeWithEncoding(cachedRope, symbolEncoding), newSymbol);
     }
 
     @TruffleBoundary
-    public RubySymbol getSymbolIfExists(Rope rope) {
-        final Rope ropeKey = normalizeRopeForLookup(rope);
+    public RubySymbol getSymbolIfExists(Rope rope, RubyEncoding encoding) {
+        final RopeWithEncoding ropeKey = normalizeRopeForLookup(rope, encoding);
         return symbolMap.get(ropeKey);
     }
 
-    private Rope normalizeRopeForLookup(Rope rope) {
+    private RopeWithEncoding normalizeRopeForLookup(Rope rope, RubyEncoding encoding) {
         if (rope instanceof NativeRope) {
             rope = ((NativeRope) rope).toLeafRope();
         }
 
         if (rope.isAsciiOnly() && rope.getEncoding() != USASCIIEncoding.INSTANCE) {
             rope = RopeOperations.withEncoding(rope, USASCIIEncoding.INSTANCE);
+            encoding = Encodings.US_ASCII;
         }
 
-        return rope;
+        return new RopeWithEncoding(rope, encoding);
     }
 
-    private RubySymbol createSymbol(LeafRope cachedRope) {
+    private RubySymbol createSymbol(LeafRope cachedRope, RubyEncoding encoding) {
         final String string = RopeOperations.decodeOrEscapeBinaryRope(cachedRope);
-        return new RubySymbol(string, cachedRope);
+        return new RubySymbol(string, cachedRope, encoding);
     }
 
     // TODO (eregon, 10/10/2015): this check could be done when a Symbol is created to be much cheaper
diff --git a/src/main/java/org/truffleruby/language/RubyContextNode.java b/src/main/java/org/truffleruby/language/RubyContextNode.java
diff --git a/src/main/java/org/truffleruby/language/RubyContextSourceNode.java b/src/main/java/org/truffleruby/language/RubyContextSourceNode.java
diff --git a/src/main/java/org/truffleruby/parser/BodyTranslator.java b/src/main/java/org/truffleruby/parser/BodyTranslator.java
diff --git a/tool/generate-core-symbols.rb b/tool/generate-core-symbols.rb

Original file line number	Diff line number	Diff line change
`@@ -1635,7 +1635,9 @@ public abstract static class RbCheckSymbolCStrNode extends CoreMethodArrayArgume`
`1635`	`1635`	`@Specialization(guards = "strings.isRubyString(string)")`
`1636`	`1636`	`protected Object checkSymbolCStr(Object string,`
`1637`	`1637`	`@CachedLibrary(limit = "2") RubyStringLibrary strings) {`
`1638`		`- final RubySymbol sym = getLanguage().symbolTable.getSymbolIfExists(strings.getRope(string));`
	`1638`	`+ final RubySymbol sym = getLanguage().symbolTable.getSymbolIfExists(`
	`1639`	`+ strings.getRope(string),`
	`1640`	`+ strings.getEncoding(string));`
`1639`	`1641`	`return sym == null ? nil : sym;`
`1640`	`1642`	`}`
`1641`	`1643`	`}`
Original file line number	Diff line number	Diff line change
`@@ -24,12 +24,12 @@ public abstract class StringToSymbolNode extends RubyContextSourceNode {`
`24`	`24`
`25`	`25`	`@Specialization`
`26`	`26`	`protected RubySymbol doString(RubyString string) {`
`27`		`- return getSymbol(string.rope);`
	`27`	`+ return getSymbol(string.rope, string.encoding);`
`28`	`28`	`}`
`29`	`29`
`30`	`30`	`@Specialization`
`31`	`31`	`protected RubySymbol doString(ImmutableRubyString string) {`
`32`		`- return getSymbol(string.rope);`
	`32`	`+ return getSymbol(string.rope, string.encoding);`
`33`	`33`	`}`
`34`	`34`
`35`	`35`	`}`
Original file line number	Diff line number	Diff line change
`@@ -40,7 +40,7 @@ protected RubyEncoding immutableStringToEncoding(ImmutableRubyString value) {`
`40`	`40`
`41`	`41`	`@Specialization`
`42`	`42`	`protected RubyEncoding symbolToEncoding(RubySymbol value) {`
`43`		`- return getContext().getEncodingManager().getRubyEncoding(value.getRope().getEncoding());`
	`43`	`+ return value.encoding;`
`44`	`44`	`}`
`45`	`45`
`46`	`46`	`@Specialization`
Original file line number	Diff line number	Diff line change
`@@ -643,7 +643,7 @@ protected RubyEncoding encodingGetObjectEncodingImmutableString(ImmutableRubyStr`
`643`	`643`
`644`	`644`	`@Specialization`
`645`	`645`	`protected RubyEncoding encodingGetObjectEncodingSymbol(RubySymbol object) {`
`646`		`- return getRubyEncodingNode.executeGetRubyEncoding(object.getRope().getEncoding());`
	`646`	`+ return object.encoding;`
`647`	`647`	`}`
`648`	`648`
`649`	`649`	`@Specialization`