mirror of
https://github.com/rcourtman/Pulse.git
synced 2026-10-23 20:25:56 +00:00
Stop timing the one-cycle NormalizeRoute root fast path (#2255)
The advisory Benchmarks check keeps failing unrelated pull requests on `NormalizeRoute/root`: alone on #2228, #2242, #2245 and #2250 (and PR 1973 on 2026-09-08), and beside other results on #2230 and #2244. **Why it isn't a signal.** The `"/"` path is a length compare and a constant return. In the last 19 paired PR runs, the 11 on AMD EPYC 7763 measured it at exactly 2.18 ns or 2.50 ns on both sides, each within 0–1%. That is about one CPU cycle. In five of those runs, base and candidate landed on different values: +14% four times and −13% once (#2252), though none touched the normaliser. A stable one-cycle shift passes p<0.05 every time, so more samples can't filter it. A looser threshold would blunt the gate for every other benchmark. **Why batching doesn't fix it.** On an amd64 host, a 64-call batch spread as widely in relative terms as a single call, because every call repeats the same few instructions. **Change.** - Drop the `root` sub-benchmark. - Add `TestNormalizeRoute_RootFastPathDoesNotAllocate`, which pins the property that matters: `"/"` and `""` return before splitting. With the fast path removed, `TestNormalizeRoute` still passes, but the new test fails with one allocation per call. It also passes under `-race`. **Real regressions stay detectable.** I ran a paired comparison through `scripts/run-ci-benchmarks.sh` (10 alternating samples, 100 ms, scoped to the HTTP metrics benchmarks), with `normalizeRoute` made to do its work twice: | Benchmark | Result | |---|---| | `NormalizeRoute/*` (6 sub-benchmarks) | +89% to +107%, p=0.000 | | `NormalizeRoute_Parallel` | +87% | | `FullMiddlewarePath` | +54% | | B/op and allocs/op | +100% | `check-bench-regression.sh` exited 1. The same tree compared with itself exited 0 with no changes. **Not in this change.** `NormalizeSegment` shows the same layout flip. It failed #2252 on its own (+25% to +39%), and in the paired run above it moved −20% to −38% even though its code was untouched. Its uuid flip is about 10 ns, so it needs its own fix rather than a small ns floor.
This commit is contained in:
@@ -13,6 +13,12 @@ import (
|
||||
|
||||
// BenchmarkNormalizeRoute measures route normalization with representative
|
||||
// API paths of varying complexity.
|
||||
//
|
||||
// The "/" fast path is deliberately not timed here. Paired CI runs measured it
|
||||
// at either 2.18ns or 2.50ns, about one CPU cycle apart, and changes that never
|
||||
// touched this code flipped it both ways by 13-30%.
|
||||
// TestNormalizeRoute_RootFastPathDoesNotAllocate pins what matters about the
|
||||
// path instead.
|
||||
func BenchmarkNormalizeRoute(b *testing.B) {
|
||||
paths := []struct {
|
||||
name string
|
||||
@@ -23,7 +29,6 @@ func BenchmarkNormalizeRoute(b *testing.B) {
|
||||
{"with_uuid", "/api/nodes/550e8400-e29b-41d4-a716-446655440000"},
|
||||
{"deep_path_truncated", "/api/v1/orgs/123/resources/456/metrics"},
|
||||
{"with_query_params", "/api/metrics-store/history?resourceType=vm&resourceId=pve1:node1:100&metric=cpu&range=1h"},
|
||||
{"root", "/"},
|
||||
{"metrics_history", "/api/metrics-store/history"},
|
||||
}
|
||||
|
||||
|
||||
@@ -246,6 +246,16 @@ func TestNormalizeRoute(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// The root and empty paths must return before splitting. Without the fast
|
||||
// path, "/" still normalizes to "/" but allocates a split slice on every call.
|
||||
func TestNormalizeRoute_RootFastPathDoesNotAllocate(t *testing.T) {
|
||||
for _, path := range []string{"/", ""} {
|
||||
if allocs := testing.AllocsPerRun(100, func() { _ = normalizeRoute(path) }); allocs != 0 {
|
||||
t.Errorf("normalizeRoute(%q) allocated %v times per call, want 0", path, allocs)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// recordAPIRequest integration tests — verify that the core recording
|
||||
// function correctly populates all three Prometheus metric vectors.
|
||||
|
||||
Reference in New Issue
Block a user