Repository navigation
THRIFT-6427: Avoid copying payload bytes in native Ruby Compact writes - #4015
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Header callbacks can change the payload encoding, breaking the existing binary callback contract.
1 open finding
What changed in this PR
Reduces temporary payload allocations in native Ruby Compact writes by allowing shared String storage while preserving callback isolation.
Changes:
- Uses
rb_str_subseqwhen the source length remains unchanged. - Adds ownership, encoding, callback, and transport round-trip coverage.
| File | Description |
|---|---|
| lib/rb/spec/compact_protocol_spec.rb | Adds payload ownership and compatibility tests. |
| lib/rb/ext/compact_protocol.c | Shares payload storage with a copying fallback. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Client: rb Use a separate mutable String whose storage Ruby can share with the input until either value changes. Preserve transport write overrides, callback mutation isolation, frozen inputs, and retained payload snapshots. Keep the existing copying path when a header callback changes the source length. Normalize the callback payload after header writes so a callback that re-encodes the source cannot change the payload's binary encoding. Add behavioral coverage for callback ownership, encodings, and transport overrides. Allocation measurements remain outside the regression suite. Co-Authored-By: OpenAI Codex (gpt-6.1-sol) <codex@openai.com>
kpumuk
force-pushed
the
rb-compact-payload-storage
branch
from
October 16, 2026 00:57
25d7182 to
94442bd
Compare
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.
Native Ruby Compact writes copy every binary payload into a new String before calling the transport. Repeated large writes therefore allocate payload-sized temporary storage even when the transport immediately discards the bytes.
write_stringreaches the same path after encoding conversion.Use
rb_str_subseqto provide a separate mutable plain String while allowing Ruby to share its backing storage until either value changes. This preserves callback mutation isolation, retained snapshots, frozen inputs, and transport write overrides. If the length-prefix callback changes the source length, retain the existing copying path.Behavioral coverage exercises ownership, encodings, empty payloads, callback exceptions, and MemoryBuffer/Framed round trips. Allocation thresholds remain outside the regression suite.
Benchmarks
Run the
compact-payload.rbreproducer in THRIFT-6427 after building the native extension:CRuby 4.0.7 on aarch64 Linux; 64 writes of one frozen 1 MiB payload to a discarding transport, with GC disabled during measurement. Three fresh processes per revision:
5248fb28)Every trial produced the corresponding value above. The Ruby sources on current master match the benchmark baseline. This measures temporary String storage rather than post-GC retained memory or throughput.
Shared backing storage changes retention costs: retaining a 4 KiB substring of an 8 MiB parent can keep approximately 8 MiB alive, whereas the previous copy retained approximately 4 KiB. Value isolation remains intact. Ruby implementations and sharing thresholds can produce different results.