On 06/12/2019 12:28, Vladimir Ivanov wrote:
* ciField.cpp - this one is to trust final fields in the foreign
memory access implementation (otherwise VM doesn't trust memory
segment bounds)
New packages aren't part of java.base. Considering the
implementation resides in an incubator module, is it enough to
consider them as trusted and well-known to the JVM?
This gives same performance as with -XX:+TrustFinalNonStaticFields -
if we remove these changes, then memory segment bounds are never
inlined. I'm happy to change this if you have other suggestions on
how to get there, of course (I can run some benchmarks w/ and w/o and
post some numbers if that helps).
I'm not questioning whether these changes are needed for the
implementation or not. I wanted to attract attention that it's not
just a mechanical expansion of the list we have done in the past, but
a new case (java.base vs incubator) and hear from others are they fine
with leaving it as is?
All the options I see (either new JVM annotation or trust final fields
by default) require significant engineering effort. So, I'd like to
clarify first whether the effort to rewrite current solution is
required or not.
Thanks for the clarification - I agree it's a bit unorthodox, but I'll
leave to others (more qualified than me) to respond on that point.
* library_call.cpp - this one is a JIT compiler change to treat
Thread.currentThread() as a well-known constant - which helps a lot
in the confinement checks (thanks Vlad!)
FTR it is tracked by JDK-8235143 [1] and the patch is under review [2].
Should I remove this from the patch then?
Yes, please.
Done in the recently submitted v2
Thanks
Maurizio
Best regards,
Vladimir Ivanov
Best regards,
Vladimir Ivanov
[1] https://bugs.openjdk.java.net/browse/JDK-8235143
[2]
https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-December/036295.html
* various Buffer-related changes; these changes are needed because
the memory access API allows a memory segment to be projected into
a byte buffer, for interop reasons. As such, we need to insert a
liveness check in the various get/put methods. Previously we had an
implementation strategy where a BB was 'decorated' by a subclass
called ScopedBuffer - but doing so required some changes to the BB
API (e.g. making certain methods non-final, so that we could
decorate them). Here I use an approach (which I have discussed with
Alan) which doesn't require any public API changes, but needs to
add a 'segment' field in Buffer - and then have constructors which
keep track of this extra parameter.
* FileChannel changes - these changes are required so that we can
reuse the Unmapper class from the MemorySegment implementation, to
deterministically deallocate a mapped memory segment. This should
be a 'straight' refactoring, no change in behavior should occur
here. Please double check.
* VarHandles - this class now provides a factory to create memory
access VarHandle - this is a bit tricky, since VarHandle cannot
really be implemented outside java.base (e.g. VarForm is not
public). So we do the usual trick where we define a bunch of proxy
interfaces (see jdk/internal/access/foreign) have the classes in
java.base refer to these - and then have the implementation classes
of the memory access API implement these interfaces.
* JavaNIOAccess, JavaLangInvokeAccess - because of the above, we
need to provide access to otherwise hidden functionalities - e.g.
creating a new scoped buffer, or retrieving the properties of a
memory access handle (e.g. offset, stride etc.), so that we can
implement the memory access API in its own separate module
* GensrcVarHandles.gmk - these changes are needed to enable the
generation of the new memory address var handle implementations;
there's an helper class per carrier (e.g.
VarHandleMemoryAddressAsBytes, ...). At runtime, when a memory
access var handle is needed, we dynamically spin a new VH
implementation which makes use of the right carrier. We need to
spin because the VH can have a variable number of access
coordinates (e.g. depending on the dimensions of the array to be
accessed). But, under the hood, all the generated implementation
will be using the same helper class.
* tests - we've tried to add fairly robust tests, often checking
all possible permutations of carriers/dimensions etc. Because of
that, the tests might not be the easiest to look at, but they have
proven to be pretty effective at shaking out issues.
I think that covers the main aspects of the implementation and
where it differs from vanilla JDK.
P.S.
In the CSR review [2], Joe raised a fair point - which is
MemoryAddress has both:
offset(long) --> move address of given offset
offset() --> return the offset of this address in its owning segment
And this was considered suboptimal, given both methods use the same
name but do something quite different (one is an accessor, another
is a 'wither'). one obvious option is to rename the first to
'withOffset'. But I think that would lead to verbose code (since
that is a very common operation). Other options are to:
* rename offset(long) to move(long), advance(long), or something else
* drop offset() - but then add an overload of
MemorySegment::asSlice which takes an address instead of a plain
long offset
I'll leave the choice to the reviewers :-)
Finally, I'd like to thank Mark, Brian, John, Alan, Paul, Vlad,
Stuart, Roger, Joe and the Panama team for the feedback provided so
far, which helped to get the API in the shape it is today.
Cheers
Maurizio
(**) There is one failure, for "java/util/TimeZone/Bug6329116.java"
- but that is unrelated to this patch, and it's a known failing test.
[1] - https://openjdk.java.net/jeps/370
[2] - https://bugs.openjdk.java.net/browse/JDK-8234050
