[GR-45118] Remove ArrayBuilderNode's expectedLength as it causes too many deoptimizations

* It would transferToInterpreterAndInvalidate() whenever it saw a
  bigger array at a given call site. But the only advantage of expectedLength
  is to avoid the branch for growing the array, which we can just BranchProfile
  and is very rare anyway given a good start(length).
* Remove ArrayBuilderNode#start() without length as that is now unused.

Author: eregon

CHANGELOG.md

@@ -127,6 +127,7 @@ Performance:
 * Splitting (copying) of call targets has been optimized by implementing `cloneUninitialized()` (@andrykonchin, @eregon).
 * `Process.pid` is now cached per process like `$$` (#2882, @horakivo)
 * Use the system `libyaml` for `psych` to improve warmup when parsing YAML (#2089, @eregon).
+* Fixed repeated deoptimizations for methods building an `Array` which is growing over multiple calls at a given call site (@eregon).
 
 Changes:
 

src/main/java/org/truffleruby/core/array/ArrayBuilderNode.java

@@ -9,7 +9,9 @@
  */
 package org.truffleruby.core.array;
 
+import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.NeverDefault;
+import com.oracle.truffle.api.profiles.BranchProfile;
 import org.truffleruby.core.array.library.ArrayStoreLibrary;
 import org.truffleruby.core.array.library.ArrayStoreLibrary.ArrayAllocator;
 import org.truffleruby.core.array.ArrayBuilderNodeFactory.AppendArrayNodeGen;
@@ -48,8 +50,6 @@ public static ArrayBuilderNode create() {
         return new ArrayBuilderProxyNode();
     }
 
-    public abstract BuilderState start();
-
     public abstract BuilderState start(int length);
 
     public abstract void appendArray(BuilderState state, int index, RubyArray array);
@@ -60,15 +60,10 @@ public static ArrayBuilderNode create() {
 
     private static class ArrayBuilderProxyNode extends ArrayBuilderNode {
 
-        @Child StartNode startNode = new StartNode(ArrayStoreLibrary.initialAllocator(false), 0);
+        @Child StartNode startNode = new StartNode(ArrayStoreLibrary.initialAllocator(false));
         @Child AppendArrayNode appendArrayNode;
         @Child AppendOneNode appendOneNode;
 
-        @Override
-        public BuilderState start() {
-            return startNode.start();
-        }
-
         @Override
         public BuilderState start(int length) {
             return startNode.start(length);
@@ -106,7 +101,7 @@ private AppendOneNode getAppendOneNode() {
             return appendOneNode;
         }
 
-        public synchronized ArrayAllocator updateStrategy(ArrayStoreLibrary.ArrayAllocator newStrategy, int newLength) {
+        public synchronized ArrayAllocator updateStrategy(ArrayStoreLibrary.ArrayAllocator newStrategy) {
             final ArrayStoreLibrary.ArrayAllocator oldStrategy = startNode.allocator;
             final ArrayStoreLibrary.ArrayAllocator updatedAllocator;
             // If two threads have raced to update the strategy then
@@ -122,11 +117,8 @@ public synchronized ArrayAllocator updateStrategy(ArrayStoreLibrary.ArrayAllocat
                 updatedAllocator = oldStrategy;
             }
 
-            final int oldLength = startNode.expectedLength;
-            final int newExpectedLength = Math.max(oldLength, newLength);
-
-            if (updatedAllocator != oldStrategy || newExpectedLength > oldLength) {
-                startNode.replace(new StartNode(updatedAllocator, newExpectedLength));
+            if (updatedAllocator != oldStrategy) {
+                startNode.replace(new StartNode(updatedAllocator));
             }
 
             if (newStrategy != oldStrategy) {
@@ -145,35 +137,21 @@ public synchronized ArrayAllocator updateStrategy(ArrayStoreLibrary.ArrayAllocat
 
     public abstract static class ArrayBuilderBaseNode extends RubyBaseNode {
 
-        protected ArrayAllocator replaceNodes(ArrayStoreLibrary.ArrayAllocator strategy, int size) {
+        protected ArrayAllocator replaceNodes(ArrayStoreLibrary.ArrayAllocator strategy) {
             final ArrayBuilderProxyNode parent = (ArrayBuilderProxyNode) getParent();
-            return parent.updateStrategy(strategy, size);
+            return parent.updateStrategy(strategy);
         }
     }
 
     public static class StartNode extends ArrayBuilderBaseNode {
 
         private final ArrayStoreLibrary.ArrayAllocator allocator;
-        private final int expectedLength;
 
-        public StartNode(ArrayStoreLibrary.ArrayAllocator allocator, int expectedLength) {
+        public StartNode(ArrayStoreLibrary.ArrayAllocator allocator) {
             this.allocator = allocator;
-            this.expectedLength = expectedLength;
-        }
-
-        public BuilderState start() {
-            if (allocator == ArrayStoreLibrary.initialAllocator(false)) {
-                return new BuilderState(allocator.allocate(0), expectedLength);
-            } else {
-                return new BuilderState(allocator.allocate(expectedLength), expectedLength);
-            }
         }
 
         public BuilderState start(int length) {
-            if (length > expectedLength) {
-                CompilerDirectives.transferToInterpreterAndInvalidate();
-                replaceNodes(allocator, length);
-            }
             if (allocator == ArrayStoreLibrary.initialAllocator(false)) {
                 return new BuilderState(allocator.allocate(0), length);
             } else {
@@ -196,15 +174,15 @@ public static AppendOneNode create() {
                 limit = "1")
         protected void appendCompatibleType(BuilderState state, int index, Object value,
                 @Bind("state.store") Object store,
-                @CachedLibrary("store") ArrayStoreLibrary arrays) {
+                @CachedLibrary("store") ArrayStoreLibrary arrays,
+                @Cached BranchProfile growProfile) {
             assert state.nextIndex == index;
             final int length = arrays.capacity(state.store);
             if (index >= length) {
-                CompilerDirectives.transferToInterpreterAndInvalidate();
+                growProfile.enter();
                 final int capacity = ArrayUtils.capacityForOneMore(getLanguage(), length);
                 state.store = arrays.expand(state.store, capacity);
                 state.capacity = capacity;
-                replaceNodes(arrays.unsharedAllocator(state.store), capacity);
             }
             arrays.write(state.store, index, value);
             state.nextIndex++;
@@ -229,7 +207,7 @@ protected void appendNewStrategy(BuilderState state, int index, Object value,
                 neededCapacity = currentCapacity;
             }
 
-            newAllocator = replaceNodes(newAllocator, neededCapacity);
+            newAllocator = replaceNodes(newAllocator);
 
             final Object newStore = newAllocator.allocate(neededCapacity);
             stores.copyContents(state.store, 0, newStore, 0, index);
@@ -258,15 +236,15 @@ protected void appendCompatibleStrategy(BuilderState state, int index, RubyArray
                 @Bind("state.store") Object store,
                 @Bind("other.getStore()") Object otherStore,
                 @CachedLibrary("store") ArrayStoreLibrary arrays,
-                @CachedLibrary("otherStore") ArrayStoreLibrary others) {
+                @CachedLibrary("otherStore") ArrayStoreLibrary others,
+                @Cached BranchProfile growProfile) {
             assert state.nextIndex == index;
             final int otherSize = other.size;
             final int neededSize = index + otherSize;
 
             int length = arrays.capacity(state.store);
             if (neededSize > length) {
-                CompilerDirectives.transferToInterpreterAndInvalidate();
-                replaceNodes(arrays.unsharedAllocator(state.store), neededSize);
+                growProfile.enter();
                 final int capacity = ArrayUtils.capacity(getLanguage(), length, neededSize);
                 state.store = arrays.expand(state.store, capacity);
                 state.capacity = capacity;
@@ -300,8 +278,7 @@ protected void appendNewStrategy(BuilderState state, int index, RubyArray other,
                 }
 
                 ArrayAllocator allocator = replaceNodes(
-                        newArrayLibrary.generalizeForStore(state.store, other.getStore()),
-                        neededCapacity);
+                        newArrayLibrary.generalizeForStore(state.store, other.getStore()));
                 newStore = allocator.allocate(neededCapacity);
 
                 newArrayLibrary.copyContents(state.store, 0, newStore, 0, index);

src/test/java/org/truffleruby/ArrayBuilderTest.java

@@ -17,6 +17,7 @@
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.Reader;
+import java.util.Arrays;
 
 import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.nodes.Node;
@@ -31,6 +32,7 @@
 import org.truffleruby.core.array.RubyArray;
 import org.truffleruby.core.array.ArrayBuilderNode.BuilderState;
 import org.truffleruby.core.array.library.ArrayStoreLibrary;
+import org.truffleruby.language.Nil;
 import org.truffleruby.shared.TruffleRuby;
 
 public class ArrayBuilderTest {
@@ -105,6 +107,22 @@ public void arrayBuilderAppendObjectTest() {
         });
     }
 
+    @Test
+    public void arrayBuilderAppendGrowTest() {
+        testInContext(() -> {
+            ArrayBuilderNode builder = createBuilder();
+            BuilderState state = builder.start(10);
+            for (int i = 0; i < 12; i++) {
+                builder.appendValue(state, i, Nil.INSTANCE);
+            }
+            Object[] result = (Object[]) builder.finish(state, 12);
+            for (int i = 0; i < 12; i++) {
+                Object e = result[i];
+                assertEquals(Nil.INSTANCE, e);
+            }
+        });
+    }
+
     @Test
     public void arrayBuilderAppendEmptyArrayTest() {
         testInContext(() -> {
@@ -180,6 +198,28 @@ public void arrayBuilderAppendObjectArrayTest() {
         });
     }
 
+    @Test
+    public void arrayBuilderAppendGrowArrayTest() {
+        testInContext(() -> {
+            ArrayBuilderNode builder = createBuilder();
+            BuilderState state = builder.start(10);
+            Object[] array = new Object[6];
+            Arrays.fill(array, Nil.INSTANCE);
+            RubyArray otherStore = new RubyArray(
+                    RubyLanguage.getCurrentContext().getCoreLibrary().arrayClass,
+                    RubyLanguage.getCurrentLanguage().arrayShape,
+                    array,
+                    array.length);
+            builder.appendArray(state, 0, otherStore);
+            builder.appendArray(state, 6, otherStore);
+            Object[] result = (Object[]) builder.finish(state, 12);
+            for (int i = 0; i < 12; i++) {
+                Object e = result[i];
+                assertEquals(Nil.INSTANCE, e);
+            }
+        });
+    }
+
     private ArrayBuilderNode createBuilder() {
         return adopt(ArrayBuilderNode.create());
     }