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
