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

[Kelvin Nilsen]

On Fri, 19 Sep 2025 21:29:32 GMT, William Kemper <wkemper at openjdk.org> wrote:

> Is the real issue that we reset `bytes_allocated_since_gc_start` without sampling the value _before_ it is reset
> (thereby losing allocations that happen during the gc)?

I guess that's a big part of the problem.  But the other problem is that there is not a natural alignment between when we reset the bytes allocated and when we reset the values of _last_sample_time and _last_sample_value.

> Would it be simpler to also take the sample just before it is reset (perhaps use an atomic exchange with zero,
> so we lose nothing)?  Then we wouldn't need to juggle an extra member variable to remember the value. There
> is no reason we aren't allowed to update the allocation rate outside of `should_start_gc`.

There is some inherent synchronization in place that avoids the need for an atomic variable.  As implemented, we only
add samples from should_start_gc(), which is invoked by a single thread "at most" once per ms only when GC is idle and we add one additional sample with GenShen from within final_mark safepoint when we are calculating the necessary reserves.

So the proposed implementation already "loses" nothing, I think.

If you want to "save a variable", we could just initialize _last_sample_value to the bytes allocated following the last sample prior to start of GC instead of initializing this value to zero.  But then we would also have to initialize bytes_allocated_since_gc_start to this same value instead of initializing that to zero.  And this would make bytes_allocated_since_gc_start() a bit imprecise in that we would really be returning bytes allocated since last sample taken before gc start.  I guess I could live with that, but I'm inclined to keep the more accurate semantics and add the state variable to keep track of the delta...

> 
> Side note - should we really call this `bytes_allocated_since_gc_end`? I think that is what the variable actually
> represents because it is reset at the end of the cycle.

I believe we do reset at the start of GC.  The reset_bytes_allocated_since_gc_start() gets called from ShenandoahControlThread::run_service() whenever gc_requested, and at the top of ShenandoahGenerationalControlThread::run_gc_cycle().

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

PR Comment: https://git.openjdk.org/jdk/pull/27398#issuecomment-3313963649