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

Ergonomics Profile #9

Draft
wants to merge 35 commits into
base: main
Choose a base branch
from
Draft

Ergonomics Profile #9

wants to merge 35 commits into from

Conversation

brunoborges
Copy link
Member

Initial implementation of the Ergonomics Profile proposal (see JEP Draft).

There are two major things missing from this proof of concept:

  1. What to do with Client/Server Class Machines algorithm?
  2. Tests

Copy link
Member

@karianna karianna left a comment

Choose a reason for hiding this comment

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

Also needs a review from VM Eng.

src/hotspot/share/gc/shared/gcConfig.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/shared/gc_globals.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/shared/gcConfig.cpp Show resolved Hide resolved
src/hotspot/share/gc/shared/gc_globals.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved
@d3r3kk
Copy link
Collaborator

d3r3kk commented Aug 8, 2023

@brunoborges please protect this from merging into our main - which is a mirror only.

We may want to merge to an ms-patches branch or some such, but not main.

@brunoborges
Copy link
Member Author

Not my intention to merge. Just to see the difference.

@brunoborges
Copy link
Member Author

I'll convert this to a Draft PR just to be sure :-)

Copy link

@jerboaa jerboaa left a comment

Choose a reason for hiding this comment

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

Hi Bruno, a couple of comments. I'm a bit concerned about the additional complexity about GC selection. Also keep in mind that those MaxRAMPercentage changes seem to affect a wider audience than intended.

src/hotspot/share/runtime/arguments.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/shared/gc_globals.hpp Outdated Show resolved Hide resolved
src/hotspot/share/runtime/arguments.cpp Show resolved Hide resolved
src/hotspot/share/gc/shared/gcConfig.cpp Show resolved Hide resolved
MinRAMPercentage is used as maxheap by the legacy ergonomics (aka shared) for when the environment has up to 512MB of RAM.
@brunoborges brunoborges marked this pull request as draft August 8, 2023 21:13
@brunoborges
Copy link
Member Author

Hey Severin, just taking note of your suggestion about disabling UseDynamicNumberOfCompilerThreads for small containers. As I mentioned over email, I think it can make sense, but for small containers up to 512M.

I wonder what others stumbling upon this would think.

karianna
karianna previously approved these changes Aug 8, 2023
Copy link
Member

@karianna karianna left a comment

Choose a reason for hiding this comment

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

My nitpick review is done but other folks who understand the semantics of this change better should review as well :-)

@karianna karianna dismissed their stale review August 8, 2023 21:16

meant to add comment, not approve

@jerboaa
Copy link

jerboaa commented Aug 9, 2023

Hey Severin, just taking note of your suggestion about disabling UseDynamicNumberOfCompilerThreads for small containers. As I mentioned over email, I think it can make sense, but for small containers up to 512M.

This idea came about with the discussions in https://bugs.openjdk.org/browse/JDK-8296125 and https://git.openjdk.org/jdk/pull/10918

@brunoborges
Copy link
Member Author

What to do with MaxDirectMemorySize ?

@brunoborges
Copy link
Member Author

What to do with ReservedCodeCacheSize? By default it is 240M and it doesn't matter how much memory is set in the environment.

InitialCodeCacheSize is 2M.

@brunoborges
Copy link
Member Author

@lifey is suggesting a few changes: microsoft/openjdk-proposals@main...lifey:openjdk-proposals:main

@karianna
Copy link
Member

@lifey is suggesting a few changes: microsoft/[email protected]:openjdk-proposals:main

Recommend @mo-beck and Kirk review that diff

Copy link

@schlosna schlosna left a comment

Choose a reason for hiding this comment

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

The proposed changes broadly seem reasonable given many of the applications and environments I have seen throughout the years, and simplifies some of the challenges folks have when moving JVM applications into hard resource limited containers.

Do you have thoughts how this might evolve over time? How should folks think about using (ideally just the auto) ergonomics profile, and what is the threshold at which additional JVM argument tuning needs to be considered?

src/hotspot/share/gc/shared/gcConfig.cpp Outdated Show resolved Hide resolved
@brunoborges
Copy link
Member Author

brunoborges commented May 10, 2024

The auto profile will be removed and instead I will be adding a flag called -XX:+AutoErgonomicsProfile, while the other flag remains to select between valid profiles. "Auto" is not meant to be a profile, and thus I am thinking about this approach.

The JEP text has changed already to reflect this. Mind to take a look at that?

Regarding more profiles in the future, it is not a goal of this JEP to consider that possibility for OpenJDK, but specific JDK vendors may choose to add more if they want to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants