Skip to content

perf(jsonrpc): gzip responses with klauspost at BestSpeed - #3957

Merged
rodrodros merged 1 commit into
mainfrom
dat/gzip
Aug 24, 2026
Merged

perf(jsonrpc): gzip responses with klauspost at BestSpeed#3957
rodrodros merged 1 commit into
mainfrom
dat/gzip

Conversation

@infrmtcs-agent

Copy link
Copy Markdown
Contributor

Summary

  • Switch the RPC response gzip writer pool from stdlib compress/gzip (default level) to klauspost/compress at BestSpeed — ~10x faster compression for a modestly larger body (~2–13% depending on payload)
  • Promote klauspost/compress from indirect to direct dependency
  • Bench: add opt-in gzip to the k6 RPC benchmark via -e GZIP=1 (sets Accept-Encoding: gzip; k6 decompresses transparently)
  • Bench: allow capping the pre-allocated VU pool via -e VUS= instead of always allocating one VU per req/s of peak rate

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Claude finished @infrmtcs-agent[bot]'s task in 6m 13s —— View job


Review: perf(jsonrpc): gzip responses with klauspost at BestSpeed

  • Gather context (diff, lint config, existing gzip tests)
  • Review jsonrpc/http.go gzip pool change
  • Review go.mod dependency promotion
  • Review bench script changes
  • Post review

No blockers — the swap is correct and the risky parts of pooling a third-party gzip writer check out. Four nits posted inline, plus one below that GitHub wouldn't anchor (the line isn't in the diff).

Severity summary

Blocker — none.

Important — none.

