Replace the per-update expiry check in SwapMeter with a removal counter

[michaelbraun]

Summary

SwapMeter.get() runs on the update path of every meter operation and called underlying.hasExpired(). For AtlasMeter that is a wall clock read on every update, to evaluate a TTL measured in minutes.

Track meter removal in the registry with a plain counter instead, and have get() compare against that. hasExpired() itself is unchanged, so destructive callers such as PolledMeter still only see real expiry.

Why removal is a separate signal from the registry version

SwapMeter already had a versionSupplier feeding hasExpired(). Reusing it for removal would be smaller, but it would be wrong: callers treat hasExpired() == true as licence to discard the meter (PolledMeter drops it, measurements() filters it out, cleanupCachedState() evicts it), and removal happens on every cleanup pass rather than rarely. Folding routine removals into that signal would report healthy meters as expired once per pass. SwapMeterExpiryReportingTest.cleanupDoesNotExpireHealthyMeters pins that down, as does cleanupDoesNotBlankOutACompositeRegistry — CompositeMeter reports expired only when all delegates do, and Spectator.globalRegistry() is one.

The counter is registry-global, so any one removal invalidates every outstanding wrapper rather than only the affected one. That costs each held reference one extra map lookup per cleanup pass, which runs once per step interval. lookupCost below is the measurable price.

Behavior

For a meter past its TTL but not yet swept, the old and new triggers are equivalent rather than merely close: lookup() goes through getOrCreate, a computeIfAbsent that does not replace expired entries, so the old re-resolve returned the same instance already held. The update lands on the same object either way. expiredButNotYetRemovedMeterKeepsSameInstance covers this.

Once the meter is actually swept, the counter moves and the next call re-resolves, which is more timely than waiting for the TTL to be observed.

For registries with no removal signal — CompositeRegistry, which has no notion of meter removal and still uses the version-only constructor — the original underlying.hasExpired() trigger is retained, so this change is a no-op there rather than a behavior change that has to be argued from how nested wrappers survive unwrap(). legacyConstructorStillResolvesOnUnderlyingExpiry pins it; stubbing the flag to false makes that test fail.

The sampling order matters and is deliberate: the removal counter is read before the meter is resolved. Sampled afterwards, a removal racing the lookup would already be accounted for and the wrapper would stay bound to a meter that is no longer registered, silently dropping every later update. Sampling early can only cost one redundant re-resolution. versionIsSampledBeforeTheMeterIsResolved drives a removal in exactly that window.

No public API is added: all five Swap* types are package private, and SwapMeter is in impl and documented as internal.

Test plan

• SwapMeterExpiryReportingTest — routine removal is not conflated with hasExpired(), composites are not blanked out, the sampling order is correct, and the version-only path keeps its original trigger.
• AtlasHeldReferenceExpiryTest — held references recover after a sweep, updates through a stale reference are not lost, sweeping one meter does not disturb others, and an expired-but-unswept meter keeps the same instance.
• ./gradlew :spectator-api:build :spectator-reg-atlas:build passes, including checkstyle, spotbugs and the JDK 17/25/26 test tasks.

Benchmark

CounterIncrement, 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 main with only the benchmark file added, so both runs measure identical call sequences.

Benchmark	Before (ops/s)	After (ops/s)	Change	What it isolates
swapCounter	17,114,400 ± 9,886	30,046,585 ± 44,960	+75.6%	Production path: increment() through a held reference
perThread	17,118,439 ± 13,348	30,000,862 ± 2,547	+75.3%	Same path, no cache-line sharing between threads
timerRecord	15,528,856 ± 643,774	26,773,658 ± 45,057	+72.4%	Timer.record()
batched	298,847,938 ± 219,232	301,014,358 ± 37,411	+0.7%	BatchUpdater amortising over 1000 updates
wallTime	40,218,577 ± 5,317	40,221,270 ± 5,707	+0.0%	Control: one wall clock read, for scale
atlasCounter	30,036,643 ± 38,915	30,016,521 ± 27,493	-0.1%	Control: same path with SwapMeter stripped off
lookupCost	42,628,599 ± 82,656	41,239,923 ± 80,697	-3.3%	Cost of re-resolving a meter from the registry

atlasCounter bypasses SwapMeter entirely, so it should not move, and it does not — that is the negative control for this change. It is worth flagging that in the combined #1277 this benchmark gained 11.8%; all of that belonged to the rollCount change now in #1280, and splitting the two is what made each contribution attributable.

perThread tracking swapCounter to within 0.1% confirms the gain is per-call path cost and not contention on a shared counter.

