> On 11 Dec 2019, at 16:43, Paul Sandoz <[email protected]> wrote:
>
> Looks very good,
+1 This looks very good to me too. Nice work.
-Chris.
> Paul.
>
>> On Dec 11, 2019, at 7:39 AM, Maurizio Cimadamore
>> <[email protected]> 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
>>>
>
