Skip to content

Commit bff1874

Browse files
msimacekmsimacek
committed
Simplify Assumption handling in MRO lookups
1 parent efc1259 commit bff1874

File tree

8 files changed

+87
-180
lines changed

8 files changed

+87
-180
lines changed

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/DynamicObjectStorage.java‎

Lines changed: 5 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@
5757
import com.oracle.graal.python.nodes.PGuards;
5858
import com.oracle.graal.python.nodes.attributes.ReadAttributeFromPythonObjectNode;
5959
import com.oracle.graal.python.nodes.util.CastToTruffleStringNode;
60-
import com.oracle.graal.python.runtime.sequence.storage.MroSequenceStorage;
6160
import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
6261
import com.oracle.truffle.api.HostCompilerDirectives.InliningCutoff;
6362
import com.oracle.truffle.api.dsl.Bind;
@@ -79,7 +78,6 @@
7978
import com.oracle.truffle.api.object.DynamicObject;
8079
import com.oracle.truffle.api.object.DynamicObjectLibrary;
8180
import com.oracle.truffle.api.object.Shape;
82-
import com.oracle.truffle.api.profiles.InlinedBranchProfile;
8381
import com.oracle.truffle.api.profiles.InlinedConditionProfile;
8482
import com.oracle.truffle.api.strings.TruffleString;
8583

