New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move MemoryAddress::copy #169
Conversation
|
Webrevs
|
* published by the Free Software Foundation. Oracle designates this | ||
* particular file as subject to the "Classpath" exception as provided | ||
* by Oracle in the LICENSE file that accompanied this code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong copyright
* published by the Free Software Foundation. Oracle designates this | |
* particular file as subject to the "Classpath" exception as provided | |
* by Oracle in the LICENSE file that accompanied this code. | |
* published by the Free Software Foundation. |
@mcimadamore This change now passes all automated pre-integration checks, type
Since the source branch of this PR was last updated there have been 2 commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge
|
/integrate |
@mcimadamore The following commits have been pushed to foreign-memaccess since your change was applied:
Your commit was automatically rebased without conflicts. Pushed as commit 3487383. |
I like it. I always thought the prior location of copy
was a little odd, but it took another method for it to really stand out to me.
@@ -308,6 +309,29 @@ | |||
*/ | |||
MemorySegment fill(byte value); | |||
|
|||
/** | |||
* Perform bulk copy from given source segment to this segment. More specifically, the bytes at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Performs a bulk copy..."
* this segment at offset {@code 0} through {@code src.byteSize() - 1}. | ||
* <p> | ||
* The result of a bulk copy is unspecified if, in the uncommon case, the source segment does not overlap with | ||
* this segment, but it instead refers to an overlapping regions of the same backing storage using different addresses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"The result of a bulk copy is unspecified if, in the uncommon case, the source segment and this segment do not overlap, but refer to overlapping regions of the same backing storage using different addresses"
@Benchmark | ||
@OutputTimeUnit(TimeUnit.NANOSECONDS) | ||
public void unsafe_fill() { | ||
unsafe.setMemory(unsafe_addr, ALLOC_SIZE, (byte)0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of an abundance of caution you might wanna choose a non-zero value just in case the JIT knows it's zeroing already zeroed memory (i suspect it does not).
Mailing list message from John Rose on panama-dev: Nice work; thank you. When working with an address a, you often want to copy *p = *q; // copy element at (= just after) pointer I guess the Panama code for such operations would contain p.segment().asSlice(a.segmentOffset(), (some size)).copyFrom(q?) What are the natural idioms that might benefit the user, It seems to me that one possible primitive would be sized slice MemoryAddress sliceAfter(long size) { That is good for source operands, since the ?size? value needs to For sources of the form *--q, maybe this also: MemoryAddress sliceBefore(long size) { Or maybe this combo: MemoryAddress sliceAt(int index, long size) { Then sliceAt(0,s) == sliceAfter(s) and sliceAt(-1,s) == sliceBefore(s). Another use case is an *unsized* slice creator for destination operands. MemoryAddress sliceAfter() { Maybe for symmetry, and for cutting off buffers after a fill pointer, this too: MemoryAddress sliceBefore() { Anyway, I like the way MS is shaping up, as a small bounded location of ? John |
Mailing list message from John Rose on panama-dev: On May 15, 2020, at 6:19 PM, John Rose <john.r.rose at oracle.com> wrote:
P.S. A little more: One idiom I didn?t cover was copying *--p = *q Here, a user will reach for an expression like this: p.addOffset(-(some size)).copyFrom(q.sliceAfter(same size)); The need to repeat the same size in two places is a breeding Something like this would help. The destination pointer can MemoryAddress copyBeforeFrom(MemorySegment src) { For symmetry, there could be this also: MemoryAddress copyAfterFrom(MemorySegment src) { |
Mailing list message from Maurizio Cimadamore on panama-dev: On 16/05/2020 02:19, John Rose wrote:
These are all interesting ideas - thanks John. I'd like to think some more and to see how they fit with the slicing For instance, sliceAt could be expressed as an overload of A feeling I have is that one of the reasons for going back and forth * cannot pass negative offset in MemroySegment::asSlice (that's because My sense is that, since copyFrom is expressed in _segments_ having ways p.segment().asSlice(a.segmentOffset(), (some size)).copyFrom(q?) In most code I've seen, you already start off with two segments (so `p` So, I believe that if we offered a no-length overload: p_segment.asSlice(a.segmentOffset()).copyFrom(q?) Things would already improve considerably in the common case (remember, That said, I think that it would also be nice to provide overloads that, p_segment.asSlice(a).copyFrom(q?) Although this method could now throw (if the address does not belong to So I think I see an incremental path to get to a more sugary final state Thanks |
As discussed, this patch moves
MemoryAddress::copy
as an instance method ofMemorySegment
, namelyMemorySegment::copyFrom
.I chose the
copyFrom
suggestion from John, since it allows us more freedom, in the future, to add overloads for different sources for the copy operation.I've rewrorked the documentation a bit to speak about segment offsets rather than addresses.
I believe all use cases touched by this change actually lead to simpler code, which is good.
I've also added a benchmark for some of the bulk operations such as fill and copyFrom - the numbers I've got look good:
Cheers
Progress
Reviewers
Download
$ git fetch https://git.openjdk.java.net/panama-foreign pull/169/head:pull/169
$ git checkout pull/169