Skip to content

Commit 113cb44

Browse files
woesswoess
committed
[GR-65834] Fix possible deopt cycle in property cache.
PullRequest: js/3568
2 parents 9052358 + b8efa71 commit 113cb44

File tree

4 files changed

+78
-99
lines changed

4 files changed

+78
-99
lines changed

‎graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/HasPropertyCacheNode.java‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,9 @@ protected HasPropertyCacheNode(Object key, JSContext context, boolean hasOwnProp
9191

9292
@ExplodeLoop
9393
public boolean hasProperty(Object thisObj) {
94-
HasCacheNode c = cacheNode;
95-
for (; c != null; c = c.next) {
96-
if (c instanceof GenericHasPropertyCacheNode) {
97-
return ((GenericHasPropertyCacheNode) c).hasProperty(thisObj, this);
94+
for (HasCacheNode c = cacheNode; c != null; c = c.next) {
95+
if (c instanceof GenericHasPropertyCacheNode generic) {
96+
return generic.hasProperty(thisObj, this);
9897
}
9998
boolean isSimpleShapeCheck = c.isSimpleShapeCheck();
10099
ReceiverCheckNode receiverCheck = c.receiverCheck;
@@ -107,7 +106,7 @@ public boolean hasProperty(Object thisObj) {
107106
guard = shape.check(jsobj);
108107
castObj = jsobj;
109108
if (!shape.getValidAssumption().isValid()) {
110-
break;
109+
continue;
111110
}
112111
} else {
113112
continue;
@@ -118,12 +117,12 @@ public boolean hasProperty(Object thisObj) {
118117
}
119118
if (guard) {
120119
if (!isSimpleShapeCheck && !receiverCheck.isValid()) {
121-
break;
120+
continue;
122121
}
123122
return c.hasProperty(castObj, this);
124123
}
125124
}
126-
deoptimize(c);
125+
deoptimize();
127126
return hasPropertyAndSpecialize(thisObj);
128127
}
129128

‎graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/PropertyCacheNode.java‎

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1108,8 +1108,8 @@ protected static boolean alwaysUseStore(JSDynamicObject store, Object key) {
11081108
(JSArrayBufferView.isJSArrayBufferView(store) && (key instanceof TruffleString indexStr) && JSRuntime.canonicalNumericIndexString(indexStr) != Undefined.instance);
11091109
}
11101110

1111-
protected final void deoptimize(CacheNode<?> stop) {
1112-
if (CompilerDirectives.inCompiledCode() && stop != null) {
1111+
protected final void deoptimize() {
1112+
if (CompilerDirectives.inCompiledCode()) {
11131113
/*
11141114
* If the invalidation assumption is initialized, we have previously retried the cache.
11151115
* Do not invalidate code here, since we might just re-shape the object (which does not
@@ -1186,30 +1186,34 @@ protected T rewriteCached(T currentHead, T newHead) {
11861186
}
11871187

11881188
/**
1189-
* Does the given map relate to any of the cached maps by upcasting? If so, obsolete the
1190-
* downcast map.
1189+
* Tries to merge the object's shape with any related shape in the cache to a more general shape
1190+
* that has the same properties but can store the values of both shapes. For example, a shape
1191+
* {@code {x: int}} could be merged with, and obsoleted by, a shape {@code {x: Object}}.
11911192
*
1192-
* @param cacheShape The new map to check against
1193-
* @return true if a map was obsoleted
1193+
* @param objShape the new shape that would be inserted into the cache
1194+
* @return true if a shape was obsoleted
11941195
*/
1195-
protected static <T extends CacheNode<T>> boolean tryMergeShapes(Shape cacheShape, T head) {
1196-
assert cacheShape.isValid();
1197-
boolean result = false;
1198-
1196+
protected static <T extends CacheNode<T>> boolean tryMergeShapes(Shape objShape, T head) {
1197+
if (!objShape.isValid()) {
1198+
return false;
1199+
}
1200+
boolean merged = false;
11991201
for (T cur = head; cur != null; cur = cur.getNext()) {
12001202
if (cur.receiverCheck == null) {
12011203
continue;
12021204
}
12031205
Shape other = cur.receiverCheck.getShape();
1204-
if (cacheShape != other && other != null && other.isValid()) {
1205-
assert cacheShape.isValid();
1206-
result |= cacheShape.tryMerge(other) != null;
1207-
if (!cacheShape.isValid()) {
1208-
break;
1206+
if (objShape != other && other != null && other.isValid()) {
1207+
Shape mergedShape = objShape.tryMerge(other);
1208+
if (mergedShape != null) {
1209+
merged = true;
1210+
if (!objShape.isValid()) {
1211+
break;
1212+
}
12091213
}
12101214
}
12111215
}
1212-
return result;
1216+
return merged;
12131217
}
12141218

12151219
protected void checkForUnstableAssumption(T head, Object thisObj) {

‎graal-js/src/com.oracle.truffle.js/src/com/oracle/truffle/js/nodes/access/PropertyGetNode.java‎

Lines changed: 28 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -213,10 +213,9 @@ public final Object getValueOrUndefined(Object thisObj, Object receiver) {
213213

214214
@ExplodeLoop
215215
protected int getValueInt(Object thisObj, Object receiver) throws UnexpectedResultException {
216-
GetCacheNode c = cacheNode;
217-
for (; c != null; c = c.next) {
218-
if (c instanceof GenericPropertyGetNode) {
219-
return c.getValueInt(thisObj, receiver, this, false);
216+
for (GetCacheNode c = cacheNode; c != null; c = c.next) {
217+
if (c instanceof GenericPropertyGetNode generic) {
218+
return generic.getValueInt(thisObj, receiver, this, false);
220219
}
221220
boolean isSimpleShapeCheck = c.isSimpleShapeCheck();
222221
ReceiverCheckNode receiverCheck = c.receiverCheck;
@@ -225,11 +224,7 @@ protected int getValueInt(Object thisObj, Object receiver) throws UnexpectedResu
225224
if (c.isConstantObjectSpecialization()) {
226225
JSDynamicObject expectedObj = c.getExpectedObject();
227226
if (thisObj != expectedObj) {
228-
if (expectedObj == null) {
229-
break;
230-
} else {
231-
continue;
232-
}
227+
continue;
233228
} else {
234229
guard = true;
235230
castObj = expectedObj;
@@ -242,7 +237,7 @@ protected int getValueInt(Object thisObj, Object receiver) throws UnexpectedResu
242237
guard = shape.check(jsobj);
243238
castObj = jsobj;
244239
if (!shape.getValidAssumption().isValid()) {
245-
break;
240+
continue;
246241
}
247242
} else {
248243
continue;
@@ -253,12 +248,12 @@ protected int getValueInt(Object thisObj, Object receiver) throws UnexpectedResu
253248
}
254249
if (guard) {
255250
if ((!isSimpleShapeCheck && !receiverCheck.isValid())) {
256-
break;
251+
continue;
257252
}
258253
return c.getValueInt(castObj, receiver, this, guard);
259254
}
260255
}
261-
deoptimize(c);
256+
deoptimize();
262257
return getValueIntAndSpecialize(thisObj, receiver);
263258
}
264259

@@ -270,10 +265,9 @@ private int getValueIntAndSpecialize(Object thisObj, Object receiver) throws Une
270265

271266
@ExplodeLoop
272267
protected double getValueDouble(Object thisObj, Object receiver) throws UnexpectedResultException {
273-
GetCacheNode c = cacheNode;
274-
for (; c != null; c = c.next) {
275-
if (c instanceof GenericPropertyGetNode) {
276-
return c.getValueDouble(thisObj, receiver, this, false);
268+
for (GetCacheNode c = cacheNode; c != null; c = c.next) {
269+
if (c instanceof GenericPropertyGetNode generic) {
270+
return generic.getValueDouble(thisObj, receiver, this, false);
277271
}
278272
boolean isSimpleShapeCheck = c.isSimpleShapeCheck();
279273
ReceiverCheckNode receiverCheck = c.receiverCheck;
@@ -282,11 +276,7 @@ protected double getValueDouble(Object thisObj, Object receiver) throws Unexpect
282276
if (c.isConstantObjectSpecialization()) {
283277
JSDynamicObject expectedObj = c.getExpectedObject();
284278
if (thisObj != expectedObj) {
285-
if (expectedObj == null) {
286-
break;
287-
} else {
288-
continue;
289-
}
279+
continue;
290280
} else {
291281
guard = true;
292282
castObj = expectedObj;
@@ -299,7 +289,7 @@ protected double getValueDouble(Object thisObj, Object receiver) throws Unexpect
299289
guard = shape.check(jsobj);
300290
castObj = jsobj;
301291
if (!shape.getValidAssumption().isValid()) {
302-
break;
292+
continue;
303293
}
304294
} else {
305295
continue;
@@ -310,12 +300,12 @@ protected double getValueDouble(Object thisObj, Object receiver) throws Unexpect
310300
}
311301
if (guard) {
312302
if ((!isSimpleShapeCheck && !receiverCheck.isValid())) {
313-
break;
303+
continue;
314304
}
315305
return c.getValueDouble(castObj, receiver, this, guard);
316306
}
317307
}
318-
deoptimize(c);
308+
deoptimize();
319309
return getValueDoubleAndSpecialize(thisObj, receiver);
320310
}
321311

@@ -327,10 +317,9 @@ private double getValueDoubleAndSpecialize(Object thisObj, Object receiver) thro
327317

328318
@ExplodeLoop
329319
protected boolean getValueBoolean(Object thisObj, Object receiver) throws UnexpectedResultException {
330-
GetCacheNode c = cacheNode;
331-
for (; c != null; c = c.next) {
332-
if (c instanceof GenericPropertyGetNode) {
333-
return c.getValueBoolean(thisObj, receiver, this, false);
320+
for (GetCacheNode c = cacheNode; c != null; c = c.next) {
321+
if (c instanceof GenericPropertyGetNode generic) {
322+
return generic.getValueBoolean(thisObj, receiver, this, false);
334323
}
335324
boolean isSimpleShapeCheck = c.isSimpleShapeCheck();
336325
ReceiverCheckNode receiverCheck = c.receiverCheck;
@@ -339,11 +328,7 @@ protected boolean getValueBoolean(Object thisObj, Object receiver) throws Unexpe
339328
if (c.isConstantObjectSpecialization()) {
340329
JSDynamicObject expectedObj = c.getExpectedObject();
341330
if (thisObj != expectedObj) {
342-
if (expectedObj == null) {
343-
break;
344-
} else {
345-
continue;
346-
}
331+
continue;
347332
} else {
348333
guard = true;
349334
castObj = expectedObj;
@@ -356,7 +341,7 @@ protected boolean getValueBoolean(Object thisObj, Object receiver) throws Unexpe
356341
guard = shape.check(jsobj);
357342
castObj = jsobj;
358343
if (!shape.getValidAssumption().isValid()) {
359-
break;
344+
continue;
360345
}
361346
} else {
362347
continue;
@@ -367,12 +352,12 @@ protected boolean getValueBoolean(Object thisObj, Object receiver) throws Unexpe
367352
}
368353
if (guard) {
369354
if ((!isSimpleShapeCheck && !receiverCheck.isValid())) {
370-
break;
355+
continue;
371356
}
372357
return c.getValueBoolean(castObj, receiver, this, guard);
373358
}
374359
}
375-
deoptimize(c);
360+
deoptimize();
376361
return getValueBooleanAndSpecialize(thisObj, receiver);
377362
}
378363

@@ -384,10 +369,9 @@ private boolean getValueBooleanAndSpecialize(Object thisObj, Object receiver) th
384369

385370
@ExplodeLoop
386371
protected Object getValueOrDefault(Object thisObj, Object receiver, Object defaultValue) {
387-
GetCacheNode c = cacheNode;
388-
for (; c != null; c = c.next) {
389-
if (c instanceof GenericPropertyGetNode) {
390-
return ((GenericPropertyGetNode) c).getValue(thisObj, receiver, defaultValue, this, false);
372+
for (GetCacheNode c = cacheNode; c != null; c = c.next) {
373+
if (c instanceof GenericPropertyGetNode generic) {
374+
return generic.getValue(thisObj, receiver, defaultValue, this, false);
391375
}
392376
boolean isSimpleShapeCheck = c.isSimpleShapeCheck();
393377
ReceiverCheckNode receiverCheck = c.receiverCheck;
@@ -396,11 +380,7 @@ protected Object getValueOrDefault(Object thisObj, Object receiver, Object defau
396380
if (c.isConstantObjectSpecialization()) {
397381
JSDynamicObject expectedObj = c.getExpectedObject();
398382
if (thisObj != expectedObj) {
399-
if (expectedObj == null) {
400-
break;
401-
} else {
402-
continue;
403-
}
383+
continue;
404384
} else {
405385
guard = true;
406386
castObj = expectedObj;
@@ -413,7 +393,7 @@ protected Object getValueOrDefault(Object thisObj, Object receiver, Object defau
413393
guard = shape.check(jsobj);
414394
castObj = jsobj;
415395
if (!shape.getValidAssumption().isValid()) {
416-
break;
396+
continue;
417397
}
418398
} else {
419399
continue;
@@ -424,12 +404,12 @@ protected Object getValueOrDefault(Object thisObj, Object receiver, Object defau
424404
}
425405
if (guard) {
426406
if ((!isSimpleShapeCheck && !receiverCheck.isValid())) {
427-
break;
407+
continue;
428408
}
429409
return c.getValue(castObj, receiver, defaultValue, this, guard);
430410
}
431411
}
432-
deoptimize(c);
412+
deoptimize();
433413
return getValueAndSpecialize(thisObj, receiver, defaultValue);
434414
}
435415

0 commit comments

Comments
 (0)