[GR-80048] Optimize cached global and builtin reads.

graalpython/com.oracle.graal.python.test/src/tests/test_global_statement.py

@@ -1,4 +1,4 @@
-# Copyright (c) 2020, 2024, Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2020, 2026, Oracle and/or its affiliates. All rights reserved.
 # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 #
 # The Universal Permissive License (UPL), Version 1.0
@@ -37,11 +37,142 @@
 # OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
 # SOFTWARE.
 
+import builtins
+import types
 import unittest
 
 
 class BasicTests(unittest.TestCase):
 
+    def make_global_reader(self, name="value", **values):
+        module = types.ModuleType("global_read_test")
+        module.__dict__.update(values)
+        exec(f"def read():\n    return {name}\n", module.__dict__)
+        return module, module.read
+
+    def warm_global_reader(self, read, expected):
+        # Exercise the cached interpreter, including its quickened LOAD_GLOBAL.
+        for _ in range(100):
+            self.assertIs(read(), expected)
+
+    def test_cached_global_reassignment(self):
+        module, read = self.make_global_reader(value=object())
+        self.warm_global_reader(read, module.value)
+        for value in (42, 3.5, None, object()):
+            module.value = value
+            self.warm_global_reader(read, value)
+            value = object()
+            module.__dict__["value"] = value
+            self.warm_global_reader(read, value)
+
+    def test_cached_global_delete_and_reinsert(self):
+        module, read = self.make_global_reader(value=object())
+        self.warm_global_reader(read, module.value)
+        del module.value
+        with self.assertRaises(NameError):
+            read()
+        module.value = object()
+        self.warm_global_reader(read, module.value)
+
+    def test_cached_global_storage_replacement(self):
+        for operation in ("clear", "non_string_key", "update_non_string_key"):
+            with self.subTest(operation=operation):
+                module, read = self.make_global_reader(value=object())
+                self.warm_global_reader(read, module.value)
+                namespace = module.__dict__
+                if operation == "clear":
+                    namespace.clear()
+                elif operation == "non_string_key":
+                    namespace[42] = "force general storage"
+                else:
+                    namespace.update({42: "force general storage"})
+                value = object()
+                namespace["value"] = value
+                self.warm_global_reader(read, value)
+                del namespace["value"]
+                with self.assertRaises(NameError):
+                    read()
+
+    def test_cached_global_shared_code_different_globals(self):
+        module, read = self.make_global_reader(value=object())
+        self.warm_global_reader(read, module.value)
+        other = types.ModuleType("other_globals")
+        other.value = object()
+        other_read = types.FunctionType(read.__code__, other.__dict__)
+        plain_value = object()
+        plain_read = types.FunctionType(read.__code__, {"value": plain_value})
+        for _ in range(100):
+            self.assertIs(other_read(), other.value)
+            self.assertIs(read(), module.value)
+            self.assertIs(plain_read(), plain_value)
+
+    def test_cached_builtin_shadow_and_delete(self):
+        module, read = self.make_global_reader("len")
+        self.warm_global_reader(read, builtins.len)
+        module.len = object()
+        self.warm_global_reader(read, module.len)
+        del module.len
+        self.warm_global_reader(read, builtins.len)
+
+    def test_cached_global_falls_back_to_builtin(self):
+        module, read = self.make_global_reader("len", len=object())
+        self.warm_global_reader(read, module.len)
+        del module.len
+        self.warm_global_reader(read, builtins.len)
+
+    def test_cached_builtin_storage_replacement(self):
+        for clear in (False, True):
+            with self.subTest(clear=clear):
+                module, read = self.make_global_reader("len")
+                self.warm_global_reader(read, builtins.len)
+                namespace = module.__dict__
+                if clear:
+                    namespace.clear()
+                else:
+                    namespace[42] = "force general storage"
+                value = object()
+                namespace["len"] = value
+                self.warm_global_reader(read, value)
+
+    def test_cached_builtin_shared_code_different_globals(self):
+        module, read = self.make_global_reader("len")
+        self.warm_global_reader(read, builtins.len)
+        other = types.ModuleType("other_globals")
+        other.len = object()
+        other_read = types.FunctionType(read.__code__, other.__dict__)
+        plain_value = object()
+        plain_read = types.FunctionType(read.__code__, {"len": plain_value})
+        for _ in range(100):
+            self.assertIs(other_read(), other.len)
+            self.assertIs(read(), builtins.len)
+            self.assertIs(plain_read(), plain_value)
+
+    def test_builtin_after_global_deleted_before_first_read(self):
+        module, read = self.make_global_reader("len", len=object())
+        # A deleted property can remain in the shape with a NO_VALUE value.
+        del module.len
+        self.warm_global_reader(read, builtins.len)
+        module.len = object()
+        self.warm_global_reader(read, module.len)
+
+    def test_cached_builtin_reassignment_and_deletion(self):
+        name = "_global_read_test_builtin"
+        self.assertFalse(hasattr(builtins, name))
+        module, read = self.make_global_reader(name)
+        try:
+            for _ in range(2):
+                value = object()
+                setattr(builtins, name, value)
+                self.warm_global_reader(read, value)
+            delattr(builtins, name)
+            with self.assertRaises(NameError):
+                read()
+            module.__dict__[name] = object()
+            self.warm_global_reader(read, module.__dict__[name])
+        finally:
+            if hasattr(builtins, name):
+                delattr(builtins, name)
+
     def test_in_local(self):
         loc = {}
         glob = {}

