[GR-42996] More heap scanning refactoring.

PullRequest: graal/15529

Author: cstancu

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/AbstractAnalysisEngine.java

@@ -207,7 +207,20 @@ public void runAnalysis(DebugContext debugContext, Function<AnalysisUniverse, Bo
     private boolean analysisModified() {
         boolean analysisModified;
         try (Timer.StopTimer ignored = verifyHeapTimer.start()) {
-            analysisModified = universe.getHeapVerifier().checkHeapSnapshot(metaAccess, executor, "after analysis", true);
+            /*
+             * After the analysis reaches a stable state check if the shadow heap contains all
+             * objects reachable from roots. If this leads to analysis state changes, an additional
+             * analysis iteration will be run.
+             * 
+             * We reuse the analysis executor, which at this point should be in before-start state:
+             * the analysis finished and it re-initialized the executor for the next iteration. The
+             * verifier controls the life cycle of the executor: it starts it and then waits until
+             * all operations are completed. The same executor is implicitly used by the shadow heap
+             * scanner and the verifier also passes it to the root scanner, so when
+             * checkHeapSnapshot returns all heap scanning and verification tasks are completed.
+             */
+            assert executor.isBeforeStart();
+            analysisModified = universe.getHeapVerifier().checkHeapSnapshot(metaAccess, executor, "during analysis", true);
         }
         /* Initialize for the next iteration. */
         executor.init(getTiming());

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/heap/HeapSnapshotVerifier.java

@@ -73,6 +73,12 @@ public HeapSnapshotVerifier(BigBang bb, ImageHeap imageHeap, ImageHeapScanner sc
         verbosity = Options.HeapVerifierVerbosity.getValue(bb.getOptions());
     }
 
+    /**
+     * Heap verification does a complete scan from roots (static fields and embedded constant) and
+     * compares the object graph against the shadow heap. If any new reachable objects or primitive
+     * values are found then the verifier automatically patches the shadow heap. If this is during
+     * analysis then the heap scanner will also notify the analysis of the new objects.
+     */
     public boolean checkHeapSnapshot(UniverseMetaAccess metaAccess, CompletionExecutor executor, String phase, boolean forAnalysis) {
         info("Verifying the heap snapshot %s%s ...", phase, (forAnalysis ? ", iteration " + iterations : ""));
         analysisModified = false;

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/heap/ImageHeapScanner.java

@@ -307,9 +307,7 @@ private Optional<JavaConstant> maybeReplace(JavaConstant constant, ScanReason re
         } else if (unwrapped instanceof ImageHeapConstant) {
             throw GraalError.shouldNotReachHere(formatReason("Double wrapping of constant. Most likely, the reachability analysis code itself is seen as reachable.", reason)); // ExcludeFromJacocoGeneratedReport
         }
-        if (unwrapped instanceof String || unwrapped instanceof Enum<?>) {
-            forceHashCodeComputation(unwrapped);
-        }
+        maybeForceHashCodeComputation(unwrapped);
 
         /* Run all registered object replacers. */
         if (constant.getJavaKind() == JavaKind.Object) {
@@ -328,12 +326,26 @@ private Optional<JavaConstant> maybeReplace(JavaConstant constant, ScanReason re
         return Optional.empty();
     }
 
+    public static void maybeForceHashCodeComputation(Object constant) {
+        if (constant instanceof String stringConstant) {
+            forceHashCodeComputation(stringConstant);
+        } else if (constant instanceof Enum<?> enumConstant) {
+            /*
+             * Starting with JDK 21, Enum caches the identity hash code in a separate hash field. We
+             * want to allow Enum values to be manually marked as immutable objects, so we eagerly
+             * initialize the hash field. This is safe because Enum.hashCode() is a final method,
+             * i.e., cannot be overwritten by the user.
+             */
+            forceHashCodeComputation(enumConstant);
+        }
+    }
+
     /**
      * For immutable Strings and other objects in the native image heap, force eager computation of
      * the hash field.
      */
     @SuppressFBWarnings(value = "RV_RETURN_VALUE_IGNORED", justification = "eager hash field computation")
-    public static void forceHashCodeComputation(Object object) {
+    private static void forceHashCodeComputation(Object object) {
         object.hashCode();
     }
 
@@ -522,24 +534,6 @@ protected boolean skipScanning() {
         return false;
     }
 
-    /**
-     * When a re-scanning is triggered while the analysis is running in parallel, it is necessary to
-     * do the re-scanning in a separate executor task to avoid deadlocks. For example,
-     * lookupJavaField might need to wait for the reachability handler to be finished that actually
-     * triggered the re-scanning.
-     *
-     * In the (legacy) Feature.duringAnalysis state, the executor is not running and we must not
-     * schedule new tasks, because that would be treated as "the analysis has not finished yet". So
-     * in that case we execute the task directly.
-     */
-    private void maybeRunInExecutor(CompletionExecutor.DebugContextRunnable task) {
-        if (bb.executorIsStarted()) {
-            bb.postTask(task);
-        } else {
-            task.run(null);
-        }
-    }
-
     public void rescanRoot(Field reflectionField) {
         if (skipScanning()) {
             return;
@@ -714,7 +708,31 @@ protected AnalysisField lookupJavaField(String className, String fieldName) {
         return metaAccess.lookupJavaField(ReflectionUtil.lookupField(getClass(className), fieldName));
     }
 
-    public void postTask(Runnable task) {
-        universe.getBigbang().postTask(debug -> task.run());
+    /**
+     * When a re-scanning is triggered while the analysis is running in parallel, it is necessary to
+     * do the re-scanning in a separate executor task to avoid deadlocks. For example,
+     * lookupJavaField might need to wait for the reachability handler to be finished that actually
+     * triggered the re-scanning. We reuse the analysis executor, whose lifetime is controlled by
+     * the analysis engine.
+     *
+     * In the (legacy) Feature.duringAnalysis state, the executor is not running and we must not
+     * schedule new tasks, because that would be treated as "the analysis has not finished yet". So
+     * in that case we execute the task directly.
+     */
+    private void maybeRunInExecutor(CompletionExecutor.DebugContextRunnable task) {
+        if (bb.executorIsStarted()) {
+            bb.postTask(task);
+        } else {
+            task.run(null);
+        }
+    }
+
+    /**
+     * Post the task to the analysis executor. Its lifetime is controlled by the analysis engine or
+     * the heap verifier such that all heap scanning tasks are also completed when analysis reaches
+     * a stable state or heap verification is completed.
+     */
+    private void postTask(Runnable task) {
+        bb.postTask(debug -> task.run());
     }
 }

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/util/CompletionExecutor.java

@@ -277,6 +277,10 @@ public void shutdown() {
         setState(State.UNUSED);
     }
 
+    public boolean isBeforeStart() {
+        return state.get() == State.BEFORE_START;
+    }
+
     public boolean isStarted() {
         return state.get() == State.STARTED;
     }

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageHeap.java

@@ -352,16 +352,9 @@ public void addConstant(final JavaConstant constant, boolean immutableFromParent
         VMError.guarantee(identityHashCode != 0, "0 is used as a marker value for 'hash code not yet computed'");
 
         Object objectConstant = hUniverse.getSnippetReflection().asObject(Object.class, uncompressed);
+        ImageHeapScanner.maybeForceHashCodeComputation(objectConstant);
         if (objectConstant instanceof String stringConstant) {
             handleImageString(stringConstant);
-        } else if (objectConstant instanceof Enum<?> enumConstant) {
-            /*
-             * Starting with JDK 21, Enum caches the identity hash code in a separate hash field. We
-             * want to allow Enum values to be manually marked as immutable objects, so we eagerly
-             * initialize the hash field. This is safe because Enum.hashCode() is a final method,
-             * i.e., cannot be overwritten by the user.
-             */
-            ImageHeapScanner.forceHashCodeComputation(enumConstant);
         }
 
         final ObjectInfo existing = objects.get(uncompressed);
@@ -457,7 +450,6 @@ private boolean assertFillerObjectSizes() {
     }
 
     private void handleImageString(final String str) {
-        ImageHeapScanner.forceHashCodeComputation(str);
         if (HostedStringDeduplication.isInternedString(str)) {
             /* The string is interned by the host VM, so it must also be interned in our image. */
             assert internedStrings.containsKey(str) || internStringsPhase.isAllowed() : "Should not intern string during phase " + internStringsPhase.toString();