Remove the division from StepLong/StepDouble rollCount

[michaelbraun]

Summary

Every update to a step value calls rollCount, which divided the current wall time by the step size to get the interval index. step is a non-trusted final instance field, so C2 cannot constant fold it, and this compiles to a real idivq on the update path of every counter, timer, gauge and distribution summary.

Cache the end of the current interval instead of its index. An in-interval update then becomes a comparison against a volatile long, and the division is paid only on an actual rollover. Boundaries are exact multiples of step, so comparing them orders the intervals exactly the way comparing the indices did.

No public API or behavior change. The only externally visible difference is toString, which used to print lastInitPos; it now prints the field that actually exists. Both classes are documented as internal implementation details subject to change.

Correctness

StepRollCountDifferentialTest drives the new implementation and a copy of the previous one over identical timestamp sequences and requires bit-for-bit equal results. It covers every entry point (addAndGet, getCurrent, setCurrent, getAndSet, min, max, poll, pollAsRate), asserting both the current value and timestamp() after each operation, over step sizes of 1ms, 7ms, 13ms, 1s, 5s and 60s and five starting alignments. The sequences deliberately include exact boundaries, boundary +/- 1, multi-interval gaps and backwards clock movement.

StepRollCountConcurrencyTest covers the boundary compare that precedes the CAS. Without it, two threads that both observe the same boundary would both roll, and the second would reset current and publish the completed interval as zero. That cannot be exercised single threaded.

I checked the tests are not vacuous by injecting four bugs; all were caught:

Injected bug	Caught by
>= weakened to > in the boundary check	differential (all step sizes)
timestamp() off by one step	differential
always carry previous forward	differential
drop the boundary compare before the CAS	concurrency test only

./gradlew :spectator-api:build passes, including checkstyle, spotbugs and the JDK 17/25/26 test tasks.

Benchmark

New StepValueUpdate JMH benchmark in spectator-api (the change is here, and it needs no registry). JDK 25, 5 forks x (5 x 1s warmup + 10 x 2s measurement) = 50 measurement iterations, single threaded. Errors are 99.9% confidence intervals including fork-to-fork variance. "Before" is this repo's main with only the benchmark file itself added, so both runs measure identical call sequences.

Benchmark	Before (ops/s)	After (ops/s)	Change	What it isolates
stepLongAddAndGetWithClock	30,501,630 ± 2,954	33,893,242 ± 2,623	+11.1%	Production shape: clock read then update, as AtlasCounter.add does
stepDoubleAddAndGetWithClock	30,235,242 ± 20,295	33,314,284 ± 17,130	+10.2%	Same, for the double valued path
stepDoublePoll	523,071,218 ± 23,004	1,421,236,753 ± 4,409,376	+171.7%	Step bookkeeping alone, no atomic update
stepLongPoll	523,059,413 ± 27,033	1,413,357,287 ± 3,879,963	+170.2%	Step bookkeeping alone, no atomic update
varyingStepLongPoll	523,041,511 ± 42,926	1,107,528,228 ± 848,189	+111.7%	Bookkeeping alone, timestamp the JIT cannot hoist
stepLongAddAndGet	492,950,974 ± 316,047	501,408,748 ± 848,167	+1.7%	Bookkeeping plus the atomic add
stepDoubleAddAndGet	228,828,698 ± 15,665	228,837,004 ± 15,354	+0.0%	Bookkeeping plus the CAS loop
varyingStepLongAddAndGet	496,459,787 ± 162,386	471,198,933 ± 271,861	-5.1%	As above, timestamp from a counter increment
rollingStepLong	108,540,850 ± 70,225	101,583,257 ± 143,372	-6.4%	Worst case: 1ms step, a rollover on every call
rollingStepDouble	108,370,102 ± 330,907	101,761,468 ± 133,115	-6.1%	Worst case: 1ms step, a rollover on every call
wallTime	41,131,379 ± 2,581	41,132,156 ± 2,244	+0.0%	Control: one wall clock read, for scale

*WithClock is the shape a real update has, matching AtlasCounter.add: read the clock, then update. That is the number worth quoting. The wallTime control matching to 0.02% across the two runs indicates the machine was not drifting between them.

The disassembly confirms the mechanism directly: idivq appears throughout the before compilation of the hot loop and is entirely absent after.

On the two apparent regressions

rollingStepLong and rollingStepDouble use a 1ms step with a timestamp advancing every call, so every single update rolls over. That is the adversarial case: the division is still paid and the boundary load is added on top. It costs ~6%, and it only applies to a meter rolling over on essentially every update. With the 5s step used in practice a rollover is one update in millions.

varyingStepLongAddAndGet at -5.0% is reproducible rather than noise, and it is worth being precise about because it is not a cost of this change. The division is long-latency work on a separate execution port, so on main it masks whatever else the loop is doing. Measured on main, that benchmark is flat at 492-510M ops/s whether the timestamp comes from a constant, a counter increment, or an array load (a throwaway variant used to diagnose this, not included here). With the division gone the same three land at 501M, 471M and 453M, tracking the cost of the timestamp source itself. In other words the loop with a trivial timestamp is faster after this change (stepLongAddAndGet, 501.4M vs 493.0M); what the slower variants measure is the benchmark's own timestamp generation emerging from behind the divide.

I ruled out the alternatives before settling on that: splitting rollCount into a fast path plus a cold rollCountSlow is not responsible (an unsplit build measures the same, 470.7M vs 471.6M); nor is field layout (padding nextStepBoundary onto its own cache line, verified in the disassembly to move it from offset 0x30 to 0x70, changes nothing); nor loop alignment or unrolling (-XX:OptoLoopAlignment=32 and -XX:LoopUnrollLimit=1 leave both builds unmoved).

A real caller reads a clock, which costs far more than anything the division could mask, which is why the production-shaped benchmark gains 11%.

[brharrington]

This test appears to be flakey. ManualClock starts at wallTime 0, so nextStepBoundary = STEP = 10000. But every thread calls value.addAndGet(now = 5 * STEP = 50000, 1L). The very first call satisfies now >= nextStepBoundary, so the intended "stays within a single interval" setup actually opens with a rollover. With all 8 threads already released from the barrier and incrementing, a thread that loses the boundary CAS still completes its CURRENT_UPDATER.addAndGet; if that lands before the winner's getAndSet(this, init), the increment is wiped. Hence expected: <800000> but was: <799993>.

Fix is one line before the barrier release:

StepLong value = new StepLong(0L, clock, STEP);
final int perThread = 100_000;
final long now = 5L * STEP;
value.addAndGet(now, 0L);   // do the rollover up front, then the test really is in-interval

[michaelbraun]

This should be fixed now

[michaelbraun]

should be fixed now