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
