Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v6]
> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that > returns the maximum byte alignment of a segment (both heap and native > segments). > > Clients can then use this method to determine if a segment is properly > aligned for any given layout (e.g. following a `MemorySegment::reinterpret` > operation). Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Update docs for maxByteAlignment - Changes: - all: https://git.openjdk.org/jdk/pull/18779/files - new: https://git.openjdk.org/jdk/pull/18779/files/e971e7f6..8b944a35 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18779=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18779=04-05 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18779.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18779/head:pull/18779 PR: https://git.openjdk.org/jdk/pull/18779
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v5]
On Tue, 16 Apr 2024 09:39:02 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Update JavaDoc
>
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 612:
>
>> 610: * the type of the {@linkplain #heapBase() backing heap storage}.
>> 611: *
>> 612: * This method can be used to ensure, a {@code segment} is
>> sufficiently aligned
>
> I would drop the use of code blocks for `segment` and layout. Also remove the
> comma after "ensure"
Also, "ensure _that_ a memory segment is ..."
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1567060697
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v5]
On Tue, 16 Apr 2024 07:49:26 GMT, Per Minborg wrote:
>> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that
>> returns the maximum byte alignment of a segment (both heap and native
>> segments).
>>
>> Clients can then use this method to determine if a segment is properly
>> aligned for any given layout (e.g. following a `MemorySegment::reinterpret`
>> operation).
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update JavaDoc
Marked as reviewed by mcimadamore (Reviewer).
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 612:
> 610: * the type of the {@linkplain #heapBase() backing heap storage}.
> 611: *
> 612: * This method can be used to ensure, a {@code segment} is
> sufficiently aligned
I would drop the use of code blocks for `segment` and layout. Also remove the
comma after "ensure"
-
PR Review: https://git.openjdk.org/jdk/pull/18779#pullrequestreview-2003148028
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1567059979
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v5]
> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that > returns the maximum byte alignment of a segment (both heap and native > segments). > > Clients can then use this method to determine if a segment is properly > aligned for any given layout (e.g. following a `MemorySegment::reinterpret` > operation). Per Minborg has updated the pull request incrementally with one additional commit since the last revision: Update JavaDoc - Changes: - all: https://git.openjdk.org/jdk/pull/18779/files - new: https://git.openjdk.org/jdk/pull/18779/files/eaf6832e..e971e7f6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18779=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18779=03-04 Stats: 8 lines in 1 file changed: 0 ins; 5 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18779.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18779/head:pull/18779 PR: https://git.openjdk.org/jdk/pull/18779
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v4]
> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that > returns the maximum byte alignment of a segment (both heap and native > segments). > > Clients can then use this method to determine if a segment is properly > aligned for any given layout (e.g. following a `MemorySegment::reinterpret` > operation). Per Minborg has updated the pull request incrementally with three additional commits since the last revision: - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/18779/files - new: https://git.openjdk.org/jdk/pull/18779/files/7b45ad5c..eaf6832e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18779=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18779=02-03 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18779.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18779/head:pull/18779 PR: https://git.openjdk.org/jdk/pull/18779
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v3]
On Mon, 15 Apr 2024 14:21:23 GMT, Per Minborg wrote: >> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that >> returns the maximum byte alignment of a segment (both heap and native >> segments). >> >> Clients can then use this method to determine if a segment is properly >> aligned for any given layout (e.g. following a `MemorySegment::reinterpret` >> operation). > > Per Minborg has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains ten additional > commits since the last revision: > > - Update after comments > - Merge branch 'master' into ms-reinterpret2 > - Update > src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java > >Co-authored-by: Jorn Vernee > - Update > src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java > >Co-authored-by: Jorn Vernee > - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java > >Co-authored-by: Maurizio Cimadamore > <54672762+mcimadam...@users.noreply.github.com> > - Fix imports and copyright > - Update maxByteAlignment docs > - Improve doc and tests > - Add a MS::maxByteAlignment method Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18779#pullrequestreview-2001722291
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v3]
On Mon, 15 Apr 2024 14:39:02 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains ten additional
>> commits since the last revision:
>>
>> - Update after comments
>> - Merge branch 'master' into ms-reinterpret2
>> - Update
>> src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java
>>
>>Co-authored-by: Jorn Vernee
>> - Update
>> src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java
>>
>>Co-authored-by: Jorn Vernee
>> - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java
>>
>>Co-authored-by: Maurizio Cimadamore
>> <54672762+mcimadam...@users.noreply.github.com>
>> - Fix imports and copyright
>> - Update maxByteAlignment docs
>> - Improve doc and tests
>> - Add a MS::maxByteAlignment method
>
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 612:
>
>> 610: * Heap:
>> 611: derived from the segment {@linkplain #address() address()}
>> 612: and the type of the backing
>> heap storage.
>
> Shoudn't the link point to `MemorySegment::heapObject` ?
I suggest you generate the javadoc and look at this method - my feeling is that
it won't be very readable. The sentence "a power of two and is derived from" is
left a bit suspended - and you have a bullet list which starts with "Native
Segment" which is not a valid "next token" for the sentence above. After which,
we say again "derived from" hence repeating.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565913273
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v3]
On Mon, 15 Apr 2024 14:21:23 GMT, Per Minborg wrote:
>> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that
>> returns the maximum byte alignment of a segment (both heap and native
>> segments).
>>
>> Clients can then use this method to determine if a segment is properly
>> aligned for any given layout (e.g. following a `MemorySegment::reinterpret`
>> operation).
>
> Per Minborg has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains ten additional
> commits since the last revision:
>
> - Update after comments
> - Merge branch 'master' into ms-reinterpret2
> - Update
> src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java
>
>Co-authored-by: Jorn Vernee
> - Update
> src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java
>
>Co-authored-by: Jorn Vernee
> - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java
>
>Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
> - Fix imports and copyright
> - Update maxByteAlignment docs
> - Improve doc and tests
> - Add a MS::maxByteAlignment method
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 100:
> 98: * expressed as a {@code long} value. The maximum alignment is always a
> power of two,
> 99: * derived from the segment address, and the segment type, as explained
> in more detail
> 100: * (see below).
Suggestion:
* below.
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 610:
> 608: * The returned alignment is always a power of two and is derived
> from:
> 609: *
> 610: * Heap:
Suggestion:
* Heap segments:
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 612:
> 610: * Heap:
> 611: derived from the segment {@linkplain #address() address()}
> 612: and the type of the backing
> heap storage.
Shoudn't the link point to `MemorySegment::heapObject` ?
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 613:
> 611: derived from the segment {@linkplain #address() address()}
> 612: and the type of the backing
> heap storage.
> 613: * Native:
Suggestion:
* Native segments:
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565907165
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565909731
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565909449
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565910025
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v3]
> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that > returns the maximum byte alignment of a segment (both heap and native > segments). > > Clients can then use this method to determine if a segment is properly > aligned for any given layout (e.g. following a `MemorySegment::reinterpret` > operation). Per Minborg has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision: - Update after comments - Merge branch 'master' into ms-reinterpret2 - Update src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Fix imports and copyright - Update maxByteAlignment docs - Improve doc and tests - Add a MS::maxByteAlignment method - Changes: - all: https://git.openjdk.org/jdk/pull/18779/files - new: https://git.openjdk.org/jdk/pull/18779/files/89bb916b..7b45ad5c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18779=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18779=01-02 Stats: 4996 lines in 212 files changed: 1382 ins; 1604 del; 2010 mod Patch: https://git.openjdk.org/jdk/pull/18779.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18779/head:pull/18779 PR: https://git.openjdk.org/jdk/pull/18779
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v2]
On Mon, 15 Apr 2024 13:17:26 GMT, Per Minborg wrote:
>> test/jdk/java/foreign/TestMemoryAlignment.java line 154:
>>
>>> 152: Arena arena = Arena.ofConfined()) {
>>> 153: var segment =channel.map(FileChannel.MapMode.READ_WRITE,
>>> 0L, 32L, arena);
>>> 154: assertTrue(segment.maxByteAlignment() >= Long.BYTES);
>>
>> Is this always the case? Smells of platform-dependent... (e.g. think also of
>> x86)
>
> I think the alignment is always the biggest of the directly supported
> primitive types so maybe we should change to `Integer.BYTES` here.
Yes, but, I mean, we don't have a test for checking what's the alignment of
`malloc` - so I wonder why we should have a test for mapped segments - it's not
like the API makes any promises there, right?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565789636
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v2]
On Mon, 15 Apr 2024 10:16:30 GMT, Maurizio Cimadamore
wrote:
>> Per Minborg has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Update
>> src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java
>>
>>Co-authored-by: Jorn Vernee
>> - Update
>> src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java
>>
>>Co-authored-by: Jorn Vernee
>> - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java
>>
>>Co-authored-by: Maurizio Cimadamore
>> <54672762+mcimadam...@users.noreply.github.com>
>
> test/jdk/java/foreign/TestMemoryAlignment.java line 154:
>
>> 152: Arena arena = Arena.ofConfined()) {
>> 153: var segment =channel.map(FileChannel.MapMode.READ_WRITE,
>> 0L, 32L, arena);
>> 154: assertTrue(segment.maxByteAlignment() >= Long.BYTES);
>
> Is this always the case? Smells of platform-dependent... (e.g. think also of
> x86)
I think the alignment is always the biggest of the directly supported primitive
types so maybe we should change to `Integer.BYTES` here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565781128
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v2]
On Mon, 15 Apr 2024 13:05:01 GMT, Per Minborg wrote: >> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 615: >> >>> 613: * the requested byte alignment during native segment allocation)} >>> 614: * >>> 615: * The returned alignment is always an even power of two and is >>> derived from: >> >> Not sure about the "even". Surely 2^3 is a valid max alignment? > > The meaning of "even" here is that it is a number 2^n, n >= 0, n ∈ Z. But > maybe "a power of two" is enough. power of two implies even, so I'd say it's better to omit - PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565769068
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v2]
On Mon, 15 Apr 2024 10:07:31 GMT, Maurizio Cimadamore wrote: >> Per Minborg has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - Update >> src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java >> >>Co-authored-by: Jorn Vernee >> - Update >> src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java >> >>Co-authored-by: Jorn Vernee >> - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java >> >>Co-authored-by: Maurizio Cimadamore >> <54672762+mcimadam...@users.noreply.github.com> > > src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 615: > >> 613: * the requested byte alignment during native segment allocation)} >> 614: * >> 615: * The returned alignment is always an even power of two and is >> derived from: > > Not sure about the "even". Surely 2^3 is a valid max alignment? The meaning of "even" here is that it is a number 2^n, n >= 0, n ∈ Z. But maybe "a power of two" is enough. - PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565762747
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v2]
On Mon, 15 Apr 2024 10:05:44 GMT, Maurizio Cimadamore wrote: >> Per Minborg has updated the pull request incrementally with three additional >> commits since the last revision: >> >> - Update >> src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java >> >>Co-authored-by: Jorn Vernee >> - Update >> src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java >> >>Co-authored-by: Jorn Vernee >> - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java >> >>Co-authored-by: Maurizio Cimadamore >> <54672762+mcimadam...@users.noreply.github.com> > > src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 408: > >> 406: * } >> 407: * >> 408: * In order to simplify determination of alignment, in the case of >> either native or heap > > This can be expressed more directly as follows: > > Clients can use the ... method to check if a memory segment supports the > alignment constraint of a memory layout, as follows: > > > I'd also advise against using a method in the snippet, as that looks like the > method is part of the API. Perhaps something like this: > > > MemoryLayout layout = ... > MemorySegment segment = ... > boolean isAligned = segment.maxByteAlignment() >= layout.byteAlignment() The example was similar to an existing method shown in a snippet in the same class, but we could change it anyway. - PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565751189
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v2]
> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that > returns the maximum byte alignment of a segment (both heap and native > segments). > > Clients can then use this method to determine if a segment is properly > aligned for any given layout (e.g. following a `MemorySegment::reinterpret` > operation). Per Minborg has updated the pull request incrementally with three additional commits since the last revision: - Update src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java Co-authored-by: Jorn Vernee - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java Co-authored-by: Maurizio Cimadamore <54672762+mcimadam...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/18779/files - new: https://git.openjdk.org/jdk/pull/18779/files/dc0505ff..89bb916b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18779=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18779=00-01 Stats: 10 lines in 3 files changed: 0 ins; 4 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18779.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18779/head:pull/18779 PR: https://git.openjdk.org/jdk/pull/18779
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints
On Mon, 15 Apr 2024 07:50:24 GMT, Per Minborg wrote:
> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that
> returns the maximum byte alignment of a segment (both heap and native
> segments).
>
> Clients can then use this method to determine if a segment is properly
> aligned for any given layout (e.g. following a `MemorySegment::reinterpret`
> operation).
src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java
line 86:
> 84: alignment = maxAlignMask();
> 85: }
> 86: return Math.min(maxAlignMask(), alignment);
Similarly here:
Suggestion:
return address() == 0
? maxAlignMask()
: Math.min(maxAlignMask(), Long.lowestOneBit(address()));
src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java
line 68:
> 66: ? 1L << 62
> 67: : alignment;
> 68: }
This relies on the fact that `lowestOneBit` returns `0` only if `address()`
returns `0`, but I suggest making the intent clearer by checking against
`address()` directly:
Suggestion:
return address() == 0
? 1L << 62
: Long.lowestOneBit(address());
(I also suggest introducing a name for the `1L << 62` constant, e.g.
`MAX_NATIVE_ALIGN`)
test/jdk/java/foreign/TestMemoryAlignment.java line 194:
> 192: new Object[]{MemorySegment.ofArray(new
> float[]{1}), Float.BYTES},
> 193: new Object[]{MemorySegment.ofArray(new
> double[]{1}), Double.BYTES},
> 194: new
> Object[]{MemorySegment.ofBuffer(ByteBuffer.allocate(8)), Byte.BYTES}
What about the other (heap) buffer types?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565638655
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565637521
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565647024
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints
On Mon, 15 Apr 2024 07:50:24 GMT, Per Minborg wrote:
> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that
> returns the maximum byte alignment of a segment (both heap and native
> segments).
>
> Clients can then use this method to determine if a segment is properly
> aligned for any given layout (e.g. following a `MemorySegment::reinterpret`
> operation).
test/jdk/java/foreign/TestMemoryAlignment.java line 154:
> 152: Arena arena = Arena.ofConfined()) {
> 153: var segment =channel.map(FileChannel.MapMode.READ_WRITE, 0L,
> 32L, arena);
> 154: assertTrue(segment.maxByteAlignment() >= Long.BYTES);
Is this always the case? Smells of platform-dependent... (e.g. think also of
x86)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565537503
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints
On Mon, 15 Apr 2024 07:50:24 GMT, Per Minborg wrote:
> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that
> returns the maximum byte alignment of a segment (both heap and native
> segments).
>
> Clients can then use this method to determine if a segment is properly
> aligned for any given layout (e.g. following a `MemorySegment::reinterpret`
> operation).
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 408:
> 406: * }
> 407: *
> 408: * In order to simplify determination of alignment, in the case of
> either native or heap
This can be expressed more directly as follows:
Clients can use the ... method to check if a memory segment supports the
alignment constraint of a memory layout, as follows:
I'd also advise against using a method in the snippet, as that looks like the
method is part of the API. Perhaps something like this:
MemoryLayout layout = ...
MemorySegment segment = ...
boolean isAligned = segment.maxByteAlignment() >= layout.byteAlignment()
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 412:
> 410: * {@snippet lang=java:
> 411: * boolean isAligned(MemorySegment segment, long offset, MemoryLayout
> layout) {
> 412: * return segment.asSlice(offset).maxByteAlignment() >=
> layout.byteAlignment;
Suggestion:
* return segment.asSlice(offset).maxByteAlignment() >=
layout.byteAlignment();
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 612:
> 610:
> 611: /**
> 612: * {@return the maximum byte alignment (which is equal to
> or higher than
Here I would just say:
Return the maximum byte alignment associated with this memory segment (link to
alignment section)
If you then want to say that the max alignment is useful for alignment check,
or allocation, that probably goes in a separate para (but I note you have one
already - e.g. to show how to check alignment).
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 615:
> 613: * the requested byte alignment during native segment allocation)}
> 614: *
> 615: * The returned alignment is always an even power of two and is
> derived from:
Not sure about the "even". Surely 2^3 is a valid max alignment?
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 618:
> 616: *
> 617: * Heap:
> 618: * the segment offset and backing array type.
Note that "segment offset" is `segment.address()`. So I don't think we want to
use a new term here. We can also say, more succinctly:
derived from the segment address and the type of the
backing heap storage (for heap segments).
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 622:
> 620: * the {@linkplain MemorySegment#address() address()}
> function.
> 621: *
> 622: * The {@linkplain MemorySegment#NULL NULL} segment returns a
> maximum byte alignment
I believe this should better be moved in the javadoc for `NULL` ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565520043
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565520387
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565525848
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565522838
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565530922
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565526441
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints
On Mon, 15 Apr 2024 07:50:24 GMT, Per Minborg wrote:
> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that
> returns the maximum byte alignment of a segment (both heap and native
> segments).
>
> Clients can then use this method to determine if a segment is properly
> aligned for any given layout (e.g. following a `MemorySegment::reinterpret`
> operation).
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 99:
> 97: * Every memory segment has a {@linkplain #maxByteAlignment() maximum
> byte alignment}
> 98: * (see Alignment"), expressed as a
> {@code long} value
> 99: * that is always an even power of two.
I think it's better not to say too much about alignment here, otherwise we end
up referring to concepts that are not well adequately introduced. Perhaps it
would be better to just say:
Every memory segment has a maximum byte alignment, expressed as a long value.
The maximum alignment is always a power of two, derived from the segment
address, and the segment type, as explained in more details below .
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565511275