lookupCost regresses 3.3%, and at these error bars that is a real effect rather than noise. It is the expected trade: constructing a wrapper now samples the removal counter as well and carries one more volatile field. It is paid once per held reference per cleanup pass, against a ~1.75x saving on every single update, but it is a regression and not worth hiding.

Retaining the original trigger for the version-only path costs nothing measurable on the hot paths: swapCounter +0.13%, perThread -0.09%, timerRecord -0.29%, all within noise.

[brharrington]

lookup() goes through the public registry method (registry.counter(id)), which allocates a fresh SwapCounter that unwrap() immediately discards.

That garbage used to be produced only for meters that had actually expired. Now one removal invalidates every outstanding wrapper, so an app holding 50k references produces 50k throwaway wrappers plus 50k normalizeId/normalizeTags passes per cleanup pass.

An internal non-wrapping resolve (getOrCreate directly) would avoid both.

[brharrington]

For every wrapper AbstractRegistry hands out, versionSupplier is the constant VERSION (always 0), so currentVersion < versionSupplier.getAsLong() is 0 < 0 forever and the assignment in the resolve branch is a wasted volatile write. Conversely CompositeRegistry never bumps removals, so there currentResolveVersion duplicates currentVersion.

No registry uses both signals, yet SwapMeter now carries three extra fields (LongSupplier ref + volatile long + boolean, ~16 bytes) on the hottest allocation path in the library — registry.counter(id, tags).increment() allocates one wrapper per call. The two version fields look collapsible into one.

[brharrington]

This flag exists only because CompositeRegistry wasn't migrated, and it puts a second path through get() — a hot path the PR is trying to shorten.

CompositeRegistry already owns an AtomicLong that increments on add/remove (lines 95 and 115); that is its removal signal. Passing it as resolveSupplier, with the version sampled before newCounter(id), would put it on the same path as AbstractRegistry and let you delete this boolean, one of the two version fields, and the five 9-line pass-through constructors added to SwapCounter/SwapTimer/SwapGauge/SwapMaxGauge/SwapDistributionSummary.

[brharrington]

removals is bumped once per removed meter, not once per cleanup pass, so the cost is not the "extra lookup per held reference per cleanup pass" the field javadoc describes.

AtlasRegistry.removeExpiredMeters() sweeping 5,000 expired meters bumps this 5,000 times over the duration of the loop. Any held reference updated during that window re-reads a higher value on every update, so each increment() runs get() → lookup() → registry.counter(id) → normalizeId + normalizeTags + CHM lookup + a throwaway wrapper, instead of the wall-clock read it replaced. A counter updating at 1M/s across a 10ms sweep does ~10k full lookups rather than one.

Coalescing to a single bump at the end of a pass would restore the documented model.

[brharrington]

{@link SwapMeter} doesn't resolve here — SwapMeter is in com.netflix.spectator.impl and this class doesn't import it. The build won't catch it because the javadoc task sets Xdoclint:none, so it ships as a dead link. Use {@code SwapMeter}, as this same block does two lines down for VERSION.

Naming nit while here: one concept carries three names across the change — VersionedIterator, removals/removalSupplier, and resolveVersion/currentResolveVersion in SwapMeter (plus "the registry version" in the PR description). Picking one term end to end, and naming the iterator after what it does, would remove the need for a couple of the explanatory comments.

[brharrington]

The registry is never started and removeExpiredMeters() is never called, so removals stays 0 for the whole run and get() always takes the false branch. That means no benchmark here exercises the re-resolve path the change introduces — the headline +75.6% measures only the no-removal steady state, and the per-update re-resolve during a sweep is structurally invisible. lookupCost measures one isolated lookup, which isn't the same thing.

A variant with a background thread calling removeExpiredMeters() at the step interval would show the real steady-state number.

Also: this substantially overlaps BatchUpdates in the same package — same registry construction, and swapCounter/batched/timerRecord re-measure cases it already has. Only atlasCounter, lookupCost, wallTime and perThread are new; folding those four into BatchUpdates would avoid a second file to keep in sync. No @TearDown here either, unlike BatchUpdates.

[brharrington]

This one looks unrelated to the PR — it covers AtlasCounter.add()'s pre-existing isFinite/positive guard before updateLastModTime(), and AtlasCounter isn't modified here. It passes identically on main.

Not against the coverage, but landing it in a SwapMeter perf change means a bisect that stops here attributes an AtlasCounter behavior lock-in to this commit, and reverting the perf change would drop coverage unrelated to it. Probably belongs in its own commit or PR.

