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

8303852: current_stack_region() gets called twice unnecessarily #15321

Closed

Conversation

dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Aug 17, 2023

We combine:

address os::current_stack_base();
size_t os::current_stack_size();

into

void os::current_stack_base_and_size(address* stack_base, size_t* stack_size)

and so avoid making two underlying system calls. The os/platform specific specializations are handled by the current_stack_region calls. It made sense to modify that to export the base directly rather than the bottom. In doing that change it made sense to standardise on the the style of the code used (variable names etc) to make it easier to see where the 5 different versions were the same and where they differ. Having 5 versions is unfortunate ( 3 BSD: one per arch - zero, x64, aarch64; and 2 linux: zero and everything else), but trying to combine them with ifdefs would be even worse in my opinion.

In theory there should be zero functional changes here.

Testing:

  • Tiers 1-3
  • All our internal builds in tiers 1-5
  • GHA

Thanks.


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

Issue

  • JDK-8303852: current_stack_region() gets called twice unnecessarily (Enhancement - P3)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15321

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 17, 2023

👋 Welcome back dholmes! 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 Aug 17, 2023

@dholmes-ora The following label will be automatically applied to this pull request:

  • hotspot

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 hotspot hotspot-dev@openjdk.org label Aug 17, 2023
…bottom.

Standardise the appoach taken with all the versions of the function.
Typo iin zero.
@dholmes-ora dholmes-ora marked this pull request as ready for review August 18, 2023 05:48
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 18, 2023
@mlbridge
Copy link

mlbridge bot commented Aug 18, 2023

Webrevs

Copy link
Member

@tstuefe tstuefe left a comment

Choose a reason for hiding this comment

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

Hi David,

looks good.

some thoughts:

  • the implementations for zero and non-zero Linux are identical and could be merged. I looked at them side by side. The zero implementation looks a bit bitrotted (e.g. does not have the latest guard size adjustments) so unifying them would make zero more correct.

  • bsd aarch64 and x64 could be merged - the only difference is a workaround in x64 that is missing on arm, from 8020753: "JNI_CreateJavaVM on Mac OSX 10.9 Mavericks corrupts the callers stack size". That workaround claims to fix the problem of not-page-aligned stack size for the primordial thread. I just tested on my M1 arm, and the primordial thread has a stack size of 2060K (java -Xlog:os* -version), which is not page-aligned (16K pages). So either this workaround is not needed (since the JVM on M1 seems happy enough) or it is a hidden bug and we need the workaround. In both cases, we could merge both versions of current_stack_region.

  • the bsd zero variant is almost equal to the arm variant, so we could merge those.

And then, since you equalized the interfaces of os::XX::current_stack_region() and os::get_stack_base_and_size, would it be possible to drop the former and implement the latter for the various OSes?

All these are thoughts, and possibly for future RFEs. THis patch looks ok as it is too.

Cheers, Thomas

src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os/windows/os_windows.cpp Outdated Show resolved Hide resolved
src/hotspot/os/linux/os_linux.cpp Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented Aug 18, 2023

@dholmes-ora 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:

8303852: current_stack_region() gets called twice unnecessarily

Reviewed-by: stuefe, pchilanomate

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 136 new commits pushed to the master branch:

  • c077be4: 8314694: Separate checked_cast from globalDefinitions.hpp
  • 68815d5: 8314734: Remove unused field TypeVariableImpl.EMPTY_ANNOTATION_ARRAY
  • 57a322d: 8308042: [macos] Developer ID Application Certificate not picked up by jpackage if it contains UNICODE characters
  • 38a9edf: 8314679: SA fails to properly attach to JVM after having just detached from a different JVM
  • 2c60cad: 8280743: HSDB "Monitor Cache Dump" command might throw NPE
  • 9435cd1: 8175874: Update Security.insertProviderAt to specify behavior when requested position is out of range.
  • dbb788f: 8294535: Add screen capture functionality to PassFailJFrame
  • fae3b02: 8314746: Remove unused private put* methods from DirectByteBufferR
  • 096b7ff: 8314810: (fs) java/nio/file/Files/CopyInterference.java should use TestUtil::supportsLinks
  • 6261020: 8312555: Ideographic characters aren't stretched by AffineTransform.scale(2, 1)
  • ... and 126 more: https://git.openjdk.org/jdk/compare/ec0cc6300a02dd92b25d9072b8b3859dab583bbd...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 the ready Pull request is ready to be integrated label Aug 18, 2023
