Block future property assumptions on indirect shape transitions.

(cherry picked from commit ed5aa9af4dc77f49a56755789c7920ce84ff5deb)

Author: woess

truffle/src/com.oracle.truffle.object/src/com/oracle/truffle/object/LayoutStrategy.java

@@ -160,7 +160,6 @@ private Location createLocationForValue(ShapeImpl oldShape, Object value, long p
 
     protected ShapeImpl definePropertyChangeFlags(ShapeImpl oldShape, Property existing, Object value, int propertyFlags, long putFlags) {
         assert existing.getFlags() != propertyFlags;
-        oldShape.onPropertyTransition(existing);
         if (existing.getLocation().canStore(value)) {
             Property newProperty = Property.create(existing.getKey(), existing.getLocation(), propertyFlags);
             return replaceProperty(oldShape, existing, newProperty);
@@ -170,7 +169,6 @@ protected ShapeImpl definePropertyChangeFlags(ShapeImpl oldShape, Property exist
     }
 
     protected ShapeImpl definePropertyGeneralize(ShapeImpl oldShape, Property oldProperty, Object value, com.oracle.truffle.api.object.LocationFactory locationFactory, long putFlags) {
-        oldShape.onPropertyTransition(oldProperty);
         if (Flags.isSeparateShape(putFlags)) {
             Location newLocation = createLocationForValue(oldShape, value, putFlags, locationFactory);
             Property newProperty = ((PropertyImpl) oldProperty).relocate(newLocation);
@@ -189,7 +187,6 @@ protected ShapeImpl generalizeProperty(Property oldProperty, Object value, Shape
         Location oldLocation = oldProperty.getLocation();
         Location newLocation = currentShape.allocator().locationForValueUpcast(value, oldLocation, putFlags);
         Property newProperty = ((PropertyImpl) oldProperty).relocate(newLocation);
-        nextShape.onPropertyTransition(oldProperty);
         return replaceProperty(nextShape, oldProperty, newProperty);
     }
 
@@ -226,10 +223,9 @@ protected ShapeImpl replaceProperty(ShapeImpl shape, Property oldProperty, Prope
 
     /** @since 0.17 or earlier */
     protected ShapeImpl removeProperty(ShapeImpl shape, Property property) {
-        shape.onPropertyTransition(property);
-
         boolean direct = shape.isShared();
         RemovePropertyTransition transition = newRemovePropertyTransition(property, direct);
+        shape.onPropertyTransition(transition);
         ShapeImpl cachedShape = shape.queryTransition(transition);
         if (cachedShape != null) {
             return ensureValid(cachedShape);
@@ -306,9 +302,8 @@ private static ShapeImpl directReplacePropertyInner(ShapeImpl shape, Property ol
             return shape;
         }
 
-        shape.onPropertyTransition(oldProperty);
-
-        Transition replacePropertyTransition = new Transition.DirectReplacePropertyTransition(oldProperty, newProperty);
+        var replacePropertyTransition = new Transition.DirectReplacePropertyTransition(oldProperty, newProperty);
+        shape.onPropertyTransition(replacePropertyTransition);
         ShapeImpl cachedShape = shape.queryTransition(replacePropertyTransition);
         if (cachedShape != null) {
             return cachedShape;
@@ -326,6 +321,7 @@ private static ShapeImpl directReplacePropertyInner(ShapeImpl shape, Property ol
     }
 
     protected ShapeImpl separateReplaceProperty(ShapeImpl shape, Property oldProperty, Property newProperty) {
+        shape.invalidateAllPropertyAssumptions();
         ShapeImpl newRoot = shape.createShape(shape.getLayout(), shape.sharedData, null, shape.objectType, PropertyMap.empty(), null, shape.getLayout().createAllocator(), shape.flags);
         ShapeImpl newShape = newRoot;
         boolean found = false;
@@ -343,6 +339,7 @@ protected ShapeImpl separateReplaceProperty(ShapeImpl shape, Property oldPropert
     }
 
     protected ShapeImpl createSeparateShape(ShapeImpl shape) {
+        shape.invalidateAllPropertyAssumptions();
         ShapeImpl newRoot = shape.createShape(shape.getLayout(), shape.sharedData, null, shape.objectType, PropertyMap.empty(), null, shape.getLayout().createAllocator(), shape.flags);
         ShapeImpl newShape = newRoot;
         for (Iterator<Property> iterator = shape.getPropertyMap().orderedValueIterator(); iterator.hasNext();) {
@@ -371,9 +368,9 @@ protected ShapeImpl addProperty(ShapeImpl shape, Property property, boolean ensu
 
     private ShapeImpl addPropertyInner(ShapeImpl shape, Property property) {
         assert !(shape.hasProperty(property.getKey())) : "duplicate property " + property.getKey();
-        shape.onPropertyTransition(property);
 
         AddPropertyTransition addTransition = newAddPropertyTransition(property);
+        shape.onPropertyTransition(addTransition);
         ShapeImpl cachedShape = shape.queryTransition(addTransition);
         if (cachedShape != null) {
             return cachedShape;

truffle/src/com.oracle.truffle.object/src/com/oracle/truffle/object/ShapeImpl.java

@@ -571,17 +571,18 @@ public ShapeImpl addProperty(Property property) {
     }
 
     /** @since 0.17 or earlier */
-    @TruffleBoundary
-    protected void onPropertyTransition(Property property) {
-        onPropertyTransitionWithKey(property.getKey());
+    protected final void onPropertyTransition(Transition.PropertyTransition propertyTransition) {
+        if (allowPropertyAssumptions()) {
+            invalidatePropertyAssumption(propertyTransition.getPropertyKey(), propertyTransition.isDirect());
+        }
     }
 
-    final void onPropertyTransitionWithKey(Object propertyKey) {
-        if (allowPropertyAssumptions()) {
-            PropertyAssumptions propertyAssumptions = getPropertyAssumptions();
-            if (propertyAssumptions != null) {
-                propertyAssumptions.invalidatePropertyAssumption(propertyKey);
-            }
+    private void invalidatePropertyAssumption(Object propertyKey, boolean onlyExisting) {
+        PropertyAssumptions propertyAssumptions = onlyExisting
+                        ? getPropertyAssumptions()
+                        : getOrCreatePropertyAssumptions();
+        if (propertyAssumptions != null) {
+            propertyAssumptions.invalidatePropertyAssumption(propertyKey, onlyExisting);
         }
     }
 
@@ -1144,7 +1145,8 @@ protected static int checkObjectFlags(int flags) {
     @TruffleBoundary
     @Override
     public Assumption getPropertyAssumption(Object key) {
-        if (allowPropertyAssumptions()) {
+        // Deny new property assumptions from being made if shape is already obsolete.
+        if (allowPropertyAssumptions() && this.isValid()) {
             Assumption propertyAssumption = getOrCreatePropertyAssumptions().getPropertyAssumption(key);
             if (propertyAssumption != null && propertyAssumption.isValid()) {
                 return propertyAssumption;
@@ -1366,14 +1368,32 @@ synchronized Assumption getPropertyAssumption(Object propertyName) {
             return assumption;
         }
 
-        synchronized void invalidatePropertyAssumption(Object propertyName) {
+        synchronized void invalidatePropertyAssumption(Object propertyName, boolean onlyExisting) {
             CompilerAsserts.neverPartOfCompilation();
             EconomicMap<Object, Assumption> map = stablePropertyAssumptions;
             Assumption assumption = map.get(propertyName);
-            if (assumption != null && assumption != Assumption.NEVER_VALID) {
+            if (assumption == Assumption.NEVER_VALID) {
+                return;
+            }
+            if (assumption != null) {
                 assumption.invalidate("invalidatePropertyAssumption");
+            }
+            /*
+             * Direct property transitions can happen only once per object as they always lead to
+             * new shapes, so we only need to invalidate already registered assumptions.
+             *
+             * Indirect property transitions, OTOH, can form transition cycles in the shape tree
+             * that may cause toggling between existing shapes for the same object, and since
+             * already cached shape transitions fly under the radar of future property assumptions,
+             * we have to block any future assumptions from being registered for this property.
+             */
+            if (assumption != null || !onlyExisting) {
                 map.put(propertyName, Assumption.NEVER_VALID);
-                propertyAssumptionsRemoved.inc();
+                if (assumption != null) {
+                    propertyAssumptionsRemoved.inc();
+                } else {
+                    propertyAssumptionsBlocked.inc();
+                }
             }
         }
 
@@ -1398,5 +1418,6 @@ Assumption getSingleContextAssumption() {
     static final DebugCounter shapeCacheWeakKeys = DebugCounter.create("Shape cache weak keys");
     static final DebugCounter propertyAssumptionsCreated = DebugCounter.create("Property assumptions created");
     static final DebugCounter propertyAssumptionsRemoved = DebugCounter.create("Property assumptions removed");
+    static final DebugCounter propertyAssumptionsBlocked = DebugCounter.create("Property assumptions blocked");
 
 }