RFR JDK-8234049: Implementation of Memory Access API (Incubator)

[Maurizio Cimadamore]

I went ahead and created a new revision:

http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v6/

Delta from latest (v5) here:

http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v6_delta/

No javadoc chnages here.

Summary of changes:

* addressed Paul feedback on MemoryScope
* on further testing, I found two issues on var handle construction from 
layouts:
    - PathElement.sequenceElement(long) does not add the sequence 
element offset to the new path element offset
    - the layout path machiner does not check that strides are a 
multiple of alignment - this allow creation of 'bad' var handles which 
are guaranteed not to work in for all indices - consider this layout:

var seq = MemoryLayout.ofSequence(JAVA_SHORT.withBitAlignment(32));

The above layout has an issue: the element requests 32-bit alignent, but 
the elements are only 16 bits apart. So, for odd access indices, there 
will be an alignment exception.

If you create a VarHandle with combinators, strides are checked against 
alignment, so it is not possible to obtain a VarHandle for the above 
layout. But with the Layout API is possible, because the layout path 
machinery does not enforce same restriction.

I've now fixed the code in LayoutPath (most of the changes are due to a 
rename from 'scale' to 'stride') to take care of this.

Bonus point: since offsets must satisfy alignment, and all strides must 
also satisfy same alignment - it follows that whetever offset can come 
out of the VH index machinery, it will be aligned. This means that, in 
the VarHandle code we can just check the base address. In other words, 
the memory location accessed by the VarHandle will be something like 
(below N denotes alignment, s1..sn are strides, O1..Om are offsets):


address = base + offset

where:

offset = N * s1 * i1 + N * s2 * i2 ... + N * sn * in + N * O1 + N * O2 + 
... N * Om
= N * (s1 * i1 + s2 * i2 ... + sn * in + O1 + O2 + ... Om)
= N * K


So, in order to show that the "address" is aligned, we only have to show 
that "base" is a multiple of N. This helps a lot, as e.g. when looping 
through a native array, the base array stays the same, and only the 
offset part changes, which means C2 will have no troubles hoisting out 
the alignment check out of the loop.


As things appear stable, I do not plan to make any other changes to the 
implementation/API ahead of integration, unless there's something 
critical which reviewers think should be fixed.

Thanks
Maurizio


On 11/12/2019 00:13, Maurizio Cimadamore wrote:
>
> On 10/12/2019 18:45, Paul Sandoz wrote:
>> MemoryScope changes look good.
>>
>> In j.u.concurrent we use ExceptionInInitializerError in the static 
>> blocks when there is an error looking up the VH,
>>
>> FWIW close can also fail because it has already been closed but the 
>> exception message does not distinguish between the two states (active 
>> or already closed).
>>   Paul.
>>
> Hi Paul,
>
> would something like this be satisfactory?
>
>
> diff -r 50ef46599c56 
> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java
> --- 
> a/src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java 
> Tue Dec 10 16:28:03 2019 +0000
> +++ 
> b/src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryScope.java 
> Wed Dec 11 00:12:20 2019 +0000
> @@ -48,7 +48,7 @@
>          try {
>              COUNT_HANDLE = 
> MethodHandles.lookup().findVarHandle(MemoryScope.class, "activeCount", 
> int.class);
>          } catch (Throwable ex) {
> -            throw new IllegalStateException(ex);
> +            throw new ExceptionInInitializerError(ex);
>          }
>      }
>
> @@ -107,6 +107,9 @@
>
>      void close() {
>          if (!COUNT_HANDLE.compareAndSet(this, UNACQUIRED, CLOSED)) {
> +            //first check if already closed...
> +            checkAliveConfined();
> +            //...if not, then we have acquired views that are still 
> active
>              throw new IllegalStateException("Cannot close a segment 
> that has active acquired views");
>          }
>          if (cleanupAction != null) {
>
>
>
> If you need a full webrev please let me know.
>
> Thanks
> Maurizio
>