Repository navigation
Conversation
Collaborator
Author
|
Closing for now. The live canary showed modest allocation savings, but no proven p99/p999 improvement. This micro-optimisation does not justify pursuing further at present. The measurements remain in the PR for reference; production was restored to the stock build after the test. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What issue is this change attempting to solve?
Why
Long-range sparse renders spend CPU on point-by-point replica merging and allocate a temporary absence array per decoded metric. Most of that work is avoidable when packed blocks contain only positive-zero values. Reducing this overhead leaves more CPU and allocation headroom for concurrent requests.
Architecture
The existing flow remains
backend response → streamed protobuf decoder → packed metrics → replica merge → dense metrics → expression evaluation.The decoder reuses absence scratch storage within one response after packing has copied its flags into an owned bitmap. It detaches scratch storage when a dense metric retains the array and discards capacity above 1 MiB. No scratch storage is shared between requests. Reads overwrite every byte before bool normalization, so reused arrays do not need an extra clearing pass.
The merger combines absence bitmaps for 64-point blocks only when every compatible replica has no nonzero value bits. All values in such a block are positive zero, so only absence healing can change the result. Dense blocks, nonzero values, negative zero and NaNs use the existing point-wise path. Replica precedence, schema compatibility, mismatch accounting and the dense output contract are preserved. This extends the existing packed implementation without adding a new configuration path.
Before / after
Usage
The current default packed backend render path uses both changes automatically. Existing dense decoding and
CARBONAPI_PACK_BACKEND_POINTS=0remain available. No new runtime switch or deployment override is introduced.How does this change solve the problem? Why is this the best approach?
Both changes use the existing representation and preserve the expression-evaluation boundary. They target allocation churn and repeated zero-point inspection without changing query semantics, response ordering, concurrency, or scheduling. Removing the final dense materialization would require a broader evaluator change and is outside this PR.
How can we be sure this works as expected?
Correctness
Benchmarks
Apple M4 Max, Go 1.26.6,
GOMAXPROCS=2. Frozen test binaries from master6be69a2and this change were run in A/B/B/A order; each table entry is the median of ten runs. These are isolated synthetic fixtures, not a whole-service latency measurement.Dense controls were within approximately 3%, with unchanged allocation counts. Merge output allocations are unchanged because downstream consumers still require dense arrays.
Validation
make test: passed the full Cairo + native gzip + zlib-ng race suite, plus system-zlib and portable backend race variants.go test -race ./pkg/types/...: passed the types and encoding packages.git diff --check: passed.Production validation
Tested commit
6c51800on one production frontend serving normal client traffic, with five unchanged sibling frontends as simultaneous controls. The sequence was 10 minutes stock, 5 minutes candidate warmup, 15 minutes candidate measurement, 5 minutes restored-stock warmup, and 10 minutes restored-stock measurement. The compiler (Go 1.27.0, linux/amd64, GOAMD64=v1), native build tags (cairo,nativegzip,zlibng), configuration and runtime environment matched the incumbent. CPU profiles ran outside the measurement windows.The candidate measurement completed 373,665 client renders and 615,817 upstream renders. There were 4 render 5xx responses (0.0011%), all in one backend TCP dial-timeout burst; unchanged siblings also recorded 5xx errors during that period. There were no unexpected process restarts or health-guard trips. The host was restored to stock and normal routing; the temporary override and canary controls were removed after evidence collection.
The table uses each complete window's scraped counter deltas. CPU and allocation cost include all service work, divided by completed client renders, so request mix still matters. Latency comes from merged nginx histogram buckets, not averaged host quantiles.
Accounting for rendered point volume and simultaneous sibling movement, candidate CPU per point changed -4.4% versus the initial stock window and -9.3% versus restored stock; allocated bytes per point changed -7.3% and -12.0%, respectively. These are descriptive results from one short canary, not a confidence interval or a matched-query replay.
Using only the same-datacenter sibling as the control gives CPU-per-point changes of -0.9% and -8.8%, and allocation-per-point changes of -5.9% and -16.3%, against the two reference windows. Allocation savings are more consistent than the small CPU differences.
Live profiles support the targeted effects:
mergeMetricsCPU share was 9.0% / 4.2% / 9.1% across stock / candidate / restored stock, and the decoder'sboolsallocation share was 6.6% / 1.6% / 6.4%. Final dense point materialization remained about 18% of candidate allocations. The profile shares came from different traffic intervals and should not be read as an exact whole-service speedup.The profiles support lower merge cost and less allocation. A reliable whole-service CPU or p99/p999 improvement remains unproven: backend timeout bursts and changing query mix affected the fleet during the candidate window. RSS also fluctuated substantially and process restarts confound footprint comparisons; no sustained RSS reduction is claimed.
Rollout and limits
The production canary has been rolled back. A longer matched evaluation is needed before claiming a tail-latency win or extrapolating the resource savings to the fleet. Large queries still fetch and materialize their input series; this PR reduces processing overhead without imposing a query cost limit. Reverting the change restores the previous decoder and merge loops.
Fixes #585