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

8261230: GC tracing of page sizes are wrong in a few places #2486

Closed
wants to merge 3 commits into from

Conversation

@kstefanj
Copy link
Contributor

@kstefanj kstefanj commented Feb 9, 2021

The usage of os::trace_page_sizes() and friends are wrongly assuming that we always get the page size requested and needs to be updated. This is done by using the helper ReservedSpace::actual_reserved_page_size() instead of blindly trusting we get what we ask for. I have plans for the future to get rid of this helper and instead record the page size used in the ReservedSpace, but for now the helper is good enough.

In G1 we used the helper but switched the order of the page size and the alignment parameter, which in turn helped the test to pass since the alignment will match the page size we expect in the test. The test had to be improved to recognize mapping failures.


Progress

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

Issue

  • JDK-8261230: GC tracing of page sizes are wrong in a few places

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2486/head:pull/2486
$ git checkout pull/2486

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 9, 2021

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

  • hotspot-gc

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.

Loading

@openjdk openjdk bot added the hotspot-gc label Feb 9, 2021
@kstefanj kstefanj marked this pull request as ready for review Feb 9, 2021
@openjdk openjdk bot added the rfr label Feb 9, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 9, 2021

Webrevs

Loading

Copy link
Member

@albertnetymk albertnetymk left a comment

In the test, there's some string matching to detect if large page is properly set up. I think it's best to include an excerpt of the log showing both the success and failure modes in the comments. This way even readers who are not intimately familiar with the gc-logs output could still feel fairly confident that the output parsing part is indeed correct.

Loading

// First check if there is a large page failure associated with
// the data structure being checked.
Copy link
Contributor Author

@kstefanj kstefanj Feb 11, 2021

Choose a reason for hiding this comment

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

Are you thinking something like this @albertnetymk?

Suggested change
// First check if there is a large page failure associated with
// the data structure being checked.
// First check if there is a large page failure associated with
// the data structure being checked. In case of a large page
// allocation failure the output will include logs like this for
// the affected data structure:
// [0.048s][debug][gc,heap,coops] Reserve regular memory without large pages
// [0.048s][info ][pagesize ] Next Bitmap: ... page_size=4K ...

Loading

Copy link
Member

@albertnetymk albertnetymk Feb 11, 2021

Choose a reason for hiding this comment

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

Yes, and also for checkLargePagesEnabled. It's not obvious to me why we parse the output in those two places, one looking for the failure mode, and the other looking for the success mode. That's why I asked for an sample of the "expected" log output.

Loading

Copy link
Contributor Author

@kstefanj kstefanj Feb 11, 2021

Choose a reason for hiding this comment

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

I think the check if large pages are enable is pretty straight forward. We should never expect large page sizes in the output unless large pages are enabled. I do however agree that this check is a bit clunky. Would it help to extract it to a separate function? Something like largePagesAllocationFailure(pattern), I could also change the name of the function above to just be largePagesEnabled() then the code reads even better?

Loading

Copy link
Member

@albertnetymk albertnetymk Feb 12, 2021

Choose a reason for hiding this comment

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

I overlooked the fact that largePagesAllocationFailure is pattern/data structure specific. I am happy with the current patch. Thank you.

Loading

Copy link
Member

@tstuefe tstuefe left a comment

Looks good. os::trace_page_sizes_for_requested_size is not easy to understand, especially with the alignment vs preferred_page_size semantic. Not sure what alignment has to do with preferred page size.

We could prevent these kind of errors and make the code more readable by introducing a page size enum. We only have a handful of valid values anyway.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Feb 11, 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:

8261230: GC tracing of page sizes are wrong in a few places

Reviewed-by: ayang, 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 52 new commits pushed to the master branch:

  • 682e78e: 8261071: AArch64: Refactor interpreter native wrappers
  • ebaa58d: 8261505: Test test/hotspot/jtreg/gc/parallel/TestDynShrinkHeap.java killed by Linux OOM Killer
  • 3210095: 8261079: Fix support for @hidden in classes and interfaces
  • 9c0ec8d: 8260941: Remove the conc_scan parameter for CardTable
  • da9895a: 8261499: Simplify HTML for javadoc links
  • 0779add: 8255059: Regressions >5% in all Javadoc benchmarks in 16-b19
  • 6a84ec6: 8260044: Parallel GC: Concurrent allocation after heap expansion may cause unnecessary full gc
  • 92ff891: 8261593: Do not use NULL pointer as write buffer parameter in jfrEmergencyDump.cpp write_repository_files
  • 60a2072: 8260431: com/sun/jdi/JdbOptions.java failed with "RuntimeException: 'prop[boo] = >foo<' missing from stdout/stderr"
  • bf47a47: 8261282: Lazy initialization of built-in ICC_Profile/ColorSpace classes is too lazy
  • ... and 42 more: https://git.openjdk.java.net/jdk/compare/8ebed28403afa1fae2505a1937694c90d27c6d6b...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.

Loading

@openjdk openjdk bot added the ready label Feb 11, 2021
Renamed helper to improve how the code read. Also extracted the failure check into a separate function.
@kstefanj
Copy link
Contributor Author

@kstefanj kstefanj commented Feb 11, 2021

Looks good. os::trace_page_sizes_for_requested_size is not easy to understand, especially with the alignment vs preferred_page_size semantic. Not sure what alignment has to do with preferred page size.

Thanks. I agree that it is not that straight forward but I think the intention here is to pass in both page size and alignment to make it easier to understand why the actual size might be large than the requested size. For example in cases like this:

[debug][gc,heap,coops] Reserve regular memory without large pages
[info ][pagesize     ] Next Bitmap: req_size=32000K base=0x00007fa4ef400000 page_size=4K alignment=2M size=32M

We could prevent these kind of errors and make the code more readable by introducing a page size enum. We only have a handful of valid values anyway.

Yes, there is certainly room for improvement in this area =)

Loading

@kstefanj
Copy link
Contributor Author

@kstefanj kstefanj commented Feb 12, 2021

/integrate

Loading

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

@openjdk openjdk bot commented Feb 12, 2021

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

  • 40ae993: 8261027: AArch64: Support for LSE atomics C++ HotSpot code
  • 9ffabf3: 8252971: WindowsFileAttributes does not know about Unix domain sockets
  • 682e78e: 8261071: AArch64: Refactor interpreter native wrappers
  • ebaa58d: 8261505: Test test/hotspot/jtreg/gc/parallel/TestDynShrinkHeap.java killed by Linux OOM Killer
  • 3210095: 8261079: Fix support for @hidden in classes and interfaces
  • 9c0ec8d: 8260941: Remove the conc_scan parameter for CardTable
  • da9895a: 8261499: Simplify HTML for javadoc links
  • 0779add: 8255059: Regressions >5% in all Javadoc benchmarks in 16-b19
  • 6a84ec6: 8260044: Parallel GC: Concurrent allocation after heap expansion may cause unnecessary full gc
  • 92ff891: 8261593: Do not use NULL pointer as write buffer parameter in jfrEmergencyDump.cpp write_repository_files
  • ... and 44 more: https://git.openjdk.java.net/jdk/compare/8ebed28403afa1fae2505a1937694c90d27c6d6b...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9f81ca8.

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

Loading

@kstefanj
Copy link
Contributor Author

@kstefanj kstefanj commented Feb 12, 2021

Thanks for the reviews @albertnetymk and @tstuefe!

Loading

@kstefanj kstefanj deleted the 8261230-gc-trace-ps branch May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants