Address review feedback

msimacek · msimacek · commit bd6246be6b5a · 2021-09-13T16:22:47.000+02:00
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/lzma/LZMANodes.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/lzma/LZMANodes.java
@@ -236,11 +236,10 @@ HashingStorage fast(VirtualFrame frame, PDict dict,
         HashingStorage slow(VirtualFrame frame, Object object,
                         @Shared("id") @Cached ConditionProfile idErrorProfile,
                         @Shared("k") @Cached ConditionProfile hasKeyErrorProfile,
-                        @Cached ConditionProfile dictErrorProfile,
                         @Shared("h") @CachedLibrary(limit = "2") HashingStorageLibrary hlib,
                         @Cached GetDictIfExistsNode getDict) {
             PDict dict = getDict.execute(object);
-            if (dictErrorProfile.profile(dict == null)) {
+            if (dict == null) {
                 throw raise(TypeError, FILTER_SPEC_MUST_BE_DICT);
             }
             return fast(frame, dict, idErrorProfile, hasKeyErrorProfile, hlib);
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/thread/ThreadLocalBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/thread/ThreadLocalBuiltins.java
@@ -126,6 +126,7 @@ protected Object doIt(VirtualFrame frame, PThreadLocal object, Object keyObj,
                         @Cached LookupAttributeInMRONode.Dynamic lookup,
                         @Cached GetClassNode getClassNode,
                         @Cached CastToJavaStringNode castKeyToStringNode) {
+            // Note: getting thread local dict has potential side-effects, don't move
             PDict localDict = getThreadLocalDict.execute(frame, object);
             String key;
             try {
@@ -239,13 +240,14 @@ protected PNone doStringKey(VirtualFrame frame, PThreadLocal object, String keyO
                         @Cached ThreadLocalNodes.GetThreadLocalDict getThreadLocalDict,
                         @Cached GetClassNode getClassNode,
                         @Cached LookupAttributeInMRONode.Dynamic getExisting) {
+            // Note: getting thread local dict has potential side-effects, don't move
+            PDict localDict = getThreadLocalDict.execute(frame, object);
             String key;
             try {
                 key = castKeyToStringNode.execute(keyObject);
             } catch (CannotCastException e) {
                 throw raise(PythonBuiltinClassType.TypeError, ErrorMessages.ATTR_NAME_MUST_BE_STRING, keyObject);
             }
-            PDict localDict = getThreadLocalDict.execute(frame, object);
             Object type = getClassNode.execute(object);
             Object descr = getExisting.execute(type, key);
             if (descr != PNone.NO_VALUE) {
@@ -314,13 +316,14 @@ protected PNone doIt(VirtualFrame frame, PThreadLocal object, Object keyObj,
                         @Cached CallBinaryMethodNode callDelete,
                         @CachedLibrary(limit = "3") HashingStorageLibrary hlib,
                         @Cached CastToJavaStringNode castKeyToStringNode) {
+            // Note: getting thread local dict has potential side-effects, don't move
+            PDict localDict = getThreadLocalDict.execute(frame, object);
             String key;
             try {
                 key = castKeyToStringNode.execute(keyObj);
             } catch (CannotCastException e) {
                 throw raise(PythonBuiltinClassType.TypeError, ErrorMessages.ATTR_NAME_MUST_BE_STRING, keyObj);
             }
-            PDict localDict = getThreadLocalDict.execute(frame, object);
             Object type = getClassNode.execute(object);
             Object descr = getExisting.execute(type, key);
             if (descr != PNone.NO_VALUE) {
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/TypeBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/TypeBuiltins.java
@@ -353,13 +353,13 @@ public final Object varArgExecute(VirtualFrame frame, @SuppressWarnings("unused"
 
         @Specialization
         Object selfInArgs(VirtualFrame frame, @SuppressWarnings("unused") PNone self, Object[] arguments, PKeyword[] keywords,
-                        @Cached CallNodeHelper callNode) {
+                        @Shared("callNode") @Cached CallNodeHelper callNode) {
             return callNode.execute(frame, arguments[0], arguments, keywords, false);
         }
 
         @Fallback
         Object selfSeparate(VirtualFrame frame, Object self, Object[] arguments, PKeyword[] keywords,
-                        @Cached CallNodeHelper callNode) {
+                        @Shared("callNode") @Cached CallNodeHelper callNode) {
             return callNode.execute(frame, self, arguments, keywords, true);
         }
     }
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/ReadAttributeFromObjectNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/ReadAttributeFromObjectNode.java
@@ -98,10 +98,16 @@ public static ReadAttributeFromObjectNode getUncachedForceType() {
     static final int MAX_DICT_TYPES = 2;
 
     // read from the DynamicObject store
-    @Specialization(guards = "getDict.execute(object) == null || isHiddenKey(key)", limit = "1")
+    @Specialization(guards = "isHiddenKey(key)")
+    protected static Object readFromDynamicStorageHidden(PythonObject object, Object key,
+                    @Shared("readDynamic") @Cached ReadAttributeFromDynamicObjectNode readAttributeFromDynamicObjectNode) {
+        return readAttributeFromDynamicObjectNode.execute(object.getStorage(), key);
+    }
+
+    @Specialization(guards = "getDict.execute(object) == null", limit = "1")
     protected static Object readFromDynamicStorage(PythonObject object, Object key,
                     @SuppressWarnings("unused") @Shared("getDict") @Cached GetDictIfExistsNode getDict,
-                    @Cached ReadAttributeFromDynamicObjectNode readAttributeFromDynamicObjectNode) {
+                    @Shared("readDynamic") @Cached ReadAttributeFromDynamicObjectNode readAttributeFromDynamicObjectNode) {
         return readAttributeFromDynamicObjectNode.execute(object.getStorage(), key);
     }
 
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/frame/ReadGlobalOrBuiltinNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/frame/ReadGlobalOrBuiltinNode.java
@@ -98,8 +98,8 @@ protected Object readGlobal(@SuppressWarnings("unused") VirtualFrame frame,
         return returnGlobalOrBuiltin(result);
     }
 
-    protected static HashingStorage getStorage(Object cachedGlobals) {
-        return ((PDict) cachedGlobals).getDictStorage();
+    protected static HashingStorage getStorage(Object globals) {
+        return ((PDict) globals).getDictStorage();
     }
 
     @Specialization(guards = "isBuiltinDictUnchangedStorage(frame, builtinProfile, cachedGlobals, cachedStorage)", assumptions = "singleContextAssumption()", limit = "1")
@@ -130,15 +130,13 @@ protected Object readGlobalBuiltinDictCached(@SuppressWarnings("unused") Virtual
         return returnGlobalOrBuiltin(result == null ? PNone.NO_VALUE : result);
     }
 
-    public static HashingStorage getGlobalStorage(VirtualFrame frame) {
-        return ((PDict) PArguments.getGlobals(frame)).getDictStorage();
-    }
-
-    @Specialization(guards = "isBuiltinDict(getGlobals(frame), builtinProfile)", replaces = {"readGlobalBuiltinDictCached", "readGlobalBuiltinDictCachedUnchangedStorage"})
-    protected Object readGlobalBuiltinDict(VirtualFrame frame,
-                    @CachedLibrary(limit = "3") HashingStorageLibrary hlib,
+    @Specialization(guards = "isBuiltinDict(globals, builtinProfile)", replaces = {"readGlobalBuiltinDictCached", "readGlobalBuiltinDictCachedUnchangedStorage"}, limit = "3")
+    protected Object readGlobalBuiltinDict(@SuppressWarnings("unused") VirtualFrame frame,
+                    @SuppressWarnings("unused") @Bind("getGlobals(frame)") Object globals,
+                    @Bind("getStorage(globals)") HashingStorage storage,
+                    @CachedLibrary("storage") HashingStorageLibrary hlib,
                     @Cached @SuppressWarnings("unused") IsBuiltinClassProfile builtinProfile) {
-        Object result = hlib.getItem(getGlobalStorage(frame), attributeId);
+        Object result = hlib.getItem(storage, attributeId);
         return returnGlobalOrBuiltin(result == null ? PNone.NO_VALUE : result);
     }
 
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/object/GetOrCreateDictNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/object/GetOrCreateDictNode.java
@@ -53,6 +53,7 @@
 import com.oracle.truffle.api.dsl.Fallback;
 import com.oracle.truffle.api.dsl.GenerateUncached;
 import com.oracle.truffle.api.dsl.Specialization;
+import com.oracle.truffle.api.profiles.BranchProfile;
 
 @GenerateUncached
 public abstract class GetOrCreateDictNode extends PNodeWithContext {
@@ -62,9 +63,11 @@ public abstract class GetOrCreateDictNode extends PNodeWithContext {
     static PDict doPythonObject(PythonObject object,
                     @Shared("getDict") @Cached GetDictIfExistsNode getDictIfExistsNode,
                     @Cached SetDictNode setDictNode,
+                    @Cached BranchProfile createDict,
                     @Cached PythonObjectFactory factory) {
         PDict dict = getDictIfExistsNode.execute(object);
         if (dict == null) {
+            createDict.enter();
             dict = factory.createDictFixedStorage(object);
             setDictNode.execute(object, dict);
         }

Original file line number	Diff line number	Diff line change
`@@ -353,13 +353,13 @@ public final Object varArgExecute(VirtualFrame frame, @SuppressWarnings("unused"`
`353`	`353`
`354`	`354`	`@Specialization`
`355`	`355`	`Object selfInArgs(VirtualFrame frame, @SuppressWarnings("unused") PNone self, Object[] arguments, PKeyword[] keywords,`
`356`		`- @Cached CallNodeHelper callNode) {`
	`356`	`+ @Shared("callNode") @Cached CallNodeHelper callNode) {`
`357`	`357`	`return callNode.execute(frame, arguments[0], arguments, keywords, false);`
`358`	`358`	`}`
`359`	`359`
`360`	`360`	`@Fallback`
`361`	`361`	`Object selfSeparate(VirtualFrame frame, Object self, Object[] arguments, PKeyword[] keywords,`
`362`		`- @Cached CallNodeHelper callNode) {`
	`362`	`+ @Shared("callNode") @Cached CallNodeHelper callNode) {`
`363`	`363`	`return callNode.execute(frame, self, arguments, keywords, true);`
`364`	`364`	`}`
`365`	`365`	`}`