Repository navigation
perf: zero-copy server wire path, allocation-free hot paths - #59
Conversation
Server allocations drop from ~17 to ~0.1 per task in bench; CPU per task roughly halves. Wire format unchanged. - Error messages are Cow<'static, str> - TaskId hex encode/parse on the stack (TaskId::to_hex) - complete_refs merges borrowed results into the finished record - submit_*_ids return TaskIds; Insert replies encode them directly - server moves zmq::Messages end to end, decodes bodies in place (RequestIn) and encodes replies from leases (Reply/LeasedView) - owner check once per lease; bytes serde for hot Vec<u8> fields Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Borrowed wire contracts and serialization enginelib/src/wire.rs, docs/protocol.md, AGENTS.md |
Wire decoding and request views support borrowed payloads. Byte fields use Serde adapters, and custom reply serialization covers leased tasks, completions, and task IDs. The protocol description documents frame handling and blocking-pool work. |
Task IDs and submission APIs enginelib/src/chunk.rs, enginelib/src/nucleus/submit.rs, enginelib/src/error.rs, AGENTS.md |
Task IDs gain stack-backed hexadecimal formatting. New submission APIs return TaskId values; existing methods convert them to strings. Error messages use Cow storage. |
Borrowed completion and finished records enginelib/src/chunk.rs, enginelib/src/nucleus/complete.rs, enginelib/tests/nucleus.rs, AGENTS.md |
Completion accepts borrowed results and processes raw finished records with shared encode and decode functions. The added test covers interleaved task types and an unregistered type. |
Frame-based transport and request routing enginelib/src/transport/server.rs, enginelib/src/api.rs, enginelib/macros/src/lib.rs |
The server passes ZeroMQ frames without copying, dispatches requests by kind, and runs Complete and Query decoding and work on the blocking pool. Lease and completion paths use borrowed representations. The tracing level and macro insertion syntax also change. |
Priority:
Estimated code review effort: 4 (Complex) | ~60 minutes
Change: Refactor
Sequence Diagram(s)
sequenceDiagram
participant Router as ZeroMQ ROUTER
participant Dispatch as Server dispatch
participant Blocking as Blocking pool
participant Nucleus
participant Reply as Reply pool
Router->>Dispatch: Pass identity and payload frames
Dispatch->>Blocking: Decode and run Complete or Query
Blocking->>Nucleus: Process borrowed request data
Nucleus-->>Blocking: Return operation result
Blocking-->>Dispatch: Return reply
Dispatch->>Reply: Encode and enqueue reply
Reply->>Router: Send identity and reply frames
Possibly related PRs
- voltaero/engine#55: Adds derived task chunks and
InsertDerived; this change updates the related derived-task submission and insertion flow.
Merge Risk: 
3c66a
-
This change reduces allocations and CPU use in the server request and completion path while keeping the wire format unchanged. No concrete defect was identified in the supplied evidence, so it appears ready to merge. Mods that pass a borrowed non-'static string to the Error constructors will need
.to_string(), as the description already notes.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 37.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 89 functions across 9 files. (2 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "perf: zero-copy server wire path, allocation-free hot paths" matches the main objective of PR #59. The changes move ZeroMQ frames without copying, decode request bodies in place, use borrow…Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "perf: zero-copy server wire path, allocation-free hot paths" matches the main objective of PR
#59. The changes move ZeroMQ frames without copying, decode request bodies in place, use borrowed results for completion, and replace allocation-heavy paths with stack-backed or borrowed forms. The title is concise, uses a conventional commit prefix, and names the primary change from the developer's perspective. Some scope (benchmarks, error type changes, AGENTS.md docs) is not named, which is acceptable.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 37.08% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 89 functions across 9 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 Clippy (1.99.0)
-
Clippy execution failed
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 @coderabbitai help to get the list of available commands.
 [Medium impact] The PR appears safe to merge; both previous findings are addressed. SummaryThis PR reduces server allocations by borrowing request data, moving ZeroMQ messages, and writing replies directly from chunk leases. Since the previous review:
Both previous findings are addressed. No new actionable issues were found.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Addresses review on the linear type scans: - complete_refs: previous item's type first, then a map keyed by the borrowed names (no owned key per item); mixed-type batches stay O(1) per item. Adds a mixed-type regression test. - owned_batch: resolve each named type's lease map once per request with a keyed lookup instead of iterating every DashMap shard per chunk. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Server heap allocations drop from 17.07 to 0.10 per task in
bench(bytes 1170 → 551/task), and server CPU per task roughly halves (~29.5 s → ~13.5 s user+sys per 10M tasks). The wire format is unchanged, so noPROTO_VERSIONbump.Wall-clock rates were measured by alternating runs of the old and new binaries on a loaded machine; CPU per task is the steadier number.
Changes
Errormessages areCow<'static, str>, so fixed messages and their per-item clones don't allocate.TaskId::to_hex,TaskId::parse).submit_batch_ids/submit_derived_idsreturnTaskIds, and Insert replies encode them directly.complete_refstakes borrowed items and merges results straight from the request buffer and the pinned finished record (encode_finished/decode_finished). It no longer clones the task type per item.zmq::Messages are moved end to end, never copied. The server decodes only the header and variant first (RequestKind), then decodes the Complete/Insert body in place (RequestIn) on the blocking pool. Replies are written straight from the chunk leases (Reply/LeasedView).Vec<u8>wire fields use#[serde(with = "bytes")], which writes the same bytes with one memcpy instead of per-byte serde.Notes
wire.rsmust stay in sync withRequest/Query/Response. Thewire::testsbyte-equality tests catch drift, and the rule is documented inAGENTS.mdanddocs/protocol.md.Errorconstructors takeimpl Into<Cow<'static, str>>, so a mod passing a borrowed non-'static&strneeds.to_string().req_id, not 0, so the client sees the error instead of timing out.cargo test --workspacepasses and clippy is clean.🤖 Generated with Claude Code
Summary by CodeRabbit