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

8257901: ZGC: Take virtual memory usage into account when sizing heap #1696

Closed
wants to merge 4 commits into from

Conversation

@pliden
Copy link
Contributor

@pliden pliden commented Dec 8, 2020

MaxVirtMemFraction limits the amount of address space the GC should use for the heap. This is used by the heuristics in Arguments::set_heap_size() to select an appropriate default max heap size. However, that heuristic can select a max heap size that will not fit with ZGC, since ZGC uses additional address space (for multi-mapping and the virtual-to-physical ratio). As a result, if the address space is limited, and -Xmx is not specified, then ZGC might refuse to start with the error:

"Failed to reserve enough address space for Java heap"

I propose we abstract MaxVirtMemFraction to make it configurable for each GC, so that the heuristics in Arguments::set_heap_size() will pick a default max heap size that also works for ZGC.

Testing: Manual testing using different ulimit -v sizes.


Progress

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

Issue

  • JDK-8257901: ZGC: Take virtual memory usage into account when sizing heap

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Dec 8, 2020

👋 Welcome back pliden! 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 openjdk bot added the rfr label Dec 8, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 8, 2020

@pliden 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.

Loading

@openjdk openjdk bot added the hotspot label Dec 8, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Dec 8, 2020

Loading

stefank
stefank approved these changes Dec 9, 2020
Copy link
Member

@stefank stefank left a comment

The functionality seems fine. However, the naming of things are very misleading. Arguments::limit_by_allocatable_memory has no indication that this can only be used to limit the heap memory. The non-ZGC GCs use MaxVirtMemFraction, which doesn't sound like something that has to do with the heap size. But, if you read the description of that flag it's apparent that it's about the max heap size. The added ZGC implementation also highly depend on the fact that max_virtual_memory_fraction() means the "max fraction of virtual memory that can be used by the heap". I think it would be good to name max_virtual_memory_fraction to something that indicates this. Maybe max_heap_to_virtual_memory_fraction() (maybe there's a more succinct name, that still covers what it does?) And limit_by_allocatable_memory should also be changed, before anyone starts to use it as generic function that can be used by the rest of the JVM.

If you agree maybe update the names in this PR, or create a separate RFE?

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Dec 9, 2020

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

8257901: ZGC: Take virtual memory usage into account when sizing heap

Reviewed-by: stefank, eosterlund, ayang, tschatzl

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

  • db5da96: 8257876: Avoid Reference.isEnqueued in tests
  • 4a839e9: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information
  • d93293f: 8256730: Code that uses Object.checkIndex() range checks doesn't optimize well
  • 869dcb6: 8257806: Optimize x86 allTrue and anyTrue vector mask operations of Vector API
  • 34650f5: 8257872: UL: -Xlog does not check number of options
  • 6847bbb: 8255918: XMLStreamFilterImpl constructor consumes XMLStreamException
  • d2f9e31: 8257638: Update usage of "type" terminology in javax.lang.model
  • f631a99: 8256888: Client manual test problem list update
  • eb1c8a1: 8257924: Use full path when running external executable from jpackage
  • 1ce2a36: 8257539: tools/jpackage/windows/WinL10nTest.java unpack.bat failed with Exit code: 1618
  • ... and 43 more: https://git.openjdk.java.net/jdk/compare/1d0adbb812bc4b40fb614f245dd8ed2ef66ff073...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 Dec 9, 2020
@pliden
Copy link
Contributor Author

@pliden pliden commented Dec 9, 2020

@stefank Pushed an update to adjust the names. Also fixed copyrights.

Loading

stefank
stefank approved these changes Dec 9, 2020
Copy link
Member

@stefank stefank left a comment

Thanks!

Loading

fisk
fisk approved these changes Dec 9, 2020
Copy link
Contributor

@fisk fisk left a comment

Looks good.

Loading

