[GR-52363] Clean up IdentityHashCodeProvider.

PullRequest: graal/17122
oracle · Feb 29, 2024 · 486451e · 486451e
2 parents 8f380d3 + 9fb3f09
commit 486451e
Show file tree

Hide file tree

Showing 31 changed files with 217 additions and 254 deletions.
diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/type/TypedConstant.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/common/type/TypedConstant.java
@@ -30,6 +30,4 @@
 
 public interface TypedConstant extends JavaConstant {
     ResolvedJavaType getType(MetaAccessProvider provider);
-
-    int getIdentityHashCode();
 }
diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/HotSpotBackendFactory.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/HotSpotBackendFactory.java
@@ -28,14 +28,14 @@
 import static jdk.vm.ci.services.Services.IS_BUILDING_NATIVE_IMAGE;
 import static jdk.vm.ci.services.Services.IS_IN_NATIVE_IMAGE;
 
-import jdk.graal.compiler.api.replacements.SnippetReflectionProvider;
 import jdk.graal.compiler.bytecode.BytecodeProvider;
 import jdk.graal.compiler.core.common.spi.ConstantFieldProvider;
 import jdk.graal.compiler.debug.Assertions;
 import jdk.graal.compiler.debug.DebugContext;
 import jdk.graal.compiler.graph.Node;
 import jdk.graal.compiler.hotspot.meta.HotSpotGraalConstantFieldProvider;
 import jdk.graal.compiler.hotspot.meta.HotSpotHostForeignCallsProvider;
+import jdk.graal.compiler.hotspot.meta.HotSpotIdentityHashCodeProvider;
 import jdk.graal.compiler.hotspot.meta.HotSpotLoweringProvider;
 import jdk.graal.compiler.hotspot.meta.HotSpotMetaAccessExtensionProvider;
 import jdk.graal.compiler.hotspot.meta.HotSpotPlatformConfigurationProvider;
@@ -192,7 +192,7 @@ public final HotSpotBackend createBackend(HotSpotGraalRuntimeProvider graalRunti
             }
             IdentityHashCodeProvider identityHashCodeProvider;
             try (InitTimer rt = timer("create IdentityHashCode provider")) {
-                identityHashCodeProvider = createIdentityHashCodeProvider(snippetReflection);
+                identityHashCodeProvider = createIdentityHashCodeProvider();
             }
             providers = new HotSpotProviders(metaAccess, codeCache, constantReflection, constantFieldProvider, foreignCalls, lowerer, null, null, registers,
                             snippetReflection, wordTypes, stampProvider, platformConfigurationProvider, metaAccessExtensionProvider, loopsDataProvider, config, identityHashCodeProvider);
@@ -222,8 +222,8 @@ public final HotSpotBackend createBackend(HotSpotGraalRuntimeProvider graalRunti
         }
     }
 
