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

8268388: Update large pages information in Java manpage #4425

Closed
wants to merge 3 commits into from

Conversation

kstefanj
Copy link
Contributor

@kstefanj kstefanj commented Jun 9, 2021

Please review this update to the text for large pages in the Java man page.

The text for LargePageSizeInBytes was reviewed in this CSR, and this change will integrate them into the actual man page. The Large Pages section further down in the man page was also a bit out-dated and have been brushed up a bit.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8268388: Update large pages information in Java manpage

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4425/head:pull/4425
$ git checkout pull/4425

Update a local copy of the PR:
$ git checkout pull/4425
$ git pull https://git.openjdk.java.net/jdk pull/4425/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4425

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4425.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 9, 2021

👋 Welcome back sjohanss! 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 openjdk bot added the rfr label Jun 9, 2021
@openjdk
Copy link

openjdk bot commented Jun 9, 2021

@kstefanj To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels
  • 2d
  • awt
  • beans
  • build
  • compiler
  • core-libs
  • hotspot
  • hotspot-compiler
  • hotspot-gc
  • hotspot-jfr
  • hotspot-runtime
  • i18n
  • javadoc
  • jdk
  • jmx
  • kulla
  • net
  • nio
  • security
  • serviceability
  • shenandoah
  • sound
  • swing

Copy link
Contributor

@tschatzl tschatzl left a comment

Lgtm, one additional remark.

src/java.base/share/man/java.1 Outdated Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented Jun 9, 2021

@kstefanj 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:

8268388: Update large pages information in Java manpage

Reviewed-by: tschatzl, lkorinth, stuefe

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

  • bb3d226: 8238213: Method resolution should stop on static error
  • 81fdeb5: 8268417: Add test from JDK-8268360
  • caf7f49: 8268122: Add specific gc cause for G1 full collections
  • 43e38a1: 8268377: Windows 32bit build fails after JDK-8268174
  • 5fbb62c: 8268163: Change the order of fallback full GCs in G1
  • 7b1e402: 8266598: Exception values for AnnotationTypeMismatchException are not always informative
  • 13d6180: 8264859: Implement Context-Specific Deserialization Filters
  • dd34a4c: 8268372: ZGC: dynamically select the number of concurrent GC threads used
  • 4388959: 8268056: Update java.net and java.nio to use switch expressions
  • 9cfd560: 8267663: [vector] Add unsigned comparison operators on AArch64
  • ... and 23 more: https://git.openjdk.java.net/jdk/compare/2717fcb1345379d9856a33148d548eccb7b708f4...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 label Jun 9, 2021
Copy link
Contributor

@lkorinth lkorinth left a comment

Looks good to me.

@kstefanj
Copy link
Contributor Author

kstefanj commented Jun 9, 2021

/label add hotspot

@openjdk openjdk bot added the hotspot label Jun 9, 2021
@openjdk
Copy link

openjdk bot commented Jun 9, 2021

@kstefanj
The hotspot label was successfully added.

@mlbridge
Copy link

mlbridge bot commented Jun 9, 2021

Webrevs

Copy link
Member

@tstuefe tstuefe left a comment

Hi Stefan,

just idle nitpicking, looks good otherwise.

Cheers, Thomas

.RS
.PP
\f[CB]#\ echo\ 4294967295\ >\ /proc/sys/kernel/shmmax\f[R]
\f[CB]#\ echo\ 4096\ >\ /sys/kernel/mm/hugepages/hugepages\-2048kB/nr_hugepages\f[R]
Copy link
Member

@tstuefe tstuefe Jun 9, 2021

Choose a reason for hiding this comment

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

  • vm.nr_hugepages is not the only way to do this setup, since we have vm.nr_overcommit_hugepages, which are a bit more flexible to use. Is the java man page supposed to be complete in explaining this, or should it only show one possible way of many?
  • most users probably just use UseLargePages and are not aware of the different flavors. So they would not know what "explicit" means. How about, instead of mentioning UseSHM and UseHugeTLBFS, not just: "if you use large pages but don't use transparent huge pages (UseTransparentHugePages), large pages need to be preallocated...".
  • Otherwise, at least reverse the order of the two options? UseSHM is mentioned first, which is maybe historical, but TLBFS is the standard.

Copy link
Contributor Author

@kstefanj kstefanj Jun 9, 2021

Choose a reason for hiding this comment

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

* vm.nr_hugepages is not the only way to do this setup, since we have vm.nr_overcommit_hugepages, which are a bit more flexible to use. Is the java man page supposed to be complete in explaining this, or should it only show one possible way of many?

I think mentioning one way to configure it is fine, we don't want to try to cover every possibility. I mean there are multiple ways of specifying each of the options as well. Most advanced users will likely refer to the kernel documentation for more details. I see this as a short explanation of one way to enable large pages.

* most users probably just use UseLargePages and are not aware of the different flavors. So they would not know what "explicit" means. How about, instead of mentioning UseSHM and UseHugeTLBFS, not just: "if you use large pages but don't use transparent huge pages (UseTransparentHugePages), large pages need to be preallocated...".

Something like:

- When using explicit large pages (options `-XX:+UseSHM` or `-XX:+UseHugeTLBFS`), the number of...
+ When using large pages and not enabling transparent huge pages (option `-XX:+UseTransparentHugePages`), the number of...
* Otherwise, at least reverse the order of the two options? UseSHM is mentioned first, which is maybe historical, but TLBFS is the standard.

Good point, I reversed the order of configuring the tow but forgot it here. But going with the above is better I thing.

@kstefanj
Copy link
Contributor Author

kstefanj commented Jun 9, 2021

@tstuefe, I updated the PR with your suggestion to mention UseTransparentHugePages. I think that is a good way to also get that option into the man page. I intend to integrate this before the fork tomorrow, I hope this is good with you.

tstuefe
tstuefe approved these changes Jun 9, 2021
Copy link
Member

@tstuefe tstuefe left a comment

Looks good Stefan. Thanks for taking my suggestion.

@kstefanj
Copy link
Contributor Author

kstefanj commented Jun 10, 2021

Thanks @tschatzl, @lkorinth and @tstuefe for the reviews.

/integrate

@openjdk openjdk bot closed this Jun 10, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 10, 2021
@openjdk
Copy link

openjdk bot commented Jun 10, 2021

@kstefanj Since your change was applied there have been 45 commits pushed to the master branch:

  • 2623b0b: 8268475: execute runtime/InvocationTests w/ -UseVtableBasedCHA
  • f839308: 8268407: ProblemList sun/tools/jstat/jstatLineCountsX.sh on linux-aarch64 due to JDK-8268211
  • 58ba48b: 8268192: LambdaMetafactory with invokespecial causes VerificationError
  • b41f3f8: 8268478: JVMCI tests failing after JDK-8268052
  • 7ff6e7b: 8267954: Shared classes that failed to load should not be loaded again
  • 991ca14: 8267430: GraphicsDevice.setDisplayMode(REFRESH_RATE_UNKNOWN) throws IAE: Unable to set display mode!
  • bf29a01: 8228343: JCMD and attach fail to work across Linux Container boundary
  • 408e0a9: 8255148: Confusing log output: SSLSocket duplex close failed
  • bbd0313: 8263203: jconsole Online User Guide has wrong URL
  • 33d34c6: 8263323: Debug Agent help output includes invalid URL
  • ... and 35 more: https://git.openjdk.java.net/jdk/compare/2717fcb1345379d9856a33148d548eccb7b708f4...master

Your commit was automatically rebased without conflicts.

Pushed as commit ece3ae3.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot integrated
4 participants