Enable zero-copy payload extraction for codecs#420
Conversation
Reviewer's GuideAdds a detailed ExecPlan 9.1.3 for enabling zero-copy payload extraction in codecs and performs a small formatting-only reflow of the task list in the existing ExecPlan 9.1.2 codec error taxonomy document. Sequence diagram for zero-copy payload extraction through frame_payload_bytessequenceDiagram
actor App
participant Connection
participant FrameCodecImpl
participant Decoder
participant BytesMutBuffer as BytesMut
participant Frame
participant Payload as Bytes
App->>Connection: read_and_process_messages()
Connection->>FrameCodecImpl: decoder()
FrameCodecImpl-->>Connection: Decoder
loop read from socket
Connection->>BytesMutBuffer: fill_from_socket()
Connection->>Decoder: decode(BytesMutBuffer)
Decoder-->>Connection: Frame?
alt frame available
Connection->>FrameCodecImpl: frame_payload_bytes(Frame)
FrameCodecImpl-->>Connection: Bytes
Note over Frame,Payload: Frame and Bytes share the same underlying buffer
Connection->>App: handle_payload(Bytes)
else no frame yet
Connection-->>App: wait_for_more_data()
end
end
Class diagram for FrameCodec zero-copy payload designclassDiagram
class FrameCodec {
<<trait>>
+type Frame
+type Decoder
+type Encoder
+decoder() Decoder
+encoder() Encoder
+frame_payload(frame: Frame) &[u8]
+frame_payload_bytes(frame: Frame) Bytes
+wrap_payload(payload: Bytes) Frame
+correlation_id(frame: Frame) Option_u64_
+max_frame_length() usize
}
class LengthDelimitedFrameCodec {
+max_frame_length: usize
+new(max_frame_length: usize) LengthDelimitedFrameCodec
+decoder() LengthDelimitedDecoder
+encoder() LengthDelimitedEncoder
+frame_payload(frame: Bytes) &[u8]
+frame_payload_bytes(frame: Bytes) Bytes
+wrap_payload(payload: Bytes) Bytes
+max_frame_length() usize
}
class HotlineFrameCodec {
+decoder() HotlineAdapter
+encoder() HotlineAdapter
+frame_payload(frame: HotlineFrame) &[u8]
+frame_payload_bytes(frame: HotlineFrame) Bytes
+wrap_payload(payload: Bytes) HotlineFrame
+max_frame_length() usize
}
class MysqlFrameCodec {
+decoder() MysqlAdapter
+encoder() MysqlAdapter
+frame_payload(frame: MysqlFrame) &[u8]
+frame_payload_bytes(frame: MysqlFrame) Bytes
+wrap_payload(payload: Bytes) MysqlFrame
+max_frame_length() usize
}
class HotlineFrame {
+transaction_id: u32
+payload: Bytes
}
class MysqlFrame {
+sequence_id: u8
+payload: Bytes
}
class LengthDelimitedDecoder {
+decode(src: BytesMut) Option_Bytes_
}
class LengthDelimitedEncoder {
+encode(frame: Bytes, dst: BytesMut) void
}
class HotlineAdapter {
+decode(src: BytesMut) Option_HotlineFrame_
+encode(frame: HotlineFrame, dst: BytesMut) void
}
class MysqlAdapter {
+decode(src: BytesMut) Option_MysqlFrame_
+encode(frame: MysqlFrame, dst: BytesMut) void
}
class Bytes {
+clone() Bytes
+as_ref() &[u8]
}
class BytesMut {
+freeze() Bytes
+extend_from_slice(src: &[u8]) void
}
FrameCodec <|.. LengthDelimitedFrameCodec
FrameCodec <|.. HotlineFrameCodec
FrameCodec <|.. MysqlFrameCodec
LengthDelimitedFrameCodec --> Bytes : Frame
HotlineFrameCodec --> HotlineFrame : Frame
MysqlFrameCodec --> MysqlFrame : Frame
HotlineFrame --> Bytes : payload
MysqlFrame --> Bytes : payload
LengthDelimitedFrameCodec --> LengthDelimitedDecoder : Decoder
LengthDelimitedFrameCodec --> LengthDelimitedEncoder : Encoder
HotlineFrameCodec --> HotlineAdapter : Decoder
HotlineFrameCodec --> HotlineAdapter : Encoder
MysqlFrameCodec --> MysqlAdapter : Decoder
MysqlFrameCodec --> MysqlAdapter : Encoder
LengthDelimitedDecoder --> BytesMut
LengthDelimitedEncoder --> BytesMut
HotlineAdapter --> BytesMut
MysqlAdapter --> BytesMut
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a new zero-copy payload extraction path by introducing Changes
Sequence Diagram(s)(Skipped — changes are API additions, documentation and tests; no new multi-component sequential control flow requiring visualisation.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In the new 9.1.3 ExecPlan, consider marking "Draft ExecPlan for 9.1.3" as completed (or removing it) so the Progress checklist reflects the current state of this document.
- The roadmap-update instructions in the ExecPlan still talk about "Update
FrameCodec::frame_payloadto return aBytes-backed view", which conflicts with the chosenframe_payload_bytes-based design; it would be clearer to align the roadmap text with the actual API approach. - Several sections reference specific file line numbers (e.g.,
src/codec.rs:57-88); to keep this plan more future-proof, it might be better to refer to types/functions by name rather than by line ranges that can drift over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new 9.1.3 ExecPlan, consider marking "Draft ExecPlan for 9.1.3" as completed (or removing it) so the Progress checklist reflects the current state of this document.
- The roadmap-update instructions in the ExecPlan still talk about "Update `FrameCodec::frame_payload` to return a `Bytes`-backed view", which conflicts with the chosen `frame_payload_bytes`-based design; it would be clearer to align the roadmap text with the actual API approach.
- Several sections reference specific file line numbers (e.g., `src/codec.rs:57-88`); to keep this plan more future-proof, it might be better to refer to types/functions by name rather than by line ranges that can drift over time.
## Individual Comments
### Comment 1
<location> `docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md:194` </location>
<code_context>
+
+### Phase 5: Documentation Updates
+
+1. Add "Zero-copy payload extraction" section to ADR 004 under "Resolved
+ Decisions"
+2. Update the "Custom frame codecs" section in `docs/users-guide.md` with
</code_context>
<issue_to_address>
**issue (review_instructions):** The acronym "ADR" is used without being defined on first use; it should be expanded (for example, "Architecture Decision Record (ADR)") when first mentioned in the prose.
The review instructions require uncommon acronyms to be defined on first use. Here, "ADR 004" appears without an earlier definition. Consider changing this to something like "Architecture Decision Record (ADR) 004" the first time it is mentioned in the body text, and then use "ADR 004" thereafter.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.md (3)
21-44: Progress status indicates incomplete documentation for completed implementation.The progress checklist shows implementation tasks completed on 2026-01-07 (lines 24-32, 37-40), including the BDD behavioural tests. However, documentation updates remain pending (lines 41-43), specifically:
docs/users-guide.mderror handling documentationdocs/adr-004-pluggable-protocol-codecs.mddesign decisions- Roadmap entry 9.1.2 marked as done
Complete the documentation updates to align with the completed implementation work. This is particularly important because ExecPlan 9.1.3 depends on the error taxonomy being fully documented and integrated.
111-112: Remove all Cucumber references from documentation.The coding guidelines explicitly require: "Replace or retire documentation that refers to Cucumber, including guides and examples in docs/". This document contains multiple Cucumber references:
- Line 111:
tests/features/directory reference- Line 112:
tests/steps/andtests/worlds/directories- Lines 222-230: Phase 8 Testing describes BDD tests in
tests/features/codec_error.feature- Lines 311-316: Concrete steps 12-13 create Cucumber feature files and step definitions
- Line 318: Reference to
tests/cucumber.rs- Lines 444-447: Files to modify table includes three Cucumber-related files
Remove all references to Cucumber test infrastructure. If behavioural testing is required, document it using the current testing approach (the guidelines mention rstest and wireframe_testing crate usage based on the external tools context).
♻️ Proposed fix: Remove Cucumber references
Update the testing strategy to remove Cucumber:
Lines 111-112, replace:
-Testing infrastructure: - -- `tests/frame_codec.rs` - Codec integration tests -- `tests/features/` - Cucumber feature files -- `tests/steps/` and `tests/worlds/` - Step definitions and world types +Testing infrastructure: + +- `tests/frame_codec.rs` - Codec integration tests +- `tests/codec_error.rs` - Error taxonomy integration testsLines 222-230, remove subsection 3:
2. **Integration tests** in `tests/codec_error.rs`: - Oversized frame triggers drop and connection continues - Max consecutive drops triggers disconnect - Mid-frame EOF surfaces to handler - Custom recovery policy hook is invoked -3. **BDD behavioural tests** in `tests/features/codec_error.feature`: - - Scenario: Oversized frame triggers drop recovery - - Scenario: Consecutive drop limit triggers disconnect - - Scenario: Clean EOF at frame boundary - - Scenario: Premature EOF mid-frame - - Scenario: Custom recovery policy hook - -4. Add step definitions in `tests/steps/codec_error_steps.rs` -5. Add world type in `tests/worlds/codec_error/`Lines 311-318, remove steps 12-14:
11. Create `tests/codec_error.rs`: - Add integration tests for error propagation - Add tests for recovery policy application -12. Create `tests/features/codec_error.feature`: - - Add behavioural scenarios for codec errors - -13. Create `tests/steps/codec_error_steps.rs` and - `tests/worlds/codec_error/`: - - Add step definitions and world type - -14. Update `tests/cucumber.rs`: - - Register new world type - -15. Update `docs/users-guide.md`: +12. Update `docs/users-guide.md`:Lines 444-447, remove Cucumber test files from table:
| `tests/codec_error.rs` | New: Integration tests | -| `tests/features/codec_error.feature` | New: BDD tests | -| `tests/steps/codec_error_steps.rs` | New: Step definitions | -| `tests/worlds/codec_error/` | New: World type | -| `tests/cucumber.rs` | Modify: Register world | | `docs/users-guide.md` | Modify: Error handling docs |Update line 40 in Progress section to reflect integration tests only:
-- [x] (2026-01-07) Add BDD behavioural tests for codec error scenarios. +- [x] (2026-01-07) Add integration tests for codec error scenarios.Also applies to: 222-230, 311-318, 444-447
353-369: Update acceptance criteria to remove Cucumber test references.The validation and acceptance criteria include "All BDD tests pass for behavioural scenarios" (line 367), which references the Cucumber infrastructure that must be removed.
Revise the acceptance criteria to focus on integration tests and unit tests only, consistent with the removal of Cucumber references throughout the document.
♻️ Proposed fix
- All unit tests pass for error taxonomy and recovery policies - All integration tests pass for error propagation -- All BDD tests pass for behavioural scenarios - `make check-fmt`, `make lint`, and `make test` succeed
🤖 Fix all issues with AI agents
In `@docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md`:
- Around line 20-42: Add an explicit dependency note in the "Context and
Orientation" section of ExecPlan 9.1.3 stating that work cannot start until
ExecPlan 9.1.2 completes its error-handling items—specifically the error
propagation surfaces and the WireframeError extensions—so that 9.1.3 can rely on
the new error taxonomy; mention ExecPlan 9.1.2 by number and list the required
completed items (error propagation surfaces, WireframeError extensions) that
must be merged before beginning 9.1.3.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md
📄 CodeRabbit inference engine (docs/contents.md)
Follow the documentation style guide conventions when writing project documentation
Write official documentation with separate guides for 'Duplex Messaging & Pushes', 'Streaming Responses', and 'Message Fragmentation', each including runnable examples and explaining relevant concepts and APIs
docs/**/*.md: Use markdown files within thedocs/directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in thedocs/directory to reflect the latest state. Ensure documentation remains accurate and current.
Documentation must use en-GB-oxendict spelling and grammar ('-ize' / '-yse' / '-our'), with the exception of the filenameLICENSE.
Reference the documentation style guide provided atdocs/documentation-style-guide.mdfor documentation writing standards.Documentation describing multi-packet streaming MUST reflect the tuple-based API and explain helper constructors that prepare the channel pair
Replace or retire documentation that refers to Cucumber, including guides and examples in docs/
docs/**/*.md: Use British English based on the Oxford English Dictionary (en-GB-oxendict), including: suffix -ize in words like 'realize' and 'organization', suffix -lyse in words like 'analyse' and 'paralyse', suffix -our in words like 'colour' and 'behaviour', suffix -re in words like 'calibre' and 'centre', double 'l' in words like 'cancelled' and 'counsellor', maintain the 'e' in words like 'likeable' and 'liveable', suffix -ogue in words like 'analogue' and 'catalogue'
The word 'outwith' is acceptable in documentation
Keep US spelling when used in an API, for example 'color'
Use the Oxford comma: 'ships, planes, and hovercraft' where it aids comprehension
Treat company names as collective nouns, for example: 'Lille Industries are expanding'
Write hea...
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md
docs/**/*.{rs,md}
📄 CodeRabbit inference engine (docs/rust-doctest-dry-guide.md)
Every doctest should validate the public API of a crate from the perspective of an external user, treating each documentation test as a separate temporary crate that imports the library as an external dependency
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
**/*.md: Validate Markdown files usingmake markdownlint.
Runmake fmtafter any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by runningmake nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks in Markdown must be wrapped at 120 columns.
Markdown tables and headings must not be wrapped.
Use dashes (-) for list bullets in Markdown.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes in Markdown.
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md
⚙️ CodeRabbit configuration file
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -yse / -our) spelling and grammar
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/execplans/9-1-2-introduce-a-codec-error-taxonomy.mddocs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md
🪛 LanguageTool
docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md
[grammar] ~16-~16: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ...at verify pointer equality to prove the receive buffer is reused. Success is visible wh...
(PREPOSITION_VERB)
[uncategorized] ~49-~49: Possible missing comma found.
Context: ...ion: Add frame_payload_bytes as a new method rather than changing the return type ...
(AI_HYDRA_LEO_MISSING_COMMA)
[typographical] ~143-~143: To join two clauses or introduce examples, consider using an em dash.
Context: ... Codec implementation: - src/codec.rs - FrameCodec trait and `LengthDelimitedF...
(DASH_RULE)
[typographical] ~144-~144: To join two clauses or introduce examples, consider using an em dash.
Context: ...tedFrameCodec-src/codec/examples.rs-HotlineFrameCodecandMysqlFrameCodec...
(DASH_RULE)
[typographical] ~145-~145: To join two clauses or introduce examples, consider using an em dash.
Context: ...MysqlFrameCodec - src/codec/tests.rs - Codec unit tests Documentation to updat...
(DASH_RULE)
[typographical] ~149-~149: To join two clauses or introduce examples, consider using an em dash.
Context: ...entation to update: - docs/roadmap.md - Mark 9.1.3 done - docs/users-guide.md ...
(DASH_RULE)
[typographical] ~150-~150: To join two clauses or introduce examples, consider using an em dash.
Context: ... Mark 9.1.3 done - docs/users-guide.md - Custom codec guidance - `docs/adr-004-pl...
(DASH_RULE)
[typographical] ~151-~151: To join two clauses or introduce examples, consider using an em dash.
Context: ...cs/adr-004-pluggable-protocol-codecs.md` - Design decisions ## Plan of Work ### P...
(DASH_RULE)
[typographical] ~184-~184: To join two clauses or introduce examples, consider using an em dash.
Context: ...ytes_reuses_memory_for_length_delimited- Verify the extractedBytes` points t...
(DASH_RULE)
[typographical] ~186-~186: To join two clauses or introduce examples, consider using an em dash.
Context: ...tline_decode_produces_zero_copy_payload` - Verify the decoded payload points int...
(DASH_RULE)
[grammar] ~187-~187: Did you mean the noun “reception” (= the act of receiving) or “receipt” (= invoice)?
Context: ... the decoded payload points into the receive buffer 3. `hotline_wrap_payload_reuses_...
(PREPOSITION_VERB)
[typographical] ~188-~188: To join two clauses or introduce examples, consider using an em dash.
Context: ...r 3. hotline_wrap_payload_reuses_bytes - Verify wrap_payload stores the `Byt...
(DASH_RULE)
[typographical] ~259-~259: To join two clauses or introduce examples, consider using an em dash.
Context: ...ayload); ``` To (no change needed - extend_from_slice works with `Bytes` v...
(DASH_RULE)
🔍 Remote MCP Deepwiki
Based on the comprehensive documentation I've retrieved from the leynos/wireframe repository, I've found extensive context about the codebase architecture and the pull request's proposed changes. Let me now provide a summary of the relevant findings.
Summary of Relevant Context for PR #420 Review
Current Architecture Foundation
The PR builds upon an established foundation where:
-
FrameProcessor Trait: The core abstraction for frame encoding/decoding is already implemented with
LengthPrefixedProcessoras the primary implementation. The existing documentation shows that frame processing separates concerns between framing logic and payload serialization. -
Error Handling Infrastructure: ExecPlan 9.1.2 introduces a structured
CodecErrortaxonomy with recovery policies. The existing error handling uses a genericWireframeError<E>type that distinguishes between I/O errors (fatal) and Protocol errors (potentially recoverable). -
Zero-Copy Principles Already Applied: The framework demonstrates existing zero-copy patterns through the
Payloadstruct, which wraps byte slices (&[u8]) without copying data. The documentation specifically notes that changing frame types fromVec<u8>toByteswould eliminate finalto_vec()calls.
ExecPlan 9.1.2 Context (Codec Error Taxonomy)
The first document introduces:
- CodecError enum wrapping framing, protocol, IO, and EOF errors
- RecoveryPolicy infrastructure with context, hooks, and configuration
- Extended error propagation surfaces by adding
Codec(CodecError)variant toWireframeError - LengthDelimitedDecoder updates to surface structured EOF errors distinguishing
CleanClose,MidFrame, andMidHeader
This establishes the foundation for the zero-copy work in 9.1.3, which depends on these error handling mechanisms.
ExecPlan 9.1.3 Zero-Copy Proposal Review Points
The PR's ExecPlan for zero-copy payload extraction proposes:
-
New
FrameCodec::frame_payload_bytes(frame: &Self::Frame) -> BytesAPI - Default implementation maintains compatibility;LengthDelimitedFrameCodecoverrides for true zero-copy- Consideration: The existing codebase uses
LengthPrefixedProcessorpattern. The proposal forFrameCodectrait appears to be an architectural refinement.
- Consideration: The existing codebase uses
-
Migration of Example Codecs (HotlineFrame, MysqlFrame) to Bytes-backed payloads
- Context: Documentation shows these are not yet implemented in the codebase reviewed, suggesting they are new codecs being added for testing/demonstration.
-
Regression Tests for Zero-Copy Extraction with pointer equality verification
- Existing Practice: Framework uses comprehensive testing with
rstest,cucumberBDD tests, and specialized testing utilities fromwireframe_testingcrate.
- Existing Practice: Framework uses comprehensive testing with
-
Documentation Updates: ADR 004 (pluggable codecs), user guide zero-copy guidance, roadmap marking 9.1.3 as complete
- Baseline: The framework already has established ADR documentation patterns and comprehensive user guides.
Key Architectural Considerations for Review
-
Trait Design Consistency: The new
frame_payload_bytesmethod should align with existingFrameProcessortrait patterns for ergonomics and discoverability. -
Backward Compatibility: The default implementation maintaining existing copy behavior ensures gradual migration and compatibility.
-
Testing Coverage: The proposed pointer-equality regression tests align with the framework's advanced testing strategy including property-based and concurrency testing.
-
Error Integration: Zero-copy payload changes should maintain compatibility with the error taxonomy introduced in 9.1.2.
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md (2)
1-518: Well-structured ExecPlan with comprehensive implementation guidance.The document provides clear, actionable steps for enabling zero-copy payload extraction. The structure follows the established ExecPlan format consistently, with detailed context, phased work plan, concrete implementation steps, and validation criteria. The decision log appropriately documents the rationale for backward compatibility and zero-copy verification approaches.
The placeholder sections ("Surprises & Discoveries" and "Outcomes & Retrospective") are appropriate for a planning document that explicitly identifies itself as a "living document" requiring updates as work proceeds.
323-347: No action required. The code block at lines 323-347 fully complies with the 120-column limit for Markdown code blocks. All lines in this section remain well under the 120-character threshold.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@docs/adr-004-pluggable-protocol-codecs.md`:
- Around line 283-288: Update the spelling in the zero-copy section: change the
heading/bullet text that says "Optimised implementations" to "Optimized
implementations" so the docs use the -ize form; the change should be applied to
the bullet describing optimized implementations that reference
frame_payload_bytes, frame.payload.clone(), and Bytes-backed frames for
consistency with coding guidelines.
In `@docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md`:
- Around line 340-370: The markdown snippet in the ADR update is missing a
closing fenced code block after the verification snippet; close the
triple-backtick fence immediately after the assertion lines so the block that
documents FrameCodec/frame_payload_bytes, Bytes/BytesMut::freeze guidance,
wrap_payload, and the MyCodec verification example is properly terminated;
ensure the closing ``` follows the assert_eq!(frame.payload.as_ptr(),
extracted.as_ptr()) line to fix the unclosed fenced code block.
- Around line 370-414: The markdown in the "Custom frame codecs" example
contains an unclosed fenced code block beginning with "```markdown" and the Rust
examples (showing MyFrame, FrameCodec impl with
frame_payload_bytes/wrap_payload, and the decode example using
BytesMut::freeze); close the second/open Rust code fence by adding a matching
``` fence immediately after the decode example to properly terminate the code
block so the rest of the document renders correctly.
- Line 345: Update the placeholder date in the section header "### Zero-copy
payload extraction (resolved 2026-01-XX)" to the completed date "2026-01-19" so
the heading reads "### Zero-copy payload extraction (resolved 2026-01-19)" to
match the Outcomes line that states "Completed 2026-01-19".
♻️ Duplicate comments (1)
docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md (1)
38-38: Expand the acronym "ADR" on first use.The acronym "ADR" appears here without prior definition. Expand to "Architecture Decision Record (ADR)" on first occurrence, then use "ADR" thereafter.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +281 to +295 fn mysql_frame_payload_bytes_reuses_memory() {
use super::examples::MysqlFrameCodec;
let codec = MysqlFrameCodec::new(128);
let payload = Bytes::from(vec![9_u8; 5]);
let frame = codec.wrap_payload(payload.clone());
let extracted = MysqlFrameCodec::frame_payload_bytes(&frame);
assert_eq!(
frame.payload.as_ptr(),
extracted.as_ptr(),
"frame_payload_bytes should return the same memory region"
);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md`:
- Line 562: Update the revision note string that currently reads "2026-01-19:
Fixed markdown formatting issues: closed unclosed fenced code" to use proper
capitalization by changing "markdown" to "Markdown" so the line reads
"2026-01-19: Fixed Markdown formatting issues: closed unclosed fenced code".
- Around line 434-436: Add a single blank line between the closing rust fence
and the closing markdown fence in the fenced code block (i.e., insert an empty
line after the rust closing ``` and before the final ``` in
docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md); then
run make markdownlint and make fmt to validate and format the Markdown.
- Around line 389-391: The closing nested fenced code block is missing a
separating blank line: locate the block that starts with ```rust and ends with
the inner ``` then add a single blank line between that inner closing ``` and
the outer closing ``` (i.e., insert a blank line after the inner ```rust closing
fence and before the final closing fence). After making the edit, run make
markdownlint and make fmt to validate and format the Markdown.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md`:
- Around line 76-81: Update the documentation wording to reflect the actual
behavior: change the description for LengthDelimitedFrameCodec so it says it
returns frame.clone() (i.e., clones the whole frame) rather than
frame.payload.clone(); keep the other entries for FrameCodec, HotlineFrameCodec,
and MysqlFrameCodec as-is describing frame_payload_bytes overriding to return
frame.payload.clone() so the doc matches the implemented symbols
FrameCodec::frame_payload_bytes and
LengthDelimitedFrameCodec::frame_payload_bytes.
♻️ Duplicate comments (1)
docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md (1)
561-562: Capitalise “Markdown” in the revision note.📝 Proposed fix
-- 2026-01-19: Fixed markdown formatting issues: removed orphan closing fences +- 2026-01-19: Fixed Markdown formatting issues: removed orphan closing fencesAs per coding guidelines, ensure proper capitalisation of technology names.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/execplans/9-1-3-enable-zero-copy-payload-extraction-for-codecs.md`:
- Line 565: Update the revision note line that currently reads "Fixed markdown
formatting issues: removed orphan closing fences" to capitalize the product name
("Markdown"); locate and edit the string in the document where the entry
"2026-01-19: Fixed markdown formatting issues: removed orphan closing fences"
appears and change "markdown" to "Markdown", then run make markdownlint and make
fmt to validate formatting and style.
|
@coderabbitai have the following now been resolved? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20999
✅ Resolved
|
Add `frame_payload_bytes` method to the `FrameCodec` trait for zero-copy payload extraction. Update example codecs to use `Bytes` instead of `Vec<u8>` and override this new method. Add zero-copy regression tests and update documentation including ADR and user guide. Mark roadmap entry 9.1.3 as done. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
- Added `frame_payload_bytes` method to `FrameCodec` trait with a default copying implementation. - Overrode `frame_payload_bytes` for LengthDelimitedFrameCodec, HotlineFrameCodec, and MysqlFrameCodec to return owned `Bytes` by cloning the internal buffer, achieving zero-copy extraction. - Updated frame types (HotlineFrame, MysqlFrame) to store payloads as `Bytes` instead of `Vec<u8>`. - Modified decoders to use `BytesMut::freeze()` for zero-copy buffer extraction. - Added zero-copy regression tests verifying pointer equality of extracted payloads. - Revised documentation to include design decisions, user guide section, and roadmap completion for milestone 9.1.3. - Ensured backward compatibility by keeping default method to copy from slice. - Performed formatting, linting, and test passes to maintain quality. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
- Added `frame_payload_bytes` method to `FrameCodec` trait for zero-copy payload extraction. - Updated `LengthDelimitedFrameCodec` and `HotlineFrameCodec` to override new method. - Modified `HotlineFrame` to use `Bytes` instead of `Vec<u8>` for zero-copy support. - Added zero-copy regression tests for multiple codecs. - Updated ADR-004 with zero-copy design decisions. - Updated user guide and roadmap accordingly. - Added prerequisite notes regarding dependency on ExecPlan 9.1.2 error taxonomy. This change enhances performance by avoiding unnecessary payload copying during frame processing. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
- Introduced helper functions to obtain payload pointers from frames for length_delimited, hotline, and mysql codecs. - Added parameterized tests verifying that `wrap_payload` reuses the Bytes without copying across all codecs. - Added parameterized tests verifying `frame_payload_bytes` returns Bytes pointing to the same memory region as the frame payload. - Replaced multiple redundant individual tests with consolidated, parameterized tests using rstest. - Added helper assertions for zero-copy verification during decoding. - Cleaned up and improved zero-copy test structure to cover multiple codecs uniformly. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
…s behavior Updated the Outcomes section to correctly describe that LengthDelimitedFrameCodec::frame_payload_bytes returns `frame.clone()` rather than `frame.payload.clone()` since its frame type is `Bytes` itself. This change improves accuracy in the documentation regarding zero-copy payload extraction for codecs. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
Fixed Markdown formatting issues by removing orphan closing fences in code blocks that violated MD040 rules. Also updated placeholder dates and expanded the "ADR" acronym for clarity. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
force-pushed
the
terragon/enable-zero-copy-payload-extraction-covhyz
branch
from
f7d2cbc to
e9e128d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/roadmap.md`:
- Around line 362-368: The roadmap wording is inaccurate: implementation added a
new method FrameCodec::frame_payload_bytes rather than changing
FrameCodec::frame_payload; update the bullet text to reference
FrameCodec::frame_payload_bytes (and note that the default codec adaptor was
updated to avoid Bytes->Vec<u8> copying on decode and a regression
test/benchmark was added) so the roadmap reflects the delivered changes and not
a signature change to frame_payload.
In `@src/codec/tests.rs`:
- Around line 139-160: The test helper wrap_and_get_payload_ptrs uses fragile
string-based matching; replace it with a typed dispatch by defining an enum
(e.g. TestCodec) with variants for LengthDelimited, Hotline, and Mysql,
implement a method like wrap_and_get_payload_ptr(&self, payload: Bytes) ->
*const u8 that matches on the enum and returns the correct pointer for each
codec (LengthDelimited -> .as_ptr(), Hotline/Mysql -> .payload.as_ptr()), update
tests/rstest parameters to pass the enum variant instead of strings, and remove
the panic branch in wrap_and_get_payload_ptrs.
- Line 134: Move the scattered import use super::examples::{HotlineFrameCodec,
MysqlFrameCodec}; up into the file's main import block alongside the other use
statements (so it's grouped with the existing imports) and delete the
duplicate/extra use line later in the file; ensure the import sits with the
other crate/module imports and follows the file's import ordering conventions.
- Consolidate zero-copy tests using a TestCodec enum for clarity and reuse. - Verify that frame payload bytes reuse memory to avoid copies across codecs. - Add pointer equality checks to ensure zero-copy behavior. - Update roadmap.md to reflect zero-copy method and regression tests. Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>
leynos
deleted the
terragon/enable-zero-copy-payload-extraction-covhyz
branch
1 participant
Summary
frame_payload_bytes(frame: &Self::Frame) -> Byteswith a default implementation that copies fromframe_payloadfor backward compatibility.frame_payload_bytesforLengthDelimitedFrameCodecto return a clone of the frame (cheap, since the frame isBytes).Bytesfor payload storage (e.g., HotlineFrame and MysqlFrame) and updates decoders/encoders to operate in zero-copy pathways (freeze and clone where appropriate).Changes
frame_payload_byteswith default copyBytespayloads for HotlineFrameBytespayloadsRationale
To reduce allocations and copies when extracting codec payloads, this change introduces a
frame_payload_bytesAPI, usesBytes-backed frames where possible, and provides zero-copy regression tests that verify reuse of the receive buffer. The approach builds on the existing zero-copy behavior in the defaultLengthDelimitedFrameCodecand generalizes to other codecs via a sharedBytes-backed frame model.Plan & Scope (Phase 9.1.3)
frame_payload_bytes(frame: &Self::Frame) -> Byteswith a default implementation that copies fromframe_payload.frame_payload_bytesforLengthDelimitedFrameCodecto returnframe.clone().HotlineFrameandMysqlFramepayload toBytes.Bytesand zero-copy pathways (freeze, clone).frame_payload_bytesinHotlineFrameCodecandMysqlFrameCodec.LengthDelimited,Hotline, andMysqlcodecs.Validation / Test plan
Documentation touched
Artifacts / Follow-up
Notes
📎 Task: https://www.terragonlabs.com/task/920295cf-3aec-4aff-bb91-ff7c6a47d5bb