Nit

  1. bench/rpc/throughput.js:5maxVUs defaults to preAllocatedVUs for ramping-arrival-rate, so an over-tight -e VUS= silently drops iterations and under-delivers the target rate, invalidating a gzip-vs-plain comparison. Suggest a dropped_iterations: ['count==0'] threshold + a positivity check on VUS.
  2. bench/rpc/run.js:42-47GZIP === '1' fails open: -e GZIP=true silently runs the uncompressed benchmark, which is the wrong-direction failure here. The rest of the file throws on bad env input.
  3. jsonrpc/http.go:20-31 — comment says a flat ~13% while the PR body says ~2–13%; and no in-tree BenchmarkGzipResponse backs the 10x claim, so a future level change won't be caught.
  4. jsonrpc/http.go:143-150 (couldn't anchor inline — outside the diff hunk) — the writer is returned to the pool still pointing at the http.ResponseWriter; neither gzip.Writer.Close nor the flate writer clears the destination. A pool entry can idle for two GC cycles, keeping a dead ResponseWriter (and its bufio buffers, *http.conn, *http.Request) reachable. One line fixes it:
    closeErr := gw.Close()
    gw.Reset(io.Discard) // don't pin the ResponseWriter in the pool
    gzipWriterPool.Put(gw)
    Pre-existing, but this PR is the one rewriting the pool. Fix this →

What I verified against the klauspost source (v1.19.1)

The two failure modes that actually matter when pooling a swapped-out compressor both hold:

  • No sticky-error poisoning. If a client disconnects mid-write, the writer's error must not survive back into the pool. gzip.Writer.Resetinit does a full *z = Writer{...}, clearing err/closed/wroteHeader, and flate's compressor.reset sets d.err = nil and huffmanBitWriter.reset sets w.err = nil. A poisoned writer can't leak to the next request.
  • No nil deref on an empty response. gzip.Writer builds its compressor lazily on first write, and Close on a never-written writer calls Write(nil) first, which writes the header and constructs the compressor before z.compressor.Close(). Safe for a zero-length resp.

Also checked: gzip.BestSpeed is the same constant (flate.BestSpeed = 1) with identical semantics; output is standard DEFLATE and jsonrpc/http_test.go already decompresses with stdlib compress/gzip, so wire compatibility is covered by existing tests. TestContentLength's gzip assertion still holds — 2500 repeated bytes compress to well under net/http's 2 KB sniff buffer even at level 1. go.mod promotion is correct, alphabetically placed, and go.sum already carried the h1: hash, so no go mod tidy churn. No depguard rule restricts the import.

One upside worth adding to the PR description

This should cut pool memory, not just CPU. stdlib's level-6 compressor carries hashHead [1<<17]uint32 (512 KiB) + hashPrev [1<<15]uint32 (128 KiB) + a 64 KiB window ≈ 700 KiB per pooled writer; klauspost's fastEncL1 carries a [1<<15]tableEntry table (128 KiB) + a 320 KiB history buffer ≈ 460 KiB. With rpc-max-concurrent-requests defaulting to 256000 the pool is bounded only by peak concurrency, so that per-writer delta scales. Worth capturing RSS alongside throughput in the bench.

Follow-up (not this PR)

Four files still use stdlib compress/gziputils/compression.go, rpc/v10/transaction.go, clients/gateway/gateway.go, clients/gateway/test_gateway.go. klauspost's gzip.Reader is also meaningfully faster, so the class/program decompression path on declare handling is a natural next target.


Ran go build ./jsonrpc/ — passes. I could not run go test; the command required approval that wasn't granted in this environment, so the gzip tests were reasoned about rather than executed.
· branch dat/gzip

@rodrodros

Copy link
Copy Markdown
Contributor

@infrmtcs could you point this PR to be after: #3956

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.67%. Comparing base (5f2d849) to head (ce6949f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3957      +/-   ##
==========================================
+ Coverage   74.63%   74.67%   +0.03%     
==========================================
  Files         462      462              
  Lines       41029    41029              
==========================================
+ Hits        30622    30638      +16     
+ Misses       8304     8289      -15     
+ Partials     2103     2102       -1     
Flag Coverage Δ
jsonv2 77.36% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread bench/rpc/throughput.js
Comment thread bench/rpc/run.js
Comment thread jsonrpc/http.go Outdated
@infrmtcs
infrmtcs changed the base branch from main to feature/compression-package August 21, 2026 08:06
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @infrmtcs's task in 6m 14s —— View job


Review: perf(jsonrpc): gzip responses with klauspost at BestSpeed

  • Gather context (diff vs feature/compression-package)
  • Review utils/compression changes
  • Review jsonrpc/http.go + rpc/v10/transaction.go
  • Review bench + test changes
  • Post review

Now that the PR is rebased onto feature/compression-package the diff is much smaller and cleaner — the pooling machinery lives in utils/compression and this PR just repoints it at klauspost. The pool logic itself is sound. One finding is worth resolving before merge; the rest are nits.

Severity summary

Blocker — none.

Important

  1. utils/compression/compression.go:11the gzip header MTIME changes for every stream Juno produces, not just RPC responses. klauspost writes uint32(z.ModTime.Unix()) unconditionally where stdlib guards it with if z.ModTime.After(time.Unix(0,0)), so with Reset leaving ModTime zero, the header goes from 00 00 00 00 to 00 09 6e 88 = 2288912640 = uint32(time.Time{}.Unix()) — a modification time of ~2042-07. This reaches the sequencer gateway via the base64 sierra_program on starknet_addDeclareTransaction (rpc/v8/transaction.go:714, rpc/v9/transaction.go:792, rpc/v10/transaction.go:238) and the persisted Cairo 0 class.Program (adapters/sn2core/sn2core.go:434). It's still valid gzip and class hashes are safe (core/class_hash.go:12 hashes the decompressed program), but it's what forced all five fixture rewrites in this PR, and writer.gz.ModTime = time.Unix(0, 0) after Reset restores byte-identical output — which would revert every one of those fixture changes back into real regression guards.

Nit
2. utils/compression/compression.go:23-28 — klauspost adds StatelessCompression = -3, below minLevel, so passing it panics with index out of range [-1]. // All gzip compression levels is no longer accurate, and GzipWriterLevel should bounds-check with a message naming the level.
3. utils/compression/compression_test.go:14 — the tests now use klauspost's level constants directly, undercutting the const block this PR added for exactly that reason. Also, nothing decompresses Juno's output with stdlib compress/gzip any more.
4. jsonrpc/http.go:127 — flat ~13% vs the PR's ~2–13%; and BenchmarkGzipWriter's doc comment claims to cover the JSON-RPC handler path, which this PR just made false (bench is DefaultCompression, handler is now BestSpeed).
5. Pre-existing, adjacent (jsonrpc/http.go:129-136) — the deferred http.Error fires after the gzip body has been written, so it emits plaintext into a response already carrying Content-Encoding: gzip and calls WriteHeader a second time. Not introduced here, but it's the block this PR edits.
6. Still open from the previous round — the two bench nits weren't addressed: VUS has no positivity check and no dropped_iterations: ['count==0'] threshold (throughput.js:5), and GZIP === '1' fails open so -e GZIP=true silently benchmarks the uncompressed path (run.js:42-47). Note -e VUS=0 also slips through, since '0' is truthy in JS.

What checked out

  • All five rewritten fixtures decode correctly. I decoded each by hand: v8/v9 is a stored block 7b 7d ({}) with CRC 0xa3a6bf43/ISIZE 2, v10 is 5b 5d 0a ([]\n) with CRC 0x7068d244/ISIZE 3, and TestGzip64 is a single 0x00 with CRC 0xd202ef8d/ISIZE 1 — every CRC and ISIZE matches the value it replaced. Content is identical; only the header timestamp and the block encoding moved. XFL is consistent too (0x04 on v10, which already used BestSpeed on the base branch; 0x00 elsewhere).
  • The level→pool mapping is unchanged. HuffmanOnly/BestCompression are the same constants in both libraries (-2/9), so levelCount stays 12 and no pool index shifts.
  • go.sum needed no changeklauspost/compress v1.19.1 already carried its h1: hash as an indirect dep, so the go.mod promotion is clean and alphabetically placed. No depguard rule restricts the import.
  • Pooling a swapped-out compressor is safe. No sticky-error poisoning (klauspost's Resetinit does a full struct reset clearing err/closed/wroteHeader; flate's compressor.reset clears d.err), and no nil deref on an empty response (Close on a never-written writer calls Write(nil) first, which builds the compressor). TestGzipWriterAfterFailedDestination covers the first case directly.
  • The proxy indirection still holdsRelease nils proxy.dst without a second flate reset, and TestGzipWriterReleaseDropsDestination proves the destination is collectable. This is the right answer to the dangling-ResponseWriter concern from the previous round; nothing to add there.

Worth adding to the PR description

This should also cut pool memory, which is a real win the description doesn't claim. stdlib's level-6 compressor carries hashHead [1<<17]uint32 (512 KiB) + hashPrev [1<<15]uint32 (128 KiB) + a 64 KiB window ≈ 700 KiB per pooled writer; klauspost's fastEncL1 carries a [1<<15]tableEntry table (128 KiB) + a 320 KiB history ≈ 460 KiB. With rpc-max-concurrent-requests defaulting to 256000 the pool is bounded only by peak concurrency, so that delta scales — worth capturing RSS alongside throughput in the bench.

Follow-up (not this PR)

Gzip64Decode (utils/compression/compression.go:150) does an unbounded io.ReadAll on attacker-supplied data — rpc/v8/class.go:70 and rpc/v9/class.go:76 feed it a base64 program from the declare path, and the 10 MB MaxRequestBodySize cap doesn't bound the inflated size (DEFLATE tops out around 1032:1). Pre-existing on main and untouched here, but klauspost's faster reader makes the ratio cheaper to reach. An io.LimitReader around the gzip reader would close it.


Static review only. go test and go build both required approval that wasn't granted in this environment, so nothing here was executed — the fixture analysis above is byte-level decoding of the base64 in the diff, not a test run.
· branch dat/gzip

Comment thread utils/compression/compression.go
Comment thread utils/compression/compression.go
Comment thread utils/compression/compression_test.go
Comment thread jsonrpc/http.go
@infrmtcs
infrmtcs deployed to Development August 21, 2026 08:21 — with GitHub Actions Active
Base automatically changed from feature/compression-package to main August 21, 2026 10:44
@github-actions

github-actions Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Claude finished @rodrodros's task in 7m 32s —— View job


Review: perf(jsonrpc): gzip responses with klauspost at BestSpeed

  • Gather context (diff vs origin/main — PR is now rebased, base includes feat: implement shared compression writer pools #3956)
  • Review utils/compression swap against the klauspost v1.19.1 source
  • Review jsonrpc/http.go + rpc/v10/transaction.go
  • Review fixture + bench changes
  • Post review

The rebase onto main landed cleanly and the diff is now genuinely small — the pooling machinery is upstream and this PR just repoints it at klauspost. No blockers. One item from the previous round is still open and worth resolving; the rest are nits.

Severity summary

Blocker — none.

Important

  1. rpc/v8/transaction_test.go:1123four RPC fixtures were rewritten with no explanation. compression_test.go:22 got a comment explaining the MTIME change; the sierra_program fixtures in rpc/v8 (×2), rpc/v9 (×2) and rpc/v10 (×1) did not. The cause is klauspost writing the gzip MTIME header unconditionally (gzip.go:192) where stdlib guards it with ModTime.After(time.Unix(0,0)), so every stream Juno emits now advertises 2288912640 = uint32(time.Time{}.Unix())2042-07 — including the base64 sierra_program POSTed to the sequencer gateway and the persisted Cairo 0 class.Program. Harmless on the wire (class hashes are computed over the decompressed program, core/class_hash.go:12), but either fix it in one line (writer.gz.ModTime = time.Unix(0, 0) after Reset, which reverts all four fixtures back into real regression guards) or state the decision in the PR description and comment the fixtures.

Nit
2. utils/compression/compression.go:24,128// All gzip compression levels is no longer true: klauspost adds StatelessCompression = -3 below HuffmanOnly, and NewWriterLevel accepts it (gzip.go:69), but gzipWriterPools only covers [-2, 9]. GzipWriterLevel(dst, gzip.StatelessCompression) indexes pools[-1] and panics with an opaque index out of range [-1]. Latent (nothing calls it, and the re-export block deliberately omits it), but newWriter already panics with a good message and the pool lookup deserves the same. Note TestGzipWriterLevelRejectsOutOfRange:191 now asserts that HuffmanOnly-1 — a level klauspost considers valid — panics; worth a comment so it doesn't read as a bug later. Fix this →
3. utils/compression/compression_test.go:154-158, 184-185, 190-191 — the klauspost import is genuinely needed for gzipAtLevel (the oracle must come from the same library), but the level constants don't need it. Using gzip.BestSpeed here undercuts the const block this PR just added for exactly that reason — the package's own tests become the first thing to violate its stated contract. Should be compression.BestSpeed, etc.
4. jsonrpc/http.go:127 — comment says a flat ~13% larger bodies, PR description says ~2–13% depending on payload; quote the range. More usefully, neither the 10x nor the 13% is guarded by anything in-tree, and BenchmarkGzipWriter's doc comment (compression_bench_test.go:62-63) claims to cover "the JSON-RPC HTTP handler" — which this PR just made false, since the bench runs GzipWriter (DefaultCompression) while the handler is now BestSpeed. Parameterising that bench over levels restores the claim and makes a future level change visible. Fix this →
5. Still open from the previous round — neither bench nit was addressed. throughput.js:5 still has no positivity check on VUS (and -e VUS=0 slips through, since '0' is truthy) and no dropped_iterations: ['count==0'] threshold, so an under-provisioned VU pool silently under-delivers the target rate and invalidates a gzip-vs-plain comparison (thread). run.js:43 still fails open — -e GZIP=true silently benchmarks the uncompressed path (thread).

What I verified against klauspost v1.19.1

The question that actually matters when you swap the compressor behind a pool shared across HTTP clients is whether one response's data can leak into another's stream. It can't:

  • No history bleed between requests. At BestSpeed klauspost uses d.fast, so compressor.reset (flate/deflate.go:835-839) takes the fast path: d.fast.Reset() truncates e.hist to zero and advances e.cur by maxMatchOffset + len(hist) (flate/fast_encoder.go:179-189), putting every stale table entry out of match range. No back-reference can reach a previous response's bytes.
  • No sticky-error poisoning. Writer.init does a full *z = Writer{...} (gzip.go:106), clearing err/closed/wroteHeader; compressor.reset sets d.err = nil and d.w.reset clears the bit-writer error. A writer poisoned by a mid-write client disconnect can't come back out of the pool broken. TestGzipWriterAfterFailedDestination covers this directly.
  • No nil deref on an empty response. Close on a never-written writer calls Write(nil) first (gzip.go:272-277), which writes the header and constructs the compressor before z.compressor.Close().
  • The level→pool mapping is unchanged. HuffmanOnly/BestCompression are the same constants in both libraries (-2/9), so levelCount stays 12 and DefaultCompression still lands on index 1. No pool index shifts.
  • Wire interop is still covered. jsonrpc/http_test.go:5 decompresses with stdlib compress/gzip, so the RPC path is tested cross-library even though Gzip64Decode is now klauspost on both sides.
  • go.mod promotion is clean — alphabetically placed, go.sum already carried the h1: hash as an indirect dep, and no depguard rule in .golangci.yaml restricts the import (the only deny is sirupsen/logrus).
  • The proxy indirection still holdsRelease nils proxy.dst without a second flate reset, which is the right answer to the dangling-ResponseWriter concern raised in round 1. Nothing to add there.

Worth adding to the PR description

This should also cut pool memory, which the description doesn't claim. stdlib's level-6 compressor carries hashHead [1<<17]uint32 (512 KiB) + hashPrev [1<<15]uint32 (128 KiB) + a 64 KiB window ≈ 700 KiB per pooled writer; klauspost's fastEncL1 carries a [1<<15]tableEntry table (128 KiB) + a 320 KiB history ≈ 460 KiB. With rpc-max-concurrent-requests defaulting to 256000 the pool is bounded only by peak concurrency, so that delta scales — worth capturing RSS alongside throughput in the bench.

Follow-up (not this PR)

Gzip64Decode (utils/compression/compression.go:159) does an unbounded io.ReadAll on attacker-supplied data — rpc/v8/class.go and rpc/v9/class.go feed it a base64 program from the declare path, and the 10 MB MaxRequestBodySize cap doesn't bound the inflated size (DEFLATE tops out around 1032:1). Pre-existing on main and untouched here, but klauspost's faster inflate makes the ratio cheaper to reach. An io.LimitReader around the gzip reader would close it.


go build ./... passes for every package except cmd/juno, which fails to link only because the Rust artifacts (libjuno_starknet_rs, libjuno_starknet_compiler_rs) and libbz2 aren't built in this environment — unrelated to this PR. go test required an approval that wasn't granted, so the 10x / 2–13% claims are unverified here and the fixture analysis above is byte-level decoding of the base64 in the diff cross-checked against the klauspost source, not a test run.
· branch dat/gzip

Comment thread rpc/v8/transaction_test.go
@infrmtcs
infrmtcs requested a review from rodrodros August 24, 2026 08:36
@rodrodros
rodrodros merged commit 518345c into main Aug 24, 2026
19 of 20 checks passed
@rodrodros
rodrodros deleted the dat/gzip branch August 24, 2026 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants