[GR-21660] [GR-26446] Stop rebalancing trees of ConcatRopes on append

PullRequest: truffleruby/2038

Author: Nicolas Laurent

src/main/java/org/truffleruby/core/rope/ConcatRope.java

@@ -12,8 +12,6 @@
 import org.jcodings.Encoding;
 import org.jcodings.specific.ASCIIEncoding;
 
-import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
-
 public class ConcatRope extends ManagedRope {
 
     private final ManagedRope left;
@@ -79,16 +77,6 @@ Rope withBinaryEncoding() {
                 balanced);
     }
 
-    @Override
-    @TruffleBoundary
-    public byte getByteSlow(int index) {
-        if (index < left.byteLength()) {
-            return left.getByteSlow(index);
-        }
-
-        return right.getByteSlow(index - left.byteLength());
-    }
-
     public ManagedRope getLeft() {
         return left;
     }

src/main/java/org/truffleruby/core/rope/ManagedRope.java

@@ -39,6 +39,11 @@ public final int characterLength() {
         return characterLength;
     }
 
+    @Override
+    protected byte getByteSlow(int index) {
+        return RopeOperations.getByteSlow(this, index);
+    }
+
     @Override
     public final byte[] getBytes() {
         if (bytes == null) {

src/main/java/org/truffleruby/core/rope/RepeatingRope.java

@@ -68,11 +68,6 @@ protected byte[] getBytesSlow() {
         return super.getBytesSlow();
     }
 
-    @Override
-    protected byte getByteSlow(int index) {
-        return child.getByteSlow(index % child.byteLength());
-    }
-
     public ManagedRope getChild() {
         return child;
     }

src/main/java/org/truffleruby/core/rope/RopeNodes.java

@@ -19,9 +19,7 @@
 import static org.truffleruby.core.rope.CodeRange.CR_UNKNOWN;
 import static org.truffleruby.core.rope.CodeRange.CR_VALID;
 
-import java.util.ArrayDeque;
 import java.util.Arrays;
-import java.util.Deque;
 
 import org.jcodings.Encoding;
 import org.jcodings.specific.ASCIIEncoding;
@@ -425,8 +423,6 @@ public static ConcatNode create() {
             return RopeNodesFactory.ConcatNodeGen.create();
         }
 
-        @Child private FlattenNode flattenNode;
-
         public abstract Rope executeConcat(Rope left, Rope right, Encoding encoding);
 
         @Specialization
@@ -467,8 +463,7 @@ protected Rope concatRightEmpty(ManagedRope left, Rope right, Encoding encoding,
         @Specialization(guards = { "!left.isEmpty()", "!right.isEmpty()", "!isCodeRangeBroken(left, right)" })
         protected Rope concat(ManagedRope left, ManagedRope right, Encoding encoding,
                 @Cached ConditionProfile sameCodeRangeProfile,
-                @Cached ConditionProfile brokenCodeRangeProfile,
-                @Cached ConditionProfile shouldRebalanceProfile) {
+                @Cached ConditionProfile brokenCodeRangeProfile) {
             try {
                 Math.addExact(left.byteLength(), right.byteLength());
             } catch (ArithmeticException e) {
@@ -477,18 +472,7 @@ protected Rope concat(ManagedRope left, ManagedRope right, Encoding encoding,
                         getContext().getCoreExceptions().argumentErrorTooLargeString(this));
             }
 
-            if (shouldRebalanceProfile.profile(
-                    left.depth() >= getContext().getOptions().ROPE_DEPTH_THRESHOLD && left instanceof ConcatRope)) {
-                left = rebalance((ConcatRope) left, getContext().getOptions().ROPE_DEPTH_THRESHOLD, getFlattenNode());
-            }
-
-            if (shouldRebalanceProfile.profile(
-                    right.depth() >= getContext().getOptions().ROPE_DEPTH_THRESHOLD && right instanceof ConcatRope)) {
-                right = rebalance((ConcatRope) right, getContext().getOptions().ROPE_DEPTH_THRESHOLD, getFlattenNode());
-            }
-
             int depth = depth(left, right);
-            /* if (depth >= 10) { System.out.println("ConcatRope depth: " + depth); } */
 
             return new ConcatRope(
                     left,
@@ -503,14 +487,6 @@ protected Rope concat(ManagedRope left, ManagedRope right, Encoding encoding,
                     isBalanced(left, right));
         }
 
-        private FlattenNode getFlattenNode() {
-            if (flattenNode == null) {
-                CompilerDirectives.transferToInterpreterAndInvalidate();
-                flattenNode = insert(FlattenNode.create());
-            }
-            return flattenNode;
-        }
-
         private boolean isBalanced(Rope left, Rope right) {
             // Our definition of balanced is centered around the notion of rebalancing. We could have a simple structure
             // such as ConcatRope(ConcatRope(LeafRope, LeafRope), LeafRope) that is balanced on its own but may contribute
@@ -532,81 +508,6 @@ private boolean isBalanced(Rope left, Rope right) {
             }
         }
 
-        @TruffleBoundary
-        private ManagedRope rebalance(ConcatRope rope, int depthThreshold, FlattenNode flattenNode) {
-            Deque<ManagedRope> currentRopeQueue = new ArrayDeque<>();
-            Deque<ManagedRope> nextLevelQueue = new ArrayDeque<>();
-
-            linearizeTree(rope.getLeft(), currentRopeQueue);
-            linearizeTree(rope.getRight(), currentRopeQueue);
-
-            final int flattenThreshold = depthThreshold / 2;
-
-            ManagedRope root = null;
-            while (!currentRopeQueue.isEmpty()) {
-                ManagedRope left = currentRopeQueue.pop();
-
-                if (left.depth() >= flattenThreshold) {
-                    // TODO (eregon, 15 June 2020): should this cache the resulting bytes by using getBytes()?
-                    left = flattenNode.executeFlatten(left);
-                }
-
-                if (currentRopeQueue.isEmpty()) {
-                    if (nextLevelQueue.isEmpty()) {
-                        root = left;
-                    } else {
-                        // If a rope can't be paired with another rope at the current level (i.e., odd numbers of ropes),
-                        // it needs to be promoted to the next level where it will be tried again. Since by definition
-                        // every rope already present in the next level must have occurred before this rope in the current
-                        // level, this rope must be added to the end of the list in the next level to maintain proper
-                        // position.
-                        nextLevelQueue.add(left);
-                    }
-                } else {
-                    ManagedRope right = currentRopeQueue.pop();
-
-                    if (right.depth() >= flattenThreshold) {
-                        // TODO (eregon, 15 June 2020): should this cache the resulting bytes by using getBytes()?
-                        right = flattenNode.executeFlatten(right);
-                    }
-
-                    final ManagedRope child = new ConcatRope(
-                            left,
-                            right,
-                            rope.getEncoding(),
-                            commonCodeRange(left.getCodeRange(), right.getCodeRange()),
-                            depth(left, right),
-                            isBalanced(left, right));
-
-                    nextLevelQueue.add(child);
-                }
-
-                if (currentRopeQueue.isEmpty() && !nextLevelQueue.isEmpty()) {
-                    currentRopeQueue = nextLevelQueue;
-                    nextLevelQueue = new ArrayDeque<>();
-                }
-            }
-
-            return root;
-        }
-
-        @TruffleBoundary
-        private void linearizeTree(ManagedRope rope, Deque<ManagedRope> ropeQueue) {
-            if (rope instanceof ConcatRope) {
-                final ConcatRope concatRope = (ConcatRope) rope;
-
-                // If a rope is known to be balanced, there's no need to rebalance it.
-                if (concatRope.isBalanced()) {
-                    ropeQueue.add(concatRope);
-                } else {
-                    linearizeTree(concatRope.getLeft(), ropeQueue);
-                    linearizeTree(concatRope.getRight(), ropeQueue);
-                }
-            } else {
-                // We never rebalance non-ConcatRopes since that requires per-rope type logic with likely minimal benefit.
-                ropeQueue.add(rope);
-            }
-        }
 
         @SuppressFBWarnings("RV")
         @Specialization(guards = { "!left.isEmpty()", "!right.isEmpty()", "isCodeRangeBroken(left, right)" })

src/main/java/org/truffleruby/core/rope/RopeOperations.java

@@ -497,6 +497,36 @@ public static byte[] flattenBytes(Rope rope) {
         return buffer;
     }
 
+    /** Used to implement {@link Rope#getByteSlow(int)} in a non-recursive fashion for some Rope subclasses. Do not call
+     * directly, call {@link Rope#getByteSlow(int) instead.} */
+    @TruffleBoundary
+    static byte getByteSlow(Rope rope, int index) {
+        while (true) {
+            final byte[] rawBytes = rope.getRawBytes();
+            if (rawBytes != null) {
+                return rawBytes[index];
+            } else if (rope instanceof ConcatRope) {
+                final ConcatRope concatRope = (ConcatRope) rope;
+                final Rope left = concatRope.getLeft();
+                if (index < left.byteLength()) {
+                    rope = left;
+                } else {
+                    rope = concatRope.getRight();
+                    index -= left.byteLength();
+                }
+            } else if (rope instanceof SubstringRope) {
+                final SubstringRope substringRope = (SubstringRope) rope;
+                rope = substringRope.getChild();
+                index += substringRope.getByteOffset();
+            } else if (rope instanceof RepeatingRope) {
+                rope = ((RepeatingRope) rope).getChild();
+                index %= rope.byteLength();
+            } else {
+                return rope.getByteSlow(index);
+            }
+        }
+    }
+
     private static int computeLoopCount(int offset, int times, int length, int patternLength) {
         // The loopCount has to be precisely determined so every repetition has at least some parts used.
         // It has to account for the beginning we don't need (offset), has to reach the end but, and must not
@@ -508,74 +538,92 @@ private static int computeLoopCount(int offset, int times, int length, int patte
 
     @TruffleBoundary
     public static int hashForRange(Rope rope, int startingHashCode, int offset, int length) {
-        if (rope instanceof LeafRope) {
-            return Hashing.stringHash(rope.getBytes(), startingHashCode, offset, length);
-        } else if (rope instanceof SubstringRope) {
-            final SubstringRope substringRope = (SubstringRope) rope;
-
-            return hashForRange(
-                    substringRope.getChild(),
-                    startingHashCode,
-                    offset + substringRope.getByteOffset(),
-                    length);
-        } else if (rope instanceof ConcatRope) {
-            final ConcatRope concatRope = (ConcatRope) rope;
-            final Rope left = concatRope.getLeft();
-            final Rope right = concatRope.getRight();
-
-            int hash = startingHashCode;
-            final int leftLength = left.byteLength();
+        class Params {
+            final Rope rope;
+            final int startingHashCode;
+            final int offset;
+            final int length;
+            final boolean readResult;
+            final int repeatCount;
+
+            Params(Rope rope, int startingHashCode, int offset, int length, boolean readResult, int repeatCount) {
+                this.rope = rope;
+                this.startingHashCode = startingHashCode;
+                this.offset = offset;
+                this.length = length;
+                this.readResult = readResult;
+                this.repeatCount = repeatCount;
+            }
+        }
 
-            if (offset < leftLength) {
-                // The left branch might not be large enough to extract the full hash code we want. In that case,
-                // we'll extract what we can and extract the difference from the right side.
-                if (offset + length > leftLength) {
-                    final int coveredByLeft = leftLength - offset;
-                    hash = hashForRange(left, hash, offset, coveredByLeft);
-                    hash = hashForRange(right, hash, 0, length - coveredByLeft);
+        final Deque<Params> workStack = new ArrayDeque<>();
+        workStack.push(new Params(rope, startingHashCode, offset, length, false, -1));
+        int resultHash = 0;
 
-                    return hash;
+        while (!workStack.isEmpty()) {
+            final Params params = workStack.pop();
+            rope = params.rope;
+            startingHashCode = params.readResult ? resultHash : params.startingHashCode;
+            offset = params.offset;
+            length = params.length;
+            final byte[] bytes = rope.getRawBytes();
+
+            if (bytes != null) {
+                resultHash = Hashing.stringHash(bytes, startingHashCode, offset, length);
+            } else if (rope instanceof SubstringRope) {
+                final SubstringRope substringRope = (SubstringRope) rope;
+                workStack.push(
+                        new Params(
+                                substringRope.getChild(),
+                                startingHashCode,
+                                offset + substringRope.getByteOffset(),
+                                length,
+                                false,
+                                -1));
+            } else if (rope instanceof ConcatRope) {
+                final ConcatRope concatRope = (ConcatRope) rope;
+                final Rope left = concatRope.getLeft();
+                final Rope right = concatRope.getRight();
+                final int leftLength = left.byteLength();
+
+                if (offset >= leftLength) {
+                    // range fully contained in right child
+                    workStack.push(new Params(right, startingHashCode, offset - leftLength, length, false, -1));
+                } else if (offset + length <= leftLength) {
+                    // range fully contained in left child
+                    workStack.push(new Params(left, startingHashCode, offset, length, false, -1));
                 } else {
-                    return hashForRange(left, hash, offset, length);
+                    final int coveredByLeft = leftLength - offset;
+                    // push right node first, starting hash is the result from the left node
+                    workStack.push(new Params(right, 0, 0, length - coveredByLeft, true, -1));
+                    workStack.push(new Params(left, startingHashCode, offset, coveredByLeft, false, -1));
                 }
-            }
-
-            return hashForRange(right, hash, offset - leftLength, length);
-        } else if (rope instanceof RepeatingRope) {
-            final RepeatingRope repeatingRope = (RepeatingRope) rope;
-            final Rope child = repeatingRope.getChild();
-
-            int bytesToHash = length;
-            final int patternLength = child.byteLength();
-
-            // Fix the offset to be appropriate for a given child. The offset is reset the first time it is
-            // consumed, so there's no need to worry about adversely affecting anything by adjusting it here.
-            offset %= child.byteLength();
+            } else if (rope instanceof RepeatingRope) {
+                final RepeatingRope repeatingRope = (RepeatingRope) rope;
+                final Rope child = repeatingRope.getChild();
+                final int patternLength = child.byteLength();
 
-            final int loopCount = computeLoopCount(offset, repeatingRope.getTimes(), bytesToHash, patternLength);
+                int count = params.repeatCount;
+                assert count != 0;
+                if (count < 0) {
+                    offset %= patternLength; // the offset is always 0 when count > 0
+                    count = computeLoopCount(offset, repeatingRope.getTimes(), length, patternLength);
+                }
 
-            int hash = startingHashCode;
-            for (int i = 0; i < loopCount; i++) {
-                final int bytesToHashThisPass = bytesToHash + offset > patternLength
+                if (count > 1) {
+                    // loop - 1 iteration, reset offset to 0, starting hash is the result from previous iteration
+                    workStack.push(new Params(rope, 0, 0, length - patternLength, true, count - 1));
+                }
+                // one iteration
+                final int bytesToHashThisPass = length + offset > patternLength
                         ? patternLength - offset
-                        : bytesToHash;
-                hash = hashForRange(
-                        child,
-                        hash,
-                        offset,
-                        bytesToHashThisPass);
-                offset = 0;
-                bytesToHash -= bytesToHashThisPass;
+                        : length;
+                workStack.push(new Params(child, startingHashCode, offset, bytesToHashThisPass, false, -1));
+            } else {
+                resultHash = Hashing.stringHash(rope.getBytes(), startingHashCode, offset, length);
             }
-
-            return hash;
-        } else if (rope instanceof LazyIntRope) {
-            return Hashing.stringHash(rope.getBytes(), startingHashCode, offset, length);
-        } else if (rope instanceof NativeRope) {
-            return Hashing.stringHash(rope.getBytes(), startingHashCode, offset, length);
-        } else {
-            throw new RuntimeException("Hash code not supported for rope of type: " + rope.getClass().getName());
         }
+        return resultHash;
     }
 
     public static RopeBuilder toRopeBuilderCopy(Rope rope) {

src/main/java/org/truffleruby/core/rope/SubstringRope.java

@@ -80,11 +80,6 @@ protected byte[] getBytesSlow() {
         return super.getBytesSlow();
     }
 
-    @Override
-    public byte getByteSlow(int index) {
-        return child.getByteSlow(index + byteOffset);
-    }
-
     public ManagedRope getChild() {
         return child;
     }