Fine, thanks!
Raffaello
On 2019-12-09 22:30, Maurizio Cimadamore wrote:
Hi again,
after some offline discussions with the hotspot team, it became clear
that the recently added restriction on source/destination segments being
disjoint is redundant and that Unsafe::copyMemory can cope with overlap.
For now I'l keep the API as is, but I will file a followup issue to
remove the restriction from the API - which, I think, will also address
your issue around lack of memmove ability.
Thanks
Maurizio
On 09/12/2019 11:10, Maurizio Cimadamore wrote:
Hi Raffaello,
I think there is room to add more copy-related features in the future.
One thing to note, however: the rationale behind having a 'copy'
method is to expose a (safe) interface to the underlying
Unsafe::copyMemory method - which is an hotspot intrinsic, hence
understood and optimized by the JIT compiler. I'd say that, if we were
to add another method like 'memmove' we'd have to work out the VM
plumbing first - and then add the MemoryAddress API method. This has
been the informal guiding principle for the operations exposed by this
API.
That said - I'll keep a note of the suggestion, and add it to the list.
Cheers
Maurizio
On 09/12/2019 10:11, Raffaello Giulietti wrote:
Hi,
will there be a
MemoryAddress.move(MemoryAddress src, MemoryAddress dst, long bytes)
method with POSIX memmove(3) semantics at some point?
That would be useful, e.g., to "open a hole" into an array by
shifting existing elements towards higher indices (provided there's
room).
MemoryAddress.copy(), with its lower-to-higher semantics, doesn't
really help here, so without move() one would need to code an
explicit loop for such a case, I guess. Not a big deal, just a little
bit annoying.
Greetings
Raffaello
On 2019-12-07 00:51, Maurizio Cimadamore wrote:
On 06/12/2019 18:29, Raffaello Giulietti wrote:
Hello,
great job!
I think that the doc of MemoryAddress.copy() should be explicit
about the direction of the copying, so it should either:
Thanks! - I'll rectify the doc to specify lower-to-higher.
Maurizio
* explicitly specify a direction, e.g., lower-to-higher addresses
* or specify that in the case of an overlap the copying is smart
enough to not destroy the src bytes before they have landed in dst
* or accept a negative third argument to encode a higher-to-lower
addresses copying direction.
Greetings
Raffaello
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