graalpython/com.oracle.graal.python.test/src/tests/test_graalpy_opts.py

@@ -63,7 +63,10 @@ def tester():
 
         for _ in range(5):
             assert tester() == 3
-        assert_contains_bytecode(tester, "ReadGlobal$ReadBuiltinFastPath")
+        if __graalpython__.is_single_context:
+            assert_contains_bytecode(tester, "ReadGlobal$ReadBuiltinFastPath")
+        else:
+            assert_contains_bytecode(tester, "ReadGlobal$ReadBuiltinFromStorage")
 
 
     def test_read_global_quickening_global():
@@ -72,7 +75,10 @@ def tester():
 
         for _ in range(5):
             assert tester() is sys
-        assert_contains_bytecode(tester, "ReadGlobal$ReadGlobalFastPath")
+        if __graalpython__.is_single_context:
+            assert_contains_bytecode(tester, "ReadGlobal$ReadGlobalFastPath")
+        else:
+            assert_contains_bytecode(tester, "ReadGlobal$ReadGlobalFromStorage")
 
 
     def test_get_attr_quickening_module():

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/frame/ReadBuiltinNode.java

@@ -98,6 +98,7 @@ Object returnBuiltinFromConstantModule(TruffleString attributeId,
                     @Cached("builtins.getShape()") Shape cachedShape,
                     @Cached(value = "getPropertyGetterWithFinalAssumption(cachedShape, attributeId)", neverDefault = false) PropertyGetter getter,
                     @Bind("getterGet(getter, builtins)") Object value) {
+        // Note: this is inlined in ReadGlobalOrBuiltinNode, keep in sync
         return value;
     }
 

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/frame/ReadGlobalOrBuiltinNode.java

@@ -31,13 +31,18 @@
 import com.oracle.graal.python.builtins.objects.common.DynamicObjectStorage;
 import com.oracle.graal.python.builtins.objects.dict.PDict;
 import com.oracle.graal.python.builtins.objects.function.PArguments;
+import com.oracle.graal.python.builtins.objects.module.PythonModule;
 import com.oracle.graal.python.builtins.objects.object.PythonObject;
 import com.oracle.graal.python.lib.PyObjectGetItem.PyObjectGetItemOrNull;
+import com.oracle.graal.python.nodes.BuiltinNames;
 import com.oracle.graal.python.nodes.ErrorMessages;
 import com.oracle.graal.python.nodes.PGuards;
+import com.oracle.graal.python.nodes.PNodeWithContext;
 import com.oracle.graal.python.nodes.PRaiseNode;
 import com.oracle.graal.python.nodes.attributes.ReadAttributeFromPythonObjectNode;
+import com.oracle.graal.python.runtime.PythonContext;
 import com.oracle.graal.python.runtime.exception.PException;
+import com.oracle.graal.python.util.PythonUtils;
 import com.oracle.truffle.api.CompilerAsserts;
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.HostCompilerDirectives.InliningCutoff;
@@ -53,10 +58,12 @@
 import com.oracle.truffle.api.dsl.GenerateUncached;
 import com.oracle.truffle.api.dsl.ImportStatic;
 import com.oracle.truffle.api.dsl.NeverDefault;
