Repository navigation
kafka: pin encode allocation counts - #1425
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Review of fa2734f2f87a87020463653c449544d39ea00964, round 1.
Must-fix: 0. Notes: 3. Outside this change: 1.
Notes
crates/spate-kafka/benches/encode_alloc.rs:28(intent): an encoder that reserves an extra 1 MiB on its first record leaves every figure in the snapshot unchanged, so the body's "any change to what an encode allocates fails CI" does not cover an allocation made once per encoder. Each shard clones its own encoder with fresh scratch (crates/spate-kafka/src/sink/encoder.rs:195-205), so a running sink pays such an allocation once per shard. Trace:crates/spate-kafka/benches/encode_alloc.rs:28→test-support/src/alloc.rs:179(Report::case) →test-support/src/alloc.rs:138(uncounted pass) →crates/spate-kafka/benches/support/encode_rig.rs:47(encode_runreuses oneKafkaEncoder) →crates/spate-kafka/src/sink/encoder.rs:74(KafkaMessage::set_payload). Proof: with the mutant applied,cargo bench -p spate-kafka --bench encode_alloc --lockedprints rows identical tocrates/spate-kafka/benches/alloc/encode_alloc.default.aarch64.tsv(the comparison exits 1, "snapshot unchanged"). The warm pass is recorded as a choice attest-support/src/alloc.rs:130-131andcrates/spate-kafka/benches/encode_alloc.rs:8-9; the note is against the body's wording.crates/spate-kafka/benches/support/encode_rig.rs:70(docs): the moved rustdoc onrecordssays the drop happens outside the collected region because "the rig is returned rather than dropped". That reason holds for the counted tier only. Inencode_alloc.rseach rig is a local that the case closure borrows (crates/spate-kafka/benches/encode_alloc.rs:28-29), and it drops when themain_withclosure ends. The conclusion holds for both includers. Proof: contradiction betweencrates/spate-kafka/benches/support/encode_rig.rs:70("the rig is returned rather than dropped") andcrates/spate-kafka/benches/encode_alloc.rs:28(let mut r = bytes_keyless();, borrowed by a closure that returns()).crates/spate-kafka/benches/support/messages.rs:25(callers): thedead_coderationale says "The bench and the fixture test each compile this module separately", and this change adds a third includer atcrates/spate-kafka/benches/encode_alloc.rs:15. The module header atcrates/spate-kafka/benches/support/messages.rs:1-2("for the message-encode bench") has the same single-bench framing. Proof:git grep -n 'support/messages.rs' fa2734f2 -- crates/spate-kafkaexits 0 and lists three#[path]includers (encode_alloc.rs:15,encode_gungraun.rs:72,tests/bench_fixtures.rs:25).crates/spate-json/benches/support/decode_rig.rs:20words the same rationale as "Each includer compiles this module separately", which stays true as includers are added.
Evidence for the notes
The intent proof ran on darwin aarch64, where the bench at the head prints rows byte-identical to encode_alloc.default.aarch64.tsv. The comparison is the bench's stdout against the committed file with its four header lines removed: exit 1 when they match, 0 when a figure moves.
One-time reservation. Mutant:
--- a/crates/spate-kafka/src/sink/encoder.rs
+++ b/crates/spate-kafka/src/sink/encoder.rs
@@ -73,6 +73,9 @@ impl KafkaMessage {
/// Set the payload by copying `payload` in.
pub fn set_payload(&mut self, payload: &[u8]) {
self.tombstone = false;
+ if self.payload.capacity() == 0 {
+ self.payload.reserve(1 << 20);
+ }
self.payload.clear();
self.payload.extend_from_slice(payload);
}With it applied the bench prints the committed figures:
bytes_keyed 0 0 0
bytes_keyless 0 0 0
json_typed 0 0 0
stamped_headers 0 0 0
stamped_over_limit 10000 0 3380000
A per-record key.to_vec() in set_key moves bytes_keyed to 10000 0 160000, so a per-record allocation is caught. No remedy was tested. Rewording the body's claim to "any change to what an encode allocates in steady state" would match the recorded warm pass.
records rustdoc. One wording that holds for both includers: "the counted tier returns the rig rather than dropping it, and the snapshot holds it outside its window, so the drop happens at teardown".
messages.rs includers. Rewording line 25 to "Each includer compiles this module separately and uses a different subset of it" matches the spate-json rig. The module-wide allow it defends is still the right shape, since encode_alloc.rs uses only RECORDS directly.
Outside this change
DEVELOPING.md:296(docs). Proposed as an issue:docs: name the allocation snapshot file after the bench stem. The Allocation snapshots section binds<name>by "benches/<name>_alloc.rs" atDEVELOPING.md:293and names the committed filebenches/alloc/<name>.<arm>.<arch>.tsv, which givesencode.default.x86_64.tsvfor this target. The driver builds the path from the bench file stem with its_allocsuffix (xtask/src/checks/alloc_snapshot.rs:75,bench: stem.to_owned()at:91), so it reads and writesencode_alloc.default.x86_64.tsv, as the committeddecode_allocfiles inspate-jsonalso show. Proof:test -f crates/spate-kafka/benches/alloc/encode.default.x86_64.tsvexits 1 at the head.--blesswrites the right name, so only someone reading or creating a file by hand is misled. The page predates this pull request.
Dimensions
Ran: performance, tests, docs, record, build, intent, callers.
Skipped:
- api: the diff adds no
pubitem reachable from a crate root, no config key, builder, feature or trait; the new items arepub(crate)inside bench targets. - delivery: no source, feed, sink runtime, tracker, ack, commit or shutdown code changes.
- liveness: no threads, channels, locks,
.await, poll loops or timers change. - failure: no error type, classification, retry or timeout changes.
- observability: no metric or log line is added, renamed or changed.
- backends: no code talks to an external system, no client library bump, no container pin or request shape change.
- security: no parser over outside bytes, no config or credential type, no
unsafe, TLS, auth, listener,Cargo.lockor workflow change. - website: no site component, style, config, mdx component or blog change.
|
Thanks for the review. All three notes are fixed in 5060273.
The outside item, the snapshot file name in |
What this changes, and why
Part of #1390.
This adds an allocation snapshot for the Kafka sink's encode path, so a change to what an encode allocates in steady state fails CI with the case and both figures. Each case is counted after one warm pass, so an allocation an encoder makes only on its first record is outside the snapshot.
The first commit moves the rig and
encode_runout ofencode_gungraun.rsintobenches/support/encode_rig.rs, unchanged, so the counted tier andbenches/encode_alloc.rsdrive one region. The second adds the target and its files. The four accepting cases allocate nothing in steady state, andstamped_over_limitpins the error each rejected record builds. The x86_64 and aarch64 files are identical apart from thearchline.Implemented by a Claude Opus coding agent under maintainer direction, and reviewed by Claude Opus agents.
Changelog: none
Invariants
None.
Semver
Checks
cargo xtask ciarea: descriptionfor one area, no AI attribution trailersChangelog: nonein this bodyAnything else
The files were written with
cargo xtask bench alloc --blessin Linux containers on a darwin host, arm64 natively and amd64 under emulation, and a second run against them exits 0. The CI legs compare against them.cargo xtask mutants --since origin/maingenerates no mutant: the diff is bench targets and committed files.