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

[Paul Sandoz]

Looks very good,
Paul.

> On Dec 11, 2019, at 7:39 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> I went ahead and created a new revision:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v6/ <http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v6/>
> Delta from latest (v5) here:
> 
> http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v6_delta/ <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 
>>