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

[Maurizio Cimadamore]

On 22/11/2019 18:30, Paul Sandoz wrote:
> 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.
I'll add thanks
>
> 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.)
Well spotted, thanks!
>
>
> 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?
yup
>
>
> MemoryHandkes.java
> —
>
> public final class MemoryHandles {
>
>      private static JavaLangInvokeAccess JLI = SharedSecrets.getJavaLangInvokeAccess();
>
> Make JLS field final
yup
>
>
> ForeignUnsafe
> —
>
> /**
>   * Obtain the offset associated with this address. If {@link #getUnsafeBase(MemoryAddress)} returns {@null} on the passed
>
> s/{@ null}/{@code null}
yup
>
>
> 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.

good  point

I'll fix these and push directly, if that's ok

Thanks
Maurizio

>
>
>> 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
>>