@@ -92,7 +90,6 @@ public final class DynamicObjectStorage extends HashingStorage {
9290
public static final int EXPLODE_LOOP_SIZE_LIMIT = 16;
9391

9492
final DynamicObject store;
95-
private final MroSequenceStorage mro;
9693

9794
static final class Store extends DynamicObject {
9895
public Store(Shape shape) {
@@ -101,20 +98,11 @@ public Store(Shape shape) {
10198
}
10299

103100
public DynamicObjectStorage(PythonLanguage lang) {
104-
this(new Store(lang.getEmptyShape()), null);
101+
this(new Store(lang.getEmptyShape()));
105102
}
106103

107104
public DynamicObjectStorage(DynamicObject store) {
108-
this(store, null);
109-
}
110-
111-
public DynamicObjectStorage(DynamicObject store, MroSequenceStorage mro) {
112105
this.store = store;
113-
this.mro = mro;
114-
}
115-
116-
public Shape getStoreShape() {
117-
return store.getShape();
118106
}
119107

120108
public DynamicObject getStore() {
@@ -289,20 +277,8 @@ private static boolean hasNext(Iterator<Object> keys) {
289277
}
290278
}
291279

292-
private static void invalidateAttributeInMROFinalAssumptions(MroSequenceStorage mro, TruffleString name, Node inliningTarget, InlinedBranchProfile profile) {
293-
if (mro != null) {
294-
profile.enter(inliningTarget);
295-
mro.invalidateAttributeInMROFinalAssumptions(name);
296-
}
297-
}
298-
299-
void setStringKey(TruffleString key, Object value, DynamicObjectLibrary dylib, Node inliningTarget, InlinedBranchProfile invalidateMroProfile) {
280+
void setStringKey(TruffleString key, Object value, DynamicObjectLibrary dylib) {
300281
dylib.put(store, key, assertNoJavaString(value));
301-
invalidateAttributeInMROFinalAssumption(key, inliningTarget, invalidateMroProfile);
302-
}
303-
304-
void invalidateAttributeInMROFinalAssumption(TruffleString key, Node inliningTarget, InlinedBranchProfile invalidateMroProfile) {
305-
invalidateAttributeInMROFinalAssumptions(mro, key, inliningTarget, invalidateMroProfile);
306282
}
307283

308284
boolean shouldTransitionOnPut() {
@@ -393,10 +369,9 @@ public static DynamicObjectStorage copyGeneric(DynamicObjectStorage receiver,
393369
@GenerateCached(false)
394370
public abstract static class DynamicObjectStorageSetStringKey extends SpecializedSetStringKey {
395371
@Specialization
396-
static void doIt(Node inliningTarget, HashingStorage self, TruffleString key, Object value,
397-
@CachedLibrary(limit = "3") DynamicObjectLibrary dylib,
398-
@Cached InlinedBranchProfile invalidateMro) {
399-
((DynamicObjectStorage) self).setStringKey(key, value, dylib, inliningTarget, invalidateMro);
372+
static void doIt(HashingStorage self, TruffleString key, Object value,
373+
@CachedLibrary(limit = "3") DynamicObjectLibrary dylib) {
374+
((DynamicObjectStorage) self).setStringKey(key, value, dylib);
400375
}
401376
}
402377
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/HashingStorageNodes.java‎

Lines changed: 9 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -280,10 +280,9 @@ static HashingStorage empty(Frame frame, Node inliningTarget, @SuppressWarnings(
280280
}
281281

282282
@Specialization(guards = "!self.shouldTransitionOnPut()")
283-
static HashingStorage domStringKey(Node inliningTarget, DynamicObjectStorage self, TruffleString key, long keyHash, Object value,
284-
@Cached InlinedBranchProfile invalidateMroProfile,
283+
static HashingStorage domStringKey(DynamicObjectStorage self, TruffleString key, long keyHash, Object value,
285284
@Shared @CachedLibrary(limit = "3") DynamicObjectLibrary dylib) {
286-
self.setStringKey(key, value, dylib, inliningTarget, invalidateMroProfile);
285+
self.setStringKey(key, value, dylib);
287286
return self;
288287
}
289288

@@ -333,9 +332,8 @@ public abstract HashingStorage execute(Frame frame, Node inliningTarget, Dynamic
333332
static HashingStorage domStringKey(Node inliningTarget, DynamicObjectStorage self, Object key, @SuppressWarnings("unused") long keyHash, Object value,
334333
@SuppressWarnings("unused") boolean transition, DynamicObjectLibrary dylib,
335334
@SuppressWarnings("unused") @Cached PyUnicodeCheckExactNode isBuiltinString,
336-
@Cached CastBuiltinStringToTruffleStringNode castStr,
337-
@Cached InlinedBranchProfile invalidateMroProfile) {
338-
self.setStringKey(castStr.execute(inliningTarget, key), value, dylib, inliningTarget, invalidateMroProfile);
335+
@Cached CastBuiltinStringToTruffleStringNode castStr) {
336+
self.setStringKey(castStr.execute(inliningTarget, key), value, dylib);
339337
return self;
340338
}
341339

@@ -405,10 +403,9 @@ static HashingStorage empty(Frame frame, Node inliningTarget, @SuppressWarnings(
405403
}
406404

407405
@Specialization(guards = "!self.shouldTransitionOnPut()")
408-
static HashingStorage domStringKey(Node inliningTarget, DynamicObjectStorage self, TruffleString key, Object value,
409-
@Cached InlinedBranchProfile invalidateMroProfile,
406+
static HashingStorage domStringKey(DynamicObjectStorage self, TruffleString key, Object value,
410407
@Shared @CachedLibrary(limit = "3") DynamicObjectLibrary dylib) {
411-
self.setStringKey(key, value, dylib, inliningTarget, invalidateMroProfile);
408+
self.setStringKey(key, value, dylib);
412409
return self;
413410
}
414411

@@ -459,9 +456,8 @@ public abstract HashingStorage execute(Frame frame, Node inliningTarget, Dynamic
459456
static HashingStorage domStringKey(Node inliningTarget, DynamicObjectStorage self, Object key, Object value,
460457
@SuppressWarnings("unused") boolean transition, DynamicObjectLibrary dylib,
461458
@SuppressWarnings("unused") @Cached PyUnicodeCheckExactNode isBuiltinString,
462-
@Cached CastBuiltinStringToTruffleStringNode castStr,
463-
@Cached InlinedBranchProfile invalidateMroProfile) {
464-
self.setStringKey(castStr.execute(inliningTarget, key), value, dylib, inliningTarget, invalidateMroProfile);
459+
@Cached CastBuiltinStringToTruffleStringNode castStr) {
460+
self.setStringKey(castStr.execute(inliningTarget, key), value, dylib);
465461
return self;
466462
}
467463

@@ -557,16 +553,10 @@ static Object domStringKey(Frame frame, Node inliningTarget, DynamicObjectStorag
557553
return null;
558554
} else {
559555
dylib.put(store, key, PNone.NO_VALUE);
560-
self.invalidateAttributeInMROFinalAssumption(key, inliningTarget, invalidateMroProfile);
561556
return val;
562557
}
563558
} else {
564-
if (dylib.putIfPresent(store, key, PNone.NO_VALUE)) {
565-
self.invalidateAttributeInMROFinalAssumption(key, inliningTarget, invalidateMroProfile);
566-
return true;
567-
} else {
568-
return false;
569-
}
559+
return dylib.putIfPresent(store, key, PNone.NO_VALUE);
570560
}
571561
}
572562

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/PythonBuiltinClass.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,8 +99,8 @@ public PythonBuiltinClassType getType() {
9999
public void onAttributeUpdate(TruffleString key, Object newValue) {
100100
assert !PythonContext.get(null).isCoreInitialized();
101101
// Ideally, startup code should not create ASTs that rely on assumptions of props of
102-
// builtins. So there should be no assumptions to invalidate yet
103-
assert !getMethodResolutionOrder().invalidateAttributeInMROFinalAssumptions(key);
102+
// builtins
103+
assert getMethodResolutionOrder().getFinalAttributeAssumption(key) == null;
104104
assert checkSpecialMethodUpdate(key, newValue);
105105
// NO_VALUE changes MRO lookup results without actually changing any Shapes in the MRO, this
106106
// can prevent some optimizations, so it is best to avoid any code that triggers such code

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/PythonManagedClass.java‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,13 +212,19 @@ public void setAttribute(TruffleString key, Object value) {
212212
*/
213213
public boolean canSkipOnAttributeUpdate(TruffleString key, @SuppressWarnings("unused") Object value, TruffleString.CodePointLengthNode codePointLengthNode,
214214
TruffleString.CodePointAtIndexNode codePointAtIndexNode) {
215+
// TODO subclasses
215216
return !methodResolutionOrder.hasAttributeInMROFinalAssumptions() &&
216217
!TpSlots.canBeSpecialMethod(key, codePointLengthNode, codePointAtIndexNode);
217218
}
218219

219220
@TruffleBoundary
220221
public void onAttributeUpdate(TruffleString key, Object value) {
221-
methodResolutionOrder.invalidateAttributeInMROFinalAssumptions(key);
222+
for (PythonAbstractClass subclass : GetSubclassesAsArrayNode.executeUncached(this)) {
223+
if (subclass instanceof PythonManagedClass managedClass) {
224+
managedClass.onAttributeUpdate(key, value);
225+
}
226+
}
227+
methodResolutionOrder.invalidateFinalAttributeAssumption(key);
222228
if (TpSlots.canBeSpecialMethod(key, CodePointLengthNode.getUncached(), CodePointAtIndexNode.getUncached())) {
223229
if (this.tpSlots != null) {
224230
// This is called during type instantiation from copyDictSlots when the tp slots are

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/TypeBuiltins.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -778,7 +778,7 @@ static Object doManaged(PythonManagedClass self,
778778
@Shared @Cached GetDictIfExistsNode getDict) {
779779
PDict dict = getDict.execute(self);
780780
if (dict == null) {
781-
dict = PFactory.createDictFixedStorage(language, self, self.getMethodResolutionOrder());
781+
dict = PFactory.createDictFixedStorage(language, self);
782782
// The mapping is unmodifiable, so we don't have to assign it back
783783
}
784784
return PFactory.createMappingproxy(language, dict);

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/LookupAttributeInMRONode.java‎

Lines changed: 14 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@
5555
import com.oracle.graal.python.runtime.PythonContext;
5656
import com.oracle.graal.python.runtime.PythonOptions;
5757
import com.oracle.graal.python.runtime.sequence.storage.MroSequenceStorage;
58+
import com.oracle.graal.python.runtime.sequence.storage.MroSequenceStorage.FinalAttributeAssumptionNode;
5859
import com.oracle.truffle.api.Assumption;
5960
import com.oracle.truffle.api.CompilerAsserts;
6061
import com.oracle.truffle.api.CompilerDirectives;
@@ -70,7 +71,6 @@
7071
import com.oracle.truffle.api.dsl.NeverDefault;
7172
import com.oracle.truffle.api.dsl.ReportPolymorphism.Megamorphic;
7273
import com.oracle.truffle.api.dsl.Specialization;
73-
import com.oracle.truffle.api.nodes.ControlFlowException;
7474
import com.oracle.truffle.api.nodes.ExplodeLoop;
7575
import com.oracle.truffle.api.nodes.ExplodeLoop.LoopExplosionKind;
7676
import com.oracle.truffle.api.nodes.Node;
@@ -177,51 +177,25 @@ static Object lookupPBCTCached(PythonBuiltinClassType klass,
177177
return cachedValue;
178178
}
179179

180-
// PythonClass specializations:
181-
182-
static final class AttributeAssumptionPair {
183-
private final Assumption assumption;
184-
private final Object value;
185-
private final boolean invalidate;
186-
private boolean invalidateNextCall;
187-
188-
AttributeAssumptionPair(Assumption assumption, Object value, boolean invalidate) {
189-
this.assumption = assumption;
190-
this.value = value;
191-
this.invalidate = invalidate;
192-
}
193-
194-
public Assumption assumption() {
195-
return assumption;
196-
}
197-
}
198-
199-
@SuppressWarnings("serial")
200-
static final class InvalidateLookupException extends ControlFlowException {
201-
private static final InvalidateLookupException INSTANCE = new InvalidateLookupException();
202-
}
203-
204180
private static boolean skipNonStaticBase(Object clsObj, boolean skipNonStaticBases) {
205181
return skipNonStaticBases && clsObj instanceof PythonClass && !((PythonClass) clsObj).isStaticBase();
206182
}
207183

208-
AttributeAssumptionPair findAttrAndAssumptionInMRO(Object klass) {
184+
FinalAttributeAssumptionNode findAttrAndAssumptionInMRO(Object klass) {
209185
CompilerAsserts.neverPartOfCompilation();
210186
// Regarding potential side effects to MRO caused by __eq__ of the keys in the dicts that we
211187
// search through: CPython seems to read the MRO once and then compute the result also
212188
// ignoring the side effects. Moreover, CPython has lookup cache, so the side effects
213189
// may or may not be visible during subsequent lookups. We want to avoid triggering the side
214-
// effects twice, so we succeed this lookup no matter what, however, we will invalidate on
215-
// the next lookup if there were some MRO side effects.
190+
// effects twice
216191
MroSequenceStorage mro = GetMroStorageNode.executeUncached(klass);
217-
Assumption attrAssumption = mro.createAttributeInMROFinalAssumption(key);
192+
FinalAttributeAssumptionNode assumptionNode = mro.getFinalAttributeAssumption(key);
193+
if (assumptionNode != null) {
194+
return assumptionNode;
195+
}
218196
Object result = PNone.NO_VALUE;
219197
for (int i = 0; i < mro.length(); i++) {
220198
PythonAbstractClass clsObj = mro.getPythonClassItemNormalized(i);
221-
if (i > 0) {
222-
assert clsObj != klass : "MRO chain is incorrect: '" + klass + "' was found at position " + i;
223-
GetMroStorageNode.executeUncached(clsObj).addAttributeInMROFinalAssumption(key, attrAssumption);
224-
}
225199
if (skipNonStaticBase(clsObj, skipNonStaticBases)) {
226200
continue;
227201
}
@@ -231,32 +205,20 @@ AttributeAssumptionPair findAttrAndAssumptionInMRO(Object klass) {
231205
break;
232206
}
233207
}
234-
if (!attrAssumption.isValid()) {
235-
return new AttributeAssumptionPair(Assumption.ALWAYS_VALID, result, true);
236-
} else {
237-
return new AttributeAssumptionPair(attrAssumption, result, false);
238-
}
208+
FinalAttributeAssumptionNode node = new FinalAttributeAssumptionNode(result);
209+
mro.putFinalAttributeAssumption(key, node);
210+
return node;
239211
}
240212

241-
@Specialization(guards = {"isSingleContext()", "isSameTypeNode.execute(inliningTarget, cachedKlass, klass)", "cachedAttrInMROInfo != null"}, //
213+
@Specialization(guards = {"isSingleContext()", "isSameTypeNode.execute(inliningTarget, cachedKlass, klass)", "cachedAttrInMROInfo.getAssumption() != null"}, //
242214
limit = "getAttributeAccessInlineCacheMaxDepth()", //
243-
assumptions = "cachedAttrInMROInfo.assumption()", //
244-
rewriteOn = InvalidateLookupException.class)
215+
assumptions = "cachedAttrInMROInfo.getAssumption()")
245216
static Object lookupConstantMROCached(Object klass,
246217
@Bind Node inliningTarget,
247218
@Cached("klass") Object cachedKlass,
248219
@Cached IsSameTypeNode isSameTypeNode,
249-
@Cached("findAttrAndAssumptionInMRO(cachedKlass)") AttributeAssumptionPair cachedAttrInMROInfo) {
250-
if (cachedAttrInMROInfo.invalidate) {
251-
// next time we will invalidate, but this time we must return the result to avoid
252-
// triggering side effects in __eq__ multiple times
253-
if (cachedAttrInMROInfo.invalidateNextCall) {
254-
CompilerDirectives.transferToInterpreterAndInvalidate();
255-
throw InvalidateLookupException.INSTANCE;
256-
}
257-
cachedAttrInMROInfo.invalidateNextCall = true;
258-
}
259-
return cachedAttrInMROInfo.value;
220+
@Cached("findAttrAndAssumptionInMRO(cachedKlass)") FinalAttributeAssumptionNode cachedAttrInMROInfo) {
221+
return cachedAttrInMROInfo.getValue();
260222
}
261223

262224
// The slow-path specializations are extracted in a separate node to have a single cutoff from

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/object/PFactory.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -726,10 +726,6 @@ public static PDict createDictFromMapGeneric(PythonLanguage language, LinkedHash
726726
return createDict(language, EconomicMapStorage.createGeneric(map));
727727
}
728728

729-
public static PDict createDictFixedStorage(PythonLanguage language, PythonObject pythonObject, MroSequenceStorage mroSequenceStorage) {
730-
return createDict(language, new DynamicObjectStorage(pythonObject, mroSequenceStorage));
731-
}
732-
733729
public static PDict createDictFixedStorage(PythonLanguage language, PythonObject pythonObject) {
734730
return createDict(language, new DynamicObjectStorage(pythonObject));
735731
}

0 commit comments

Comments
 (0)