usagetracker: skip groups with nothing to remove during Cleanup - #16473
usagetracker: skip groups with nothing to remove during Cleanup#16473colega wants to merge 2 commits into
Conversation
The existing BenchmarkMapCleanup is pinned at 1M entries and allocates one map per iteration, so it cannot be pushed to the shard sizes that large partitions reach, and it only ever measures one expired fraction. These benchmarks sweep both, so we can tell the cost of walking the map apart from the cost of removing entries from it. BenchmarkMapRehashRealistic sizes the rehash the way nextSize would, unlike BenchmarkMapRehash which passes the entry count as a group count and so rehashes into a map four times larger than the real one. BenchmarkTrackerStoreCleanupLockStall measures the number that matters operationally: how long trackSeries blocks while cleanup holds a shard mutex.
Cleanup walked every slot of every group with three unpredictable branches per
slot, and that walk, not the removals, dominated its cost: at 16M entries per
shard it spent 3.5ns per entry even when nothing at all had expired. Since
trackerStore.cleanup holds the shard mutex for the whole call, that lands
directly on trackSeries tail latency, and it grows with the shard. At 1e9 series
per partition a single shard takes ~274ms.
The eviction test becomes a single unsigned range check.
watermark.GreaterOrEqualThan(value) is (watermark-value) mod 120 < 60, so the
values that expire are the 60 minutes ending at the watermark. Under the
xor-encoding that is uint8(x-lo) <= length, with lo = 255-watermark and length
either 59 or 195 depending on whether those 60 minutes wrap the clock face. A
second compare against 1 drops the empty and tombstone markers. Being a
whole-group test, it lets scanExpired skip straight past groups holding nothing
to remove.
There are three implementations: SSE2 on amd64, NEON on arm64, and a portable
SWAR one that is also selectable with -tags nosimd. The unsigned comparisons on
arm64 are written with UMIN and UMAX because the assembler only learned CMHS and
CMHI in Go 1.27.
Group size stays at 8 rather than widening to 16 the way dolthub/swiss does for
its SIMD path. Widening would only help match and matchEmpty, and profiling puts
match at ~6% of Put, which is dominated by cache misses that vectors do not
remove. An 8 byte group already fits one general purpose register.
cleanupGroup now visits slots from the highest index down. Removing a slot fills
the hole with the last occupied slot of the group, and going downwards
guarantees that slot holds a live entry, because every expired slot above it has
already been dealt with.
Measured at 16M entries per shard, ns per entry, arm64:
expired before portable NEON
0% 3.500 0.571 0.088
1% 3.529 0.740 0.293
5% 3.873 1.397 0.979
25% 4.617 3.915 3.328
100% 6.314 6.258 5.888
The gain narrows as more entries expire because what remains is the removal
itself, which touches three separate cache lines on a map far larger than L3 and
is memory-bound. Steady state sits at the low end of that table: cleanup runs
every minute against a 20 minute idle timeout, so it only removes the series
that went idle in that minute. The trackSeries stall behind a shard mutex, at 4M
entries per shard with 5% expired, is 11.6ms.
loadSnapshot now drops timestamps at or above clock.Cycle. They can only come
from a corrupt file, and the range check is exact only within [0, 120), so
without this a corrupt byte could become an entry that never expires.
![semgrep-code-grafana[bot]](https://avatars.githubusercontent.com/u/7195757?s=60&v=4){kind=small}
| // scanExpired returns the index of the first group in p[:n] that holds at least one | ||
| // occupied, expired slot, or n when there is none. | ||
| func scanExpired(p *data, n int, lo, length uint8) int { | ||
| for i, d := range unsafe.Slice(p, n) { |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
Using the unsafe package in Go gives you low-level memory management and many of the strengths of the C language, but also steps around the type safety of Go and can lead to buffer overflows and possible arbitrary code execution by an attacker. Only use this package if you absolutely know what you're doing.
Why this might be safe to ignore:
This is an intentional use of unsafe.Slice to iterate over an internally managed array in a generic, build-tagged implementation. The broad low-confidence rule does not show attacker-controlled input or an unsafe bound, so this finding is not reasonably exploitable in this context.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by use-of-unsafe-block.
We're currently testing semgrep's diff-aware PR comment feature on a subset of our repos-- if you run into issues or find this spammy, please reach out to @danny.cooper in slack and give feedback.
For backwards compatability with gosec, its best to use polyglot suppression comments of the following format for false positives:
// #nosec <gosec rule ID> nosemgrep: <semgrep rule ID>
You can view more details about this finding in the Semgrep AppSec Platform.
| } | ||
|
|
||
| func castUint64Data(d *data) uint64 { | ||
| return *(*uint64)(unsafe.Pointer(d)) |
There was a problem hiding this comment.
Semgrep identified a blocking 🔴 issue in your code:
Using the unsafe package in Go gives you low-level memory management and many of the strengths of the C language, but also steps around the type safety of Go and can lead to buffer overflows and possible arbitrary code execution by an attacker. Only use this package if you absolutely know what you're doing.
Why this might be safe to ignore:
This is an intentional performance-oriented cast from a fixed-size data buffer to uint64, used by local byte-lane operations rather than attacker-controlled pointer manipulation. The low-confidence rule broadly flags unsafe usage, but this code does not present a practical memory-safety or code-execution issue in context.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by use-of-unsafe-block.
We're currently testing semgrep's diff-aware PR comment feature on a subset of our repos-- if you run into issues or find this spammy, please reach out to @danny.cooper in slack and give feedback.
For backwards compatability with gosec, its best to use polyglot suppression comments of the following format for false positives:
// #nosec <gosec rule ID> nosemgrep: <semgrep rule ID>
You can view more details about this finding in the Semgrep AppSec Platform.
1 participant
What this PR does
The usage-tracker removes idle series once a minute, and
trackerStore.cleanupholds a shard mutex for the whole of a shard'sMap.Cleanup. That call walked every slot of every group with three unpredictable branches per slot, and the walk, not the removals, was what it spent its time on: 3.5ns per entry at 16M entries per shard even when nothing had expired at all. It scales with the shard, so at 1e9 series per partition a single shard takes ~274ms, andtrackSeriessits behind it.The clock comparison that decides whether an entry expires turns out to be expressible as one unsigned byte range check, which makes it a whole-group test instead of a per-slot one. Cleanup uses it to jump straight past groups that hold nothing to remove. There are three implementations: SSE2 on amd64, NEON on arm64, and a portable SWAR one, which is also what
-tags nosimdselects.Measured at 16M entries per shard, ns per entry, arm64:
The gain narrows as more entries expire, because what is left is the removal itself: it touches three separate cache lines on a map much larger than L3, so it is memory-bound and vectors do not help it. Steady state is the low end of that table, since cleanup runs every minute against a 20 minute idle timeout and only removes what went idle in that minute. End to end, the
trackSeriesstall behind a shard mutex at 4M entries per shard with 5% expired is 11.6ms, and the whole-shard time at 1e9 series per partition drops from 274ms to 209ms even at an unfavourable 25% expired.No behaviour change for callers, other than
loadSnapshotnow dropping timestamps outside the clock face. Those can only come from a corrupt file, and the range check is exact only within it, so without that a corrupt byte could become an entry that never expires.Group size deliberately stays at 8 rather than widening to 16 the way dolthub/swiss does on its SIMD path. Widening would only help
matchandmatchEmpty, and profiling putsmatchat ~6% ofPut, which is dominated by cache misses. An 8 byte group already fits in one general purpose register, so SWAR is the right tool there.Worth a careful look at
cleanupGroup, which now visits slots from the highest index down so that the slot moved into a hole is always a live entry.TestCleanupMatchesLegacykeeps the old loop in the test file and requires both to agree on contents and counters over random maps, andTestCleanupPreservesLookupscovers the probe chains that the reordering could break. The range check itself is verified exhaustively against the clock comparison, and the assembly against the portable implementation.Tested on arm64/NEON, arm64/nosimd, amd64/SSE2 and amd64/nosimd, on Go 1.26.7 as pinned by the build image.
Which issue(s) this PR fixes or relates to
n/a
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]. If changelog entry is not needed, please add thechangelog-not-neededlabel to the PR.about-versioning.mdupdated with experimental features.