-    protected IdentityHashCodeProvider createIdentityHashCodeProvider(SnippetReflectionProvider snippetReflection) {
-        return new IdentityHashCodeProvider(snippetReflection);
+    protected IdentityHashCodeProvider createIdentityHashCodeProvider() {
+        return new HotSpotIdentityHashCodeProvider();
     }
 
     protected abstract HotSpotBackend createBackend(GraalHotSpotVMConfig config, HotSpotGraalRuntimeProvider graalRuntime, HotSpotProviders providers);

diff --git a/.../code/HostedIdentityHashCodeProvider.java → ...meta/HotSpotIdentityHashCodeProvider.java b/.../code/HostedIdentityHashCodeProvider.java → ...meta/HotSpotIdentityHashCodeProvider.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2024, 2024, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -22,34 +22,23 @@
  * or visit www.oracle.com if you need additional information or have any
  * questions.
  */
-package com.oracle.svm.hosted.code;
+package jdk.graal.compiler.hotspot.meta;
 
-import com.oracle.svm.core.SubstrateUtil;
-import com.oracle.svm.core.hub.DynamicHub;
-import com.oracle.svm.core.util.VMError;
-
-import jdk.graal.compiler.api.replacements.SnippetReflectionProvider;
 import jdk.graal.compiler.nodes.spi.IdentityHashCodeProvider;
+import jdk.vm.ci.hotspot.HotSpotObjectConstant;
 import jdk.vm.ci.meta.JavaConstant;
+import jdk.vm.ci.meta.JavaKind;
 
-public class HostedIdentityHashCodeProvider extends IdentityHashCodeProvider {
-
-    public HostedIdentityHashCodeProvider(SnippetReflectionProvider snippetReflection) {
-        super(snippetReflection);
-    }
+public class HotSpotIdentityHashCodeProvider implements IdentityHashCodeProvider {
 
     @Override
-    public int identityHashCode(JavaConstant constant) {
-        VMError.guarantee(SubstrateUtil.HOSTED);
-        Object object = snippetReflection.asObject(Object.class, constant);
-        if (object instanceof DynamicHub) {
-            /*
-             * We need to use the identity hash code of the original java.lang.Class object and not
-             * of the DynamicHub, so that hash maps that are filled during image generation and use
-             * Class keys still work at run time.
-             */
-            return System.identityHashCode(((DynamicHub) object).getHostedJavaClass());
+    public Integer identityHashCode(JavaConstant constant) {
+        if (constant == null || constant.getJavaKind() != JavaKind.Object) {
+            return null;
+        } else if (constant.isNull()) {
+            /* System.identityHashCode is specified to return 0 when passed null. */
+            return 0;
         }
-        return super.identityHashCode(constant);
+        return ((HotSpotObjectConstant) constant).getIdentityHashCode();
     }
 }
diff --git a/...aal.compiler/src/jdk/graal/compiler/hotspot/replacements/HotSpotIdentityHashCodeNode.java b/...aal.compiler/src/jdk/graal/compiler/hotspot/replacements/HotSpotIdentityHashCodeNode.java
@@ -24,16 +24,14 @@
  */
 package jdk.graal.compiler.hotspot.replacements;
 
+import org.graalvm.word.LocationIdentity;
+
 import jdk.graal.compiler.graph.NodeClass;
 import jdk.graal.compiler.nodeinfo.NodeCycles;
 import jdk.graal.compiler.nodeinfo.NodeInfo;
 import jdk.graal.compiler.nodeinfo.NodeSize;
 import jdk.graal.compiler.nodes.ValueNode;
 import jdk.graal.compiler.replacements.nodes.IdentityHashCodeNode;
-import org.graalvm.word.LocationIdentity;
-
-import jdk.vm.ci.hotspot.HotSpotObjectConstant;
-import jdk.vm.ci.meta.JavaConstant;
 
 @NodeInfo(cycles = NodeCycles.CYCLES_4, size = NodeSize.SIZE_16)
 public class HotSpotIdentityHashCodeNode extends IdentityHashCodeNode {
@@ -48,9 +46,4 @@ public HotSpotIdentityHashCodeNode(ValueNode object, int bci) {
     public LocationIdentity getKilledLocationIdentity() {
         return HotSpotReplacementsUtil.MARK_WORD_LOCATION;
     }
-
-    @Override
-    protected int getIdentityHashCode(JavaConstant constant) {
-        return ((HotSpotObjectConstant) constant).getIdentityHashCode();
-    }
 }
diff --git a/...ler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/spi/IdentityHashCodeProvider.java b/...ler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/spi/IdentityHashCodeProvider.java
@@ -24,18 +24,24 @@
  */
 package jdk.graal.compiler.nodes.spi;
 
-import jdk.graal.compiler.api.replacements.SnippetReflectionProvider;
 import jdk.vm.ci.meta.JavaConstant;
 
-public class IdentityHashCodeProvider {
-
-    protected final SnippetReflectionProvider snippetReflection;
-
-    public IdentityHashCodeProvider(SnippetReflectionProvider snippetReflection) {
-        this.snippetReflection = snippetReflection;
-    }
+/**
+ * GR-52365: This interface can be removed when {@link jdk.vm.ci.meta.ConstantReflectionProvider}
+ * offers an {@code identityHashCode} method too.
+ */
+public interface IdentityHashCodeProvider {
 
-    public int identityHashCode(JavaConstant constant) {
-        return System.identityHashCode(snippetReflection.asObject(Object.class, constant));
-    }
+    /**
+     * Reads the identity hash code of the given object. Returns {@code null} if the constant is not
+     * an object, or if the value is not available at this point.
+     * <p>
+     * This method usually computes and stores the identity hash code if it was not computed for the
+     * object beforehand, but it is not required to do so - it can also return {@code null} if the
+     * computation is not possible or desired at the current time.
+     * <p>
+     * For the {@link JavaConstant#isNull() null constant}, this method returns zero as specified by
+     * {@link System#identityHashCode(Object)}.
+     */
+    Integer identityHashCode(JavaConstant constant);
 }
diff --git a/...rc/jdk.graal.compiler/src/jdk/graal/compiler/replacements/nodes/IdentityHashCodeNode.java b/...rc/jdk.graal.compiler/src/jdk/graal/compiler/replacements/nodes/IdentityHashCodeNode.java
@@ -24,22 +24,19 @@
  */
 package jdk.graal.compiler.replacements.nodes;
 
-import jdk.graal.compiler.core.common.type.AbstractObjectStamp;
 import jdk.graal.compiler.core.common.type.IntegerStamp;
-import jdk.graal.compiler.debug.Assertions;
 import jdk.graal.compiler.graph.Node;
 import jdk.graal.compiler.graph.NodeClass;
 import jdk.graal.compiler.nodeinfo.NodeInfo;
 import jdk.graal.compiler.nodes.AbstractStateSplit;
 import jdk.graal.compiler.nodes.ConstantNode;
 import jdk.graal.compiler.nodes.DeoptBciSupplier;
-import jdk.graal.compiler.nodes.NodeView;
 import jdk.graal.compiler.nodes.ValueNode;
 import jdk.graal.compiler.nodes.memory.SingleMemoryKill;
 import jdk.graal.compiler.nodes.spi.Canonicalizable;
 import jdk.graal.compiler.nodes.spi.CanonicalizerTool;
+import jdk.graal.compiler.nodes.spi.CoreProviders;
 import jdk.graal.compiler.nodes.spi.Lowerable;
-import jdk.vm.ci.meta.JavaConstant;
 
 @NodeInfo
 public abstract class IdentityHashCodeNode extends AbstractStateSplit implements Canonicalizable, Lowerable, SingleMemoryKill, DeoptBciSupplier {
@@ -71,20 +68,17 @@ public void setBci(int bci) {
 
     @Override
     public Node canonical(CanonicalizerTool tool) {
-        if (object.isConstant()) {
-            assert object.stamp(NodeView.DEFAULT) instanceof AbstractObjectStamp : Assertions.errorMessage(object);
-            JavaConstant c = (JavaConstant) object.asConstant();
+        ValueNode result = canonical(object, tool);
+        return result != null ? result : this;
+    }
 
-            int identityHashCode;
-            if (c.isNull()) {
-                identityHashCode = 0;
-            } else {
-                identityHashCode = getIdentityHashCode(c);
+    protected static ValueNode canonical(ValueNode object, CoreProviders providers) {
+        if (object.isJavaConstant()) {
+            Integer identityHashCode = providers.getIdentityHashCodeProvider().identityHashCode(object.asJavaConstant());
+            if (identityHashCode != null) {
+                return ConstantNode.forInt(identityHashCode);
             }
-            return ConstantNode.forInt(identityHashCode);
         }
-        return this;
+        return null;
     }
-
-    protected abstract int getIdentityHashCode(JavaConstant constant);
 }
diff --git a/....graal.pointsto.standalone/src/com/oracle/graal/pointsto/standalone/PointsToAnalyzer.java b/....graal.pointsto.standalone/src/com/oracle/graal/pointsto/standalone/PointsToAnalyzer.java
@@ -77,7 +77,6 @@
 import jdk.graal.compiler.debug.Indent;
 import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration;
 import jdk.graal.compiler.nodes.graphbuilderconf.InvocationPlugins;
-import jdk.graal.compiler.nodes.spi.IdentityHashCodeProvider;
 import jdk.graal.compiler.options.OptionValues;
 import jdk.graal.compiler.phases.util.Providers;
 import jdk.graal.compiler.printer.GraalDebugHandlersFactory;
@@ -148,11 +147,10 @@ private PointsToAnalyzer(String mainEntryClass, OptionValues options) {
         StandaloneConstantReflectionProvider aConstantReflection = new StandaloneConstantReflectionProvider(aUniverse, HotSpotJVMCIRuntime.runtime());
         StandaloneConstantFieldProvider aConstantFieldProvider = new StandaloneConstantFieldProvider(aMetaAccess);
         AnalysisMetaAccessExtensionProvider aMetaAccessExtensionProvider = new AnalysisMetaAccessExtensionProvider();
-        IdentityHashCodeProvider identityHashCodeProvider = originalProviders.getIdentityHashCodeProvider();
         HostedProviders aProviders = new HostedProviders(aMetaAccess, null, aConstantReflection, aConstantFieldProvider,
                         originalProviders.getForeignCalls(), originalProviders.getLowerer(), originalProviders.getReplacements(),
                         originalProviders.getStampProvider(), snippetReflection, new WordTypes(aMetaAccess, wordKind),
-                        originalProviders.getPlatformConfigurationProvider(), aMetaAccessExtensionProvider, originalProviders.getLoopsDataProvider(), identityHashCodeProvider);
+                        originalProviders.getPlatformConfigurationProvider(), aMetaAccessExtensionProvider, originalProviders.getLoopsDataProvider(), originalProviders.getIdentityHashCodeProvider());
         standaloneHost.initializeProviders(aProviders);
         analysisName = getAnalysisName(mainEntryClass);
         ClassInclusionPolicy classInclusionPolicy = new ClassInclusionPolicy.DefaultAllInclusionPolicy("Included in the base image");
@@ -162,7 +160,7 @@ private PointsToAnalyzer(String mainEntryClass, OptionValues options) {
         aUniverse.setBigBang(bigbang);
         ImageHeap heap = new ImageHeap();
         StandaloneImageHeapScanner heapScanner = new StandaloneImageHeapScanner(bigbang, heap, aMetaAccess,
-                        snippetReflection, aConstantReflection, new AnalysisObjectScanningObserver(bigbang), analysisClassLoader, new HostedValuesProvider(aUniverse), identityHashCodeProvider);
+                        snippetReflection, aConstantReflection, new AnalysisObjectScanningObserver(bigbang), analysisClassLoader, new HostedValuesProvider(aUniverse));
         aUniverse.setHeapScanner(heapScanner);
         HeapSnapshotVerifier heapVerifier = new StandaloneHeapSnapshotVerifier(bigbang, heap, heapScanner);
         aUniverse.setHeapVerifier(heapVerifier);

diff --git a/....standalone/src/com/oracle/graal/pointsto/standalone/heap/StandaloneImageHeapScanner.java b/....standalone/src/com/oracle/graal/pointsto/standalone/heap/StandaloneImageHeapScanner.java
@@ -39,7 +39,6 @@
 import com.oracle.graal.pointsto.util.AnalysisError;
 
 import jdk.graal.compiler.api.replacements.SnippetReflectionProvider;
-import jdk.graal.compiler.nodes.spi.IdentityHashCodeProvider;
 import jdk.vm.ci.code.BytecodePosition;
 import jdk.vm.ci.meta.ConstantReflectionProvider;
 import jdk.vm.ci.meta.JavaConstant;
@@ -51,8 +50,8 @@ public class StandaloneImageHeapScanner extends ImageHeapScanner {
     private Predicate<AnalysisField> shouldScanField;
 
     public StandaloneImageHeapScanner(BigBang bb, ImageHeap heap, AnalysisMetaAccess aMetaAccess, SnippetReflectionProvider aSnippetReflection, ConstantReflectionProvider aConstantReflection,
-                    ObjectScanningObserver aScanningObserver, ClassLoader classLoader, HostedValuesProvider hostedValuesProvider, IdentityHashCodeProvider identityHashCodeProvider) {
-        super(bb, heap, aMetaAccess, aSnippetReflection, aConstantReflection, aScanningObserver, hostedValuesProvider, identityHashCodeProvider);
+                    ObjectScanningObserver aScanningObserver, ClassLoader classLoader, HostedValuesProvider hostedValuesProvider) {
+        super(bb, heap, aMetaAccess, aSnippetReflection, aConstantReflection, aScanningObserver, hostedValuesProvider);
         this.classLoader = classLoader;
         shouldScanConstant = constant -> isClassLoaderAllowed(metaAccess.lookupJavaType(constant).getJavaClass().getClassLoader());
         shouldScanField = field -> isClassLoaderAllowed(field.getDeclaringClass().getJavaClass().getClassLoader());

diff --git a/...tevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/heap/ImageHeapArray.java b/...tevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/heap/ImageHeapArray.java
@@ -30,14 +30,13 @@
 
 public abstract class ImageHeapArray extends ImageHeapConstant {
 
-    public static ImageHeapArray create(AnalysisType type, int length, int identityHashCode) {
+    public static ImageHeapArray create(AnalysisType type, int length) {
         assert type.isArray() : type;
-        return type.getComponentType().getStorageKind().isPrimitive() ? new ImageHeapPrimitiveArray(type, length, identityHashCode) : new ImageHeapObjectArray(type, length, identityHashCode);
+        return type.getComponentType().getStorageKind().isPrimitive() ? new ImageHeapPrimitiveArray(type, length) : new ImageHeapObjectArray(type, length);
     }
 
-    protected ImageHeapArray(ConstantData constantData, boolean compressed) {
+    ImageHeapArray(ConstantData constantData, boolean compressed) {
         super(constantData, compressed);
-        assert constantData.type.isArray() : constantData.type;
     }
 
     public abstract Object getElement(int idx);

diff --git a/...m/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/heap/ImageHeapConstant.java b/...m/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/heap/ImageHeapConstant.java
@@ -39,7 +39,6 @@
 
 import jdk.graal.compiler.core.common.type.CompressibleConstant;
 import jdk.graal.compiler.core.common.type.TypedConstant;
-import jdk.graal.compiler.nodes.spi.IdentityHashCodeProvider;
 import jdk.vm.ci.meta.JavaConstant;
 import jdk.vm.ci.meta.JavaKind;
 import jdk.vm.ci.meta.MetaAccessProvider;
@@ -56,7 +55,7 @@ public abstract class ImageHeapConstant implements JavaConstant, TypedConstant,
 
     public static final VarHandle isReachableHandle = ReflectionUtil.unreflectField(ConstantData.class, "isReachable", MethodHandles.lookup());
 
-    public abstract static class ConstantData {
+    abstract static class ConstantData {
         /**
          * Stores the type of this object.
          */
@@ -67,17 +66,11 @@ public abstract static class ConstantData {
          */
         private final JavaConstant hostedObject;
         /**
-         * The identity hash code for the heap object. This can be the same as the identity hash
-         * code of the referenced hosted object or not. The value is provided via a
-         * {@link IdentityHashCodeProvider} which can decide to use a previously computed identity
-         * hash code, e.g., to accommodate for replaced constants.
-         * <p>
-         * For objects without a backing object in the heap of the image builder VM we create a
-         * "virtual" identity hash code that has the same properties as the image builder VM by
-         * using the identity hash code of a new and otherwise unused object in the image builder
-         * VM.
-         * <p>
-         * The value is guaranteed to be positive.
+         * The identity hash code for the heap object. This field is only used if
+         * {@link #hostedObject} is null, i.e., for objects without a backing object in the heap of
+         * the image builder VM. We create a "virtual" identity hash code that has the same
+         * properties as the image builder VM by using the identity hash code of a new and otherwise
+         * unused object in the image builder VM.
          */
         private final int identityHashCode;
         /**
@@ -93,16 +86,23 @@ public abstract static class ConstantData {
          */
         @SuppressWarnings("unused") private volatile Object isReachable;
 
-        ConstantData(AnalysisType type, JavaConstant object, int identityHashCode) {
+        ConstantData(AnalysisType type, JavaConstant hostedObject) {
             Objects.requireNonNull(type);
             this.type = type;
-            this.hostedObject = CompressibleConstant.uncompress(object);
-            /*
-             * The Java HotSpot VM only returns positive numbers for the identity hash code, so we
-             * want to have the same restriction on Substrate VM in order to not surprise users.
-             */
-            AnalysisError.guarantee(identityHashCode > 0, "The provided identity hashcode value must be a positive number to be on par with the Java HotSpot VM.");
-            this.identityHashCode = identityHashCode;
+            this.hostedObject = CompressibleConstant.uncompress(hostedObject);
+
+            if (hostedObject == null) {
+                /*
+                 * No backing object in the heap of the image builder VM. We want a "virtual"
+                 * identity hash code that has the same properties as the image builder VM, so we
+                 * use the identity hash code of a new and otherwise unused object in the image
+                 * builder VM.
+                 */
+                this.identityHashCode = System.identityHashCode(new Object());
+            } else {
+                /* This value must never be used later on. */
+                this.identityHashCode = -1;
+            }
         }
 
         @Override
@@ -155,8 +155,13 @@ public Object getReachableReason() {
         return constantData.isReachable;
     }
 
-    @Override
+    public boolean hasIdentityHashCode() {
+        return constantData.identityHashCode > 0;
+    }
+
     public int getIdentityHashCode() {
+        AnalysisError.guarantee(constantData.hostedObject == null, "ImageHeapConstant only stores the identity hash code when there is no hosted object.");
+        AnalysisError.guarantee(constantData.identityHashCode > 0, "The provided identity hashcode value must be a positive number to be on par with the Java HotSpot VM.");
         return constantData.identityHashCode;
     }
 
@@ -243,10 +248,9 @@ public String toValueString() {
      * Returns a new image heap instance, as if {@link Object#clone} was called on the original
      * object. If the type is not cloneable, then null is returned.
      * <p>
-     * The new constant is never backed by a hosted object, regardless of the input object. The
-     * caller must provide a unique identity hash code.
+     * The new constant is never backed by a hosted object, regardless of the input object.
      */
-    public abstract ImageHeapConstant forObjectClone(int identityHashCode);
+    public abstract ImageHeapConstant forObjectClone();
 
     @Override
     public boolean equals(Object o) {