[foreign-memaccess] RFR 8234644: Misc cleanup of the memory access API implementation

[Paul Sandoz]

Just some small stuff and some observations.

Paul.


X-ScopedBuffer.java.template
—

+/**
+ * This class decorated the ByteBuffer class, by adding checks for the validity of the temporal bounds associated
+ * with the underlying memory segment.
+ */

 class Scoped$Type$Buffer extends $Type$Buffer {

s/decorated/decorates


MemoryScope.java
—

MemoryScope acquire() {
    if (activeCount.updateAndGet(i -> i == CLOSED ? CLOSED : ++i) == CLOSED) {
        //cannot get here - segment is not alive!
        throw new IllegalStateException();
    }
    return new MemoryScope(ref, this::release);
}

Can you add a “fixme” comment about overflow? it's rare but possible. An easy fix would be to switch the count to be a long.

Separately, later on we can improve on the footprint of the memory scope if needed, both in the use of an Atomic* and a Runnable (possible tradeoff with two impls, the root scope and the acquired scope).


MemorySegmentImpl.java
—

+    private void checkIntSize(String typeName) {
+        if (length > Integer.MAX_VALUE) {
+            throw new UnsupportedOperationException(String.format("Segment is too large to wrap as %s. Size: %d", typeName, length));
+        }
+    }


FWIW the max array length may actually be slightly less than Integer.MAX_VALUE due to VM restrictions.  If a purpose here is to avoid an OOME in the case of allocating a byte[], then we might want to conservatively limit to say Integer.MAX_VALUE - 8 (as is done in ArrayDeque, ConcurrentHashMap etc) (Ideally we should have a method in say Unsafe that tells us what the max array length can be.)


LayoutPath.java
—

Possible optimize the scales type from List<Long> to long[]

 149     public static LayoutPathImpl nestedPath(MemoryLayout layout, long offset, List<Long> scales, LayoutPathImpl encl) {
 150         return new LayoutPathImpl(layout, offset, scales, encl);
 151     }

Can be private?


MemoryHandkes.java
—

public final class MemoryHandles {

    private static JavaLangInvokeAccess JLI = SharedSecrets.getJavaLangInvokeAccess();

Make JLS field final 


ForeignUnsafe
—

/**
 * Obtain the offset associated with this address. If {@link #getUnsafeBase(MemoryAddress)} returns {@null} on the passed

s/{@ null}/{@code null}


AbstractLayout
—

void checkAlignment(long alignmentBitCount) {
    if (alignmentBitCount <= 0 || //alignment must be positive
            ((alignmentBitCount & (alignmentBitCount - 1)) != 0L) || //alignment must be a power of two
            (alignmentBitCount < 8)) { //alignment must be greater than 8
        throw new IllegalArgumentException("Invalid alignment: " + alignmentBitCount);
    }
}


First check is redundant, dominated by the last check.


> On Nov 22, 2019, at 4:34 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> Hi,
> now that the API javadoc is mostly in shape, I wanted to do a pass over the implementation, to make sure that classes have at least some minimal documentation, and that methods inside the impl classes are organized in a meaningful way (this should help the review process a bit).
> 
> Webrev:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/8234644/
> 
> Cheers
> Maurizio
>