@dholmes-ora
Copy link
Member Author

Thanks for looking at this @tstuefe - as I noted I didn't try to merge things further as I thought it would makes things more complex. But to address your specific points:

the implementations for zero and non-zero Linux are identical and could be merged.

But as you then state they are not actually identical because zero is missing some of the guard page checks. But zero also has the IA64 code which causes a subtle change in the way size needs to be adjusted. So they are not the same and while they could potentially be merged I much prefer zero code to be kept isolated to zero so that the folks interested in zero can deal with it.

bsd aarch64 and x64 could be merged - the only difference is a workaround in x64 that is missing on arm, from 8020753

I did wonder about that workaround but it was not something I wanted to touch in this PR. If it is not needed afterall, or actually needed on Aarch64 then that is something for a different RFE (or actual bug if the workaround is needed on aarch64).

the bsd zero variant is almost equal to the arm variant, so we could merge those.

Again I like to see zero code isolated, but in addition there is nowhere to put such code except in the shared os_bsd.cpp and then have it ifdef'd out for x86. That said, if the x86 and aarch64 should be the same then yes we could have a single shared version. But again future RFE (I will file one).

And then, since you equalized the interfaces of os::XX::current_stack_region() and os::get_stack_base_and_size, would it be possible to drop the former and implement the latter for the various OSes?

Yes I think it can. The arch specific versions would just be moved to os namespace instead, and no shared implementation just to redirect to the arch-specific ones.

Thanks.

@tstuefe
Copy link
Member

tstuefe commented Aug 21, 2023

Still good. Thanks for fixing this.

Copy link
Contributor

@pchilano pchilano left a comment

Choose a reason for hiding this comment

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

Looks good to me.

src/hotspot/os/linux/os_linux.cpp Outdated Show resolved Hide resolved
src/hotspot/os_cpu/bsd_x86/os_bsd_x86.cpp Outdated Show resolved Hide resolved
src/hotspot/os_cpu/bsd_zero/os_bsd_zero.cpp Outdated Show resolved Hide resolved
@dholmes-ora
Copy link
Member Author

Thanks for the review @pchilano and for catching those typos!

/integrate

@openjdk
Copy link

openjdk bot commented Aug 23, 2023

Going to push as commit 4a50e87.
Since your change was applied there have been 136 commits pushed to the master branch:

  • c077be4: 8314694: Separate checked_cast from globalDefinitions.hpp
  • 68815d5: 8314734: Remove unused field TypeVariableImpl.EMPTY_ANNOTATION_ARRAY
  • 57a322d: 8308042: [macos] Developer ID Application Certificate not picked up by jpackage if it contains UNICODE characters
  • 38a9edf: 8314679: SA fails to properly attach to JVM after having just detached from a different JVM
  • 2c60cad: 8280743: HSDB "Monitor Cache Dump" command might throw NPE
  • 9435cd1: 8175874: Update Security.insertProviderAt to specify behavior when requested position is out of range.
  • dbb788f: 8294535: Add screen capture functionality to PassFailJFrame
  • fae3b02: 8314746: Remove unused private put* methods from DirectByteBufferR
  • 096b7ff: 8314810: (fs) java/nio/file/Files/CopyInterference.java should use TestUtil::supportsLinks
  • 6261020: 8312555: Ideographic characters aren't stretched by AffineTransform.scale(2, 1)
  • ... and 126 more: https://git.openjdk.org/jdk/compare/ec0cc6300a02dd92b25d9072b8b3859dab583bbd...master

Your commit was automatically rebased without conflicts.

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

openjdk bot commented Aug 23, 2023

@dholmes-ora Pushed as commit 4a50e87.

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

@dholmes-ora dholmes-ora deleted the 8303852-current_stack_region branch November 24, 2023 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated
3 participants