[GR-54108] Fix BuiltinMethodDescriptors leak.

PullRequest: graalpython/3371

Author: steve-s

graalpython/com.oracle.graal.python.test/src/com/oracle/graal/python/test/advanced/LeakTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 2024, 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
@@ -41,6 +41,7 @@
 package com.oracle.graal.python.test.advanced;
 
 import java.io.IOException;
+import java.io.OutputStream;
 import java.lang.management.ManagementFactory;
 import java.lang.management.ThreadInfo;
 import java.lang.management.ThreadMXBean;
@@ -98,10 +99,14 @@ public static void main(String[] args) {
 
     private boolean sharedEngine = false;
     private boolean keepDump = false;
+    private int repeatAndCheckSize = -1;
+    private boolean nullStdout = false;
     private String languageId;
     private String code;
     private List<String> forbiddenClasses = new ArrayList<>();
 
+    private static final int REPEAT_AND_CHECK_BASLINE_ITERATION = 32;
+
     private final class SystemExit extends RuntimeException {
         private static final long serialVersionUID = 1L;
 
@@ -123,7 +128,7 @@ private void dumpAndAnalyze() {
 
             MBeanServer server = doFullGC();
             String threadDump = getThreadDump();
-            Path dumpFile = dumpHeap(server);
+            Path dumpFile = dumpHeap(server, keepDump);
             boolean fail = checkForLeaks(dumpFile);
             if (fail) {
                 System.err.print(threadDump);
@@ -181,50 +186,6 @@ private boolean checkForLeaks(Path dumpFile) {
             return fail;
         }
 
-        private Path dumpHeap(MBeanServer server) {
-            Path dumpFile = null;
-            try {
-                Path p = Files.createTempDirectory("leakTest");
-                if (!keepDump) {
-                    p.toFile().deleteOnExit();
-                }
-                dumpFile = p.resolve("heapdump.hprof");
-                if (!keepDump) {
-                    dumpFile.toFile().deleteOnExit();
-                } else {
-                    System.out.println("Dump file: " + dumpFile.toString());
-                }
-                HotSpotDiagnosticMXBean mxBean = ManagementFactory.newPlatformMXBeanProxy(server,
-                                "com.sun.management:type=HotSpotDiagnostic", HotSpotDiagnosticMXBean.class);
-                mxBean.dumpHeap(dumpFile.toString(), true);
-            } catch (IOException e) {
-                throw new RuntimeException(e);
-            }
-            return dumpFile;
-        }
-
-        private MBeanServer doFullGC() {
-            // do this a few times to dump a small heap if we can
-            MBeanServer server = null;
-            for (int i = 0; i < 10; i++) {
-                System.gc();
-                Runtime.getRuntime().freeMemory();
-                server = ManagementFactory.getPlatformMBeanServer();
-                try {
-                    ObjectName objectName = new ObjectName("com.sun.management:type=DiagnosticCommand");
-                    server.invoke(objectName, "gcRun", new Object[]{null}, new String[]{String[].class.getName()});
-                } catch (MalformedObjectNameException | InstanceNotFoundException | ReflectionException | MBeanException e) {
-                    throw new RuntimeException(e);
-                }
-                try {
-                    Thread.sleep(2000);
-                } catch (InterruptedException e1) {
-                    // do nothing
-                }
-            }
-            return server;
-        }
-
         private int getCntAndErrors(JavaClass cls, List<String> errors) {
             int cnt = cls.getInstancesCount();
             if (cnt > 0) {
@@ -253,6 +214,58 @@ public final Throwable fillInStackTrace() {
         }
     }
 
+    private static MBeanServer doFullGC() {
+        // do this a few times to dump a small heap if we can
+        MBeanServer server = null;
+        for (int i = 0; i < 10; i++) {
+            System.gc();
+            Runtime.getRuntime().freeMemory();
+            server = ManagementFactory.getPlatformMBeanServer();
+            try {
+                ObjectName objectName = new ObjectName("com.sun.management:type=DiagnosticCommand");
+                server.invoke(objectName, "gcRun", new Object[]{null}, new String[]{String[].class.getName()});
+            } catch (MalformedObjectNameException | InstanceNotFoundException | ReflectionException | MBeanException e) {
+                throw new RuntimeException(e);
+            }
+            try {
+                Thread.sleep(2000);
+            } catch (InterruptedException e1) {
+                // do nothing
+            }
+        }
+        return server;
+    }
+
+    private static Path dumpHeap(MBeanServer server, boolean keepDump) {
+        Path dumpFile = null;
+        try {
+            Path p = Files.createTempDirectory("leakTest");
+            if (!keepDump) {
+                p.toFile().deleteOnExit();
+            }
+            dumpFile = p.resolve("heapdump.hprof");
+            if (!keepDump) {
+                dumpFile.toFile().deleteOnExit();
+            } else {
+                System.out.println("Dump file: " + dumpFile.toString());
+            }
+            HotSpotDiagnosticMXBean mxBean = ManagementFactory.newPlatformMXBeanProxy(server,
+                            "com.sun.management:type=HotSpotDiagnostic", HotSpotDiagnosticMXBean.class);
+            mxBean.dumpHeap(dumpFile.toString(), true);
+        } catch (IOException e) {
+            throw new RuntimeException(e);
+        }
+        return dumpFile;
+    }
+
+    private static long getJavaHeapSize(boolean createHeapDump) {
+        MBeanServer server = doFullGC();
+        if (createHeapDump) {
+            dumpHeap(server, true);
+        }
+        return Runtime.getRuntime().totalMemory() - Runtime.getRuntime().freeMemory();
+    }
+
     @Override
     protected List<String> preprocessArguments(List<String> arguments, Map<String, String> polyglotOptions) {
         ArrayList<String> unrecognized = new ArrayList<>();
@@ -269,6 +282,14 @@ protected List<String> preprocessArguments(List<String> arguments, Map<String, S
                 code = arguments.get(++i);
             } else if (arg.equals("--forbidden-class")) {
                 forbiddenClasses.add(arguments.get(++i));
+            } else if (arg.equals("--repeat-and-check-size")) {
+                repeatAndCheckSize = Integer.parseInt(arguments.get(++i));
+                if (repeatAndCheckSize <= REPEAT_AND_CHECK_BASLINE_ITERATION) {
+                    System.err.printf("--repeat-and-check-size must be at least %d\n", REPEAT_AND_CHECK_BASLINE_ITERATION);
+                    System.exit(1);
+                }
+            } else if (arg.equals("--null-stdout")) {
+                nullStdout = true;
             } else {
                 unrecognized.add(arg);
             }
@@ -284,26 +305,59 @@ protected void launch(Builder contextBuilder) {
             contextBuilder.engine(engine);
         }
         contextBuilder.allowExperimentalOptions(true).allowAllAccess(true);
+        if (nullStdout) {
+            contextBuilder.out(OutputStream.nullOutputStream());
+        }
 
         try (Context c = contextBuilder.build()) {
             try {
                 c.eval(getLanguageId(), code);
             } catch (PolyglotException e) {
-                if (e.isExit()) {
-                    if (e.getExitStatus() == 0) {
-                        throw new SystemExit();
-                    } else {
-                        exit(e.getExitStatus());
+                handleException(e);
+            }
+        }
+
+        if (repeatAndCheckSize > 0) {
+            long initialSize = -1;
+            for (int i = 0; i < repeatAndCheckSize; i++) {
+                if (i == REPEAT_AND_CHECK_BASLINE_ITERATION) {
+                    // Give the system some time to stabilize, fill caches, etc.
+                    initialSize = getJavaHeapSize(keepDump);
+                    System.out.printf("Baseline heap size: %,d\n", initialSize);
+                }
+                try (Context c = contextBuilder.build()) {
+                    try {
+                        c.eval(getLanguageId(), code);
+                    } catch (PolyglotException e) {
+                        handleException(e);
                     }
-                } else {
-                    e.printStackTrace();
-                    exit(255);
                 }
             }
+            // the check at the end will make a dump anyway, so no createHeapDump flag here
+            long currentSize = getJavaHeapSize(false);
+            System.out.printf("Heap size after all repetitions: %,d\n", currentSize);
+            if (currentSize > initialSize * 1.1) {
+                System.err.printf("Heap size grew too much after repeated context creations and invocations. From %,d bytes to %,d bytes.\n", initialSize, currentSize);
+                System.exit(255);
+            }
         }
+
         throw new SystemExit();
     }
 
+    private void handleException(PolyglotException e) {
+        if (e.isExit()) {
+            if (e.getExitStatus() == 0) {
+                throw new SystemExit();
+            } else {
+                exit(e.getExitStatus());
+            }
+        } else {
+            e.printStackTrace();
+            exit(255);
+        }
+    }
+
     @Override
     protected String getLanguageId() {
         return languageId;

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

@@ -410,21 +410,16 @@ def __str__(self):
     if skip_leak_tests:
         return
 
-    common_args = ["--lang", "python",
-                   "--forbidden-class", "com.oracle.graal.python.builtins.objects.object.PythonObject",
-                   "--python.ForceImportSite", "--python.TRegexUsesSREFallback=false"]
-
-    if not all([
-        # test leaks with Python code only
-        run_leak_launcher(common_args + ["--code", "pass", ]),
-        # test leaks when some C module code is involved
-        run_leak_launcher(common_args + ["--code", 'import _testcapi, mmap, bz2; print(memoryview(b"").nbytes)']),
-        # test leaks with shared engine Python code only
-        run_leak_launcher(common_args + ["--shared-engine", "--code", "pass"]),
-        # test leaks with shared engine when some C module code is involved
-        run_leak_launcher(common_args + ["--shared-engine", "--code", 'import _testcapi, mmap, bz2; print(memoryview(b"").nbytes)'])
-    ]):
-        mx.abort(1)
+    # test leaks with Python code only
+    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)'])
+    # test leaks with shared engine Python code only
+    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)'])
 
 
 PYTHON_ARCHIVES = ["GRAALPYTHON_GRAALVM_SUPPORT"]
@@ -2996,7 +2991,10 @@ def exclude_files(*files):
 def run_leak_launcher(input_args):
     print(shlex.join(["mx", "python-leak-test", *input_args]))
 
-    args = input_args
+    args = ["--lang", "python",
+            "--forbidden-class", "com.oracle.graal.python.builtins.objects.object.PythonObject",
+            "--python.ForceImportSite", "--python.TRegexUsesSREFallback=false"]
+    args += input_args
     args = [
         "--keep-dump",
         "--experimental-options",
@@ -3015,24 +3013,25 @@ def run_leak_launcher(input_args):
     vm_args.append("com.oracle.graal.python.test.advanced.LeakTest")
     out = mx.OutputCapture()
     retval = mx.run_java(vm_args + graalpython_args, jdk=jdk, env=env, nonZeroIsFatal=False, out=mx.TeeOutputCapture(out))
-    dump_path = out.data.strip().partition("Dump file:")[2].strip()
+    dump_paths = re.findall(r'Dump file: (\S+)', out.data.strip())
     if retval == 0:
         print("PASSED")
-        if dump_path:
+        if dump_paths:
             print("Removing heapdump for passed test")
-            os.unlink(dump_path)
-        return True
+            for p in dump_paths:
+                os.unlink(p)
     else:
         print("FAILED")
-        if 'CI' in os.environ and dump_path:
-            save_path = os.path.join(SUITE.dir, "dumps", "leak_test")
-            try:
-                os.makedirs(save_path)
-            except OSError:
-                pass
-            dest = shutil.copy(dump_path, save_path)
-            print(f"Heapdump file kept in {dest}")
-        return False
+        if 'CI' in os.environ and dump_paths:
+            for i, dump_path in enumerate(dump_paths):
+                save_path = os.path.join(SUITE.dir, "dumps", f"leak_test{i}")
+                try:
+                    os.makedirs(save_path)
+                except OSError:
+                    pass
+                dest = shutil.copy(dump_path, save_path)
+                print(f"Heapdump file {dump_path} kept in {dest}")
+        mx.abort(1)
 
 
 def no_return(fn):