On 06/12/2019 11:53, Jorn Vernee wrote:
> * drop offset() - but then add an overload of MemorySegment::asSlice
which takes an address instead of a plain long offset
This sounds good to me, since it fits with what we're doing with
makeUncheckedSegment(MemoryAddress, length), and we added the offset()
accessor to support slicing. I don't like changing the name of
offset(long) to advance(long) since the offset can be negative as well
(and 'advance' implies only positive).
Right - I'd say if we go down this path we need at least two overload:
* MemorySegment::asSlice(MemoryAddress, long)
* MemoryAddress::offset(MemoryAddress)
And then we could be offset() free :-)
>> I don't see the point of having MemoryLayouts separated from
MemoryLayout.
> Possibly - I found myself thinking that too - although, with the
subsequent Panama step (ABI support) we'll be adding a ton of
ABI-dependent layouts in here... (but we could address that in other
ways also).
We had some ideas to move the ABI constants to separate classes I
remember. I.e. provide separate classes for different platform ABIs
(implementing SystemABI), and stick the constants in there. Together
with the idea to make JAVA_INT into Integer::LAYOUT later, I think the
remaining constants fit into MemoryLayout pretty naturally, and we can
remove MemoryLayouts.
Yeah - that's what I was referring to when I said " (but we could
address that in other ways also). " :-)
Maurizio
Jorn
On 06/12/2019 11:43, Maurizio Cimadamore wrote:
Hi,
here's an updated version of the patch:
http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v2/
And a delta of the changes since last version here:
http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v2_delta/
The javadoc has been updated inline here:
http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html
Summary of changes:
* fixed spurious protected methods in AbstractLayout and subclasses
which leaked into API
* removed library_call.cpp changes, as these are being tracked
separately by Vlad
* compacted ILOAD code in AddressVarHandleGenerator
* replaced uses of ++i/--i with i + 1/i - 1 in MemoryScope
I have made no changes to the *name* of the methods in the API. In
fact, I suggest we keep a list of the names we'd like to revisit, and
we address them all at once at the end of the review (once we're
happy with the contents). Here's a list of the current open naming
issues:
* MemoryAddress::offset() vs. MemoryAddress::offset(long) -- not much
distance between these two semantically different operations
* MemorySegment::isAccessible() -- as the A* word is overloaded, some
other name should be found?
* MemorySegment::acquire() -- replace with MemorySegment::fork?
Cheers
Maurizio
On 05/12/2019 21:04, Maurizio Cimadamore wrote:
Hi,
as part of the effort to upstream the changes related to JEP 370
(foreign memory access API) [1], I'd like to ask for a code review
for the corresponding core-libs and hotspot changes:
http://cr.openjdk.java.net/~mcimadamore/panama/8234049/
A javadoc for the memory access API is also available here:
http://cr.openjdk.java.net/~mcimadamore/panama/memaccess_javadoc/jdk/incubator/foreign/package-summary.html
Note: the patch passes tier1, tier2 and tier3 testing (**)
Here is a brief summary of the changes in java.base and hotspot (the
remaining new files are implementation classes and tests for the new
API):
* ciField.cpp - this one is to trust final fields in the foreign
memory access implementation (otherwise VM doesn't trust memory
segment bounds)
* Modules.gmk - these changes are needed to require that the
incubating module is loaded by the boot loader (otherwise the above
changes are useless)
* 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!)
* 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