[brharrington]

Both new test files are single-threaded, so the concurrency-sensitive part of the change — the check-then-act on currentResolveVersion — had no coverage. removalCounterIsSampledBeforeTheMeterIsResolved uses a getOrCreate hook on one thread, which pins the sampling order in the constructor but not the window in get().

Addressed in 2992479 — I pushed SwapMeterConcurrentResolveTest, which pins the interleaving (one thread parked inside lookup() while another updates the same wrapper) and fails without the ordering fix.

I did try a load-based variant as well and dropped it: a thread that enters get() before the removal can legitimately write to the old meter in any lock-free design, so "no updates lost under load" isn't a sound assertion. The deterministic test pins the actual invariant instead.

[brharrington]

Thanks for digging into this — the steady-state win is real and worth having. I pushed three commits to the branch; the first is a straight bug fix to your design, but the second two replace the removal-counter mechanism, so I want to lay out the reasoning rather than leave you to reverse-engineer it from the diff.

Why the counter had to change

removals is a single global counter, so any removal invalidates every outstanding SwapMeter, including wrappers holding meters that are still registered. It's also bumped once per removed meter rather than once per pass, so a cleanup pass over N meters makes each of H held references re-resolve up to N times — and each re-resolve is a full registry.counter(id) (normalizeId + normalizeTags + map lookup + a throwaway wrapper), rather than the wall-clock read it replaced.

That didn't show up in CounterIncrement because the benchmark never starts the registry and never calls removeExpiredMeters(), so removals stays at 0 and every update takes the cheap path. I added CleanupPassCost to measure the case the change actually affects: one cleanup pass plus one update through every held reference.

Measured against main (µs/op, lower is better; 2 forks, SingleShotTime so the per-invocation setup is outside the timed window):

held	removed	main	removal counter	this branch
100	100	30.5	61.4 (2.02x)	37.8 (1.24x)
100	1000	91.9	115.6 (1.26x)	121.6 (1.32x)
1000	100	119.1	195.8 (1.64x)	131.5 (1.10x)
1000	1000	153.0	209.6 (1.37x)	192.3 (1.26x)

So the counter version was up to 2x slower than main for updates to meters that hadn't expired — the cost scales with removed x held, and it's paid on the update path, so more traffic makes it worse.

What replaced it

Mark the meter instead of counting removals. Meter gains an isRemoved() default that falls back to hasExpired(), so registries that don't opt in keep today's behaviour; AtlasMeter overrides it with a flag set as part of the removal, and the iterator marks each meter it removes. A wrapper then re-resolves only when its own meter is gone — a held reference to a live meter never re-resolves, so removed x held drops out of the cost model entirely. The residual cost is roughly 26ns per removed meter, paid once for that meter rather than on every update.

Steady state is unaffected — if anything slightly better, since there's no shared AtomicLong line to read (CounterIncrement.swapCounter, ops/s):

	ops/s	vs main
main	23,176,139	—
removal counter	37,105,571	+60%
this branch	39,026,152	+68%

For scale, atlasCounter — the same increment with the SwapMeter layer stripped off — is 39,781,758 ops/s, so the wrapper now costs about 0.5ns per update.

Two follow-on simplifications

The publish-ordering race. get() wrote currentResolveVersion before the re-resolve completed, so a second thread on the same wrapper saw the new version, skipped the resolve and kept writing to the meter the sweep had just removed — that update was silently dropped. Verified with a deterministic repro (registry whose newCounter blocks on a latch, two threads on one held reference): main and the fixed code both land 2.0 on the registered meter, the unfixed branch lands 1.0. Fixed in the first commit by publishing the version last; the later commits make it structurally impossible, since there's no version left to publish.

The version field. With removal handled per-meter, the only remaining use of versionSupplier was CompositeRegistry's shape changes — every other registry passed a constant supplier and evaluated 0 < 0 on every update. That's now a Generation token: one volatile boolean shared by the meters created against a given shape, with Generation.PERMANENT for fixed-shape registries. SwapMeter goes from two staleness fields to one, and both signals are now the same shape of check.

That also fixes CompositeRegistry.removeAll(), which installed a new empty shape without bumping the version, so held references never noticed the sub-registries had gone. Two tests added for it.

One number I can't fully explain: the sweep cost still grows somewhat with held even though live meters shouldn't re-resolve (6.2µs -> 15.0µs at removed=100 going from 100 to 1000 held). It's barely outside the error bars and updateHeldReferencesOnly shows no such effect, so I've left it as unexplained rather than chased it.