@@ -46,6 +47,6 @@ size_t ZAddressSpaceLimit::mark_stack() {

size_t ZAddressSpaceLimit::heap_view() {
// Allow all heap views to occupy 50% of the address space
Copy link
Member

@albertnetymk albertnetymk Dec 9, 2020

Choose a reason for hiding this comment

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

The "50%" description is inaccurate, right?

Loading

Copy link
Contributor Author

@pliden pliden Dec 9, 2020

Choose a reason for hiding this comment

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

No, that's still correct.

Loading

// Used by heap size heuristics to determine max amount
// of address space to use for the heap.
Copy link
Contributor

@tschatzl tschatzl Dec 9, 2020

Choose a reason for hiding this comment

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

Could this explanation moved to the hpp file? Then some IDEs will automatically pick it up. I'd expect a comments to the implementation, i.e. of the value 1 here instead of a generic explanation of what the method does.

Loading

Copy link
Contributor Author

@pliden pliden Dec 9, 2020

Choose a reason for hiding this comment

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

Ok, I'll move the comment.

Loading

@@ -1655,11 +1655,12 @@ jint Arguments::set_ergonomics_flags() {
return JNI_OK;
}

julong Arguments::limit_by_allocatable_memory(julong limit) {
julong Arguments::limit_heap_by_allocatable_memory(julong limit) {
Copy link
Contributor

@tschatzl tschatzl Dec 9, 2020

Choose a reason for hiding this comment

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

If we are already messing with the name in this change, would it be possible to change return value and parameter to size_t? I'm good with moving this change of the parameter types to a different issue, but as mentioned, since we are already changing stuff around here...

Loading

Copy link
Contributor Author

@pliden pliden Dec 9, 2020

Choose a reason for hiding this comment

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

I'd prefer to handle that as a separate issue, since such a change would propagate into os::has_allocatable_memory_limit() and affect all os:es.

Loading

Copy link
Contributor

@tschatzl tschatzl Dec 9, 2020

Choose a reason for hiding this comment

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

okay.

Loading

@@ -365,7 +365,7 @@ class Arguments : AllStatic {
static jint set_shared_spaces_flags_and_archive_paths();
// limits the given memory size by the maximum amount of memory this process is
// currently allowed to allocate or reserve.
static julong limit_by_allocatable_memory(julong size);
static julong limit_heap_by_allocatable_memory(julong size);
Copy link
Contributor

@tschatzl tschatzl Dec 9, 2020

Choose a reason for hiding this comment

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

Please clarify the documentation of this method also according to the results of this discussion.

Loading

Copy link
Contributor Author

@pliden pliden Dec 9, 2020

Choose a reason for hiding this comment

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

Will fix.

Loading

@pliden
Copy link
Contributor Author

@pliden pliden commented Dec 9, 2020

Pushed an update which addresses @tschatzl's comments.

Loading

@stefank
Copy link
Member

@stefank stefank commented Dec 10, 2020

Latest update looks good.

Loading

fisk
fisk approved these changes Dec 10, 2020
@pliden
Copy link
Contributor Author

@pliden pliden commented Dec 10, 2020

Thanks all for reviewing!

/integrate

Loading

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

@openjdk openjdk bot commented Dec 10, 2020

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

  • 29ffffa: 8257997: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java again reports leaks after JDK-8257884
  • db5da96: 8257876: Avoid Reference.isEnqueued in tests
  • 4a839e9: 8256459: java/net/httpclient/ManyRequests.java and java/net/httpclient/LineBodyHandlerTest.java fail infrequently with java.net.ConnectException: Connection timed out: no further information
  • d93293f: 8256730: Code that uses Object.checkIndex() range checks doesn't optimize well
  • 869dcb6: 8257806: Optimize x86 allTrue and anyTrue vector mask operations of Vector API
  • 34650f5: 8257872: UL: -Xlog does not check number of options
  • 6847bbb: 8255918: XMLStreamFilterImpl constructor consumes XMLStreamException
  • d2f9e31: 8257638: Update usage of "type" terminology in javax.lang.model
  • f631a99: 8256888: Client manual test problem list update
  • eb1c8a1: 8257924: Use full path when running external executable from jpackage
  • ... and 44 more: https://git.openjdk.java.net/jdk/compare/1d0adbb812bc4b40fb614f245dd8ed2ef66ff073...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0a0691e.

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

Loading

@pliden pliden deleted the 8257901_max_virt_mem_fraction branch Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants