RFR: 8368015: Shenandoah: fix error in computation of average allocation rate [v2]

William Kemper wkemper at openjdk.org
Mon Sep 22 23:13:01 UTC 2025


On Mon, 22 Sep 2025 19:13:41 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> We use bytes_allocated_since_gc_start() to compute allocation rates.  This leaves a blind spot, as our current implementation ignores allocations and the time period between the moment we begin GC and the first time we update the allocation rate following the start of GC.  When this happens, we typically find that the sampled number of allocations is smaller than the allocations that had accumulated by the time we triggered the start of the current GC cycle.
>> 
>> This PR adds tracking for that accounting gap.
>
> Kelvin Nilsen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   remove state variable previous GC allocated

Should we move [this line](https://github.com/openjdk/jdk/pull/27398/files#diff-5cbba824b4e4b914425672f0d1bea49b44aa48940fd07f517b7312055927c575R249) where we take the allocation sample above the early exit when `_start_gc_is_pending`? Not directly related to this PR, but seems to be in the spirit of making the allocation rate sampling more accurate.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 364:

> 362: 
> 363: double ShenandoahAllocationRate::force_sample(size_t allocated, size_t &unaccounted_bytes_allocated) {
> 364:   const double MinSampleTime = 0.002;    // Do not sample if time since last update is less than 2 ms

Would it make sense to "carry over" the `unaccounted_bytes_allocated` if, at the time of reset, the last sample was taken less than `ShenandoahAllocationRate::_interval_sec` (instead of `MinSampleTime`)? I believe this would result in more samples being carried over.

src/hotspot/share/gc/shenandoah/heuristics/shenandoahAdaptiveHeuristics.cpp line 382:

> 380: }
> 381: 
> 382: double ShenandoahAllocationRate::sample(size_t allocated, bool force_update) {

`force_update` looks unused here.

-------------

Changes requested by wkemper (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/27398#pullrequestreview-3255437150
PR Review Comment: https://git.openjdk.org/jdk/pull/27398#discussion_r2370548105
PR Review Comment: https://git.openjdk.org/jdk/pull/27398#discussion_r2370489243


More information about the hotspot-gc-dev mailing list