Skip to content

Commit c4d815c

Browse files
eregoneregon
committed
[GR-45118] Remove ArrayBuilderNode's expectedLength as it causes too many deoptimizations
PullRequest: truffleruby/3733
2 parents d3c18a6 + 92b873d commit c4d815c

File tree

5 files changed

+91
-90
lines changed

5 files changed

+91
-90
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ Performance:
127127
* Splitting (copying) of call targets has been optimized by implementing `cloneUninitialized()` (@andrykonchin, @eregon).
128128
* `Process.pid` is now cached per process like `$$` (#2882, @horakivo)
129129
* Use the system `libyaml` for `psych` to improve warmup when parsing YAML (#2089, @eregon).
130+
* Fixed repeated deoptimizations for methods building an `Array` which is growing over multiple calls at a given call site (@eregon).
130131

131132
Changes:
132133

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

Lines changed: 17 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99
*/
1010
package org.truffleruby.core.array;
1111

12+
import com.oracle.truffle.api.dsl.Cached;
1213
import com.oracle.truffle.api.dsl.NeverDefault;
14+
import com.oracle.truffle.api.profiles.BranchProfile;
1315
import org.truffleruby.core.array.library.ArrayStoreLibrary;
1416
import org.truffleruby.core.array.library.ArrayStoreLibrary.ArrayAllocator;
1517
import org.truffleruby.core.array.ArrayBuilderNodeFactory.AppendArrayNodeGen;
@@ -48,8 +50,6 @@ public static ArrayBuilderNode create() {
4850
return new ArrayBuilderProxyNode();
4951
}
5052

51-
public abstract BuilderState start();
52-
5353
public abstract BuilderState start(int length);
5454

5555
public abstract void appendArray(BuilderState state, int index, RubyArray array);
@@ -60,15 +60,10 @@ public static ArrayBuilderNode create() {
6060

6161
private static class ArrayBuilderProxyNode extends ArrayBuilderNode {
6262

63-
@Child StartNode startNode = new StartNode(ArrayStoreLibrary.initialAllocator(false), 0);
63+
@Child StartNode startNode = new StartNode(ArrayStoreLibrary.initialAllocator(false));
6464
@Child AppendArrayNode appendArrayNode;
6565
@Child AppendOneNode appendOneNode;
6666

67-
@Override
68-
public BuilderState start() {
69-
return startNode.start();
70-
}
71-
7267
@Override
7368
public BuilderState start(int length) {
7469
return startNode.start(length);
@@ -106,7 +101,7 @@ private AppendOneNode getAppendOneNode() {
106101
return appendOneNode;
107102
}
108103

109-
public synchronized ArrayAllocator updateStrategy(ArrayStoreLibrary.ArrayAllocator newStrategy, int newLength) {
104+
public synchronized ArrayAllocator updateStrategy(ArrayStoreLibrary.ArrayAllocator newStrategy) {
110105
final ArrayStoreLibrary.ArrayAllocator oldStrategy = startNode.allocator;
111106
final ArrayStoreLibrary.ArrayAllocator updatedAllocator;
112107
// If two threads have raced to update the strategy then
@@ -122,11 +117,8 @@ public synchronized ArrayAllocator updateStrategy(ArrayStoreLibrary.ArrayAllocat
122117
updatedAllocator = oldStrategy;
123118
}
124119

125-
final int oldLength = startNode.expectedLength;
126-
final int newExpectedLength = Math.max(oldLength, newLength);
127-
128-
if (updatedAllocator != oldStrategy || newExpectedLength > oldLength) {
129-
startNode.replace(new StartNode(updatedAllocator, newExpectedLength));
120+
if (updatedAllocator != oldStrategy) {
121+
startNode.replace(new StartNode(updatedAllocator));
130122
}
131123

132124
if (newStrategy != oldStrategy) {
@@ -145,35 +137,21 @@ public synchronized ArrayAllocator updateStrategy(ArrayStoreLibrary.ArrayAllocat
145137

146138
public abstract static class ArrayBuilderBaseNode extends RubyBaseNode {
147139

148-
protected ArrayAllocator replaceNodes(ArrayStoreLibrary.ArrayAllocator strategy, int size) {
140+
protected ArrayAllocator replaceNodes(ArrayStoreLibrary.ArrayAllocator strategy) {
149141
final ArrayBuilderProxyNode parent = (ArrayBuilderProxyNode) getParent();
150-
return parent.updateStrategy(strategy, size);
142+
return parent.updateStrategy(strategy);
151143
}
152144
}
153145

154146
public static class StartNode extends ArrayBuilderBaseNode {
155147

156148
private final ArrayStoreLibrary.ArrayAllocator allocator;
157-
private final int expectedLength;
158149

159-
public StartNode(ArrayStoreLibrary.ArrayAllocator allocator, int expectedLength) {
150+
public StartNode(ArrayStoreLibrary.ArrayAllocator allocator) {
160151
this.allocator = allocator;
161-
this.expectedLength = expectedLength;
162-
}
163-
164-
public BuilderState start() {
165-
if (allocator == ArrayStoreLibrary.initialAllocator(false)) {
166-
return new BuilderState(allocator.allocate(0), expectedLength);
167-
} else {
168-
return new BuilderState(allocator.allocate(expectedLength), expectedLength);
169-
}
170152
}
171153

172154
public BuilderState start(int length) {
173-
if (length > expectedLength) {
174-
CompilerDirectives.transferToInterpreterAndInvalidate();
175-
replaceNodes(allocator, length);
176-
}
177155
if (allocator == ArrayStoreLibrary.initialAllocator(false)) {
178156
return new BuilderState(allocator.allocate(0), length);
179157
} else {
@@ -196,15 +174,15 @@ public static AppendOneNode create() {
196174
limit = "1")
197175
protected void appendCompatibleType(BuilderState state, int index, Object value,
198176
@Bind("state.store") Object store,
199-
@CachedLibrary("store") ArrayStoreLibrary arrays) {
177+
@CachedLibrary("store") ArrayStoreLibrary arrays,
178+
@Cached BranchProfile growProfile) {
200179
assert state.nextIndex == index;
201180
final int length = arrays.capacity(state.store);
202181
if (index >= length) {
203-
CompilerDirectives.transferToInterpreterAndInvalidate();
182+
growProfile.enter();
204183
final int capacity = ArrayUtils.capacityForOneMore(getLanguage(), length);
205184
state.store = arrays.expand(state.store, capacity);
206185
state.capacity = capacity;
207-
replaceNodes(arrays.unsharedAllocator(state.store), capacity);
208186
}
209187
arrays.write(state.store, index, value);
210188
state.nextIndex++;
@@ -229,7 +207,7 @@ protected void appendNewStrategy(BuilderState state, int index, Object value,
229207
neededCapacity = currentCapacity;
230208
}
231209

232-
newAllocator = replaceNodes(newAllocator, neededCapacity);
210+
newAllocator = replaceNodes(newAllocator);
233211

234212
final Object newStore = newAllocator.allocate(neededCapacity);
235213
stores.copyContents(state.store, 0, newStore, 0, index);
@@ -258,15 +236,15 @@ protected void appendCompatibleStrategy(BuilderState state, int index, RubyArray
258236
@Bind("state.store") Object store,
259237
@Bind("other.getStore()") Object otherStore,
260238
@CachedLibrary("store") ArrayStoreLibrary arrays,
261-
@CachedLibrary("otherStore") ArrayStoreLibrary others) {
239+
@CachedLibrary("otherStore") ArrayStoreLibrary others,
240+
@Cached BranchProfile growProfile) {
262241
assert state.nextIndex == index;
263242
final int otherSize = other.size;
264243
final int neededSize = index + otherSize;
265244

266245
int length = arrays.capacity(state.store);
267246
if (neededSize > length) {
268-
CompilerDirectives.transferToInterpreterAndInvalidate();
269-
replaceNodes(arrays.unsharedAllocator(state.store), neededSize);
247+
growProfile.enter();
270248
final int capacity = ArrayUtils.capacity(getLanguage(), length, neededSize);
271249
state.store = arrays.expand(state.store, capacity);
272250
state.capacity = capacity;
@@ -300,8 +278,7 @@ protected void appendNewStrategy(BuilderState state, int index, RubyArray other,
300278
}
301279

302280
ArrayAllocator allocator = replaceNodes(
303-
newArrayLibrary.generalizeForStore(state.store, other.getStore()),
304-
neededCapacity);
281+
newArrayLibrary.generalizeForStore(state.store, other.getStore()));
305282
newStore = allocator.allocate(neededCapacity);
306283

307284
newArrayLibrary.copyContents(state.store, 0, newStore, 0, index);

‎src/main/java/org/truffleruby/core/array/ArrayConcatNode.java

Lines changed: 23 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -32,55 +32,45 @@ public ArrayConcatNode(RubyNode[] children) {
3232
this.children = children;
3333
}
3434

35+
@ExplodeLoop
3536
@Override
3637
public RubyArray execute(VirtualFrame frame) {
3738
if (arrayBuilderNode == null) {
3839
CompilerDirectives.transferToInterpreterAndInvalidate();
3940
arrayBuilderNode = insert(ArrayBuilderNode.create());
4041
}
41-
if (children.length == 1) {
42-
return executeSingle(frame);
43-
} else {
44-
return executeMultiple(frame);
45-
}
46-
}
4742

48-
private RubyArray executeSingle(VirtualFrame frame) {
49-
BuilderState state = arrayBuilderNode.start();
50-
final Object childObject = children[0].execute(frame);
51-
52-
final int size;
53-
if (isArrayProfile.profile(childObject instanceof RubyArray)) {
54-
final RubyArray childArray = (RubyArray) childObject;
55-
size = childArray.size;
56-
arrayBuilderNode.appendArray(state, 0, childArray);
57-
} else {
58-
size = 1;
59-
arrayBuilderNode.appendValue(state, 0, childObject);
43+
// Compute the total length
44+
int totalLength = 0;
45+
final Object[] values = new Object[children.length];
46+
for (int n = 0; n < children.length; n++) {
47+
final Object value = values[n] = children[n].execute(frame);
48+
49+
if (isArrayProfile.profile(value instanceof RubyArray)) {
50+
totalLength += ((RubyArray) value).size;
51+
} else {
52+
totalLength++;
53+
}
6054
}
61-
return createArray(arrayBuilderNode.finish(state, size), size);
62-
}
6355

64-
@ExplodeLoop
65-
private RubyArray executeMultiple(VirtualFrame frame) {
66-
BuilderState state = arrayBuilderNode.start();
67-
int length = 0;
56+
// Create a builder with the right length and append values
57+
BuilderState state = arrayBuilderNode.start(totalLength);
58+
int index = 0;
6859

6960
for (int n = 0; n < children.length; n++) {
70-
final Object childObject = children[n].execute(frame);
61+
final Object value = values[n];
7162

72-
if (isArrayProfile.profile(childObject instanceof RubyArray)) {
73-
final RubyArray childArray = (RubyArray) childObject;
74-
final int size = childArray.size;
75-
arrayBuilderNode.appendArray(state, length, childArray);
76-
length += size;
63+
if (isArrayProfile.profile(value instanceof RubyArray)) {
64+
final RubyArray childArray = (RubyArray) value;
65+
arrayBuilderNode.appendArray(state, index, childArray);
66+
index += childArray.size;
7767
} else {
78-
arrayBuilderNode.appendValue(state, length, childObject);
79-
length++;
68+
arrayBuilderNode.appendValue(state, index, value);
69+
index++;
8070
}
8171
}
8272

83-
return createArray(arrayBuilderNode.finish(state, length), length);
73+
return createArray(arrayBuilderNode.finish(state, index), index);
8474
}
8575

8676
@ExplodeLoop

‎src/main/java/org/truffleruby/core/regexp/TruffleRegexpNodes.java

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -419,23 +419,20 @@ protected <T> RubyArray fillinInstrumentData(Map<T, AtomicInteger> map, ArrayBui
419419
BuilderState state = arrayBuilderNode.start(arraySize);
420420
int n = 0;
421421
for (Entry<T, AtomicInteger> e : map.entrySet()) {
422-
arrayBuilderNode
423-
.appendValue(state, n++,
424-
StringOperations.createUTF8String(context, getLanguage(), e.getKey().toString()));
422+
arrayBuilderNode.appendValue(state, n++,
423+
StringOperations.createUTF8String(context, getLanguage(), e.getKey().toString()));
425424
arrayBuilderNode.appendValue(state, n++, e.getValue().get());
426425
}
427426
return createArray(arrayBuilderNode.finish(state, n), n);
428427
}
429428

430429
@TruffleBoundary
431-
protected <T> RubyArray fillinInstrumentData(Set<T> map, ArrayBuilderNode arrayBuilderNode,
432-
RubyContext context) {
430+
protected <T> RubyArray fillinInstrumentData(Set<T> map, ArrayBuilderNode arrayBuilderNode) {
433431
final int arraySize = (LITERAL_REGEXPS.size() + DYNAMIC_REGEXPS.size()) * 2;
434432
BuilderState state = arrayBuilderNode.start(arraySize);
435433
int n = 0;
436434
for (T e : map) {
437-
arrayBuilderNode
438-
.appendValue(state, n++, e);
435+
arrayBuilderNode.appendValue(state, n++, e);
439436
arrayBuilderNode.appendValue(state, n++, 1);
440437
}
441438
return createArray(arrayBuilderNode.finish(state, n), n);
@@ -480,8 +477,7 @@ protected Object buildStatsArray(boolean literalRegexps,
480477
@Cached ArrayBuilderNode arrayBuilderNode) {
481478
return fillinInstrumentData(
482479
literalRegexps ? LITERAL_REGEXPS : DYNAMIC_REGEXPS,
483-
arrayBuilderNode,
484-
getContext());
480+
arrayBuilderNode);
485481
}
486482
}
487483

@@ -503,23 +499,20 @@ public abstract static class UnusedRegexpsArray extends RegexpStatsNode {
503499

504500
@TruffleBoundary
505501
@Specialization
506-
protected Object buildUnusedRegexpsArray(
507-
@Cached ArrayBuilderNode arrayBuilderNode) {
502+
protected Object buildUnusedRegexpsArray() {
508503
final Set<RubyRegexp> compiledRegexps = allCompiledRegexps();
509504
final Set<RubyRegexp> matchedRegexps = allMatchedRegexps();
510505

511506
final Set<RubyRegexp> unusedRegexps = new HashSet<>(compiledRegexps);
512507
unusedRegexps.removeAll(matchedRegexps);
513508

514-
final BuilderState state = arrayBuilderNode.start(unusedRegexps.size());
509+
Object[] array = new Object[unusedRegexps.size()];
515510
int n = 0;
516511
for (RubyRegexp entry : unusedRegexps) {
517-
arrayBuilderNode
518-
.appendValue(state, n++,
519-
StringOperations.createUTF8String(getContext(), getLanguage(), entry.toString()));
512+
array[n++] = StringOperations.createUTF8String(getContext(), getLanguage(), entry.toString());
520513
}
521514

522-
return createArray(arrayBuilderNode.finish(state, n), n);
515+
return createArray(array);
523516
}
524517
}
525518

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

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import java.io.InputStream;
1818
import java.io.InputStreamReader;
1919
import java.io.Reader;
20+
import java.util.Arrays;
2021

2122
import com.oracle.truffle.api.frame.VirtualFrame;
2223
import com.oracle.truffle.api.nodes.Node;
@@ -31,6 +32,7 @@
3132
import org.truffleruby.core.array.RubyArray;
3233
import org.truffleruby.core.array.ArrayBuilderNode.BuilderState;
3334
import org.truffleruby.core.array.library.ArrayStoreLibrary;
35+
import org.truffleruby.language.Nil;
3436
import org.truffleruby.shared.TruffleRuby;
3537

3638
public class ArrayBuilderTest {
@@ -43,7 +45,7 @@ public class ArrayBuilderTest {
4345
public void emptyBuilderTest() {
4446
testInContext(() -> {
4547
ArrayBuilderNode builder = createBuilder();
46-
BuilderState state = builder.start();
48+
BuilderState state = builder.start(0);
4749
assertEquals(ArrayStoreLibrary.initialStorage(false), builder.finish(state, 0));
4850
});
4951
}
@@ -105,6 +107,22 @@ public void arrayBuilderAppendObjectTest() {
105107
});
106108
}
107109

110+
@Test
111+
public void arrayBuilderAppendGrowTest() {
112+
testInContext(() -> {
113+
ArrayBuilderNode builder = createBuilder();
114+
BuilderState state = builder.start(10);
115+
for (int i = 0; i < 12; i++) {
116+
builder.appendValue(state, i, Nil.INSTANCE);
117+
}
118+
Object[] result = (Object[]) builder.finish(state, 12);
119+
for (int i = 0; i < 12; i++) {
120+
Object e = result[i];
121+
assertEquals(Nil.INSTANCE, e);
122+
}
123+
});
124+
}
125+
108126
@Test
109127
public void arrayBuilderAppendEmptyArrayTest() {
110128
testInContext(() -> {
@@ -180,6 +198,28 @@ public void arrayBuilderAppendObjectArrayTest() {
180198
});
181199
}
182200

201+
@Test
202+
public void arrayBuilderAppendGrowArrayTest() {
203+
testInContext(() -> {
204+
ArrayBuilderNode builder = createBuilder();
205+
BuilderState state = builder.start(10);
206+
Object[] array = new Object[6];
207+
Arrays.fill(array, Nil.INSTANCE);
208+
RubyArray otherStore = new RubyArray(
209+
RubyLanguage.getCurrentContext().getCoreLibrary().arrayClass,
210+
RubyLanguage.getCurrentLanguage().arrayShape,
211+
array,
212+
array.length);
213+
builder.appendArray(state, 0, otherStore);
214+
builder.appendArray(state, 6, otherStore);
215+
Object[] result = (Object[]) builder.finish(state, 12);
216+
for (int i = 0; i < 12; i++) {
217+
Object e = result[i];
218+
assertEquals(Nil.INSTANCE, e);
219+
}
220+
});
221+
}
222+
183223
private ArrayBuilderNode createBuilder() {
184224
return adopt(ArrayBuilderNode.create());
185225
}

0 commit comments

Comments
 (0)