Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc #18007

Closed
wants to merge 6 commits into from

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Feb 26, 2024

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)


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8326711 to be approved

Issues

  • JDK-8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc (Bug - P4)
  • JDK-8326711: Dubious claim on long[]/double[] alignment in MemorySegment javadoc (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18007/head:pull/18007
$ git checkout pull/18007

Update a local copy of the PR:
$ git checkout pull/18007
$ git pull https://git.openjdk.org/jdk.git pull/18007/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18007

View PR using the GUI difftool:
$ git pr show -t 18007

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18007.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Feb 26, 2024

👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Feb 26, 2024

@JornVernee The following label will be automatically applied to this pull request:

  • core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the core-libs core-libs-dev@openjdk.org label Feb 26, 2024
@JornVernee JornVernee marked this pull request as ready for review February 26, 2024 14:23
@openjdk openjdk bot added the rfr Pull request is ready for review label Feb 26, 2024
@mlbridge
Copy link

mlbridge bot commented Feb 26, 2024

Webrevs

* (1004, 1008, 1012, 1016) are 4-byte aligned. And, the segment can be accessed at
* offsets 0, 2, 4, 6, etc under a 2-byte alignment constraint, because the target
* addresses (e.g. 1000, 1002, 1004, 1006) are 2-byte aligned.</li>
* (e.g. 1000), so that successive long elements occur at 8-byte aligned addresses
Copy link
Contributor

@mcimadamore mcimadamore Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) <em>maximum</em>
 * 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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got the second one already. Will modify the 1st and 3rd

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. If you already are an OpenJDK Author, Committer or Reviewer, please click here 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.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

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 .

@@ -161,7 +161,7 @@ public void testStructSizeAndAlign() {
ValueLayout.JAVA_LONG
);
assertEquals(struct.byteSize(), 1 + 1 + 2 + 4 + 8);
assertEquals(struct.byteAlignment(), ADDRESS.byteSize());
assertEquals(struct.byteAlignment(), 8);
Copy link
Contributor

@mcimadamore mcimadamore Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this PR:
#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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't seem to be the right PR link?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found the right one: 44218b1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched back to using the old generator (and removed the newer one): fad15a6

Copy link
Contributor

@mcimadamore mcimadamore Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, PR number was similar to this and got confused:
#14007

@openjdk
Copy link

openjdk bot commented Feb 26, 2024

@JornVernee This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc

Reviewed-by: mcimadamore

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 158 new commits pushed to the master branch:

  • 0d35450: 8327040: Problemlist ActionListenerCalledTwiceTest.java test failing in macos14
  • 8d6f784: 8327056: Remove unused static char array in JvmtiAgentList::lookup
  • 43af120: 8326959: Improve JVMCI option help
  • 742c776: 8322743: C2: prevent lock region elimination in OSR compilation
  • d29cefb: 8326838: JFR: Native mirror events
  • b8fc418: 8326525: com/sun/tools/attach/BasicTests.java does not verify AgentLoadException case
  • d9aa1de: 8318605: Enable parallelism in vmTestbase/nsk/stress/stack tests
  • bbfda65: 8326897: (fs) The utility TestUtil.supportsLinks is wrongly used to check for hard link support
  • db0e2b8: 8326944: (ch) Minor typo in the ScatteringByteChannel.read(ByteBuffer[] dsts,int offset,int length) javadoc
  • 8f6edd8: 8326975: Parallel: Remove redundant PSOldGen::is_allocated
  • ... and 148 more: https://git.openjdk.org/jdk/compare/4c7b313e0dc917cdaffbb2ecc86d1347683acad0...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added ready Pull request is ready to be integrated csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated csr Pull request needs approved CSR before integration labels Feb 26, 2024
@JornVernee
Copy link
Member Author

/integrate

@openjdk
Copy link

openjdk bot commented Mar 5, 2024

Going to push as commit 2372aba.
Since your change was applied there have been 201 commits pushed to the master branch:

  • c653e67: 8327225: Revert DataInputStream.readUTF to static final
  • a089ed2: 8326936: RISC-V: Shenandoah GC crashes due to incorrect atomic memory operations
  • 560cf59: 8327287: Remove unused FLSVerifyDictionary debug option
  • fec51d4: 8327130: Serial: Remove Generation::record_spaces_top
  • e9adceb: 8327208: Remove unused method java.util.jar.Manifest.make72Safe
  • d6f2a17: 8325881: Require minimum gcc version 10
  • 0b95909: 8327224: G1: comment in G1BarrierSetC2::post_barrier() refers to nonexistent new_deferred_store_barrier()
  • c589555: 8325095: C2: bailout message broken: ResourceArea allocated string used after free
  • b7540df: 8327007: javax/swing/JSpinner/8008657/bug8008657.java fails
  • e1b661f: 8319900: Recursive lightweight locking: riscv64 implementation
  • ... and 191 more: https://git.openjdk.org/jdk/compare/4c7b313e0dc917cdaffbb2ecc86d1347683acad0...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Mar 5, 2024
@openjdk openjdk bot closed this Mar 5, 2024
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 5, 2024
@openjdk
Copy link

openjdk bot commented Mar 5, 2024

@JornVernee Pushed as commit 2372aba.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@JornVernee JornVernee deleted the X86_Long_Double_Align branch March 5, 2024 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
3 participants