+import com.oracle.truffle.api.dsl.NonIdempotent;
 import com.oracle.truffle.api.dsl.Specialization;
 import com.oracle.truffle.api.frame.Frame;
 import com.oracle.truffle.api.frame.VirtualFrame;
 import com.oracle.truffle.api.nodes.Node;
+import com.oracle.truffle.api.object.PropertyGetter;
 import com.oracle.truffle.api.object.Shape;
 import com.oracle.truffle.api.profiles.InlinedBranchProfile;
 import com.oracle.truffle.api.strings.TruffleString;
@@ -65,7 +72,7 @@
 @GenerateInline(false)       // footprint reduction 48 -> 30
 @Proxyable(storeBytecodeIndex = false, allowUncached = true)
 @ConstantOperand(type = TruffleString.class)
-@ImportStatic(PGuards.class)
+@ImportStatic({PGuards.class, PNodeWithContext.class, PythonUtils.class})
 public abstract class ReadGlobalOrBuiltinNode extends Node {
     public abstract Object execute(VirtualFrame frame, TruffleString name);
 
@@ -87,6 +94,22 @@ public static ReadGlobalOrBuiltinNode getUncached() {
         return ReadGlobalOrBuiltinNodeGen.getUncached();
     }
 
+    /**
+     * If globals are a dictionary owned by {@link PythonModule}, then using the shape flag
+     * {@link PythonObject#HAS_MATERIALIZED_DICT} as part of shape check, we can detect when the dictionary storage
+     * of that module has changed, and we must invalidate our cache. This allows us to check only identity of
+     * the globals object and its shape and in the runtime guards avoid the pointer chasing done in this method,
+     * which should be used only at specialization time.
+     */
+    public static PythonModule getGlobalsOwner(VirtualFrame frame) {
+        CompilerAsserts.neverPartOfCompilation();
+        Object obj = PArguments.getGlobals(frame);
+        if (obj instanceof PDict dict && dict.getDictStorage() instanceof DynamicObjectStorage dom && dom.getStore() instanceof PythonModule module) {
+            return module;
+        }
+        return null;
+    }
+
     public static Shape getGlobalsStorageShape(VirtualFrame frame) {
         Object obj = PArguments.getGlobals(frame);
         if (obj instanceof PDict dict && dict.getDictStorage() instanceof DynamicObjectStorage dom) {
@@ -107,9 +130,41 @@ public static Shape getGlobalsStorageShapeIfPropMissing(VirtualFrame frame, Truf
     }
 
     @ForceQuickening
-    @Specialization(guards = {"cachedGlobalsShape != null", "cachedGlobalsShape == getGlobalsStorageShape(frame)"}, //
+    @Specialization(guards = {
+                    /* static: */ "isSingleContext(inliningTarget)", "globalsOwner != null", "globalsShape != null", //
+                    /* static: */ "!hasMaterializedDict(globalsShape)", "builtinGetter != null", //
+                    /* dynamic: */ "getGlobals(frame) == cachedGlobals", "globalsShape == getGlobalsOwnerShape(globalsOwner)", //
+                    /* dynamic: */ "getterAccepts(builtinGetter, builtins)", "!isNoValue(result)"}, //
                     excludeForUncached = true, limit = "1")
     public static Object readBuiltinFastPath(VirtualFrame frame, TruffleString attributeId,
+                    @Bind Node inliningTarget,
+                    @Cached("getGlobals(frame)") Object cachedGlobals,
+                    @Cached("getGlobalsOwner(frame)") PythonModule globalsOwner,
+                    @Cached("getGlobalsStorageShapeIfPropMissing(frame, attributeId)") Shape globalsShape,
+                    @Cached("getBuiltins(inliningTarget)") PythonModule builtins,
+                    @Cached("getBuiltinGetter(builtins, attributeId)") PropertyGetter builtinGetter,
+                    @Bind("getValue(builtins, builtinGetter)") Object result) {
+        // Note: this is inlined version of ReadBuiltinNode#returnBuiltinFromConstantModule, keep in sync
+        // Both shape checks also guard against replacement of the module-backed dict storages.
+        return result;
+    }
+
+    public static PythonModule getBuiltins(Node node) {
+        CompilerAsserts.neverPartOfCompilation();
+        PythonContext context = PythonContext.get(node);
+        return context.isInitialized() ? context.getBuiltins() : context.lookupBuiltinModule(BuiltinNames.T_BUILTINS);
+    }
+
+    public static PropertyGetter getBuiltinGetter(PythonModule builtins, TruffleString name) {
+        // The getter retains the shape, so no separate cached builtins shape is needed.
+        Shape shape = builtins.getShape();
+        return PGuards.hasMaterializedDict(shape) ? null : PythonUtils.getPropertyGetterWithFinalAssumption(shape, name);
+    }
+
+    @ForceQuickening
+    @Specialization(guards = {"cachedGlobalsShape != null", "cachedGlobalsShape == getGlobalsStorageShape(frame)"}, //
+                    replaces = "readBuiltinFastPath", excludeForUncached = true, limit = "1")
+    public static Object readBuiltinFromStorage(VirtualFrame frame, TruffleString attributeId,
                     @Cached("getGlobalsStorageShapeIfPropMissing(frame, attributeId)") Shape cachedGlobalsShape,
                     @Shared("readFromBuiltinsNode") @Cached ReadBuiltinNode readFromBuiltinsNode) {
         return readFromBuiltinsNode.execute(attributeId);
@@ -123,16 +178,59 @@ public static Object readFastFromGlobalStore(VirtualFrame frame, TruffleString n
         return PNone.NO_VALUE;
     }
 
+    @NonIdempotent
+    public static Object getValue(PythonModule m, PropertyGetter getter) {
+        assert m.checkDictFlags();
+        return getter.get(m);
+    }
+
+    @NonIdempotent
+    public static Object getGlobals(VirtualFrame frame) {
+        return PArguments.getGlobals(frame);
+    }
+
+    @NonIdempotent
+    public static Shape getGlobalsOwnerShape(PythonModule module) {
+        return module.getShape();
+    }
+
+    @NonIdempotent
+    public static boolean getterAccepts(PropertyGetter getter, PythonModule module) {
+        return getter.accepts(module);
+    }
+
     @ForceQuickening
-    @Specialization(guards = "!isNoValue(result)", replaces = "readBuiltinFastPath", excludeForUncached = true, limit = "1")
+    @Specialization(guards = {
+                    /* static: */ "isSingleContext(inliningTarget)", "globalsOwner != null", "!hasMaterializedDict(globalsShape)", "getter != null", //
+                    /* dynamic: */ "getGlobals(frame) == cachedGlobals", "getterAccepts(getter, globalsOwner)", "!isNoValue(result)"}, //
+                    replaces = "readBuiltinFromStorage", excludeForUncached = true, limit = "1")
     public static Object readGlobalFastPath(VirtualFrame frame, TruffleString attributeId,
+                    @Bind Node inliningTarget,
+                    @Cached("getGlobals(frame)") Object cachedGlobals,
+                    @Cached("getGlobalsOwner(frame)") PythonModule globalsOwner,
+                    @Cached("globalsOwner.getShape()") Shape globalsShape,
+                    @Cached("getPropertyGetterWithFinalAssumption(globalsShape, attributeId)") PropertyGetter getter,
+                    @Bind("getValue(globalsOwner, getter)") Object result) {
+        CompilerAsserts.partialEvaluationConstant(attributeId);
+        // since the shape does not have MATERIALIZED_DICT shape, and we do shape check on the owner,
+        // the dict storage must not have been replaced
+        assert cachedGlobals instanceof PDict d && //
+                        d.getDictStorage() instanceof DynamicObjectStorage s && //
+                        s.getStore() == globalsOwner;
+        return result;
+    }
+
+    @ForceQuickening
+    @Specialization(guards = "!isNoValue(result)", replaces = "readGlobalFastPath", excludeForUncached = true, limit = "1")
+    public static Object readGlobalFromStorage(VirtualFrame frame, TruffleString attributeId,
                     @Cached(inline = false) ReadAttributeFromPythonObjectNode readNode,
                     @Bind("readFastFromGlobalStore(frame, attributeId, readNode)") Object result) {
         return result;
     }
 
+    @ForceQuickening
     @StoreBytecodeIndex
-    @Specialization(replaces = {"readBuiltinFastPath", "readGlobalFastPath"})
+    @Specialization(replaces = "readGlobalFromStorage")
     public static Object readGlobalOrBuiltinGeneric(VirtualFrame frame, TruffleString attributeId,
                     @Bind Node inliningTarget,
                     @Shared("readFromBuiltinsNode") @Cached ReadBuiltinNode readFromBuiltinsNode,