Fix leak of BuiltinMethodDescriptors

Author: steve-s

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/function/BuiltinMethodDescriptor.java

@@ -56,6 +56,7 @@
 import com.oracle.graal.python.nodes.function.builtins.PythonTernaryBuiltinNode;
 import com.oracle.graal.python.nodes.function.builtins.PythonUnaryBuiltinNode;
 import com.oracle.truffle.api.CompilerAsserts;
+import com.oracle.truffle.api.dsl.GeneratedBy;
 import com.oracle.truffle.api.dsl.NodeFactory;
 
 /**
@@ -251,6 +252,11 @@ public boolean forceSplitDirectCalls() {
 
     private static BuiltinMethodDescriptor get(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type, Builtin builtinAnnotation) {
         CompilerAsserts.neverPartOfCompilation();
+        if (factory.getClass().getAnnotation(GeneratedBy.class) == null) {
+            // For non-generated factories, we do not assume that they are singletons, so we cannot
+            // use them for our cache that must be bounded in size
+            return null;
+        }
         Class<? extends PythonBuiltinBaseNode> nodeClass = factory.getNodeClass();
         BuiltinMethodDescriptor result = null;
         if (PythonUnaryBuiltinNode.class.isAssignableFrom(nodeClass)) {
@@ -349,6 +355,10 @@ public final boolean equals(Object o) {
             return false;
         }
         BuiltinMethodDescriptor that = (BuiltinMethodDescriptor) o;
+        // Rudimentary check of the assumption that builtin node factories are singletons, if we
+        // create&cache BuiltinMethodDescriptors with non-singleton factories, we do not have the
+        // guarantee that the cache size is bounded
+        assert (factory.getNodeClass() == that.factory.getNodeClass()) == (factory == that.factory) : name;
         return factory == that.factory && type == that.type && name.equals(that.name);
     }
 

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/tuple/StructSequence.java

@@ -230,13 +230,12 @@ public boolean equals(Object o) {
             if (this == o) {
                 return true;
             }
-            if (!(o instanceof BuiltinTypeDescriptor)) {
+            if (!(o instanceof BuiltinTypeDescriptor that)) {
                 return false;
             }
             if (!super.equals(o)) {
                 return false;
             }
-            BuiltinTypeDescriptor that = (BuiltinTypeDescriptor) o;
             return type == that.type;
         }
 
@@ -258,6 +257,12 @@ public static void initType(PythonLanguage language, Object klass, Descriptor de
 
     @TruffleBoundary
     public static void initType(PythonObjectSlowPathFactory factory, PythonLanguage language, Object klass, Descriptor desc) {
+        // TODO: (GR-54701) some builtin nodes created here close over the "desc" object - it should
+        // be their key for the call targets cache in language, but one can create new Descriptor
+        // object at runtime via C API and those are context specific. There are no runtime data, so
+        // we can cache Descriptor instances in language, but it should be weak cache and we should
+        // make sure that once the type that is initialized is GC'ed, they can be GC'ed too.
+
         assert IsSubtypeNode.getUncached().execute(klass, PythonBuiltinClassType.PTuple);
 
         long flags = TypeNodes.GetTypeFlagsNode.executeUncached(klass);

mx.graalpython/mx_graalpython.py

@@ -411,13 +411,13 @@ def __str__(self):
         return
 
     # test leaks with Python code only
-    run_leak_launcher(["--code", "pass", ]),
-    run_leak_launcher(["--repeat-and-check-size", "--code", "--null-stdout", "print('hello')"]),
+    run_leak_launcher(["--code", "pass", ])
+    run_leak_launcher(["--repeat-and-check-size", "250", "--null-stdout", "--code", "print('hello')"])
     # test leaks when some C module code is involved
-    run_leak_launcher(["--code", 'import _testcapi, mmap, bz2; print(memoryview(b"").nbytes)']),
+    run_leak_launcher(["--code", 'import _testcapi, mmap, bz2; print(memoryview(b"").nbytes)'])
     # test leaks with shared engine Python code only
-    run_leak_launcher(["--shared-engine", "--code", "pass"]),
-    run_leak_launcher(["--shared-engine", "--repeat-and-check-size", "--code", "--null-stdout", "print('hello')"]),
+    run_leak_launcher(["--shared-engine", "--code", "pass"])
+    # TODO: (GR-54727) run_leak_launcher(["--shared-engine", "--repeat-and-check-size", "250", "--null-stdout", "--code", "print('hello')"]),
     # test leaks with shared engine when some C module code is involved
     run_leak_launcher(["--shared-engine", "--code", 'import _testcapi, mmap, bz2; print(memoryview(b"").nbytes)'])