8312116: GenShen: make instantaneous allocation rate triggers more timely#29039
8312116: GenShen: make instantaneous allocation rate triggers more timely#29039kdnilsen wants to merge 106 commits intoopenjdk:masterfrom
Conversation
When allocation rate appears to be accelerating, predict consumption of memory according to an accelerated rate of consumption. This expedites triggers under critical phase changes.
Set both acceleration sample size and momentary spike sample size to 10. Remove the restriction that momentary spike sample size must be strictly less than acceleration sample size. These changes were motivated by experiments with Extremem workloads. More experiments are in progress and further changes may be implemented based on those resuts.
Also refine future predicted gc time to account for possible delay before we start the GC cycle.
Also tidy up the descriptions of new sample size parameters.
As originally implemented, we apply penalties to the triggering heuristic every time we experience a degenerated cycle. This has the effect of forcing GC triggers to spiral out of control. This commit changes the penalty mechanism. When a degen happens through no fault of the heuristic triggering mechanism, we do not pile on additional penalties. Specifically, we consider that heuristic triggering is not responsible for a degenerated cycle that is associated with a GC that began immediately following the end of the previous GC cycle.
Added tag jdk-25+10 for changeset a637ccf
… <= _highest_valid_narrow_klass_id) failed: narrowKlass ID out of range (3131947710) Reviewed-by: shade
(and less expensive monitoring of triggering conditions)
…erated-triggers-gh
…erated-triggers-gh
…erated-triggers-gh
| } | ||
| // else, leave current_rate = y_max, acceleration = 0 | ||
| } | ||
| // and here also, leave current_rate = y_max, acceleration = 0 |
There was a problem hiding this comment.
y_max is no more. fix these two comments.
| range, \ | ||
| constraint) \ | ||
| \ | ||
| product(double, ShenandoahAccelerationSamplePeriod, 0.0145, EXPERIMENTAL, \ |
There was a problem hiding this comment.
Let's change this option to ms rather than seconds for consistency with existing parameters.
| size_t spike_headroom = capacity / 100 * ShenandoahAllocSpikeFactor; | ||
| size_t penalties = capacity / 100 * _gc_time_penalties; | ||
| avg_cycle_time = _gc_cycle_time_history->davg() + (_margin_of_error_sd * _gc_cycle_time_history->dsd()); | ||
| avg_alloc_rate = _allocation_rate.upper_bound(_margin_of_error_sd); |
There was a problem hiding this comment.
Before we test any trigger conditions, we should consider whether a certain minimum amount of memory has been allocated. Move the test from accelerated-triggers below to apply to all triggers.
There was a problem hiding this comment.
Changed this code.
| } | ||
|
|
||
| ShenandoahAdaptiveHeuristics::~ShenandoahAdaptiveHeuristics() {} | ||
| void ShenandoahAdaptiveHeuristics::compute_headroom_adjustment(size_t mutator_available) { |
There was a problem hiding this comment.
No need for mutator_available as an argument and no need to compute byte_allocated_at_start_of_idle.
Comment: if someone changes soft_max_capacity(), this should be called to recompute.
There was a problem hiding this comment.
I've simplified implementation of compute_headroom_adjustment() and removed unnecessary argument.
I've added a call to compute_headroom_adjustment() when soft_max_capacity is changed.
| // before we need to start the next GC. | ||
| void start_idle_span() override; | ||
|
|
||
| // If old-generation marking finishes during an idle span and immediate old-generation garbage is identified, we will rebuild |
There was a problem hiding this comment.
Maybe this is redundant with start_idle_span or not even necessary (since start_idle_span doesn't need bytes_available.
There was a problem hiding this comment.
I've removed resume_idle_span().
| // source of feedback to adjust trigger parameters. | ||
| TruncatedSeq _available; | ||
|
|
||
| ShenandoahFreeSet* _free_set; |
There was a problem hiding this comment.
Can we use TruncatedSeq::predict_next() for this linear prediction?
Also, can we get rid of _regulator_thread, _control_thread, is_generational?
There was a problem hiding this comment.
I've removed _regulator_thread, _control_thread, _is_generational from ShenanoahAdaptiveHeuristics.
TruncatedSeq::predict_next() assumes all data samples are equidistant and does not allow a parameter to predict the value at a specific future time, so it does not provide the same functionality as the abstraction introduced in this PR.
…aptive triggers to wait for some garbage to accumulate
…ft_capacity is managed
…andoahAdaptiveHeuristics
|
/integrate |
|
Going to push as commit 0b183bf.
Your commit was automatically rebased without conflicts. |
| _partitions.leftmost(ShenandoahFreeSetPartitionId::OldCollector), | ||
| _partitions.rightmost(ShenandoahFreeSetPartitionId::OldCollector)); | ||
| old_region_count++; | ||
| assert(ac = ShenandoahHeapRegion::region_size_bytes(), "Cannot move to old unless entire region is in alloc capacity"); |
There was a problem hiding this comment.
Is this assignment expected here?
There was a problem hiding this comment.
Thanks for the catch. If the asserted condition is true, this is harmless. But certainly not what was intended.
I will fix in a follow-on patch.
| if (i > 0) { | ||
| // first sample not included in weighted average because it has no weight. | ||
| double sample_weight = x_array[i] - x_array[i-1]; | ||
| weighted_y_sum = y_array[i] * sample_weight; |
There was a problem hiding this comment.
Should this be changed to
weighted_y_sum += y_array[i] * sample_weight;
?
There was a problem hiding this comment.
Will also correct this in a follow-on patch. Thanks.
After studying large numbers of GC logs with degenerated cycles that have resulted from "late" triggers, we propose the following general improvements:
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/29039/head:pull/29039$ git checkout pull/29039Update a local copy of the PR:
$ git checkout pull/29039$ git pull https://git.openjdk.org/jdk.git pull/29039/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 29039View PR using the GUI difftool:
$ git pr show -t 29039Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/29039.diff
Using Webrev
Link to Webrev Comment