[GR-31158] Refactor attr write for classes.

steve-s · steve-s · commit 8a78fdd3558b · 2021-06-15T08:50:24.000Z
PullRequest: graalpython/1847
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/PythonManagedClass.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/PythonManagedClass.java
@@ -209,15 +209,32 @@ private boolean computeNeedsNativeAllocation() {
     @Override
     @TruffleBoundary
     public void setAttribute(Object key, Object value) {
-        invalidateFinalAttribute(key);
         super.setAttribute(key, value);
+        onAttributeUpdate(key, value);
     }
 
-    @TruffleBoundary
-    public final void invalidateFinalAttribute(Object key) {
+    /**
+     * Fast-path check designed for PE code.
+     */
+    public final boolean canSkipOnAttributeUpdate(String key, @SuppressWarnings("unused") Object value) {
+        return !methodResolutionOrder.hasAttributeInMROFinalAssumptions() &&
+                        !SpecialMethodSlot.canBeSpecial(key);
+    }
+
+    public final void onAttributeUpdate(Object key, Object value) {
+        // In compilation: use a profile and call the String key overload
         CompilerAsserts.neverPartOfCompilation();
         if (key instanceof String) {
-            methodResolutionOrder.invalidateAttributeInMROFinalAssumptions((String) key);
+            onAttributeUpdate((String) key, value);
+        }
+    }
+
+    @TruffleBoundary
+    public final void onAttributeUpdate(String key, Object value) {
+        methodResolutionOrder.invalidateAttributeInMROFinalAssumptions(key);
+        SpecialMethodSlot slot = SpecialMethodSlot.findSpecialSlot(key);
+        if (slot != null) {
+            SpecialMethodSlot.fixupSpecialMethodSlot(this, slot, value);
         }
     }
 
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/WriteAttributeToDynamicObjectNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/WriteAttributeToDynamicObjectNode.java
@@ -55,12 +55,12 @@
 @GenerateUncached
 public abstract class WriteAttributeToDynamicObjectNode extends ObjectAttributeNode {
 
-    public abstract boolean execute(Object primary, Object key, Object value);
-
     public abstract boolean execute(Object primary, HiddenKey key, Object value);
 
     public abstract boolean execute(Object primary, String key, Object value);
 
+    public abstract boolean execute(Object primary, Object key, Object value);
+
     public static WriteAttributeToDynamicObjectNode create() {
         return WriteAttributeToDynamicObjectNodeGen.create();
     }
@@ -76,8 +76,8 @@ static boolean writeDirect(DynamicObject dynamicObject, String key, Object value
         return true;
     }
 
-    @Specialization(guards = "isHiddenKey(key)", limit = "getAttributeAccessInlineCacheMaxDepth()")
-    static boolean writeDirectHidden(DynamicObject dynamicObject, Object key, Object value,
+    @Specialization(limit = "getAttributeAccessInlineCacheMaxDepth()")
+    static boolean writeDirectHidden(DynamicObject dynamicObject, HiddenKey key, Object value,
                     @CachedLibrary("dynamicObject") DynamicObjectLibrary dylib) {
         dylib.put(dynamicObject, key, value);
         return true;
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/WriteAttributeToObjectNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/WriteAttributeToObjectNode.java
@@ -50,6 +50,8 @@
 import com.oracle.graal.python.builtins.objects.dict.PDict;
 import com.oracle.graal.python.builtins.objects.object.PythonObject;
 import com.oracle.graal.python.builtins.objects.object.PythonObjectLibrary;
+import com.oracle.graal.python.builtins.objects.type.PythonBuiltinClass;
+import com.oracle.graal.python.builtins.objects.type.PythonClass;
 import com.oracle.graal.python.builtins.objects.type.PythonManagedClass;
 import com.oracle.graal.python.builtins.objects.type.SpecialMethodSlot;
 import com.oracle.graal.python.builtins.objects.type.TypeNodes.IsTypeNode;
@@ -70,14 +72,17 @@
 import com.oracle.truffle.api.dsl.ImportStatic;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.library.CachedLibrary;
-import com.oracle.truffle.api.nodes.Node;
+import com.oracle.truffle.api.object.DynamicObjectLibrary;
+import com.oracle.truffle.api.object.HiddenKey;
 import com.oracle.truffle.api.profiles.BranchProfile;
 
 @ImportStatic(PythonOptions.class)
 public abstract class WriteAttributeToObjectNode extends ObjectAttributeNode {
 
     public abstract boolean execute(Object primary, Object key, Object value);
 
+    public abstract boolean execute(Object primary, HiddenKey key, Object value);
+
     public static WriteAttributeToObjectNode create() {
         return WriteAttributeToObjectNotTypeNodeGen.create();
     }
@@ -97,61 +102,97 @@ protected static boolean isAttrWritable(PythonObject self, Object key) {
         return (self.getShape().getFlags() & PythonObject.HAS_SLOTS_BUT_NO_DICT_FLAG) == 0;
     }
 
-    private static void handlePossiblePythonClass(HandlePythonClassProfiles profiles, PythonObject object, Object keyObj, Object value) {
-        if (PythonManagedClass.isInstance(object)) {
-            profiles.isManagedClass.enter();
-            handlePythonClass(profiles, PythonManagedClass.cast(object), keyObj, value);
+    private static String castKey(CastToJavaStringNode castNode, Object value) {
+        try {
+            return castNode.execute(value);
+        } catch (CannotCastException ex) {
+            throw CompilerDirectives.shouldNotReachHere(ex);
         }
     }
 
-    private static void handlePythonClass(HandlePythonClassProfiles profiles, PythonManagedClass object, Object keyObj, Object value) {
-        String key = profiles.castKey(keyObj);
-        if (key == null) {
-            return;
-        }
-        object.invalidateFinalAttribute(key);
-        // Note: we need to handle builtin classes here, because during initialization we are
-        // setting attributes of some builtin types to Python functions (when given builtin method
-        // is not intrinsified in Java)
-        if (SpecialMethodSlot.canBeSpecial(key)) {
-            profiles.isSpecialKey.enter();
-            SpecialMethodSlot slot = SpecialMethodSlot.findSpecialSlot(key);
-            if (slot != null) {
-                SpecialMethodSlot.fixupSpecialMethodSlot(object, slot, value);
+    @Specialization(guards = "isAttrWritable(object, key)", limit = "getAttributeAccessInlineCacheMaxDepth()")
+    static boolean writeHiddenKeyToDynamicStorage(PythonObject object, HiddenKey key, Object value,
+                    @CachedLibrary("object.getStorage()") DynamicObjectLibrary dylib) {
+        // HiddenKeys are always written to the storage and do not have any other special handling
+        dylib.put(object.getStorage(), key, value);
+        return true;
+    }
+
+    @Specialization(guards = {"!isHiddenKey(key)", "!lib.hasDict(object)", "isAttrWritable(object, key)", "!isManagedClass(object)"}, limit = "1")
+    static boolean writeToDynamicStorageNoType(PythonObject object, Object key, Object value,
+                    @Cached CastToJavaStringNode castToStrNode,
+                    @CachedLibrary("object") @SuppressWarnings("unused") PythonObjectLibrary lib,
+                    @CachedLibrary(limit = "getAttributeAccessInlineCacheMaxDepth()") DynamicObjectLibrary dylib) {
+        // Objects w/o dict that are not classes do not have any special handling
+        String strKey = castKey(castToStrNode, key);
+        dylib.put(object.getStorage(), strKey, value);
+        return true;
+    }
+
+    @Specialization(guards = {"!isHiddenKey(key)", "!lib.hasDict(klass)", "isAttrWritable(klass, key)"}, limit = "1")
+    static boolean writeToDynamicStorageBuiltinType(PythonBuiltinClass klass, Object key, Object value,
+                    @CachedLibrary("klass") @SuppressWarnings("unused") PythonObjectLibrary lib,
+                    @Cached CastToJavaStringNode castToStrNode,
+                    @Cached BranchProfile callAttrUpdate,
+                    @CachedLibrary(limit = "getAttributeAccessInlineCacheMaxDepth()") DynamicObjectLibrary dylib) {
+        String strKey = castKey(castToStrNode, key);
+        try {
+            dylib.put(klass, strKey, value);
+            return true;
+        } finally {
+            if (!klass.canSkipOnAttributeUpdate(strKey, value)) {
+                callAttrUpdate.enter();
+                klass.onAttributeUpdate(strKey, value);
             }
         }
     }
 
-    // write to the DynamicObject
-    @Specialization(guards = {
-                    "isAttrWritable(object, key)",
-                    "isHiddenKey(key) || !lib.hasDict(object)"
-    }, limit = "1")
-    static boolean writeToDynamicStorage(PythonObject object, Object key, Object value,
-                    @CachedLibrary("object") @SuppressWarnings("unused") PythonObjectLibrary lib,
-                    @Cached WriteAttributeToDynamicObjectNode writeAttributeToDynamicObjectNode,
-                    @Exclusive @Cached HandlePythonClassProfiles handlePythonClassProfiles) {
+    static boolean[] createFlag() {
+        return new boolean[1];
+    }
+
+    @Specialization(guards = {"!isHiddenKey(key)", "!lib.hasDict(klass)", "isAttrWritable(klass, key)"}, limit = "1")
+    static boolean writeToDynamicStoragePythonClass(PythonClass klass, Object key, Object value,
+                    @CachedLibrary("klass") @SuppressWarnings("unused") PythonObjectLibrary lib,
+                    @Cached CastToJavaStringNode castToStrNode,
+                    @Cached BranchProfile callAttrUpdate,
+                    @CachedLibrary(limit = "getAttributeAccessInlineCacheMaxDepth()") DynamicObjectLibrary dylib) {
+        String strKey = castKey(castToStrNode, key);
         try {
-            return writeAttributeToDynamicObjectNode.execute(object.getStorage(), key, value);
+            dylib.put(klass, strKey, value);
+            return true;
         } finally {
-            handlePossiblePythonClass(handlePythonClassProfiles, object, key, value);
+            if (!klass.canSkipOnAttributeUpdate(strKey, value)) {
+                callAttrUpdate.enter();
+                klass.onAttributeUpdate(strKey, value);
+            }
         }
     }
 
     // write to the dict
-    @Specialization(guards = {
-                    "!isHiddenKey(key)",
-                    "lib.hasDict(object)"
-    }, limit = "1")
-    static boolean writeToDict(PythonObject object, Object key, Object value,
+    @Specialization(guards = {"!isHiddenKey(key)", "lib.hasDict(object)", "!isManagedClass(object)"}, limit = "1")
+    static boolean writeToDictNoType(PythonObject object, Object key, Object value,
                     @CachedLibrary("object") PythonObjectLibrary lib,
                     @Cached BranchProfile updateStorage,
-                    @CachedLibrary(limit = "1") HashingStorageLibrary hlib,
-                    @Exclusive @Cached HandlePythonClassProfiles handlePythonClassProfiles) {
+                    @CachedLibrary(limit = "1") HashingStorageLibrary hlib) {
+        return writeToDict(lib.getDict(object), key, value, updateStorage, hlib);
+    }
+
+    @Specialization(guards = {"!isHiddenKey(key)", "lib.hasDict(klass)"}, limit = "1")
+    static boolean writeToDictBuiltinType(PythonManagedClass klass, Object key, Object value,
+                    @Cached CastToJavaStringNode castToStrNode,
+                    @Cached BranchProfile callAttrUpdate,
+                    @CachedLibrary("klass") PythonObjectLibrary lib,
+                    @Cached BranchProfile updateStorage,
+                    @CachedLibrary(limit = "1") HashingStorageLibrary hlib) {
+        String strKey = castKey(castToStrNode, key);
         try {
-            return writeToDict(lib.getDict(object), key, value, updateStorage, hlib);
+            return writeToDict(lib.getDict(klass), strKey, value, updateStorage, hlib);
         } finally {
-            handlePossiblePythonClass(handlePythonClassProfiles, object, key, value);
+            if (!klass.canSkipOnAttributeUpdate(strKey, value)) {
+                callAttrUpdate.enter();
+                klass.onAttributeUpdate(strKey, value);
+            }
         }
     }
 
@@ -277,46 +318,4 @@ private String castKey(Object keyObj) {
             return castKeyNode.execute(keyObj);
         }
     }
-
-    protected static final class HandlePythonClassProfiles extends Node {
-        private static final HandlePythonClassProfiles UNCACHED = new HandlePythonClassProfiles(BranchProfile.getUncached(), BranchProfile.getUncached(), BranchProfile.getUncached(),
-                        CastToJavaStringNode.getUncached());
-        final BranchProfile isManagedClass;
-        final BranchProfile isUserClass;
-        final BranchProfile isSpecialKey;
-        @Child CastToJavaStringNode castKeyNode;
-
-        public HandlePythonClassProfiles(BranchProfile isManagedClass, BranchProfile isUserClass, BranchProfile isSpecialKey, CastToJavaStringNode castKeyNode) {
-            this.isManagedClass = isManagedClass;
-            this.isUserClass = isUserClass;
-            this.isSpecialKey = isSpecialKey;
-            this.castKeyNode = castKeyNode;
-        }
-
-        public static HandlePythonClassProfiles create() {
-            return new HandlePythonClassProfiles(BranchProfile.create(), BranchProfile.create(), BranchProfile.create(), null);
-        }
-
-        public static HandlePythonClassProfiles getUncached() {
-            return UNCACHED;
-        }
-
-        String castKey(Object key) {
-            if (castKeyNode == null) {
-                // fast-path w/o node for two most common situations
-                if (key instanceof String) {
-                    return (String) key;
-                } else if (isHiddenKey(key)) {
-                    return null;
-                }
-                CompilerDirectives.transferToInterpreterAndInvalidate();
-                castKeyNode = insert(CastToJavaStringNode.create());
-            }
-            try {
-                return castKeyNode.execute(key);
-            } catch (CannotCastException ex) {
-                return null;
-            }
-        }
-    }
 }
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/sequence/storage/MroSequenceStorage.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/runtime/sequence/storage/MroSequenceStorage.java
@@ -71,6 +71,7 @@ public final class MroSequenceStorage extends TypedSequenceStorage {
      * assumptions will be invalidated if the mro changes.
      */
     private final Map<String, List<Assumption>> attributesInMROFinalAssumptions;
+    private boolean hasAttributesInMROFinalAssumptions;
 
     @CompilationFinal(dimensions = 1) private final PythonAbstractClass[] values;
 
@@ -228,6 +229,7 @@ public Assumption createAttributeInMROFinalAssumption(String name) {
         List<Assumption> attrAssumptions = attributesInMROFinalAssumptions.getOrDefault(name, null);
         if (attrAssumptions == null) {
             attrAssumptions = new ArrayList<>();
+            hasAttributesInMROFinalAssumptions = true;
             attributesInMROFinalAssumptions.put(name, attrAssumptions);
         }
 
@@ -241,6 +243,7 @@ public void addAttributeInMROFinalAssumption(String name, Assumption assumption)
         List<Assumption> attrAssumptions = attributesInMROFinalAssumptions.getOrDefault(name, null);
         if (attrAssumptions == null) {
             attrAssumptions = new ArrayList<>();
+            hasAttributesInMROFinalAssumptions = true;
             attributesInMROFinalAssumptions.put(name, attrAssumptions);
         }
 
@@ -301,4 +304,7 @@ public Object[] getCopyOfInternalArray() {
         return getInternalArray();
     }
 
+    public final boolean hasAttributeInMROFinalAssumptions() {
+        return hasAttributesInMROFinalAssumptions;
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -71,6 +71,7 @@ public final class MroSequenceStorage extends TypedSequenceStorage {`
`71`	`71`	`* assumptions will be invalidated if the mro changes.`
`72`	`72`	`*/`
`73`	`73`	`private final Map<String, List<Assumption>> attributesInMROFinalAssumptions;`
	`74`	`+ private boolean hasAttributesInMROFinalAssumptions;`
`74`	`75`
`75`	`76`	`@CompilationFinal(dimensions = 1) private final PythonAbstractClass[] values;`
`76`	`77`
`@@ -228,6 +229,7 @@ public Assumption createAttributeInMROFinalAssumption(String name) {`
`228`	`229`	`List<Assumption> attrAssumptions = attributesInMROFinalAssumptions.getOrDefault(name, null);`
`229`	`230`	`if (attrAssumptions == null) {`
`230`	`231`	`attrAssumptions = new ArrayList<>();`
	`232`	`+ hasAttributesInMROFinalAssumptions = true;`
`231`	`233`	`attributesInMROFinalAssumptions.put(name, attrAssumptions);`
`232`	`234`	`}`
`233`	`235`
`@@ -241,6 +243,7 @@ public void addAttributeInMROFinalAssumption(String name, Assumption assumption)`
`241`	`243`	`List<Assumption> attrAssumptions = attributesInMROFinalAssumptions.getOrDefault(name, null);`
`242`	`244`	`if (attrAssumptions == null) {`
`243`	`245`	`attrAssumptions = new ArrayList<>();`
	`246`	`+ hasAttributesInMROFinalAssumptions = true;`
`244`	`247`	`attributesInMROFinalAssumptions.put(name, attrAssumptions);`
`245`	`248`	`}`
`246`	`249`
`@@ -301,4 +304,7 @@ public Object[] getCopyOfInternalArray() {`
`301`	`304`	`return getInternalArray();`
`302`	`305`	`}`
`303`	`306`
	`307`	`+ public final boolean hasAttributeInMROFinalAssumptions() {`
	`308`	`+ return hasAttributesInMROFinalAssumptions;`
	`309`	`+ }`
`304`	`310`	`}`