Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

On Dec 9, 2019, at 10:16 AM, Peter Levart wrote: > What do you think? Might this be better performance wise? Yes, a fast positive proof that the access is allowed can be implemented along the lines you mention. The owner token could be overloaded in additional ways, if and when we do more

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

> On 11 Dec 2019, at 16:43, Paul Sandoz 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 >> wrote: >> >> I went ahead and created a new revision: >> >>

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Looks very good, Paul. > On Dec 11, 2019, at 7:39 AM, 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: > >

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

> On Dec 10, 2019, at 4:51 AM, Maurizio Cimadamore > wrote: > > And, another, small iteration > > http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v5 > > Delta compared to previous version (v4): > > http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v5_delta > > Javadoc updated

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

And, another, small iteration http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v5 Delta compared to previous version (v4): http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v5_delta Javadoc updated in place:

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Thanks for the extra testing! Maurizio On 10/12/2019 10:03, Andrew Dinn wrote: Hi Maurizio, It's nice to see this squeezing into jdk14. On 09/12/2019 19:23, Maurizio Cimadamore wrote: Another iteration: http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4/ I eyeballed all the changes

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Hi Maurizio, It's nice to see this squeezing into jdk14. On 09/12/2019 19:23, Maurizio Cimadamore wrote: > Another iteration: > > http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4/ I eyeballed all the changes to the Buffer classes and saw no issues. Also, apologies for the mixed

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

+1 Paul. > On Dec 9, 2019, at 11:23 AM, Maurizio Cimadamore > wrote: > > Another iteration: > > http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4/ > > Delta of the changes since last version (v3) here: > > http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4_delta/ > > The

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Another iteration: http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4/ Delta of the changes since last version (v3) here: http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v4_delta/ The javadoc has been updated inline here:

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Oops, my bad, in my haste I misread the original code. (Effectively a reduction on OptionalLong where empty dominates, but the code to write that is I think less clear than the imperative loop.) Paul. > On Dec 7, 2019, at 11:02 AM, Maurizio Cimadamore > wrote: > > > On 06/12/2019 21:04,

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Hi Peter, this looks like a clever optimization which basically takes advantage of the fact that you can only access a segment if you are the same thread as the segment owner. I think after we integrate, I'd love to give your approach a try and see what happens performance-wise. Right now we

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Hi Maurizio, Nice work! I see the API prefixes each independent access to memory with a call to the following MemorySegmentImpl method:  157 @Override  158 public final void checkValidState() {  159 if (owner != Thread.currentThread()) {  160 throw new

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Thanks Roger, I will address your comments. Maurizio On 09/12/2019 15:31, Roger Riggs wrote: Hi, Great work! Comments, mostly related to readability: (Based on the 12/5 webrev) FileChannelImpl.java: 1008-1009: else should be on the same line as }. 1124-1125: use

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Hi, Great work! Comments, mostly related to readability: (Based on the 12/5 webrev) FileChannelImpl.java: 1008-1009: else should be on the same line as }. 1124-1125: use Objects.requireNonNull(mode, "mode"); 1132-1144: odd mix of use of  single line 'if' vs. with brackets. (yes, its

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Yes, for bytes the 'swappiness' is redundant - that said, both the layout API and the MemoryHandles API will still ask for endianness regardless of the size, so having some extra (possibly redundant) constants is good. Also, as you might have noted from some of the comments on panama-dev,

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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).

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Another update: http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v3/ And a delta of the changes since last version (v2) here: http://cr.openjdk.java.net/~mcimadamore/panama/8234049_v3_delta/ The javadoc has been updated inline here:

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

On 06/12/2019 21:04, Paul Sandoz wrote: GroupLayout.java — 80 OptionalLong sizeof(List elems) { 81 long size = 0; 82 for (MemoryLayout elem : elems) { 83 if (AbstractLayout.optSize(elem).isPresent()) { 84 size

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Thanks Paul, I'll do a pass and fix the issues you found. Some comments inline: On 06/12/2019 21:04, Paul Sandoz wrote: I mostly looked at the API and implementation and not the tests. s/offset/add or plus ? add ‘l’ to the offset of this memory address the result of which is the offset of

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

I mostly looked at the API and implementation and not the tests. s/offset/add or plus ? add ‘l’ to the offset of this memory address the result of which is the offset of the returned memory address. If we ever have controlled operator overloading that’s how I would like to express it :-)

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Hi, MemoryLayouts exposes BITS_8_BE and BITS_8_LE. Is there a reason to have both or is just love for symmetries (which I share)? 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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Hello, great job! I think that the doc of MemoryAddress.copy() should be explicit about the direction of the copying, so it should either: * 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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

* 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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

> * 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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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:

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

On 05/12/2019 23:27, Maurizio Cimadamore wrote: 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

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Couple of naming ideas: MemorySegment.isAccessible() is a very overloaded term, isOwnByCurrentThread() is maybe better ? One solution here would be to, instead, have an accessor called ownerThread() - then clients can test doing ownerThread() != Thread.currentThread (or some other thread).

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

, "hotspot-dev" Envoyé: Jeudi 5 Décembre 2019 22:04:55 Objet: RFR JDK-8234049: Implementation of Memory Access API (Incubator) 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 correspondin

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

; , "hotspot-dev" > > Envoyé: Jeudi 5 Décembre 2019 22:04:55 > Objet: RFR JDK-8234049: Implementation of Memory Access API (Incubator) > Hi, > as part of the effort to upstream the changes related to JEP 370 > (foreign memory access API) [1], I'd like to as

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

On 05/12/2019 22:39, Vladimir Ivanov wrote: Awesome work, Maurizio! Regarding hotspot changes: * 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.

Re: RFR JDK-8234049: Implementation of Memory Access API (Incubator)

Awesome work, Maurizio! Regarding hotspot changes: * 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

RFR JDK-8234049: Implementation of Memory Access API (Incubator)

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