Review feedback.

Author: christianhaeubl

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/ObjectHeaderImpl.java

@@ -355,8 +355,7 @@ public static boolean isAlignedHeader(UnsignedWord header) {
 
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public static boolean isUnalignedObject(Object obj) {
-        ObjectHeader oh = Heap.getHeap().getObjectHeader();
-        UnsignedWord header = oh.readHeaderFromObject(obj);
+        UnsignedWord header = getObjectHeaderImpl().readHeaderFromObject(obj);
         return isUnalignedHeader(header);
     }
 
@@ -367,8 +366,7 @@ public static boolean isUnalignedHeader(UnsignedWord header) {
 
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public static void setRememberedSetBit(Object o) {
-        ObjectHeader oh = Heap.getHeap().getObjectHeader();
-        UnsignedWord oldHeader = oh.readHeaderFromObject(o);
+        UnsignedWord oldHeader = getObjectHeaderImpl().readHeaderFromObject(o);
         assert oldHeader.and(FORWARDED_OR_MARKED2_BIT).equal(0);
         UnsignedWord newHeader = oldHeader.or(REMSET_OR_MARKED1_BIT);
         writeHeaderToObject(o, newHeader);
@@ -384,8 +382,7 @@ public static void setMarked(Object o) {
         if (!SerialGCOptions.useCompactingOldGen()) { // not guarantee(): always folds, prevent call
             throw VMError.shouldNotReachHere("Only for compacting GC.");
         }
-        ObjectHeader oh = Heap.getHeap().getObjectHeader();
-        UnsignedWord header = oh.readHeaderFromObject(o);
+        UnsignedWord header = getObjectHeaderImpl().readHeaderFromObject(o);
         assert header.and(FORWARDED_OR_MARKED2_BIT).equal(0) : "forwarded or already marked";
         /*
          * The remembered bit is already set if the object was in the old generation before, or
@@ -396,16 +393,14 @@ public static void setMarked(Object o) {
 
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public static void unsetMarkedAndKeepRememberedSetBit(Object o) {
-        ObjectHeader oh = Heap.getHeap().getObjectHeader();
-        UnsignedWord header = oh.readHeaderFromObject(o);
+        UnsignedWord header = getObjectHeaderImpl().readHeaderFromObject(o);
         assert isMarkedHeader(header);
         writeHeaderToObject(o, header.and(FORWARDED_OR_MARKED2_BIT.not()));
     }
 
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public static boolean isMarked(Object o) {
-        ObjectHeader oh = Heap.getHeap().getObjectHeader();
-        return isMarkedHeader(oh.readHeaderFromObject(o));
+        return isMarkedHeader(getObjectHeaderImpl().readHeaderFromObject(o));
     }
 
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
@@ -418,8 +413,7 @@ public static boolean isMarkedHeader(UnsignedWord header) {
 
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     static boolean isPointerToForwardedObject(Pointer p) {
-        ObjectHeader oh = Heap.getHeap().getObjectHeader();
-        Word header = oh.readHeaderFromPointer(p);
+        Word header = getObjectHeaderImpl().readHeaderFromPointer(p);
         return isForwardedHeader(header);
     }
 

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/remset/CardTableBasedRememberedSet.java

@@ -188,7 +188,8 @@ public void dirtyCardIfNecessary(Object holderObject, Object object) {
     @Override
     @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
     public void dirtyAllReferencesIfNecessary(Object obj) {
-        Word header = ObjectHeader.readHeaderFromObject(obj);
+        ObjectHeader oh = Heap.getHeap().getObjectHeader();
+        Word header = oh.readHeaderFromObject(obj);
         if (RememberedSet.get().hasRememberedSet(header) && mayContainReferences(obj)) {
             ForcedSerialPostWriteBarrier.force(OffsetAddressNode.address(obj, 0), false);
         }

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/meta/SubstrateBasicLoweringProvider.java

@@ -260,7 +260,7 @@ protected ValueNode createReadHub(StructuredGraph graph, ValueNode object, Lower
         int hubOffset = ol.getHubOffset();
         int bytesToRead = ol.getHubSize();
         long reservedHubBitsMask = oh.getReservedHubBitsMask();
-        if (hubOffset > 0 && hubOffset + ol.getHubSize() <= Long.BYTES && target.arch.getByteOrder() == ByteOrder.LITTLE_ENDIAN) {
+        if (hubOffset == Integer.BYTES && hubOffset + ol.getHubSize() == Long.BYTES && target.arch.getByteOrder() == ByteOrder.LITTLE_ENDIAN) {
             /* Prepare to emit a 64-bit read at offset 0 (reduces the code size). */
             hubOffset = 0;
             bytesToRead = Long.BYTES;

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/hub/DynamicHub.java

@@ -572,12 +572,12 @@ public void setClassInitializationInfo(ClassInitializationInfo classInitializati
     @Platforms(Platform.HOSTED_ONLY.class)
     public void setSharedData(int layoutEncoding, int monitorOffset, int identityHashOffset, long referenceMapIndex,
                     boolean isInstantiated) {
-        VMError.guarantee(monitorOffset == (char) monitorOffset, "Class %s has an invalid monitor field offset. Most likely, its objects are larger than supported.", name);
+        VMError.guarantee(monitorOffset == -1 || monitorOffset == (char) monitorOffset, "Class %s has an invalid monitor field offset. Most likely, its objects are larger than supported.", name);
         VMError.guarantee(identityHashOffset == -1 || identityHashOffset == (char) identityHashOffset,
                         "Class %s has an invalid identity hash code field offset. Most likely, its objects are larger than supported.", name);
 
         this.layoutEncoding = layoutEncoding;
-        this.monitorOffset = (char) monitorOffset;
+        this.monitorOffset = monitorOffset == -1 ? 0 : (char) monitorOffset;
         this.identityHashOffset = identityHashOffset == -1 ? 0 : (char) identityHashOffset;
 
         if ((int) referenceMapIndex != referenceMapIndex) {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/identityhashcode/IdentityHashCodeSupport.java

@@ -26,11 +26,8 @@
 
 import static com.oracle.svm.core.Uninterruptible.CALLED_FROM_UNINTERRUPTIBLE_CODE;
 
-import java.nio.ByteBuffer;
 import java.util.SplittableRandom;
 
-import org.graalvm.nativeimage.Platform;
-import org.graalvm.nativeimage.Platforms;
 import org.graalvm.word.LocationIdentity;
 import org.graalvm.word.Pointer;
 import org.graalvm.word.SignedWord;
@@ -44,6 +41,7 @@
 import com.oracle.svm.core.snippets.SubstrateForeignCallTarget;
 import com.oracle.svm.core.threadlocal.FastThreadLocalFactory;
 import com.oracle.svm.core.threadlocal.FastThreadLocalObject;
+import com.oracle.svm.core.util.VMError;
 
 import jdk.graal.compiler.api.directives.GraalDirectives;
 import jdk.graal.compiler.nodes.NamedLocationIdentity;
@@ -175,6 +173,9 @@ public static int readIdentityHashCodeFromField(Object obj) {
 
     @SubstrateForeignCallTarget(stubCallingConvention = false)
     public static int generateIdentityHashCode(Object obj) {
+        /* The guarantee makes the code a bit smaller as obj is non-null afterward. */
+        VMError.guarantee(obj != null);
+
         ObjectLayout ol = ConfigurationValues.getObjectLayout();
         assert !ol.isIdentityHashFieldOptional();
 
@@ -193,7 +194,7 @@ public static int generateIdentityHashCode(Object obj) {
             int existingValue;
             int newValue;
             do {
-                existingValue = Unsafe.getUnsafe().getIntVolatile(obj, offset);
+                existingValue = Unsafe.getUnsafe().getIntOpaque(obj, offset);
                 int existingHash = extractIdentityHashCode(existingValue, numBits, shift);
                 if (existingHash != 0) {
                     return existingHash;
@@ -205,7 +206,7 @@ public static int generateIdentityHashCode(Object obj) {
             long existingValue;
             long newValue;
             do {
-                existingValue = Unsafe.getUnsafe().getLongVolatile(obj, offset);
+                existingValue = Unsafe.getUnsafe().getLongOpaque(obj, offset);
                 int existingHash = extractIdentityHashCode(existingValue, numBits, shift);
                 if (existingHash != 0) {
                     return existingHash;
@@ -219,7 +220,7 @@ public static int generateIdentityHashCode(Object obj) {
     }
 
     /** This method may only be called after the hub pointer was already written. */
-    public static void writeIdentityHashCodeToAuxImage(Pointer hashCodePtr, int value) {
+    public static void writeIdentityHashCodeToImageHeap(Pointer hashCodePtr, int value) {
         ObjectLayout ol = ConfigurationValues.getObjectLayout();
         int numBits = ol.getIdentityHashCodeNumBits();
         int shift = ol.getIdentityHashCodeShift();
@@ -238,27 +239,6 @@ public static void writeIdentityHashCodeToAuxImage(Pointer hashCodePtr, int valu
         }
     }
 
-    /** This method may only be called after the object header was already written. */
-    @Platforms(Platform.HOSTED_ONLY.class)
-    public static void writeIdentityHashCodeToImageHeap(ByteBuffer buffer, int hashCodePos, int value) {
-        ObjectLayout ol = ConfigurationValues.getObjectLayout();
-        int numBits = ol.getIdentityHashCodeNumBits();
-        int shift = ol.getIdentityHashCodeShift();
-        long mask = ol.getIdentityHashCodeMask();
-
-        int totalBits = numBits + shift;
-        if (totalBits <= Integer.SIZE) {
-            int oldValue = buffer.getInt(hashCodePos);
-            assertIdentityHashCodeZero(oldValue, mask);
-            buffer.putInt(hashCodePos, encodeIdentityHashCode(oldValue, value, shift));
-        } else {
-            assert totalBits <= Long.SIZE;
-            long oldValue = buffer.getLong(hashCodePos);
-            assertIdentityHashCodeZero(oldValue, mask);
-            buffer.putLong(hashCodePos, encodeIdentityHashCode(oldValue, value, shift));
-        }
-    }
-
     @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
     private static int extractIdentityHashCode(int value, int numBits, int shift) {
         int left = Integer.SIZE - numBits - shift;
@@ -300,8 +280,8 @@ private static void assertHasIdentityHashField(Object obj) {
 
     /**
      * Note that the result of this method is prone to race conditions. All races that can happen
-     * don't matter for the current caller though (we only want to identify objects that definitely
-     * don't have an identity hash code field).
+     * don't matter for the current callers though (we only want to know if the given object
+     * definitely has an identity hash code field - once it has one, it won't be taken away).
      */
     @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
     private static boolean hasIdentityHashField(Object obj) {

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

@@ -502,7 +502,7 @@ private void addObjectToImageHeap(final JavaConstant constant, boolean immutable
         if (type.isInstanceClass()) {
             final HostedInstanceClass clazz = (HostedInstanceClass) type;
             // If the type has a monitor field, it has a reference field that is written.
-            if (clazz.getMonitorFieldOffset() != 0) {
+            if (clazz.getMonitorFieldOffset() >= 0) {
                 written = true;
                 references = true;
                 // also not immutable: users of registerAsImmutable() must take precautions

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

@@ -55,6 +55,7 @@
 import com.oracle.svm.core.image.ImageHeapLayoutInfo;
 import com.oracle.svm.core.imagelayer.ImageLayerBuildingSupport;
 import com.oracle.svm.core.meta.MethodPointer;
+import com.oracle.svm.core.util.HostedByteBufferPointer;
 import com.oracle.svm.core.util.VMError;
 import com.oracle.svm.hosted.DeadlockWatchdog;
 import com.oracle.svm.hosted.code.CEntryPointLiteralFeature;
@@ -286,13 +287,7 @@ private void writeHubPointer(RelocatableBuffer buffer, int index, ObjectInfo obj
         if (NativeImageHeap.useHeapBase()) {
             long targetOffset = hubInfo.getOffset();
             long encoding = objectHeader.encodeHubPointerForImageHeap(obj, targetOffset);
-
-            if (hubSize == Long.BYTES) {
-                buffer.getByteBuffer().putLong(index, encoding);
-            } else {
-                assert hubSize == Integer.BYTES;
-                buffer.getByteBuffer().putInt(index, NumUtil.safeToUInt(encoding));
-            }
+            writeValue(buffer, index, encoding, hubSize);
         } else {
             assert hubSize == referenceSize();
             // The address of the DynamicHub target will be added by the link editor.
@@ -361,12 +356,16 @@ private static void writePrimitive(RelocatableBuffer buffer, int index, JavaCons
     }
 
     private void writeReferenceValue(RelocatableBuffer buffer, int index, long value) {
-        if (referenceSize() == Long.BYTES) {
+        writeValue(buffer, index, value, referenceSize());
+    }
+
+    private static void writeValue(RelocatableBuffer buffer, int index, long value, int size) {
+        if (size == Long.BYTES) {
             buffer.getByteBuffer().putLong(index, value);
-        } else if (referenceSize() == Integer.BYTES) {
+        } else if (size == Integer.BYTES) {
             buffer.getByteBuffer().putInt(index, NumUtil.safeToUInt(value));
         } else {
-            throw shouldNotReachHere("Unsupported reference size: " + referenceSize());
+            throw shouldNotReachHere("Unsupported value size: " + size);
         }
     }
 
@@ -468,15 +467,17 @@ private void writeObject(ObjectInfo info, RelocatableBuffer buffer) {
 
             /* Write the identity hashcode */
             assert idHashOffset >= 0;
-            IdentityHashCodeSupport.writeIdentityHashCodeToImageHeap(bufferBytes, getIndexInBuffer(info, idHashOffset), info.getIdentityHashCode());
+            HostedByteBufferPointer identityHashPtr = new HostedByteBufferPointer(bufferBytes, getIndexInBuffer(info, idHashOffset));
+            IdentityHashCodeSupport.writeIdentityHashCodeToImageHeap(identityHashPtr, info.getIdentityHashCode());
         } else if (clazz.isArray()) {
 
             JavaKind kind = clazz.getComponentType().getStorageKind();
             ImageHeapConstant constant = info.getConstant();
 
             int length = heap.hConstantReflection.readArrayLength(constant);
             bufferBytes.putInt(getIndexInBuffer(info, objectLayout.getArrayLengthOffset()), length);
-            IdentityHashCodeSupport.writeIdentityHashCodeToImageHeap(bufferBytes, getIndexInBuffer(info, objectLayout.getArrayIdentityHashOffset(kind, length)), info.getIdentityHashCode());
+            HostedByteBufferPointer identityHashPtr = getHashCodePtr(info, bufferBytes, objectLayout, kind, length);
+            IdentityHashCodeSupport.writeIdentityHashCodeToImageHeap(identityHashPtr, info.getIdentityHashCode());
 
             if (clazz.getComponentType().isPrimitive()) {
                 ImageHeapPrimitiveArray imageHeapArray = (ImageHeapPrimitiveArray) constant;
@@ -492,6 +493,10 @@ private void writeObject(ObjectInfo info, RelocatableBuffer buffer) {
         }
     }
 
+    private HostedByteBufferPointer getHashCodePtr(ObjectInfo info, ByteBuffer bufferBytes, ObjectLayout objectLayout, JavaKind kind, int length) {
+        return new HostedByteBufferPointer(bufferBytes, getIndexInBuffer(info, objectLayout.getArrayIdentityHashOffset(kind, length)));
+    }
+
     private int getIndexInBuffer(ObjectInfo objInfo, long offset) {
         long index = objInfo.getOffset() + offset - heapLayout.getStartOffset();
         return NumUtil.safeToInt(index);

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/HostedInstanceClass.java

@@ -39,7 +39,7 @@ public class HostedInstanceClass extends HostedClass {
     protected int afterFieldsOffset;
     protected int instanceSize;
     protected boolean monitorFieldNeeded = false;
-    protected int monitorFieldOffset = 0;
+    protected int monitorFieldOffset = -1;
     protected int identityHashOffset = -1;
 
     public HostedInstanceClass(HostedUniverse universe, AnalysisType wrapped, JavaKind kind, JavaKind storageKind, HostedClass superClass, HostedInterface[] interfaces) {
@@ -127,8 +127,8 @@ public int getMonitorFieldOffset() {
     }
 
     public void setMonitorFieldOffset(int offset) {
-        assert this.monitorFieldOffset == 0 : "setting monitor field offset twice";
-        assert offset > 0;
+        assert this.monitorFieldOffset == -1 : "setting monitor field offset twice";
+        assert offset >= 0;
         this.monitorFieldOffset = offset;
     }
 

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/meta/UniverseBuilder.java

@@ -989,7 +989,7 @@ private static ReferenceMapEncoder.Input createReferenceMap(HostedType type) {
              * If the instance type has a monitor field, add it to the reference map.
              */
             final int monitorOffset = instanceClass.getMonitorFieldOffset();
-            if (monitorOffset != 0) {
+            if (monitorOffset >= 0) {
                 referenceMap.markReferenceAtOffset(monitorOffset, true);
             }
         }