Looks very good,
Paul.
> On Dec 11, 2019, at 7:39 AM, Maurizio Cimadamore
> <maurizio.cimadam...@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
>>
