8249879: Split MemorySegment and MemoryAddress #260
Conversation
|
👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into |
Webrevs
|
| /** alignment constraint (in bytes, expressed as a bit mask) **/ | ||
| final long alignmentMask; | ||
|
|
||
| MemoryAccessVarHandleBase(VarForm form, boolean be, long length, long offset, long alignmentMask) { | ||
| /** alignment constraint (in bytes, expressed as a bit mask) **/ |
There was a problem hiding this comment.
comment need to be updated
| @@ -313,27 +313,32 @@ else if (viewComponentType == float.class) { | |||
| * @param carrier the Java carrier type. | |||
| * @param alignmentMask alignment requirement to be checked upon access. In bytes. Expressed as a mask. | |||
There was a problem hiding this comment.
need comment for skipOffsetCheck
| } else { | ||
| if ((address & alignmentMask) != 0) { | ||
| throw MemoryAccessVarHandleBase.newIllegalStateExceptionForMisalignedAccess(address); | ||
| } |
There was a problem hiding this comment.
Seems a little odd to me, as there is no performance gain with skipOffsetCheck.
The else block check only address but not base and offset separately is not the same, although I think that's correct as it's what matters.
However, if this is desired, I don't see why we don't simply check address without the difference.
There was a problem hiding this comment.
The final address is the sum of two components - a base B and an offset O. In a hot loop, B is typically constant (e.g. the base address of a segment) while O keeps changing. So, if the alignment check is only performed on B, the VM can prove that this check can be hoisted outside the loop. Of course that leaves the correctness problem - e.g. what if B is aligned but B + O is not? Luckily, by construction, this is not possible - skipOffsetCheck is only set when we construct a memory access var handle from a layout - and in that case the layout API make sure that the O part of the final address is aligned accordingly.
So, while it "looks" as if there's no performance gain, in reality the gain is quite big, because that move allows the alignment check to be moved outside the loop.
There was a problem hiding this comment.
Thanks for the explanation
| * and whose new size is computed by subtracting the specified offset from this segment size. | ||
| * @param offset The new segment base offset (relative to the current segment base address), specified in bytes. | ||
| * @return a new memory segment view with updated base/limit addresses. | ||
| * @throws IndexOutOfBoundsException if {@code offset < 0}, {@code offset > byteSize()}, {@code newSize < 0}, or {@code newSize > byteSize() - offset} |
There was a problem hiding this comment.
| * @throws IndexOutOfBoundsException if {@code offset < 0}, {@code offset > byteSize()}, {@code newSize < 0}, or {@code newSize > byteSize() - offset} | |
| * @throws IndexOutOfBoundsException if {@code offset < 0}, {@code offset > byteSize()} |
| * (see {@link MemoryAddress#segmentOffset(MemorySegment)}) from this segment size. | ||
| * @param address The new segment base offset (relative to the current segment base address), specified in bytes. | ||
| * @return a new memory segment view with updated base/limit addresses. | ||
| * @throws IndexOutOfBoundsException if {@code offset < 0}, {@code offset > byteSize()}, {@code newSize < 0}, or {@code newSize > byteSize() - offset} |
There was a problem hiding this comment.
address.segmentOffset can throw IllegalArgumentException.
As IndexOutOfBoundException maybe simply saying the calculated offset < 0 or > byteSize()
|
@mcimadamore This change now passes all automated pre-integration checks, type
Since the source branch of this PR was last updated there have been 76 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 ➡️ To integrate this PR with the above commit message to the |
| //note: the offset portion has already been aligned-checked, by construction | ||
| if ((base & alignmentMask) != 0) { | ||
| throw MemoryAccessVarHandleBase.newIllegalStateExceptionForMisalignedAccess(address); | ||
| if (skipOffsetCheck) { |
There was a problem hiding this comment.
There are two alignment checks, one to the type size in bytes itself, and another to presumably multiples of the type size, otherwise the first would fail. The name of skipOffsetCheck is a little confusing, skipAlignmentMaskCheck would be more verbose but accurate. Documenting the rational on MemoryAccessVarHandleBase would i think help maintainers/reviewers of the code.
| @@ -46,37 +46,20 @@ | |||
| * (all primitive types but {@code void} and {@code boolean} are supported), as well as the alignment constraint and the | |||
| * byte order associated to a memory access var handle. The resulting memory access var handle can then be combined in various ways | |||
| * to emulate different addressing modes. The var handles created by this class feature a <em>mandatory</em> coordinate type | |||
| * (of type {@link MemoryAddress}), and zero or more {@code long} coordinate types, which can be used to emulate | |||
| * multi-dimensional array indexing. | |||
| * (of type {@link MemorySegment}), and one {@code long} coordinate types, which represents the offset, in bytes, relative | |||
| @@ -125,7 +105,7 @@ default MemoryAddress address() { | |||
| * The <em>unchecked</em> memory address instance modelling the {@code NULL} address. This address is <em>not</em> backed by | |||
| * a memory segment and hence it cannot be dereferenced. | |||
There was a problem hiding this comment.
Refers to "backed by a memory segment".
| */ | ||
| long segmentOffset(); | ||
| long segmentOffset(MemorySegment segment); |
There was a problem hiding this comment.
It might be worth explaining how this works e.g. equivalent to this.toRawLongValue() - segment.address().toRawLongValue(), and pointing out the resulting offset can be beyond the spatial bounds of the segment (negative value, or greater).
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryHandles.java
Outdated
Show resolved
Hide resolved
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
Show resolved
Hide resolved
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/MemoryAddressImpl.java
Show resolved
Hide resolved
| @@ -147,15 +146,15 @@ public int segment_loop() { | |||
| public int segment_loop_static() { | |||
| int res = 0; | |||
| for (int i = 0; i < ELEM_SIZE; i ++) { | |||
| res += MemoryAccess.getIntAtIndex(segment.address(), i); | |||
| res += MemoryAccess.getIntAtIndex(segment, i * CARRIER_SIZE); | |||
There was a problem hiding this comment.
Why this multiply by CARRIER_SIZE? That should be done by the accessor.
There was a problem hiding this comment.
mistake - will fix (but this has nothing to do with this patch, I think?)
There was a problem hiding this comment.
Well, this patch adds the multiply...
There was a problem hiding this comment.
Well, this patch adds the multiply...
Ugh - that's a bug then
|
/integrate |
openjdk
bot
added
integrated
|
@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 21442f6. |
This patch splits, as the title suggests, the memory segment abstraction from the memory address abstraction, as described in this email:
https://mail.openjdk.java.net/pipermail/panama-dev/2020-July/009928.html
Aside from the obvious updates to the two interfaces (and their implementation), this patch greatly simplifies the way memory access var handles are generated; instead of spinning them on the fly (see
MemoryAccessVarHandleGenerator, which is now removed) now all var handles can be derived from a primitive from which takes a segment and a byte offset. This caused the removal of several methods in theJavaLangInvokeAccessinterface.The new memory segment interface has some more methods to perform slicing - which can basically be used to give a new base to an existing segment and can therefore be used in a lot of cases as a replacement for
MemoryAddress::addOffset.The memory address interface is simplified, and features an additional
segmentOffsetmethod, which returns the byte offset of an address relative to the given segment (this is similar to the oldMemorySegment::rebasemethod, which is now removed).The
MemoryAccessclass needed a lot of tweaks in the various signatures, to take a segment instead of a base address.I tried to update the documentation as best as I could, but it's possible I missed some references to the old relationship between segment and addresses, please double check.
Of course integrating this patch is gonna cause issues in foreign-abi and foreign-jextract, as these branches make (heavy) use of memory access var handles. I suspect that integration will trigger a merge failure, so we can fix followup issues as part of the merge process, so that we can guarantee that all the branches remain buildable.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/panama-foreign pull/260/head:pull/260$ git checkout pull/260