8236073: G1: Use SoftMaxHeapSize to guide GC heuristics#24211
8236073: G1: Use SoftMaxHeapSize to guide GC heuristics#24211caoman wants to merge 9 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back manc! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
|
@mo-beck Here is my implementation for |
|
This probably requires fixing https://bugs.openjdk.org/browse/JDK-8352765 before users try to use |
|
This PR is ready for review. Included tests cover important functionality of |
|
With the changes to |
Yes. This is the expected behavior if user sets |
The original patch on the CR only set the guidance for the marking. It did not interact with heap sizing directly at all like the change does. What is the reason for this change? (Iirc, in tests long time ago, with that original patch, and also adapting So similar to @walulyai I would strongly prefer for GC thrashing will also prevent progress with marking, and actually cause more marking because of objects not having enough time to die. This just makes the situation worse until the heap gets scaled back to However at the moment, changing the GC activity threshold internally will not automatically shrink the heap as you would expect, since currently shrinking is controlled by marking using the That gets us back to JDK-8238687 and JDK-8248324... @walulyai is currently working on the former issue again, testing it, maybe you two could work together on that to see whether basing this work on what @walulyai is cooking up is a better way forward, if needed modifying Otherwise, if there really is need to get this functionality asap, even only making it a guide for the marking should at least give some effect (but I think without changing |
|
There also seems to be a concurrency issue with reading the So e.g. the assignment of Probably an The other multiple re-readings of the |
|
Re Thomas' comment:
Because without changing heap sizing directly, setting For other concerns, I think one fundamental issue is the precedence of heap sizing flags: should the JVM respect
If user sets a too small However, I can see that
Yes, fixing these two issues would be great regardless of |
|
Re: concurrency issue with reading I updated to
I don't see where the re-read is. I think in any code path from I agree it is a data race if |
In the current approach, it is not that we are respecting the user's request, we are violating the request just that we do this only during GCs. So eventually you have back to back GCs that will expand the heap to whatever heapsize the application requires. My interpretation of
Agreed, these ratios are problematic, and we should find a solution that removes them. We also need to agree on the purpose of |
The compiler could be(*) free to call (*) Probably not after looking again, given that it's not marked as The situation would be much worse if somehow |
Exactly.
With no flag to interfere (no As you mention, there is need for some strategy to reconcile divergent goals - ultimately G1 needs a single value that tells it to resize the heap in which direction in which degree. Incidentally, the way So it seems fairly straightforward to have any outside "memory pressure" effect this intermediate control value instead of everyone overriding each other in multiple places in the code. Now there is some question about the weights of these factors: we (in the gc team) prefer to keep G1's balancing between throughput and latency, particularly if the input this time is some value explicitly containing "soft" in its name. Using the 25% from ZGC as a max limit for gc cpu usage if we are (way) beyond what the user desires seems good enough for an initial guess. Not too high, guaranteeing some application progress in the worst case (for this factor!), not too low, guaranteeing that the intent of the user setting this value is respected. (One can see |
|
Thank you both for the quick and detailed responses!
I was unaware that G1 plans to stop using In comparison, this PR's approach for a high-precedence, "harder" I'll need some discussion with my team about what we would do next. Meanwhile, @mo-beck do you guys have preference on how
Somewhat related to above, our experience with our internal algorithm that adjusts I'm not sure if |
Last time this has been mentioned in the hotspot-gc-dev list has been here. I remember giving multiple outlines to everyone involved earlier, each mentioning that I will look through the existing bugs and see if I there is a need for a(nother) master bug.
Obviously there are issues to sort out. :) |
|
Filed JDK-8353716. |
|
Also collected thoughts and existing documents with some additional rough explanations. |
|
Thank you for creating JDK-8353716!
Apology for overlooking previous mentions about I guess this is a good example that a one-pager doc/umbrella bug provides cleaner communication and additional values over email discussion, especially when one party already has a pretty detailed plan for how it should be done. |
Don't worry, I should have been better with following up with that summary about thoughts/plans communicated so far somewhere publicly. Let's go forward with that CR summarizing the respective (current) general direction. |
Thanks for the thoughtful work here — this PR is a solid step toward strengthening G1’s memory footprint management, and I support it. This patch adds support for The behavior when the live set exceeds the soft target has come up in the discussion. My view remains that the heap should be influenced by the value, not strictly bound to it. That’s the balance I’ve been aiming for in describing how it integrates into the control loop — SoftMax helps inform decisions, but doesn’t unconditionally restrict them. I agree that we’ll want to follow up with logic that can respond to GC pressure and workload needs, to avoid any unintended performance issues. I’ll update JDK-8353716 to reflect this, and I’ll continue the thread on the mailing list to coordinate the next phase. |
|
@caoman This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
/touch |
|
@tschatzl The pull request is being re-evaluated and the inactivity timeout has been reset. |
|
@caoman This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
@caoman This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
Hi all,
I have implemented SoftMaxHeapSize for G1 as attached. It is completely reworked compared to previous PR, and excludes code for
CurrentMaxHeapSize. I believe I have addressed all direct concerns from previous email thread, such as:MinHeapSize;MinHeapFreeRatio,MaxHeapFreeRatio;This recent thread also has some context.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24211/head:pull/24211$ git checkout pull/24211Update a local copy of the PR:
$ git checkout pull/24211$ git pull https://git.openjdk.org/jdk.git pull/24211/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24211View PR using the GUI difftool:
$ git pr show -t 24211Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24211.diff
Using Webrev
Link to Webrev Comment