Repository navigation
Fix: zero-copy model scan and live contention profiles - #1609
Conversation
Two gaps the pprof pass on a live proxy caught, both still open after the memory fixes in rossoctl#1604 and rossoctl#1606: requestModel copied every routed request body. gjson.ParseBytes is Parse(string(body)), so reading the top-level keys for "model" made one full copy of the body per request — ~1GB/hour of churn on a proxy that was mostly idle, third behind tool-prune's copies. rossoctl#1604 stopped the copy being pinned; this stops it being made. The scan now runs over unsafe.String aliasing the body: safe because gjson never writes what it parses, the body is not written concurrently on the request path, and the only string that escapes the scan is the name, which rossoctl#1604 already clones. TestRequestModelDoesNotCopyTheBody pins the property: one call on a 4MiB body allocates under 64KiB, where the old code allocated the 4MiB. /debug/pprof/block and /mutex were permanently empty. The handlers from rossoctl#1603 exist, but the runtime records contention only when asked, and nothing ever called SetBlockProfileRate or SetMutexProfileFraction — the two endpoints answered 200 with zero samples, on the box and on every deployment with loopback-only listeners. WithPprof now turns the recorders on with the handlers, under the same loopback gate. Both cost nothing unless the event they measure happens. TestStatServerPprofEnablesContentionProfiles reads the rates back process-globally: both were left at zero before. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 14 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Summary
Both fixes are correct and pinned by tests that fail without them. The unsafe.String alias in requestModel holds up: gjson.ValidBytes already works on the bytes without copying, ForEach only reads, keys with JSON escapes are unescaped into fresh allocations, and the one string that escapes is still strings.Cloned. Turning on the block and mutex recorders behind the existing WithPprof (loopback-only) gate makes /debug/pprof/block and /mutex usable. I left three non-blocking notes: the sampling rate, a comment that is now stale, and a test helper that changes global state when it only needs to read it.
Author: huang195 (MEMBER — maintainer)
Areas reviewed: Go (core/pipeline, core/observe), tests, security (unsafe aliasing, pprof gating)
Agent/IDE config (.claude/.vscode): none
Commits: 1 commit, all signed-off: yes
CI status: passing (all required checks green; CodeRabbit/Spellcheck skipped)
Local verification: go test -run 'TestRequestModel|TestStatServerPprof' ./pipeline/ ./observe/ and go vet pass at 7e94002 (Go 1.27.3).
Nit (PR body): the description has no ## Summary heading. The content is thorough, this is only about the template.
| // when the event they measure happens — a blocked goroutine or a contended | ||
| // mutex — so they ride the same gate as the handlers rather than running | ||
| // on deployments where pprof is off. | ||
| runtime.SetBlockProfileRate(1) |
There was a problem hiding this comment.
suggestion: A rate of 1 for both recorders means every blocking event and every contended unlock gets recorded, and each one walks the stack. "Costs only when the event happens" is true, but in a proxy those events happen all the time: channel and select waits in the HTTP/2 transport and server, sync.Cond in the body pipes, WaitGroups. The runtime docs suggest sampled values for long-running processes. For example:
runtime.SetBlockProfileRate(int(10 * time.Microsecond)) // one sample per ~10µs blocked
runtime.SetMutexProfileFraction(100) // ~1% of contention eventsThe profiles still catch any real contention problem, at a small fraction of the per-event cost. pprof scales sampled counts back up, so the output stays comparable. This is only enabled on loopback-only (laptop) installs, so it is not blocking. Also, as a process-global side effect of building a server, it is worth one line in the WithPprof doc comment: "also enables block/mutex profiling for the whole process".
| // never writes the string it parses, the body is not written concurrently on | ||
| // the request path, and nothing that outlives the scan points into it: the | ||
| // only string that escapes, below, is cloned. | ||
| gjson.Parse(unsafe.String(unsafe.SliceData(body), len(body))).ForEach(func(key, value gjson.Result) bool { |
There was a problem hiding this comment.
nit: The comment above the strings.Clone further down (around L145: "Cloned off gjson's parse copy of the body: Result.String() is a substring of that copy …") is now out of date. There is no parse copy anymore, and model.String() is a substring of an alias of c.Body itself. That also makes the clone more important, not just about pinning. Without it, the returned string would share bytes with a mutable []byte, so a later write to the body (for example an in-place sjson set, or a plugin reusing the buffer) would change a Go string, which is meant to be immutable. Consider rewording it to something like: "Cloned off the body: Result.String() aliases c.Body (see the zero-copy scan above), so an uncloned name would both pin the body and change if the body were written."
| if !strings.Contains(w.Body.String(), "TestStatServerPprofEnablesContentionProfiles") { | ||
| t.Error("a 100ms blocked receive is absent from /debug/pprof/block: the recorder is off") | ||
| } | ||
| if prev := runtime.SetMutexProfileFraction(1); prev == 0 { |
There was a problem hiding this comment.
nit: runtime.SetMutexProfileFraction(1) reads the previous value but also sets it. If WithPprof regressed, the test would still flag it, but it would also quietly switch mutex profiling on for the rest of the package's tests. A negative argument only reads the value and leaves it unchanged (per the runtime docs: "If rate < 0, it returns the current rate without changing it"):
if runtime.SetMutexProfileFraction(-1) == 0 {The comment above ("Mutex has a getter") then matches what the code does.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
Two gaps left open after the memory fixes in #1604 and #1606, both found by profiling the live proxy through #1603's
/debug/pprofand both re-verified againstmainafter those merges.1.
requestModelstill copies every routed request bodygjson.ParseBytesisParse(string(body)), so reading the top-level keys for"model"makes one full copy of the body per request — measured at ~1GB/hour of allocation churn on a proxy that was mostly idle, third behind tool-prune's copies. #1604 stopped that copy being pinned; this stops it being made.The scan now runs over an
unsafe.Stringalias of the body. Safe because:TestRequestModelDoesNotCopyTheBodypins the property: one call on a 4MiB body allocates under 64KiB. Verified red before the fix (4104KiB) and green after.2.
/debug/pprof/blockand/mutexwere permanently emptyThe handlers from #1603 exist, but the runtime records contention only when asked, and nothing ever called
SetBlockProfileRateorSetMutexProfileFraction— both endpoints answered 200 with zero samples, so a contention problem on the box could not have been seen.WithPprofnow turns the recorders on with the handlers, under the same loopback-only gate; both cost nothing unless the event they measure happens.TestStatServerPprofEnablesContentionProfilesproves the block recorder by recording one event (a 100ms blocked receive) and finding this test's own frame in the text dump — the rate has no getter to read back. Verified red against the un-fixed server and green after.Not addressed here
The usage-pane poll path (
handleUsage → ledgerSnapshotFor) still allocates ~2GB/hour in per-query filtering and marshaling even with #1593's day cache — smaller than the two above, and a different shape of fix.Assisted-By: Claude (Anthropic AI) noreply@anthropic.com