Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v3]
On Mon, 26 Feb 2024 15:10:55 GMT, Maurizio Cimadamore
wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> re-widen test
>
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 328:
>
>> 326: * physical address 1010.
>> 327: * The starting physical address of a {@code long[]} array will be
>> 8-byte aligned
>> 328: * (e.g. 1000), so that successive long elements occur at 8-byte
>> aligned addresses
>
> I believe there might be other changes required. I see the following
> sentences in the javadoc:
>
>
> * In other words, heap segments feature a (platform-dependent)
> maximum
> * alignment which is derived from the size of the elements of the Java array
> backing the
> * segment, as shown in the following table:
> ```
>
>
> * In such circumstances, clients have two options. They can use a heap
> segment backed
> * by a different array type (e.g. {@code long[]}), capable of supporting
> greater maximum
> * alignment. More specifically, the maximum alignment associated with {@code
> long[]} is
> * set to {@code ValueLayout.JAVA_LONG.byteAlignment()} which is a
> platform-dependent
> * value (set to {@code ValueLayout.ADDRESS.byteSize()}). That is, {@code
> long[]}) is
> * guaranteed to provide at least 8-byte alignment in 64-bit platforms, but
> only 4-byte
> * alignment in 32-bit platforms:
> ```
>
> ```
> * In practice, the Java runtime lays out arrays in memory so that each
> n-byte element
> * occurs at an n-byte aligned physical address (except for {@code long[]} and
> * {@code double[]}, where alignment is platform-dependent, as explained
> below).
> ```
> Hi @mcimadamore, thanks for making a comment in an OpenJDK project!
>
> All comments and discussions in the OpenJDK Community must be made available
> under the OpenJDK [Terms of Use](https://openjdk.java.net/legal/tou/). If you
> already are an OpenJDK [Author](https://openjdk.java.net/bylaws#author),
> [Committer](https://openjdk.java.net/bylaws#committer) or
> [Reviewer](https://openjdk.java.net/bylaws#reviewer), please click
> [here](https://bugs.openjdk.java.net/secure/CreateIssue.jspa?pid=11300&issuetype=1)
> to open a new issue so that we can record that fact. Please Use "Add GitHub
> user mcimadamore" for the summary.
>
> If you are not an OpenJDK Author, Committer or Reviewer, simply check the box
> below to accept the OpenJDK Terms of Use for your comments.
>
> * [ ] I agree to the [OpenJDK Terms of
> Use](https://openjdk.java.net/legal/tou/) for all comments I make in a
> project in the [OpenJDK GitHub organization](https://github.com/openjdk).
>
> Your comment will be automatically restored once you have accepted the
> OpenJDK [Terms of Use](https://openjdk.java.net/legal/tou/).
It is strange to see this comment because @mcimadamore had already been a
member in OpenJDK. The SKARA bot may meet a bug. CC'ing @erikj79 and
@zhaosongzs .
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1506099034
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
On Mon, 26 Feb 2024 19:19:56 GMT, Jorn Vernee wrote: >> Found the right one: >> https://github.com/openjdk/jdk/commit/44218b1c9e5daa33557aac9336251cf8398d81eb > > Switched back to using the old generator (and removed the newer one): > https://github.com/openjdk/jdk/pull/18007/commits/fad15a66b68b07b8bb17fce3c4a7d8788ef44015 Sorry, PR number was similar to this and got confused: https://github.com/openjdk/jdk/pull/14007 - PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r150336
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v3]
On Mon, 26 Feb 2024 19:17:56 GMT, Jorn Vernee wrote: >> This patch changes the alignment for `JAVA_LONG` and `JAVA_DOUBLE` to 8, >> regardless of the underlying platform. This means that atomic access modes >> work on memory segments wrapping `long[]` or `double[]`, as they already do >> when using `MethodHandless::arrayAccessVarHandle`. >> >> After discussion, we came to the conclusion that it is reasonable for the >> JDK to require the elements of a `long[]` and `double[]` to be 8 byte >> aligned. It is ultimately up to the JDK to set these requirements, which are >> for the VM to implement. >> >> I was seeing a stack overflow when running >> test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java on x86, so I've >> lowered the recursion to 50 (which is still more than enough I think). >> >> Testing: `jdk_foreign` on x64 Windows, x64 Windows + fallback linker, and >> x86 Linux (uses fallback linker) > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > re-widen test Marked as reviewed by mcimadamore (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18007#pullrequestreview-1902076898
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
On Mon, 26 Feb 2024 19:13:41 GMT, Jorn Vernee wrote: >> That doesn't seem to be the right PR link? > > Found the right one: > https://github.com/openjdk/jdk/commit/44218b1c9e5daa33557aac9336251cf8398d81eb Switched back to using the old generator (and removed the newer one): https://github.com/openjdk/jdk/pull/18007/commits/fad15a66b68b07b8bb17fce3c4a7d8788ef44015 - PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1503174593
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
On Mon, 26 Feb 2024 19:10:39 GMT, Jorn Vernee wrote: >> test/jdk/java/foreign/TestLayouts.java line 164: >> >>> 162: ); >>> 163: assertEquals(struct.byteSize(), 1 + 1 + 2 + 4 + 8); >>> 164: assertEquals(struct.byteAlignment(), 8); >> >> Looking at this PR: >> https://github.com/openjdk/jdk/pull/18007 >> >> This test seemed to cover more case before that PR - e.g. a layout generator >> was tweaked to exclude long/doubles. I believe revert the test changes now? > > That doesn't seem to be the right PR link? Found the right one: https://github.com/openjdk/jdk/commit/44218b1c9e5daa33557aac9336251cf8398d81eb - PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1503166930
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v3]
> This patch changes the alignment for `JAVA_LONG` and `JAVA_DOUBLE` to 8, > regardless of the underlying platform. This means that atomic access modes > work on memory segments wrapping `long[]` or `double[]`, as they already do > when using `MethodHandless::arrayAccessVarHandle`. > > After discussion, we came to the conclusion that it is reasonable for the JDK > to require the elements of a `long[]` and `double[]` to be 8 byte aligned. It > is ultimately up to the JDK to set these requirements, which are for the VM > to implement. > > I was seeing a stack overflow when running > test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java on x86, so I've > lowered the recursion to 50 (which is still more than enough I think). > > Testing: `jdk_foreign` on x64 Windows, x64 Windows + fallback linker, and x86 > Linux (uses fallback linker) Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: re-widen test - Changes: - all: https://git.openjdk.org/jdk/pull/18007/files - new: https://git.openjdk.org/jdk/pull/18007/files/b145a58b..fad15a66 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18007&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18007&range=01-02 Stats: 10 lines in 1 file changed: 0 ins; 4 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18007/head:pull/18007 PR: https://git.openjdk.org/jdk/pull/18007
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
On Mon, 26 Feb 2024 16:21:38 GMT, Maurizio Cimadamore wrote: >> Jorn Vernee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> review comments > > test/jdk/java/foreign/TestLayouts.java line 164: > >> 162: ); >> 163: assertEquals(struct.byteSize(), 1 + 1 + 2 + 4 + 8); >> 164: assertEquals(struct.byteAlignment(), 8); > > Looking at this PR: > https://github.com/openjdk/jdk/pull/18007 > > This test seemed to cover more case before that PR - e.g. a layout generator > was tweaked to exclude long/doubles. I believe revert the test changes now? That doesn't seem to be the right PR link? - PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1503163065
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
On Mon, 26 Feb 2024 16:05:51 GMT, Jorn Vernee wrote: >> This patch changes the alignment for `JAVA_LONG` and `JAVA_DOUBLE` to 8, >> regardless of the underlying platform. This means that atomic access modes >> work on memory segments wrapping `long[]` or `double[]`, as they already do >> when using `MethodHandless::arrayAccessVarHandle`. >> >> After discussion, we came to the conclusion that it is reasonable for the >> JDK to require the elements of a `long[]` and `double[]` to be 8 byte >> aligned. It is ultimately up to the JDK to set these requirements, which are >> for the VM to implement. >> >> I was seeing a stack overflow when running >> test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java on x86, so I've >> lowered the recursion to 50 (which is still more than enough I think). >> >> Testing: `jdk_foreign` on x64 Windows, x64 Windows + fallback linker, and >> x86 Linux (uses fallback linker) > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > review comments test/jdk/java/foreign/TestLayouts.java line 164: > 162: ); > 163: assertEquals(struct.byteSize(), 1 + 1 + 2 + 4 + 8); > 164: assertEquals(struct.byteAlignment(), 8); Looking at this PR: https://github.com/openjdk/jdk/pull/18007 This test seemed to cover more case before that PR - e.g. a layout generator was tweaked to exclude long/doubles. I believe revert the test changes now? - PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1502938943
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
On Mon, 26 Feb 2024 15:10:55 GMT, Maurizio Cimadamore
wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments
>
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 328:
>
>> 326: * physical address 1010.
>> 327: * The starting physical address of a {@code long[]} array will be
>> 8-byte aligned
>> 328: * (e.g. 1000), so that successive long elements occur at 8-byte
>> aligned addresses
>
> I believe there might be other changes required. I see the following
> sentences in the javadoc:
>
>
> * In other words, heap segments feature a (platform-dependent)
> maximum
> * alignment which is derived from the size of the elements of the Java array
> backing the
> * segment, as shown in the following table:
> ```
>
>
> * In such circumstances, clients have two options. They can use a heap
> segment backed
> * by a different array type (e.g. {@code long[]}), capable of supporting
> greater maximum
> * alignment. More specifically, the maximum alignment associated with {@code
> long[]} is
> * set to {@code ValueLayout.JAVA_LONG.byteAlignment()} which is a
> platform-dependent
> * value (set to {@code ValueLayout.ADDRESS.byteSize()}). That is, {@code
> long[]}) is
> * guaranteed to provide at least 8-byte alignment in 64-bit platforms, but
> only 4-byte
> * alignment in 32-bit platforms:
> ```
>
> ```
> * In practice, the Java runtime lays out arrays in memory so that each
> n-byte element
> * occurs at an n-byte aligned physical address (except for {@code long[]} and
> * {@code double[]}, where alignment is platform-dependent, as explained
> below).
> ```
I got the second one already. Will modify the 1st and 3rd
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1502882211
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
> This patch changes the alignment for `JAVA_LONG` and `JAVA_DOUBLE` to 8, > regardless of the underlying platform. This means that atomic access modes > work on memory segments wrapping `long[]` or `double[]`, as they already do > when using `MethodHandless::arrayAccessVarHandle`. > > After discussion, we came to the conclusion that it is reasonable for the JDK > to require the elements of a `long[]` and `double[]` to be 8 byte aligned. It > is ultimately up to the JDK to set these requirements, which are for the VM > to implement. > > I was seeing a stack overflow when running > test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java on x86, so I've > lowered the recursion to 50 (which is still more than enough I think). > > Testing: `jdk_foreign` on x64 Windows, x64 Windows + fallback linker, and x86 > Linux (uses fallback linker) Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.org/jdk/pull/18007/files - new: https://git.openjdk.org/jdk/pull/18007/files/787e32bb..b145a58b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18007&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18007&range=00-01 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18007/head:pull/18007 PR: https://git.openjdk.org/jdk/pull/18007
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc
On Mon, 26 Feb 2024 13:24:11 GMT, Jorn Vernee wrote:
> This patch changes the alignment for `JAVA_LONG` and `JAVA_DOUBLE` to 8,
> regardless of the underlying platform. This means that atomic access modes
> work on memory segments wrapping `long[]` or `double[]`, as they already do
> when using `MethodHandless::arrayAccessVarHandle`.
>
> After discussion, we came to the conclusion that it is reasonable for the JDK
> to require the elements of a `long[]` and `double[]` to be 8 byte aligned. It
> is ultimately up to the JDK to set these requirements, which are for the VM
> to implement.
>
> I was seeing a stack overflow when running
> test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java on x86, so I've
> lowered the recursion to 50 (which is still more than enough I think).
>
> Testing: `jdk_foreign` on x64 Windows, x64 Windows + fallback linker, and x86
> Linux (uses fallback linker)
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 328:
> 326: * physical address 1010.
> 327: * The starting physical address of a {@code long[]} array will be
> 8-byte aligned
> 328: * (e.g. 1000), so that successive long elements occur at 8-byte
> aligned addresses
I believe there might be other changes required. I see the following sentences
in the javadoc:
* In other words, heap segments feature a (platform-dependent) maximum
* alignment which is derived from the size of the elements of the Java array
backing the
* segment, as shown in the following table:
```
* In such circumstances, clients have two options. They can use a heap segment
backed
* by a different array type (e.g. {@code long[]}), capable of supporting
greater maximum
* alignment. More specifically, the maximum alignment associated with {@code
long[]} is
* set to {@code ValueLayout.JAVA_LONG.byteAlignment()} which is a
platform-dependent
* value (set to {@code ValueLayout.ADDRESS.byteSize()}). That is, {@code
long[]}) is
* guaranteed to provide at least 8-byte alignment in 64-bit platforms, but
only 4-byte
* alignment in 32-bit platforms:
```
```
* In practice, the Java runtime lays out arrays in memory so that each n-byte
element
* occurs at an n-byte aligned physical address (except for {@code long[]} and
* {@code double[]}, where alignment is platform-dependent, as explained below).
```
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1502771056
RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc
This patch changes the alignment for `JAVA_LONG` and `JAVA_DOUBLE` to 8, regardless of the underlying platform. This means that atomic access modes work on memory segments wrapping `long[]` or `double[]`, as they already do when using `MethodHandless::arrayAccessVarHandle`. After discussion, we came to the conclusion that it is reasonable for the JDK to require the elements of a `long[]` and `double[]` to be 8 byte aligned. It is ultimately up to the JDK to set these requirements, which are for the VM to implement. I was seeing a stack overflow when running test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java on x86, so I've lowered the recursion to 50 (which is still more than enough I think). Testing: `jdk_foreign` on x64 Windows, x64 Windows + fallback linker, and x86 Linux (uses fallback linker) - Commit messages: - add missing tag - Remove references to 32-bit difference in javadoc - Test fixes - Set alignment of JAVA_LONG and JAVA_DOUBLE to 8 bytes Changes: https://git.openjdk.org/jdk/pull/18007/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18007&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8326172 Stats: 54 lines in 10 files changed: 18 ins; 18 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/18007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18007/head:pull/18007 PR: https://git.openjdk.org/jdk/pull/18007
