Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]
On Mon, 20 May 2024 16:42:19 GMT, Paul Sandoz wrote:
> some additional overhead e.g.,
>
> ```
> if (derefAdapters.length == 0) {
> // insert align check for the root layout on the initial MS +
> offset
> List> coordinateTypes = handle.coordinateTypes();
> MethodHandle alignCheck =
> MethodHandles.insertArguments(MH_CHECK_ENCL_LAYOUT, 2, rootLayout());
> handle = MethodHandles.collectCoordinates(handle, 0, alignCheck);
> int[] reorder = IntStream.concat(IntStream.of(0, 1),
> IntStream.range(0, coordinateTypes.size())).toArray();
> handle = MethodHandles.permuteCoordinates(handle,
> coordinateTypes, reorder);
> }
> ```
True, the chain for segment var handle is quite long (and that is not a result
of this patch, it has always been more or less like that). Funnily, I was
staring at this piece of code the other day, and I think this can be dealt with
morally with a `foldArguments`, but we don't have the equivalent of that in the
VH/coordinates world. Maybe we should add that (or at least implement
internally) as that would simplify the adaptation, avoiding the permute step
(which is typically rather heavy).
-
PR Comment: https://git.openjdk.org/jdk/pull/19251#issuecomment-2120979087
Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]
On Mon, 20 May 2024 16:31:18 GMT, Maurizio Cimadamore
wrote:
>
> Ah, got it. You mean more support in VarHandleGuards. Yes, I have a separate
> patch for that (actually had that for quite a while), but we're not super
> sure how to evaluate what impact it has :-)
Ah, i did not realize that. Yes its tricky, it was designed for VHs to
fields/arrays, to really minimize their overhead. With segments there is
already some additional overhead e.g.,
if (derefAdapters.length == 0) {
// insert align check for the root layout on the initial MS + offset
List> coordinateTypes = handle.coordinateTypes();
MethodHandle alignCheck =
MethodHandles.insertArguments(MH_CHECK_ENCL_LAYOUT, 2, rootLayout());
handle = MethodHandles.collectCoordinates(handle, 0, alignCheck);
int[] reorder = IntStream.concat(IntStream.of(0, 1),
IntStream.range(0, coordinateTypes.size())).toArray();
handle = MethodHandles.permuteCoordinates(handle, coordinateTypes,
reorder);
}
So perhaps it does not make much difference.
-
PR Comment: https://git.openjdk.org/jdk/pull/19251#issuecomment-2120813942
Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]
On Mon, 20 May 2024 09:45:31 GMT, Maurizio Cimadamore wrote: > > Separately, we might be missing a few long argument accepting guard methods > > for simpler cases as I suspect they are still focused more on int index > > types. > > Not sure I understand what guard methods you are referring to here? Ah, got it. You mean more support in VarHandleGuards. Yes, I have a separate patch for that (actually had that for quite a while), but we're not super sure how to evaluate what impact it has :-) - PR Comment: https://git.openjdk.org/jdk/pull/19251#issuecomment-2120796446
Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]
On Fri, 17 May 2024 23:42:17 GMT, Paul Sandoz wrote: > Separately, we might be missing a few long argument accepting guard methods > for simpler cases as I suspect they are still focused more on int index types. Not sure I understand what guard methods you are referring to here? - PR Comment: https://git.openjdk.org/jdk/pull/19251#issuecomment-2120083825
Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]
On Thu, 16 May 2024 14:37:15 GMT, Maurizio Cimadamore
wrote:
>> When creating a nested memory access var handle, we ensure that the segment
>> is accessed at the correct alignment for the root layout being accessed. But
>> we do not ensure that the segment has at least the size of the accessed root
>> layout. Example:
>>
>>
>> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"),
>> JAVA_INT.withName("y")));
>> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(),
>> PathElement.groupElement("x"));
>>
>>
>> If I have a memory segment whose size is 12, I can successfully call
>> X_HANDLE on it with index 1, like so:
>>
>>
>> MemorySegment segment = Arena.ofAuto().allocate(12);
>> int x = (int)X_HANDLE.get(segment, 0, 1);
>>
>>
>> This seems incorrect: after all, the provided segment doesn't have enough
>> space to fit _two_ elements of the nested structs.
>>
>> This PR adds checks to detect this kind of scenario earlier, thus improving
>> usability. To achieve this we could, in principle, just tweak
>> `LayoutPath::checkEnclosingLayout` and add the required size check.
>>
>> But doing so will add yet another redundant check on top of the other
>> already added by [pull/19124](https://git.openjdk.org/jdk/pull/19124).
>> Instead, this PR rethinks how memory segment var handles are created, in
>> order to eliminate redundant checks.
>>
>> The main observation is that size and alignment checks depend on an
>> _enclosing_ layout. This layout *might* be the very same value layout being
>> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in
>> the general case, the accessed value layout and the enclosing layout might
>> differ (e.g. think about accessing a struct field).
>>
>> Furthermore, the enclosing layout check depends on the _base_ offset at
>> which the segment is accessed, _prior_ to any index computation that occurs
>> if the accessed layout path has any open elements. It is important to notice
>> that this base offset is only available when looking at the var handle that
>> is returned to the user. For instance, an indexed var handle with
>> coordinates:
>>
>>
>> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */,
>> long /* index 3 */)
>>
>>
>> Is obtained through adaptation, by taking a more basic var handle of the
>> form:
>>
>>
>> (MemorySegment, long /* offset */)
>>
>>
>> And then injecting the result of the index multiplication into `offset`. As
>> such, we can't add an enclosing layout check inside the var handle returned
>> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the
>> *wrong* off...
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix copyrights
Took me a few passes to work it all out. Looks good. All the bounds checking
action now consistently passes through the call to `checkEnclosingLayout` in
adaption of the raw (and unsafe) segment accessing VarHandle.
Separately, we might be missing a few long argument accepting guard methods for
simpler cases as I suspect they are still focused more on int index types.
-
PR Review: https://git.openjdk.org/jdk/pull/19251#pullrequestreview-2064493620
Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]
On Fri, 17 May 2024 15:54:04 GMT, Paul Sandoz wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix copyrights
>
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 630:
>
>> 628: * The access operation must fall inside the spatial bounds
>> of the accessed
>> 629: * memory segment, or an {@link IndexOutOfBoundsException} is
>> thrown. This is the case
>> 630: * when {@code B + A <= S}, where {@code O} is the base offset
>> (defined above),
>
> Do you mean `{@code O + A <= S}`?
> (Still working my way through the changes...)
Yes, apologies
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1605237785
Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]
On Thu, 16 May 2024 14:37:15 GMT, Maurizio Cimadamore
wrote:
>> When creating a nested memory access var handle, we ensure that the segment
>> is accessed at the correct alignment for the root layout being accessed. But
>> we do not ensure that the segment has at least the size of the accessed root
>> layout. Example:
>>
>>
>> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"),
>> JAVA_INT.withName("y")));
>> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(),
>> PathElement.groupElement("x"));
>>
>>
>> If I have a memory segment whose size is 12, I can successfully call
>> X_HANDLE on it with index 1, like so:
>>
>>
>> MemorySegment segment = Arena.ofAuto().allocate(12);
>> int x = (int)X_HANDLE.get(segment, 0, 1);
>>
>>
>> This seems incorrect: after all, the provided segment doesn't have enough
>> space to fit _two_ elements of the nested structs.
>>
>> This PR adds checks to detect this kind of scenario earlier, thus improving
>> usability. To achieve this we could, in principle, just tweak
>> `LayoutPath::checkEnclosingLayout` and add the required size check.
>>
>> But doing so will add yet another redundant check on top of the other
>> already added by [pull/19124](https://git.openjdk.org/jdk/pull/19124).
>> Instead, this PR rethinks how memory segment var handles are created, in
>> order to eliminate redundant checks.
>>
>> The main observation is that size and alignment checks depend on an
>> _enclosing_ layout. This layout *might* be the very same value layout being
>> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in
>> the general case, the accessed value layout and the enclosing layout might
>> differ (e.g. think about accessing a struct field).
>>
>> Furthermore, the enclosing layout check depends on the _base_ offset at
>> which the segment is accessed, _prior_ to any index computation that occurs
>> if the accessed layout path has any open elements. It is important to notice
>> that this base offset is only available when looking at the var handle that
>> is returned to the user. For instance, an indexed var handle with
>> coordinates:
>>
>>
>> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */,
>> long /* index 3 */)
>>
>>
>> Is obtained through adaptation, by taking a more basic var handle of the
>> form:
>>
>>
>> (MemorySegment, long /* offset */)
>>
>>
>> And then injecting the result of the index multiplication into `offset`. As
>> such, we can't add an enclosing layout check inside the var handle returned
>> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the
>> *wrong* off...
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix copyrights
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 630:
> 628: * The access operation must fall inside the spatial bounds
> of the accessed
> 629: * memory segment, or an {@link IndexOutOfBoundsException} is
> thrown. This is the case
> 630: * when {@code B + A <= S}, where {@code O} is the base offset
> (defined above),
Do you mean `{@code O + A <= S}`?
(Still working my way through the changes...)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1605231757
Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]
> When creating a nested memory access var handle, we ensure that the segment
> is accessed at the correct alignment for the root layout being accessed. But
> we do not ensure that the segment has at least the size of the accessed root
> layout. Example:
>
>
> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"),
> JAVA_INT.withName("y")));
> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(),
> PathElement.groupElement("x"));
>
>
> If I have a memory segment whose size is 12, I can successfully call X_HANDLE
> on it with index 1, like so:
>
>
> MemorySegment segment = Arena.ofAuto().allocate(12);
> int x = (int)X_HANDLE.get(segment, 0, 1);
>
>
> This seems incorrect: after all, the provided segment doesn't have enough
> space to fit _two_ elements of the nested structs.
>
> This PR adds checks to detect this kind of scenario earlier, thus improving
> usability. To achieve this we could, in principle, just tweak
> `LayoutPath::checkEnclosingLayout` and add the required size check.
>
> But doing so will add yet another redundant check on top of the other already
> added by [pull/19124](https://git.openjdk.org/jdk/pull/19124). Instead, this
> PR rethinks how memory segment var handles are created, in order to eliminate
> redundant checks.
>
> The main observation is that size and alignment checks depend on an
> _enclosing_ layout. This layout *might* be the very same value layout being
> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in
> the general case, the accessed value layout and the enclosing layout might
> differ (e.g. think about accessing a struct field).
>
> Furthermore, the enclosing layout check depends on the _base_ offset at which
> the segment is accessed, _prior_ to any index computation that occurs if the
> accessed layout path has any open elements. It is important to notice that
> this base offset is only available when looking at the var handle that is
> returned to the user. For instance, an indexed var handle with coordinates:
>
>
> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */, long
> /* index 3 */)
>
>
> Is obtained through adaptation, by taking a more basic var handle of the form:
>
>
> (MemorySegment, long /* offset */)
>
>
> And then injecting the result of the index multiplication into `offset`. As
> such, we can't add an enclosing layout check inside the var handle returned
> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the
> *wrong* offset (e.g. an offset in which the index part has already been mixed
> in)...
Maurizio Cimadamore has updated the pull request incrementally with one
additional commit since the last revision:
Fix copyrights
-
Changes:
- all: https://git.openjdk.org/jdk/pull/19251/files
- new: https://git.openjdk.org/jdk/pull/19251/files/236007c3..a7b09d9d
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=19251=01
- incr: https://webrevs.openjdk.org/?repo=jdk=19251=00-01
Stats: 8 lines in 5 files changed: 0 ins; 3 del; 5 mod
Patch: https://git.openjdk.org/jdk/pull/19251.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/19251/head:pull/19251
PR: https://git.openjdk.org/jdk/pull/19251
Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v2]
On Thu, 16 May 2024 14:34:41 GMT, Maurizio Cimadamore
wrote:
>> When creating a nested memory access var handle, we ensure that the segment
>> is accessed at the correct alignment for the root layout being accessed. But
>> we do not ensure that the segment has at least the size of the accessed root
>> layout. Example:
>>
>>
>> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"),
>> JAVA_INT.withName("y")));
>> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(),
>> PathElement.groupElement("x"));
>>
>>
>> If I have a memory segment whose size is 12, I can successfully call
>> X_HANDLE on it with index 1, like so:
>>
>>
>> MemorySegment segment = Arena.ofAuto().allocate(12);
>> int x = (int)X_HANDLE.get(segment, 0, 1);
>>
>>
>> This seems incorrect: after all, the provided segment doesn't have enough
>> space to fit _two_ elements of the nested structs.
>>
>> This PR adds checks to detect this kind of scenario earlier, thus improving
>> usability. To achieve this we could, in principle, just tweak
>> `LayoutPath::checkEnclosingLayout` and add the required size check.
>>
>> But doing so will add yet another redundant check on top of the other
>> already added by [pull/19124](https://git.openjdk.org/jdk/pull/19124).
>> Instead, this PR rethinks how memory segment var handles are created, in
>> order to eliminate redundant checks.
>>
>> The main observation is that size and alignment checks depend on an
>> _enclosing_ layout. This layout *might* be the very same value layout being
>> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in
>> the general case, the accessed value layout and the enclosing layout might
>> differ (e.g. think about accessing a struct field).
>>
>> Furthermore, the enclosing layout check depends on the _base_ offset at
>> which the segment is accessed, _prior_ to any index computation that occurs
>> if the accessed layout path has any open elements. It is important to notice
>> that this base offset is only available when looking at the var handle that
>> is returned to the user. For instance, an indexed var handle with
>> coordinates:
>>
>>
>> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */,
>> long /* index 3 */)
>>
>>
>> Is obtained through adaptation, by taking a more basic var handle of the
>> form:
>>
>>
>> (MemorySegment, long /* offset */)
>>
>>
>> And then injecting the result of the index multiplication into `offset`. As
>> such, we can't add an enclosing layout check inside the var handle returned
>> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the
>> *wrong* off...
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix copyrights
test/jdk/java/foreign/TestHeapAlignment.java line 48:
> 46: assertAligned(align, layout, () ->
> layout.varHandle().get(segment, 0L));
> 47: assertAligned(align, layout, () ->
> layout.varHandle().set(segment, 0L, val));
> 48: MemoryLayout seq = MemoryLayout.sequenceLayout(1, layout);
This change was an actual issue in the test, which was made manifest by the new
checks
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1603496914
