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&pr=18779&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18779&range=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
