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

[Kelvin Nilsen]

On Wed, 24 Sep 2025 23:07:40 GMT, William Kemper <wkemper at openjdk.org> wrote:

>> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 2329:
>> 
>>> 2327:   // unaccounted_bytes is the bytes not accounted for by our forced sample.  If the sample interval is too short,
>>> 2328:   // the "forced sample" will not happen, and any recently allocated bytes are "unaccounted for".  We pretend these
>>> 2329:   // bytes are allocated after the start of subsequent gc.
>> 
>> I think these comments should be made slightly more abstract and moved to the spec of `reset` and of the `force` methods. These almost feel like a bit of the specifics of the sampling and the "residue" computation leaking through into objects that it shouldn't leak into. So a bit of thought on how to organize this best by pushing everything to one place would work better from the standpoint of code maintainability and robustness.
>> 
>> As I understand the issue is that we use two separate APIs with no locking to read the allocated bytes and to reset these bytes, all while these are being concurrently incremented. So you want to have the reset method detect the residue allocation between the two points, and ascribe it to the rate computation to be used in the next sample. I feel there may be a simpler way to do this by pushing all of these mechanics & details down into the sampling method which has direct access to the counter that it can manipulate for computing rates and upon request for resetting.
>
> I agree. What we're really try to track is the mutator's allocation rate. There's no particular reason it needs to be implemented in terms of bytes allocated _since gc start_. Of course, we must reset (or at least, reduce) the value periodically to avoid rollover, but again, we don't need to do that only when we start a gc. As @kdnilsen noted, having the control thread take the samples also means we don't collect samples during the GC cycle. We should consider having mutators (or some other thread) collect the samples and manage the allocation counter.

Thanks for review.  I'll update docs in a separate PR (since I integrated already).

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27398#discussion_r2377265803