[Truffle] Removed helper method for String length in favor of using n…

…odes. The helper didn't realize optimize the way I thought it would with the inject probabilities and as a result, both branches were always compiled, undermining single-byte-optimizable optimizations.
jruby · Oct 13, 2015 · 0db6d2d · 0db6d2d
1 parent e3c3512
commit 0db6d2d
Showing 4 changed files with 75 additions and 64 deletions.
diff --git a/truffle/src/main/java/org/jruby/truffle/nodes/core/StringNodes.java b/truffle/src/main/java/org/jruby/truffle/nodes/core/StringNodes.java
@@ -428,7 +428,8 @@ public GetIndexNode(RubyContext context, SourceSection sourceSection) {
 
         @Specialization(guards = "wasNotProvided(length) || isRubiniusUndefined(length)")
         public Object getIndex(VirtualFrame frame, DynamicObject string, int index, Object length) {
-            int normalizedIndex = StringOperations.normalizeIndex(string, index);
+            final int stringLength = getSizeNode().executeInteger(frame, string);
+            int normalizedIndex = StringOperations.normalizeIndex(stringLength, index);
             final ByteList bytes = StringOperations.getByteList(string);
 
             if (normalizedIndex < 0 || normalizedIndex >= bytes.length()) {
@@ -467,12 +468,7 @@ public Object sliceObjectRange(VirtualFrame frame, DynamicObject string, Dynamic
         private Object sliceRange(VirtualFrame frame, DynamicObject string, int begin, int end, boolean doesExcludeEnd) {
             assert RubyGuards.isRubyString(string);
 
-            if (sizeNode == null) {
-                CompilerDirectives.transferToInterpreter();
-                sizeNode = insert(StringNodesFactory.SizeNodeFactory.create(getContext(), getSourceSection(), new RubyNode[]{null}));
-            }
-
-            final int stringLength = sizeNode.executeInteger(frame, string);
+            final int stringLength = getSizeNode().executeInteger(frame, string);
             begin = StringOperations.normalizeIndex(stringLength, begin);
 
             if (begin < 0 || begin > stringLength) {
@@ -574,6 +570,15 @@ protected boolean isRubiniusUndefined(Object object) {
             return object == getContext().getCoreLibrary().getRubiniusUndefined();
         }
 
+        private SizeNode getSizeNode() {
+            if (sizeNode == null) {
+                CompilerDirectives.transferToInterpreter();
+                sizeNode = insert(StringNodesFactory.SizeNodeFactory.create(getContext(), getSourceSection(), new RubyNode[]{null}));
+            }
+
+            return sizeNode;
+        }
+
     }
 
     @CoreMethod(names = "ascii_only?")
@@ -1275,11 +1280,13 @@ public Object initializeCopy(DynamicObject self, DynamicObject from) {
     public abstract static class InsertNode extends CoreMethodNode {
 
         @Child private CallDispatchHeadNode concatNode;
+        @Child private SizeNode sizeNode;
         @Child private TaintResultNode taintResultNode;
 
         public InsertNode(RubyContext context, SourceSection sourceSection) {
             super(context, sourceSection);
             concatNode = DispatchHeadNodeFactory.createMethodCall(context);
+            sizeNode = StringNodesFactory.SizeNodeFactory.create(context, sourceSection, new RubyNode[] {});
             taintResultNode = new TaintResultNode(context, sourceSection);
         }
 
@@ -1302,7 +1309,8 @@ public Object insert(VirtualFrame frame, DynamicObject string, int index, Dynami
                 index++;
             }
 
-            StringNodesHelper.replaceInternal(string, StringNodesHelper.checkIndex(string, index, this), 0, otherString);
+            final int stringLength = sizeNode.executeInteger(frame, string);
+            StringNodesHelper.replaceInternal(string, StringNodesHelper.checkIndex(stringLength, index, this), 0, otherString);
 
             return taintResultNode.maybeTaint(otherString, string);
         }
@@ -1800,7 +1808,7 @@ public SuccNode(RubyContext context, SourceSection sourceSection) {
         @TruffleBoundary
         @Specialization
         public DynamicObject succ(DynamicObject string) {
-            if (StringOperations.length(string) > 0) {
+            if (Layouts.STRING.getByteList(string).realSize() > 0) {
                 return Layouts.STRING.createString(Layouts.CLASS.getInstanceFactory(Layouts.BASIC_OBJECT.getLogicalClass(string)), StringSupport.succCommon(getContext().getRuntime(), StringOperations.getByteList(string)), StringSupport.CR_UNKNOWN, null);
             } else {
                 return Layouts.STRING.createString(Layouts.CLASS.getInstanceFactory(Layouts.BASIC_OBJECT.getLogicalClass(string)), new ByteList(), StringSupport.CR_UNKNOWN, null);
@@ -2323,25 +2331,23 @@ public DynamicObject clear(DynamicObject string) {
 
     public static class StringNodesHelper {
 
-        public static int checkIndex(DynamicObject string, int index, RubyNode node) {
-            assert RubyGuards.isRubyString(string);
-
-            if (index > StringOperations.length(string)) {
+        public static int checkIndex(int length, int index, RubyNode node) {
+            if (index > length) {
                 CompilerDirectives.transferToInterpreter();
 
                 throw new RaiseException(
                         node.getContext().getCoreLibrary().indexError(String.format("index %d out of string", index), node));
             }
 
             if (index < 0) {
-                if (-index > StringOperations.length(string)) {
+                if (-index > length) {
                     CompilerDirectives.transferToInterpreter();
 
                     throw new RaiseException(
                             node.getContext().getCoreLibrary().indexError(String.format("index %d out of string", index), node));
                 }
 
-                index += StringOperations.length(string);
+                index += length;
             }
 
             return index;

diff --git a/truffle/src/main/java/org/jruby/truffle/nodes/rubinius/ByteArrayNodes.java b/truffle/src/main/java/org/jruby/truffle/nodes/rubinius/ByteArrayNodes.java
@@ -12,12 +12,16 @@
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.dsl.Specialization;
+import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.object.DynamicObject;
 import com.oracle.truffle.api.source.SourceSection;
 
+import org.jruby.truffle.nodes.RubyNode;
 import org.jruby.truffle.nodes.core.CoreClass;
 import org.jruby.truffle.nodes.core.CoreMethod;
 import org.jruby.truffle.nodes.core.CoreMethodArrayArgumentsNode;
+import org.jruby.truffle.nodes.core.StringNodes;
+import org.jruby.truffle.nodes.core.StringNodesFactory;
 import org.jruby.truffle.nodes.core.UnaryCoreMethodNode;
 import org.jruby.truffle.runtime.RubyContext;
 import org.jruby.truffle.runtime.control.RaiseException;
@@ -103,18 +107,21 @@ public int size(DynamicObject bytes) {
     @CoreMethod(names = "locate", required = 3, lowerFixnumParameters = {1, 2})
     public abstract static class LocateNode extends CoreMethodArrayArgumentsNode {
 
+        @Child private StringNodes.SizeNode sizeNode;
+
         public LocateNode(RubyContext context, SourceSection sourceSection) {
             super(context, sourceSection);
+            sizeNode = StringNodesFactory.SizeNodeFactory.create(context, sourceSection, new RubyNode[] {});
         }
 
         @Specialization(guards = "isRubyString(pattern)")
-        public Object getByte(DynamicObject bytes, DynamicObject pattern, int start, int length) {
+        public Object getByte(VirtualFrame frame, DynamicObject bytes, DynamicObject pattern, int start, int length) {
             final int index = new ByteList(Layouts.BYTE_ARRAY.getBytes(bytes), start, length).indexOf(StringOperations.getByteList(pattern));
 
             if (index == -1) {
                 return nil();
             } else {
-                return start + index + StringOperations.length(pattern);
+                return start + index + sizeNode.executeInteger(frame, pattern);
             }
         }
 

diff --git a/truffle/src/main/java/org/jruby/truffle/nodes/rubinius/StringPrimitiveNodes.java b/truffle/src/main/java/org/jruby/truffle/nodes/rubinius/StringPrimitiveNodes.java
@@ -219,6 +219,7 @@ public static abstract class StringByteSubstringPrimitiveNode extends RubiniusPr
 
         @Child private TaintResultNode taintResultNode;
         @Child private AllocateObjectNode allocateObjectNode;
+        @Child private StringNodes.SizeNode sizeNode;
 
         public StringByteSubstringPrimitiveNode(RubyContext context, SourceSection sourceSection) {
             super(context, sourceSection);
@@ -227,8 +228,8 @@ public StringByteSubstringPrimitiveNode(RubyContext context, SourceSection sourc
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, int index, NotProvided length) {
-            final Object subString = stringByteSubstring(string, index, 1);
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, int index, NotProvided length) {
+            final Object subString = stringByteSubstring(frame, string, index, 1);
 
             if (subString == nil()) {
                 return subString;
@@ -242,14 +243,15 @@ public Object stringByteSubstring(DynamicObject string, int index, NotProvided l
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, int index, int length) {
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, int index, int length) {
             final ByteList bytes = StringOperations.getByteList(string);
 
             if (length < 0) {
                 return nil();
             }
 
-            final int normalizedIndex = StringOperations.normalizeIndex(string, index);
+            final int stringLength = getSizeNode().executeInteger(frame, string);
+            final int normalizedIndex = StringOperations.normalizeIndex(stringLength, index);
 
             if (normalizedIndex < 0 || normalizedIndex > bytes.length()) {
                 return nil();
@@ -265,13 +267,13 @@ public Object stringByteSubstring(DynamicObject string, int index, int length) {
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, int index, long length) {
-            return stringByteSubstring(string, (long) index, length);
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, int index, long length) {
+            return stringByteSubstring(frame, string, (long) index, length);
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, int index, double length) {
-            return stringByteSubstring(string, index, (int) length);
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, int index, double length) {
+            return stringByteSubstring(frame, string, index, (int) length);
         }
 
         @Specialization
@@ -280,17 +282,17 @@ public Object stringByteSubstring(DynamicObject string, int index, DynamicObject
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, long index, NotProvided length) {
-            return stringByteSubstring(string, index, 1);
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, long index, NotProvided length) {
+            return stringByteSubstring(frame, string, index, 1);
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, long index, int length) {
-            return stringByteSubstring(string, index, (long) length);
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, long index, int length) {
+            return stringByteSubstring(frame, string, index, (long) length);
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, long index, long length) {
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, long index, long length) {
             if (index > Integer.MAX_VALUE || index < Integer.MIN_VALUE) {
                 CompilerDirectives.transferToInterpreter();
                 throw new RaiseException(getContext().getCoreLibrary().argumentError("index out of int range", this));
@@ -299,12 +301,12 @@ public Object stringByteSubstring(DynamicObject string, long index, long length)
                 CompilerDirectives.transferToInterpreter();
                 throw new RaiseException(getContext().getCoreLibrary().argumentError("length out of int range", this));
             }
-            return stringByteSubstring(string, (int) index, (int) length);
+            return stringByteSubstring(frame, string, (int) index, (int) length);
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, long index, double length) {
-            return stringByteSubstring(string, index, (int) length);
+        public Object stringByteSubstring(VirtualFrame frame,DynamicObject string, long index, double length) {
+            return stringByteSubstring(frame, string, index, (int) length);
         }
 
         @Specialization
@@ -313,23 +315,23 @@ public Object stringByteSubstring(DynamicObject string, long index, DynamicObjec
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, double index, NotProvided length) {
-            return stringByteSubstring(string, (int) index, 1);
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, double index, NotProvided length) {
+            return stringByteSubstring(frame, string, (int) index, 1);
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, double index, int length) {
-            return stringByteSubstring(string, (int) index, length);
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, double index, int length) {
+            return stringByteSubstring(frame, string, (int) index, length);
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, double index, long length) {
-            return stringByteSubstring(string, (int) index, length);
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, double index, long length) {
+            return stringByteSubstring(frame, string, (int) index, length);
         }
 
         @Specialization
-        public Object stringByteSubstring(DynamicObject string, double index, double length) {
-            return stringByteSubstring(string, (int) index, (int) length);
+        public Object stringByteSubstring(VirtualFrame frame, DynamicObject string, double index, double length) {
+            return stringByteSubstring(frame, string, (int) index, (int) length);
         }
 
         @Specialization
@@ -347,6 +349,14 @@ public Object stringByteSubstring(DynamicObject string, DynamicObject index, Obj
             return null;
         }
 
+        private StringNodes.SizeNode getSizeNode() {
+            if (sizeNode == null) {
+                CompilerDirectives.transferToInterpreter();
+                sizeNode = insert(StringNodesFactory.SizeNodeFactory.create(getContext(), getSourceSection(), new RubyNode[]{null}));
+            }
+
+            return sizeNode;
+        }
     }
 
     @RubiniusPrimitive(name = "string_check_null_safe", needsSelf = false)
@@ -383,7 +393,7 @@ public StringChrAtPrimitiveNode(RubyContext context, SourceSection sourceSection
 
         @TruffleBoundary
         @Specialization
-        public Object stringChrAt(DynamicObject string, int byteIndex) {
+        public Object stringChrAt(VirtualFrame frame, DynamicObject string, int byteIndex) {
             // Taken from Rubinius's Character::create_from.
 
             final ByteList bytes = StringOperations.getByteList(string);
@@ -416,7 +426,7 @@ public Object stringChrAt(DynamicObject string, int byteIndex) {
                 );
             }
 
-            return stringByteSubstringNode.stringByteSubstring(string, byteIndex, n);
+            return stringByteSubstringNode.stringByteSubstring(frame, string, byteIndex, n);
         }
 
     }
@@ -1191,8 +1201,12 @@ public static abstract class StringSubstringPrimitiveNode extends RubiniusPrimit
         @Child private AllocateObjectNode allocateNode;
         @Child private TaintResultNode taintResultNode;
         private final ConditionProfile negativeLengthProfile = ConditionProfile.createBinaryProfile();
+        private final ConditionProfile emptyStringProfile = ConditionProfile.createBinaryProfile();
         private final ConditionProfile tooLargeBeginProfile = ConditionProfile.createBinaryProfile();
         private final ConditionProfile negativeBeginProfile = ConditionProfile.createBinaryProfile();
+        private final ConditionProfile stillNegativeBeginProfile = ConditionProfile.createBinaryProfile();
+        private final ConditionProfile tooLargeTotalProfile = ConditionProfile.createBinaryProfile();
+        private final ConditionProfile stillNegativeLengthProfile = ConditionProfile.createBinaryProfile();
 
         public StringSubstringPrimitiveNode(RubyContext context, SourceSection sourceSection) {
             super(context, sourceSection);
@@ -1209,27 +1223,27 @@ public Object stringSubstringSingleByteOptimizable(DynamicObject string, int beg
             }
 
             final int length = StringOperations.getByteList(string).getRealSize();
-            if (length == 0) {
+            if (emptyStringProfile.profile(length == 0)) {
                 len = 0;
             }
 
             if (tooLargeBeginProfile.profile(beg > length)) {
                 return nil();
             }
 
-            if (beg < 0) {
+            if (negativeBeginProfile.profile(beg < 0)) {
                 beg += length;
 
-                if (negativeBeginProfile.profile(beg < 0)) {
+                if (stillNegativeBeginProfile.profile(beg < 0)) {
                     return nil();
                 }
             }
 
-            if ((beg + len) > length) {
+            if (tooLargeTotalProfile.profile((beg + len) > length)) {
                 len = length - beg;
             }
 
-            if (len <= 0) {
+            if (stillNegativeLengthProfile.profile(len <= 0)) {
                 len = 0;
                 beg = 0;
             }

diff --git a/truffle/src/main/java/org/jruby/truffle/runtime/core/StringOperations.java b/truffle/src/main/java/org/jruby/truffle/runtime/core/StringOperations.java
@@ -139,26 +139,10 @@ public static void forceEncoding(DynamicObject string, Encoding encoding) {
         clearCodeRange(string);
     }
 
-    public static int length(DynamicObject string) {
-        if (CompilerDirectives.injectBranchProbability(
-                CompilerDirectives.FASTPATH_PROBABILITY,
-                StringSupport.isSingleByteOptimizable(getCodeRangeable(string), StringOperations.getByteList(string).getEncoding()))) {
-
-            return StringOperations.getByteList(string).getRealSize();
-
-        } else {
-            return StringSupport.strLengthFromRubyString(getCodeRangeable(string));
-        }
-    }
-
     public static int normalizeIndex(int length, int index) {
         return ArrayOperations.normalizeIndex(length, index);
     }
 
-    public static int normalizeIndex(DynamicObject rubyString, int index) {
-        return normalizeIndex(length(rubyString), index);
-    }
-
     public static int clampExclusiveIndex(DynamicObject string, int index) {
         assert RubyGuards.isRubyString(string);
         return ArrayOperations.clampExclusiveIndex(StringOperations.getByteList(string).length(), index);