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

[Maurizio Cimadamore]

I've just pushed this patch to jdk/jdk14.

I'd like to thank all the reviewers for the great (and quick) feedback.

Cheers
Maurizio

On 11/12/2019 15:39, Maurizio Cimadamore wrote:
> 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
>>