encode: encode_to_bytes writes through Vec<u8>, not BytesMut - #437
Conversation
`<BytesMut as BufMut>::put_slice` is not `#[inline]` and LLVM folds `reserve_inner` into it, so it stays out of line even under fat LTO: every tag and varint byte written through a `BytesMut` was a call, a reserve check, a one-byte libc memcpy and an `advance_mut` re-check. `Vec<u8>`'s impl inlines to a compare and a store. `encode_to_bytes` / `try_encode_to_bytes` on `Message`, `ViewEncode` and generated lazy views now delegate to the `Vec` entry points and convert with `Bytes::from`, which is what `BytesMut::freeze` on a vec-backed buffer did anyway (same `Bytes` representation; no allocation when `len == capacity`, which the two-pass size guarantees). `Rope`'s tail is written through the inlined inherent `BytesMut::extend_from_slice`. benchmarks/buffa gains `benches/encode_sink.rs` (Vec vs BytesMut sinks, `encode_to_vec` vs `encode_to_bytes`, and the "frame into your own BytesMut" case) plus a `bench-nolto` profile and `task bench-encode-sink` to run it at both profiles. encode_to_bytes, main -> this change, median per dataset batch on pinned cores (no-LTO / fat-LTO): log_record 21.4 / 23.3 us -> 6.46 / 6.04 us (encode_to_vec: 6.1 / 5.9) api_response 8.31 / 8.15 us -> 2.17 / 1.97 us (encode_to_vec: 2.0 / 1.9) google_message1 257 / 264 ns -> 64 / 64 ns (encode_to_vec: 61 / 60) log_record view 20.9 / 20.4 us -> 5.62 / 5.51 us
|
All contributors have signed the CLA ✍️ ✅ |
rpb-ant
changed the title
encode_to_bytes writes through Vec<u8>, not BytesMut
- encode_sink bench: gated behind a non-default `encode_sink` feature so its IDs stay out of `task bench` and saved baselines; throughput is payload bytes like the rest of the suite; the sink rows reuse one pre-grown buffer so they differ only in per-put cost; media_frame added as the bytes-heavy control; shape named google_message1_proto3. - bench-nolto inherits release (cargo's defaults by construction); task bench-encode-sink keeps the two profiles' criterion results apart. - Comments that said LTO closes the gap now say what the data says; the encode_to_vec rationale and debug_assert_two_pass doc no longer describe encode_to_bytes as a direct body; the *_to_bytes docs state they are defined via *_to_vec. - Tests: encode into a caller's BytesMut matches encode_to_vec (the one place the blanket BufMut sink is still exercised); generated lazy-view encode_to_bytes/try_encode_to_bytes; Rope::put_u64_le bytes. - Changelog scoped to the tag-dense shapes measured.
|
[claude code] Reviewed at Iain's request and, with rpb's OK, pushed the follow-ups directly (afaea86, be192a9). The library change itself checked out on every axis we looked at: What the follow-up commits change, all outside the runtime except comments:
Metal numbers (quieted
So: confirmed on both profiles, LTO rescues nothing (main's LTO One design note for a follow-up rather than this PR: the root cause is the blanket The push dismissed the existing approval, so this needs a re-stamp. |
2 participants
encode_to_byteswas 3–4× slower thanencode_to_vecon every benchmark shape, with or without LTO; it is now within noise of it. Pure performance change; no behavioural or wire impact.Why
encode_to_bytesbuilt aBytesMut::with_capacity(size), wrote the message through it, and froze it.bytesdoes not mark<BytesMut as BufMut>::put_slice#[inline](it does forVec<u8>),BytesMuthas noput_u8override, and LLVM foldsreserve_innerintoput_slice, so it stays out of line even under fat LTO. Every tag and varint byte our encoders write throughput_u8therefore became a call, a reserve check, a one-byte libcmemcpy, and anadvance_mutre-check; throughVec<u8>the same write is a compare and a store.The doc comment already said
encode_to_bytesis "equivalent toBytes::from(self.encode_to_vec())" — this makes the implementation literally that. The upstream fix is proposed separately in tokio-rs/bytes#850; this change is worthwhile regardless, since buffa cannot make its users take a newerbytes, and theVecpath is never slower.Change
Message::{encode_to_bytes, try_encode_to_bytes},ViewEncode::{encode_to_bytes, try_encode_to_bytes}and the generated lazy-view inherent methods delegate to theirVectwins and convert withBytes::from. Output bytes and the returnedBytesrepresentation are unchanged:BytesMut::freezeon a vec-backed buffer already went throughFrom<Vec<u8>> for Bytes, and withlen == capacity(guaranteed by the two-pass size) that conversion allocates nothing.Rope's tail writes go through the inherent, inlinedBytesMut::extend_from_sliceinstead of itsBufMutimpl;to_contiguous_bytesbuilds aVec.benchmarks/buffa/benches/encode_sink.rscompares the sinks —encode_to_vecvsencode_to_bytes, encode into a pre-sizedVecvsBytesMut, and "frame a header then encode into your ownBytesMut" vs "frame thenput_slice(encode_to_vec())" — onlog_record,api_response,google_message1and thelog_recordview. Abench-noltoprofile andtask bench-encode-sinkrun it at both the LTO bench profile and the profile a downstreamcargo build --releasegets.Results
encode_to_bytes,main→ this change, criterion median per dataset batch, one pinned core (no-LTO / fat-LTO).Development machine (Xeon 8488C):
encode_to_vec(reference)Clean EC2 instances (Amazon Linux 2023, rustc 1.95.0):
encode_to_vecon the same EC2 runs: 5.4–5.6 / 8.0–8.6 µs, 1.8–2.0 / 2.6–2.8 µs, 58–63 / 89–98 ns — soencode_to_bytesis now within ~5 % of it on both architectures, and LTO did not rescue the old path on either.Callers that
encodestraight into their ownBytesMutstill pay the slow sink untilbyteschanges; the bench's last two rows show that encoding to aVecand appending it with oneput_sliceis ~2.7× faster even counting the copy (log_record: 21.9 µs vs 8.0 µs).Compatibility
Patch-level. No API or wire change; regenerated lazy-view code is source-compatible (method signatures unchanged) and previously generated code keeps working against the new runtime.
Testing
cargo test -p buffa -p buffa-codegen -p buffa-test,cargo clippy -p buffa -p buffa-codegen --all-targets -- -D warnings,cargo fmt --check. The existingencode_to_bytes_over_limit_panicsand two-pass ledger tests cover the delegated paths; benchmarks as above.Follow-ups (not in this PR)
encode_to_veccan write throughVec::spare_capacity_mut()(&mut [MaybeUninit<u8>]implementsBufMut) andset_lenonce, since the size is exact: measured 1.25× (varint-heavy) to 3.2× (string-heavy) over the currentVecpath with generated code unchanged, for one line ofunsafe. Separate PR with its own safety argument.Rope's tail as aVec<u8>: a further ~1.3–1.5× on byte-heavy tails, at ≤2 small allocations per flushed segment.