From 312a0119882c30883f5fb6daf69966d2411882b9 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 11:40:41 -0700 Subject: [PATCH 01/11] table: reach message children without a table through their Message impl --- buffa/src/table/bridge.rs | 211 +++++++++++++++++ buffa/src/table/decode.rs | 6 +- buffa/src/table/encode.rs | 9 +- buffa/src/table/mod.rs | 1 + buffa/src/table/shape.rs | 44 +++- buffa/src/table/size.rs | 4 +- buffa/src/table/tests.rs | 484 +++++++++++++++++++++++++++++++++++++- 7 files changed, 740 insertions(+), 19 deletions(-) create mode 100644 buffa/src/table/bridge.rs diff --git a/buffa/src/table/bridge.rs b/buffa/src/table/bridge.rs new file mode 100644 index 00000000..c3706217 --- /dev/null +++ b/buffa/src/table/bridge.rs @@ -0,0 +1,211 @@ +//! Message-typed fields whose message is not a table message. +//! +//! A table records, for each message field, how to size, write and decode the +//! child. When the child has a table, the interpreters recurse into it. When +//! it has none (it was generated with the unrolled codec, by another crate, or +//! is a well-known type) they call the child's [`Message`] impl through the +//! function pointers of a [`DynVt`], one per operation, compiled once per +//! child type. +//! +//! A pointer cannot be generic over the write sink, so the write function +//! takes the [`PreSized`] cursor that every `BufMut` is written through. Any +//! other sink gets the child's bytes from a scratch buffer, which costs one +//! copy of the child and, for a segmented sink such as [`Rope`](crate::Rope), +//! the reference-counted segments of large `bytes` fields inside it. + +use core::marker::PhantomData; + +use super::{decode, encode, size, MessageTable}; +use crate::encode_sink::{write_to_new_vec, PreSized}; +use crate::encoding::encode_varint; +use crate::{DecodeContext, DecodeError, EncodeSink, Message, SizeCache}; + +/// The operations of one child message type, reached through its [`Message`] +/// impl. Built by [`MsgVt::new_dyn`](super::MsgVt::new_dyn) and +/// [`RepVt::new_dyn`](super::RepVt::new_dyn). +pub(super) struct DynVt { + /// The encoded size of the child, recording nested sizes in the cache. + /// + /// # Safety + /// + /// The argument points to a live message of the type the vtable was built + /// for. + size: unsafe fn(*const u8, &mut SizeCache) -> u32, + /// Write the child through a cursor, consuming sizes from the cache. + /// + /// # Safety + /// + /// As for `size`. + write: unsafe fn(*const u8, &mut SizeCache, &mut PreSized<'_>), + /// Merge a length-prefixed encoding of the child from the front of the + /// buffer, as generated code does for a message field. + /// + /// # Safety + /// + /// As for `size`, and the argument is exclusive. + merge: unsafe fn(*mut u8, &mut &[u8], DecodeContext<'_>) -> Result<(), DecodeError>, +} + +/// # Safety +/// +/// `msg` points to a live `T`. +unsafe fn size_thunk(msg: *const u8, cache: &mut SizeCache) -> u32 { + // SAFETY: the caller passes a pointer to a live `T`. + unsafe { (*msg.cast::()).compute_size(cache) } +} + +/// # Safety +/// +/// `msg` points to a live `T`. +unsafe fn write_thunk(msg: *const u8, cache: &mut SizeCache, buf: &mut PreSized<'_>) { + // SAFETY: the caller passes a pointer to a live `T`. + unsafe { (*msg.cast::()).write_to(cache, buf) } +} + +/// # Safety +/// +/// `msg` points to a live `T` that nothing else accesses. +unsafe fn merge_thunk( + msg: *mut u8, + buf: &mut &[u8], + ctx: DecodeContext<'_>, +) -> Result<(), DecodeError> { + // SAFETY: the caller passes an exclusive pointer to a live `T`. + unsafe { (*msg.cast::()).merge_length_delimited(buf, ctx) } +} + +/// Holds the `'static` [`DynVt`] of `T`. +struct Thunks(PhantomData T>); + +impl Thunks { + const VT: &'static DynVt = &DynVt { + size: size_thunk::, + write: write_thunk::, + merge: merge_thunk::, + }; +} + +/// How to reach the message of a message field: through its table, or through +/// its [`Message`] impl. +#[derive(Clone, Copy)] +pub(super) enum Child { + Table(&'static MessageTable), + Dyn(&'static DynVt), +} + +impl Child { + pub(super) const fn of() -> Self { + Self::Dyn(Thunks::::VT) + } + + /// The encoded size of the message at `base`, recording nested sizes in + /// `cache`. + /// + /// # Safety + /// + /// `base` points to a live message of the type this was built for. + #[inline] + pub(super) unsafe fn compute_size(self, base: *const u8, cache: &mut SizeCache) -> u32 { + // SAFETY: forwarded from the caller. + unsafe { + match self { + Self::Table(table) => size::compute_size(table, base, cache), + Self::Dyn(vt) => (vt.size)(base, cache), + } + } + } + + /// Write the message at `base`, whose encoded size is `len`, to `buf`. + /// + /// # Safety + /// + /// `base` points to a live message of the type this was built for. + #[inline] + pub(super) unsafe fn write_to( + self, + base: *const u8, + len: u32, + cache: &mut SizeCache, + buf: &mut K, + ) { + // SAFETY: forwarded from the caller. + unsafe { + match self { + Self::Table(table) => encode::write_message(table, base, cache, buf), + Self::Dyn(vt) => write_dyn(vt, base, len, cache, buf), + } + } + } + + /// Decode a length-prefixed message from the front of `buf` into the + /// message at `base`. + /// + /// # Safety + /// + /// `base` points to a live message of the type this was built for, which + /// nothing else accesses. + #[inline] + pub(super) unsafe fn merge_sub( + self, + base: *mut u8, + buf: &mut &[u8], + ctx: DecodeContext<'_>, + ) -> Result<(), DecodeError> { + // SAFETY: forwarded from the caller. + unsafe { + match self { + Self::Table(table) => decode::merge_sub(table, base, buf, ctx), + Self::Dyn(vt) => (vt.merge)(base, buf, ctx), + } + } + } +} + +/// Write the message at `base`, of encoded size `len`, to `buf` through its +/// [`DynVt`]. +/// +/// # Safety +/// +/// `base` points to a live message of the type `vt` was built for. +#[inline(never)] +unsafe fn write_dyn( + vt: &DynVt, + base: *const u8, + len: u32, + cache: &mut SizeCache, + buf: &mut K, +) { + let mut through_cursor = |cursor: &mut PreSized<'_>| { + // SAFETY: forwarded from the caller. + unsafe { (vt.write)(base, &mut *cache, cursor) } + }; + if buf.__with_pre_sized(&mut through_cursor) { + return; + } + let len = len as usize; + let scratch = write_to_new_vec(len, |cursor| { + // SAFETY: forwarded from the caller. + unsafe { (vt.write)(base, &mut *cache, cursor) } + }); + crate::message::debug_assert_two_pass(scratch.len(), len); + buf.put_slice(&scratch); +} + +/// Write `len` and then the message at `base` to `buf`, as the value of a +/// length-delimited field whose tag has been written. +/// +/// # Safety +/// +/// `base` points to a live message of the type `child` was built for. +#[inline] +pub(super) unsafe fn write_field_value( + child: Child, + base: *const u8, + cache: &mut SizeCache, + buf: &mut K, +) { + let len = cache.consume_next(); + encode_varint(u64::from(len), buf); + // SAFETY: forwarded from the caller. + unsafe { child.write_to(base, len, cache, buf) }; +} diff --git a/buffa/src/table/decode.rs b/buffa/src/table/decode.rs index cce1bc19..1054ad01 100644 --- a/buffa/src/table/decode.rs +++ b/buffa/src/table/decode.rs @@ -121,7 +121,7 @@ unsafe fn merge_slice( /// # Safety /// /// `base` points to a live message of the type `table` describes. -unsafe fn merge_sub( +pub(super) unsafe fn merge_sub( table: &MessageTable, base: *mut u8, buf: &mut &[u8], @@ -351,7 +351,7 @@ unsafe fn merge_msg( let vt = table.rep_vt(e); ctx.register_element_memory(vt.size)?; let elem = (vt.push)(slot); - let decoded = merge_sub(vt.table, elem, buf, ctx); + let decoded = vt.child.merge_sub(elem, buf, ctx); if decoded.is_err() { // Like unrolled code, which decodes into a local and pushes // it only on success, leave no partial element behind. @@ -361,7 +361,7 @@ unsafe fn merge_msg( } else { let vt = table.msg_vt(e); let child = (vt.place)(slot); - merge_sub(vt.table, child, buf, ctx) + vt.child.merge_sub(child, buf, ctx) } } } diff --git a/buffa/src/table/encode.rs b/buffa/src/table/encode.rs index aeb8ef42..05039483 100644 --- a/buffa/src/table/encode.rs +++ b/buffa/src/table/encode.rs @@ -7,6 +7,7 @@ use super::{ REQUIRED, }; use crate::alloc::{string::String, vec::Vec}; +use super::bridge::write_field_value; use crate::encode_sink::PreSized; use crate::encoding::encode_varint; use crate::{types, EncodeSink, SizeCache, UnknownFields}; @@ -55,7 +56,7 @@ unsafe fn write_pre_sized( /// # Safety /// /// As for [`write_to`]. -unsafe fn write_message( +pub(super) unsafe fn write_message( table: &MessageTable, base: *const u8, cache: &mut SizeCache, @@ -314,16 +315,14 @@ unsafe fn write_msg( let (ptr, len) = (vt.parts)(slot); for i in 0..len { put_tag(e, buf); - encode_varint(u64::from(cache.consume_next()), buf); - write_message(vt.table, ptr.add(i * vt.size), cache, buf); + write_field_value(vt.child, ptr.add(i * vt.size), cache, buf); } } else { let vt = table.msg_vt(e); let child = (vt.get)(slot); if !child.is_null() { put_tag(e, buf); - encode_varint(u64::from(cache.consume_next()), buf); - write_message(vt.table, child, cache, buf); + write_field_value(vt.child, child, cache, buf); } } } diff --git a/buffa/src/table/mod.rs b/buffa/src/table/mod.rs index 1fdd784c..8da60f96 100644 --- a/buffa/src/table/mod.rs +++ b/buffa/src/table/mod.rs @@ -298,6 +298,7 @@ pub trait KindSlot { kind_table!(define_kind); // After the macros above, whose textual scope covers only what follows them. +mod bridge; mod decode; mod encode; mod scalar; diff --git a/buffa/src/table/shape.rs b/buffa/src/table/shape.rs index 214d47ba..b968c14c 100644 --- a/buffa/src/table/shape.rs +++ b/buffa/src/table/shape.rs @@ -4,9 +4,10 @@ use core::marker::PhantomData; +use super::bridge::Child; use super::{Table, IMPLICIT, OPTIONAL, REPEATED}; use crate::alloc::vec::Vec; -use crate::{EnumValue, Enumeration, MessageField, ProtoBox}; +use crate::{EnumValue, Enumeration, Message, MessageField, ProtoBox}; // --------------------------------------------------------------------------- // Singular message fields @@ -36,10 +37,10 @@ impl> MsgSlot for MessageField { } } -/// Descriptor of a singular message field: the child's table and how to reach -/// the child through the field's storage. +/// Descriptor of a singular message field: how to encode, size and decode the +/// child and how to reach it through the field's storage. pub struct MsgVt { - pub(super) table: &'static super::MessageTable, + pub(super) child: Child, /// The message in the field, created with its default if unset. /// /// # Safety @@ -78,7 +79,23 @@ impl MsgVt { #[must_use] pub const fn new(table: &'static Table) -> Self { Self { - table: &table.raw, + child: Child::Table(&table.raw), + place: place_impl::, + get: get_impl::, + } + } + + /// Describe a field of type `F` whose message has no table here: a + /// message generated with the unrolled codec, by another crate, or one of + /// the well-known types. The child is reached through its [`Message`] + /// impl, so it may use any codec. + #[must_use] + pub const fn new_dyn() -> Self + where + F::Msg: Message, + { + Self { + child: Child::of::(), place: place_impl::, get: get_impl::, } @@ -91,7 +108,7 @@ impl MsgVt { /// Descriptor of a repeated message field, a `Vec`. pub struct RepVt { - pub(super) table: &'static super::MessageTable, + pub(super) child: Child, /// The size in bytes of one element. pub(super) size: usize, /// Append a default element and return a pointer to it. @@ -148,7 +165,20 @@ impl RepVt { #[must_use] pub const fn new(table: &'static Table) -> Self { Self { - table: &table.raw, + child: Child::Table(&table.raw), + size: core::mem::size_of::(), + push: push_impl::, + pop: pop_impl::, + parts: parts_impl::, + } + } + + /// Describe a `Vec` field whose messages have no table here; see + /// [`MsgVt::new_dyn`]. + #[must_use] + pub const fn new_dyn() -> Self { + Self { + child: Child::of::(), size: core::mem::size_of::(), push: push_impl::, pop: pop_impl::, diff --git a/buffa/src/table/size.rs b/buffa/src/table/size.rs index e2430f32..f983cfb8 100644 --- a/buffa/src/table/size.rs +++ b/buffa/src/table/size.rs @@ -233,7 +233,7 @@ unsafe fn size_msg( let mut size = 0; for i in 0..len { let idx = cache.reserve(); - let inner = compute_size(vt.table, ptr.add(i * vt.size), cache); + let inner = vt.child.compute_size(ptr.add(i * vt.size), cache); cache.set(idx, inner); size += tl + varint_len(u64::from(inner)) as u64 + u64::from(inner); } @@ -245,7 +245,7 @@ unsafe fn size_msg( return 0; } let idx = cache.reserve(); - let inner = compute_size(vt.table, child, cache); + let inner = vt.child.compute_size(child, cache); cache.set(idx, inner); tl + varint_len(u64::from(inner)) as u64 + u64::from(inner) } diff --git a/buffa/src/table/tests.rs b/buffa/src/table/tests.rs index 05346705..38094098 100644 --- a/buffa/src/table/tests.rs +++ b/buffa/src/table/tests.rs @@ -3,9 +3,10 @@ use super::*; use crate::alloc::{string::String, vec, vec::Vec}; use crate::bytes::Buf; +use crate::encoding::WireType; use crate::{ - DecodeOptions, EnumValue, Enumeration, Inline, Message, MessageField, Rope, UnknownFieldData, - UnknownFields, + types, DecodeOptions, EnumValue, Enumeration, Inline, Message, MessageField, Rope, + UnknownFieldData, UnknownFields, }; // --------------------------------------------------------------------------- @@ -1001,3 +1002,482 @@ fn writing_through_a_nested_cursor_continues_after_the_bytes_already_written() { msg.encode(&mut rope); assert_eq!(&rope.to_contiguous_bytes()[..], &expected[..]); } + +// --------------------------------------------------------------------------- +// Children without a table +// --------------------------------------------------------------------------- + +/// `int32 n = 1; string s = 2; Hand next = 3; Inner inner = 4;`, written by +/// hand as the unrolled codec writes a message, so that it has no table. +#[derive(Clone, Debug, Default, PartialEq)] +struct Hand { + n: i32, + s: String, + next: MessageField, + inner: MessageField, + unknown: UnknownFields, +} + +crate::impl_default_instance!(Hand); + +impl Message for Hand { + fn compute_size(&self, cache: &mut SizeCache) -> u32 { + let mut size = 0u64; + if self.n != 0 { + size += 1 + types::int32_encoded_len(self.n) as u64; + } + if !self.s.is_empty() { + size += 1 + types::string_encoded_len(&self.s) as u64; + } + if let Some(next) = self.next.as_option() { + let slot = cache.reserve(); + let inner = next.compute_size(cache); + cache.set(slot, inner); + size += 1 + crate::encoding::varint_len(u64::from(inner)) as u64 + u64::from(inner); + } + if let Some(inner) = self.inner.as_option() { + let slot = cache.reserve(); + let len = inner.compute_size(cache); + cache.set(slot, len); + size += 1 + crate::encoding::varint_len(u64::from(len)) as u64 + u64::from(len); + } + size += self.unknown.encoded_len() as u64; + crate::saturate_size(size) + } + + fn write_to(&self, cache: &mut SizeCache, buf: &mut impl EncodeSink) { + if self.n != 0 { + types::put_int32_field(1, self.n, buf); + } + if !self.s.is_empty() { + types::put_string_field(2, &self.s, buf); + } + if let Some(next) = self.next.as_option() { + buf.put_u8(0x1a); + crate::encoding::encode_varint(u64::from(cache.consume_next()), buf); + next.write_to(cache, buf); + } + if let Some(inner) = self.inner.as_option() { + buf.put_u8(0x22); + crate::encoding::encode_varint(u64::from(cache.consume_next()), buf); + inner.write_to(cache, buf); + } + self.unknown.write_to(buf); + } + + fn merge_field( + &mut self, + tag: Tag, + buf: &mut impl Buf, + ctx: DecodeContext<'_>, + ) -> Result<(), DecodeError> { + match tag.field_number() { + 1 => { + crate::encoding::check_wire_type(tag, WireType::Varint)?; + self.n = types::decode_int32(buf)?; + } + 2 => { + crate::encoding::check_wire_type(tag, WireType::LengthDelimited)?; + self.s = types::decode_string(buf)?; + } + 3 => { + crate::encoding::check_wire_type(tag, WireType::LengthDelimited)?; + self.next + .get_or_insert_default() + .merge_length_delimited(buf, ctx)?; + } + 4 => { + crate::encoding::check_wire_type(tag, WireType::LengthDelimited)?; + self.inner + .get_or_insert_default() + .merge_length_delimited(buf, ctx)?; + } + _ => self + .unknown + .push(crate::encoding::decode_unknown_field(tag, buf, ctx)?), + } + Ok(()) + } + + fn clear(&mut self) { + *self = Self::default(); + } +} + +/// A table message whose children are reached through `Message`: a hand-written +/// message and a table message, singular, repeated and inline, next to one +/// child reached through its table. +#[derive(Clone, Debug, Default, PartialEq)] +struct Bridged { + hand: MessageField, + hands: Vec, + tabled: MessageField, + tabled_list: Vec, + direct: MessageField, + hand_inline: MessageField>, + tail: i32, + unknown: UnknownFields, +} + +static BRIDGED: Table = crate::__table!( + Bridged, + abi = ABI, + entries = [ + crate::__table_entry!( + Bridged, + hand, + MsgSingular, + 1, + aux = 0, + slot = MessageField + ), + crate::__table_entry!(Bridged, hands, MsgRepeated, 2, aux = 1, slot = Vec), + crate::__table_entry!( + Bridged, + tabled, + MsgSingular, + 3, + aux = 2, + slot = MessageField + ), + crate::__table_entry!( + Bridged, + tabled_list, + MsgRepeated, + 4, + aux = 3, + slot = Vec + ), + crate::__table_entry!( + Bridged, + direct, + MsgSingular, + 5, + aux = 4, + slot = MessageField + ), + crate::__table_entry!( + Bridged, + hand_inline, + MsgSingular, + 6, + aux = 5, + slot = MessageField> + ), + crate::__table_entry!(Bridged, tail, Int32Implicit, 7), + ], + dense = &dense::<8>(&[1, 2, 3, 4, 5, 6, 7]), + aux = [ + Aux::Msg(&MsgVt::new_dyn::>()), + Aux::Rep(&RepVt::new_dyn::()), + Aux::Msg(&MsgVt::new_dyn::>()), + Aux::Rep(&RepVt::new_dyn::()), + Aux::Msg(&MsgVt::new::>(&INNER)), + Aux::Msg(&MsgVt::new_dyn::>>()), + ], + unknown = unknown, +); + +table_message!(Bridged, BRIDGED); + +fn hand(n: i32, s: &str) -> Hand { + Hand { + n, + s: s.into(), + ..Hand::default() + } +} + +fn bridged() -> Bridged { + let mut deep = hand(1, "deep"); + deep.next = MessageField::some(hand(2, "")); + deep.inner = MessageField::some(Inner { + id: 3, + label: "tabled inside".into(), + next: MessageField::some(Inner { + id: 4, + ..Inner::default() + }), + ..Inner::default() + }); + let mut with_unknown = Inner { + id: 9, + ..Inner::default() + }; + with_unknown + .unknown + .push(crate::UnknownField { + number: 900, + data: UnknownFieldData::Varint(5), + }); + Bridged { + hand: MessageField::some(deep), + hands: vec![hand(7, "a"), Hand::default(), hand(0, "c")], + tabled: MessageField::some(with_unknown), + tabled_list: vec![ + Inner { + id: 1, + ..Inner::default() + }, + Inner::default(), + ], + direct: MessageField::some(Inner { + label: "direct".into(), + ..Inner::default() + }), + hand_inline: MessageField::some(hand(-1, "inline")), + tail: 70, + unknown: UnknownFields::new(), + } +} + +#[test] +fn a_child_without_a_table_encodes_the_bytes_a_table_child_does() { + let child = Inner { + id: 5, + label: "same".into(), + ..Inner::default() + }; + let through_message = Bridged { + tabled: MessageField::some(child.clone()), + ..Bridged::default() + } + .encode_to_vec(); + let through_table = Bridged { + direct: MessageField::some(child.clone()), + ..Bridged::default() + } + .encode_to_vec(); + // Field 3 against field 5: only the tag differs. + assert_eq!(through_message[0], 0x1a); + assert_eq!(through_table[0], 0x2a); + assert_eq!(through_message[1..], through_table[1..]); + + let wire = Bridged { + hand: MessageField::some(hand(1, "a")), + hands: vec![hand(2, "")], + tail: 3, + ..Bridged::default() + } + .encode_to_vec(); + let expected: &[u8] = &[ + 0x0a, 0x05, 0x08, 0x01, 0x12, 0x01, b'a', // 1: {n: 1, s: "a"} + 0x12, 0x02, 0x08, 0x02, // 2: {n: 2} + 0x38, 0x03, // 7: 3 + ]; + assert_eq!(wire, expected); +} + +#[test] +fn children_without_a_table_round_trip() { + let msg = bridged(); + let bytes = msg.encode_to_vec(); + assert_eq!(bytes.len() as u32, msg.encoded_len()); + assert_eq!(Bridged::decode_from_slice(&bytes).unwrap(), msg); + assert_eq!( + Bridged::decode_from_slice(&Bridged::default().encode_to_vec()).unwrap(), + Bridged::default() + ); +} + +#[test] +fn a_child_without_a_table_reaches_every_sink() { + let msg = bridged(); + let expected = msg.encode_to_vec(); + + let mut rope = Rope::new(); + msg.encode(&mut rope); + assert_eq!(&rope.to_contiguous_bytes()[..], &expected[..]); + + let mut bytes_mut = crate::bytes::BytesMut::new(); + msg.encode(&mut bytes_mut); + assert_eq!(&bytes_mut[..], &expected[..]); + + let mut roomy = Vec::with_capacity(expected.len()); + msg.encode(&mut roomy); + assert_eq!(roomy, expected); + + // A sink whose chunk is shorter than the message is written through a + // scratch buffer first, and so is every message in a Rope. + let mut small = crate::bytes::BytesMut::with_capacity(1); + msg.encode_length_delimited(&mut small); + let mut framed = Vec::new(); + crate::encoding::encode_varint(expected.len() as u64, &mut framed); + framed.extend_from_slice(&expected); + assert_eq!(&small[..], &framed[..]); +} + +#[test] +fn children_without_a_table_decode_from_a_non_contiguous_buffer() { + let msg = bridged(); + let bytes = msg.encode_to_vec(); + for split in 1..bytes.len() { + let (head, tail) = bytes.split_at(split); + let mut chained = head.chain(tail); + let mut decoded = Bridged::default(); + with_ctx(|ctx| decoded.merge(&mut chained, ctx)).unwrap(); + assert_eq!(decoded, msg, "split at {split}"); + } +} + +#[test] +fn a_singular_child_without_a_table_merges() { + let mut msg = Bridged::default(); + for wire in [ + // hand {n = 1, s = "x"}, then hand {s = "y", next = {n = 2}}. + &[0x0a, 0x05, 0x08, 0x01, 0x12, 0x01, b'x'][..], + &[0x0a, 0x07, 0x12, 0x01, b'y', 0x1a, 0x02, 0x08, 0x02][..], + ] { + with_ctx(|ctx| msg.merge(&mut &wire[..], ctx)).unwrap(); + } + let hand = msg.hand.as_option().unwrap(); + assert_eq!((hand.n, hand.s.as_str()), (1, "y")); + assert_eq!(hand.next.as_option().unwrap().n, 2); +} + +#[test] +fn unknown_fields_in_a_child_without_a_table_are_kept() { + let msg = bridged(); + let decoded = Bridged::decode_from_slice(&msg.encode_to_vec()).unwrap(); + let tabled = decoded.tabled.as_option().unwrap(); + assert_eq!(tabled.unknown.iter().count(), 1); +} + +#[test] +fn the_recursion_limit_applies_through_a_child_without_a_table() { + let wrap = |field: u8, inner: Vec| { + let mut wire = vec![field]; + crate::encoding::encode_varint(inner.len() as u64, &mut wire); + wire.extend(inner); + wire + }; + // The child is a hand-written message, so its own decode counts the depth. + assert!(Bridged::decode_from_slice(&wrap(0x0a, nested(50))).is_ok()); + assert!(matches!( + Bridged::decode_from_slice(&wrap(0x0a, nested(150))), + Err(DecodeError::RecursionLimitExceeded) + )); + // And a table child reached through `Message` counts it too. + assert!(Bridged::decode_from_slice(&wrap(0x1a, nested(50))).is_ok()); + assert!(matches!( + Bridged::decode_from_slice(&wrap(0x1a, nested(150))), + Err(DecodeError::RecursionLimitExceeded) + )); + assert!(matches!( + DecodeOptions::new() + .with_recursion_limit(10) + .decode_from_slice::(&wrap(0x0a, nested(50))), + Err(DecodeError::RecursionLimitExceeded) + )); +} + +#[test] +fn the_element_memory_limit_applies_to_repeated_children_without_a_table() { + // 1000 empty elements of `hands` (2), then of `tabled_list` (4). + let hands: Vec = (0..1000).flat_map(|_| [0x12, 0x00]).collect(); + let tabled: Vec = (0..1000).flat_map(|_| [0x22, 0x00]).collect(); + for wire in [&hands, &tabled] { + assert!(Bridged::decode_from_slice(wire).is_ok()); + assert!(matches!( + DecodeOptions::new() + .with_element_memory_limit(100) + .decode_from_slice::(wire), + Err(DecodeError::ElementMemoryLimitExceeded) + )); + } +} + +#[test] +fn a_repeated_child_without_a_table_that_fails_to_decode_is_not_kept() { + // `hands`: one valid element, then one whose string is not UTF-8. + let wire = [0x12, 0x02, 0x08, 0x01, 0x12, 0x03, 0x12, 0x01, 0xff]; + let mut msg = Bridged::default(); + let result = with_ctx(|ctx| msg.merge(&mut &wire[..], ctx)); + assert_eq!(result, Err(DecodeError::InvalidUtf8)); + assert_eq!(msg.hands.len(), 1); + assert_eq!(msg.hands[0].n, 1); +} + +#[test] +fn a_truncated_child_without_a_table_is_an_error_or_ends_on_a_field_boundary() { + let bytes = bridged().encode_to_vec(); + let mut ok = 0; + for end in 0..bytes.len() { + match Bridged::decode_from_slice(&bytes[..end]) { + Ok(_) => ok += 1, + Err(e) => assert!( + matches!(e, DecodeError::UnexpectedEof | DecodeError::VarintTooLong), + "prefix {end}: {e}" + ), + } + } + assert!(ok > 1 && ok < bytes.len() / 2); + assert!(Bridged::decode_from_slice(&bytes).is_ok()); +} + +#[test] +fn a_length_that_runs_past_a_child_without_a_table_is_an_error() { + // `hand` is 2 bytes and its string declares 5. + assert_eq!( + Bridged::decode_from_slice(&[0x0a, 0x02, 0x12, 0x05, b'a', b'b', b'c']), + Err(DecodeError::UnexpectedEof) + ); + // A child longer than the size limit. + assert_eq!( + Bridged::decode_from_slice(&[0x0a, 0x80, 0x80, 0x80, 0x80, 0x08]), + Err(DecodeError::MessageTooLarge) + ); +} + +#[test] +#[cfg(debug_assertions)] +#[should_panic(expected = "two-pass traversal mismatch")] +fn a_child_that_writes_what_it_did_not_size_is_caught_in_a_scratch_buffer() { + /// Sizes itself as empty and writes a byte. + #[derive(Clone, Default, PartialEq)] + struct Liar; + crate::impl_default_instance!(Liar); + impl Message for Liar { + fn compute_size(&self, _: &mut SizeCache) -> u32 { + 0 + } + fn write_to(&self, _: &mut SizeCache, buf: &mut impl EncodeSink) { + buf.put_u8(1); + } + fn merge_field( + &mut self, + _: Tag, + _: &mut impl Buf, + _: DecodeContext<'_>, + ) -> Result<(), DecodeError> { + unreachable!() + } + fn clear(&mut self) {} + } + #[derive(Clone, Default, PartialEq)] + struct HoldsLiar { + liar: MessageField, + } + static HOLDS_LIAR: Table = crate::__table!( + HoldsLiar, + abi = ABI, + entries = [crate::__table_entry!( + HoldsLiar, + liar, + MsgSingular, + 1, + aux = 0, + slot = MessageField + )], + dense = &dense::<2>(&[1]), + aux = [Aux::Msg(&MsgVt::new_dyn::>())], + unknown = none, + ); + table_message!(HoldsLiar, HOLDS_LIAR); + let msg = HoldsLiar { + liar: MessageField::some(Liar), + }; + // Written into a Rope, the child goes through a scratch buffer sized by + // `compute_size`, which its write overruns. + msg.encode(&mut Rope::new()); +} From e49b2fdac8e79859e503e52ae83a82e525066897 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 11:47:49 -0700 Subject: [PATCH 02/11] codegen: let a table message hold messages that have no table --- .../added-20260923-codec-strategy-table.yaml | 2 +- DESIGN.md | 2 +- buffa-build/src/lib.rs | 11 +- buffa-codegen/src/lib.rs | 34 ++- buffa-codegen/src/table_codec.rs | 32 ++- buffa-codegen/src/table_plan.rs | 213 +++--------------- buffa-codegen/src/tests/table_codec.rs | 160 +++++++------ buffa/src/table/mod.rs | 6 + docs/guide.md | 8 +- 9 files changed, 160 insertions(+), 308 deletions(-) diff --git a/.changes/unreleased/added-20260923-codec-strategy-table.yaml b/.changes/unreleased/added-20260923-codec-strategy-table.yaml index 8f240a69..4186c03e 100644 --- a/.changes/unreleased/added-20260923-codec-strategy-table.yaml +++ b/.changes/unreleased/added-20260923-codec-strategy-table.yaml @@ -1,4 +1,4 @@ kind: Added body: |- - **Table-driven message codec** (#469, refs #463). `buffa_build::Config::codec_strategy(CodecStrategy::Table)` (plugin option `codec_strategy=table`, `CodeGenConfig::codec_strategy`) generates each message's binary `Message` implementation from a static table and interpreters that every message shares, instead of code specialised to the message's fields. It roughly halves the compiled size of a large schema at `opt-level = "z"`, and messages made of many small fields are slower. `CodecStrategy::Unrolled` stays the default. `codec_strategy_in(strategy, &[paths])` (plugin option `codec_strategy_in==`, repeatable) chooses the strategy for matching messages and the messages nested in them, and the last matching rule wins. The wire format does not change. A message that has a `oneof`, `map`, or group field, or holds a message that is not a table, stays unrolled, and `CodeGenWarning::TableCodecFallbackSummary` counts those; a rule that names such a message by its exact path is an error. The generated code needs Rust 1.77 or later, which `buffa-build` checks, and compiles in a crate with `#![forbid(unsafe_code)]`; see the guide's "Smaller generated code" section for the rest. + **Table-driven message codec** (#469, refs #463). `buffa_build::Config::codec_strategy(CodecStrategy::Table)` (plugin option `codec_strategy=table`, `CodeGenConfig::codec_strategy`) generates each message's binary `Message` implementation from a static table and interpreters that every message shares, instead of code specialised to the message's fields. It roughly halves the compiled size of a large schema at `opt-level = "z"`, and messages made of many small fields are slower. `CodecStrategy::Unrolled` stays the default. `codec_strategy_in(strategy, &[paths])` (plugin option `codec_strategy_in==`, repeatable) chooses the strategy for matching messages and the messages nested in them, and the last matching rule wins. The wire format does not change. A message that has a `oneof`, `map`, or group field stays unrolled, and `CodeGenWarning::TableCodecFallbackSummary` counts those; a table message may hold any message, and reaches one that has no table through its `Message` impl; a rule that names such a message by its exact path is an error. The generated code needs Rust 1.77 or later, which `buffa-build` checks, and compiles in a crate with `#![forbid(unsafe_code)]`; see the guide's "Smaller generated code" section for the rest. time: 2026-09-23T03:10:00+00:00 diff --git a/DESIGN.md b/DESIGN.md index ee41af9f..f58c9afa 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -566,7 +566,7 @@ Three decisions shape the runtime: - **The `unsafe` lives in `buffa`.** `__table!` and `__table_entry!` contain the `unsafe` blocks and witness each field's type against its kind, so a table that names the wrong kind for a field does not compile, and generated code compiles under `#![forbid(unsafe_code)]`. `Table::new` also checks the layout constants at compile time. The interpreters run under Miri in CI. - **The interpreters are not generic over the sink or the input where that is avoidable.** `Message::encode` and its siblings write any `BufMut` through one shared, non-generic cursor (`buffa/src/encode_sink.rs`), and decoding runs over a contiguous `&[u8]`, so the interpreters are compiled once in `buffa`, at its `opt-level`, and not once per caller. -- **A table refers to the tables of its children,** so a message can use the table only if every message it holds does. The planner in `buffa-codegen` (`table_plan.rs`) starts from the messages that asked for the table, removes those the interpreters cannot handle (oneofs, maps, groups and the types of group fields, `MessageSet`, extension ranges with JSON, custom string, bytes, or collection types), and then removes every message that holds a removed one, until none is left. A message that holds a type from another crate, such as a well-known type, is removed the same way, because the static table of that type is not visible. +- **A child reaches the interpreters through its table or its `Message` impl.** A message field's `Aux` descriptor holds either the child's `MessageTable`, which the interpreters recurse into, or a `DynVt` of three function pointers instantiated for the child type, `compute_size`, `write_to::`, and `merge_length_delimited`, which call the child's `Message` impl (`table/bridge.rs`). The second form serves an unrolled child, a child from another crate, and a well-known type, so a message can use the table whatever it holds. A function pointer cannot be generic over the sink, so the write thunk takes the `PreSized` cursor that every `BufMut` is written through, and any other sink receives the child from a scratch buffer, which costs one copy. The planner in `buffa-codegen` (`table_plan.rs`) selects the messages that asked for the table, drops those the interpreters cannot handle (oneofs, maps, groups and the types of group fields, `MessageSet`, extension ranges with JSON, custom string, bytes, or collection types), and the emitter names a child's static table only if the plan gave the child one. ### Owned decode: intentional throughput trade-offs diff --git a/buffa-build/src/lib.rs b/buffa-build/src/lib.rs index 78c64771..4376e2be 100644 --- a/buffa-build/src/lib.rs +++ b/buffa-build/src/lib.rs @@ -1601,13 +1601,10 @@ impl Config { /// names the message by its exact path. A rule that matches no message /// produces a warning. /// - /// A table message holds only table messages, and a rule does not extend - /// to the messages a message holds. Selecting a message with a rule - /// therefore also needs rules for everything it holds, unless the global - /// setting is [`CodecStrategy::Table`]. Choosing [`CodecStrategy::Unrolled`] - /// for a message keeps every message that holds it unrolled, with no - /// warning, and that usually includes the root message an application - /// encodes. + /// A rule does not extend to the messages a message holds. A table message + /// reaches a child that is not a table message, such as one set to + /// [`CodecStrategy::Unrolled`], through the child's `Message` impl, so + /// keeping a hot message unrolled does not affect the messages that hold it. #[must_use] pub fn codec_strategy_in(mut self, strategy: CodecStrategy, paths: &[impl AsRef]) -> Self { for raw in paths.iter().map(AsRef::as_ref) { diff --git a/buffa-codegen/src/lib.rs b/buffa-codegen/src/lib.rs index 0caa2227..403d17e4 100644 --- a/buffa-codegen/src/lib.rs +++ b/buffa-codegen/src/lib.rs @@ -1101,20 +1101,24 @@ pub enum CodecStrategy { /// unknown fields are preserved; /// - a message with a field of a non-default string, bytes, or collection /// type, such as `use_bytes_type`, `string_type`, `bytes_type`, and - /// `repeated_type` select; - /// - a message with a field whose message type stays `Unrolled` for any - /// reason, is not selected for the table, or is generated by another - /// crate, such as a well-known type. + /// `repeated_type` select. /// - /// A table message therefore holds only table messages, and selecting a - /// message does not select the messages it holds. + /// A table message may hold any message. It reaches a child that is a table + /// message through the child's table, and any other child, whether it is + /// [`Unrolled`](Self::Unrolled), generated by another crate, or a + /// well-known type, through the child's `Message` impl. Selecting a message + /// does not select the messages it holds, so a rule for a message and a rule + /// for its children are independent. /// - /// A table message differs from an unrolled one in three ways. A field that + /// A table message differs from an unrolled one in four ways. A field that /// declares a length past the end of its enclosing message fails at once /// with `DecodeError::UnexpectedEof`, where unrolled code can report a /// different error for the same rejected input. `clear()` resets the message /// to its default, which releases the capacity of its strings and vectors. /// Decoding gathers a `Buf` that is not contiguous into one buffer first. + /// Encoding a child that has no table into a sink that is not a `BufMut`, + /// such as `Rope`, stages the child in a buffer first, so the `bytes` fields + /// inside it are copied and not shared by reference count. /// /// A message that another crate or codegen run uses as the type of a group /// or editions `DELIMITED` field must not be selected, because @@ -1929,12 +1933,9 @@ pub struct CodeGenConfig { /// asked for something impossible. A rule that matches no generated message /// produces a [`CodeGenWarning::CodecStrategyRuleMatchedNothing`]. /// - /// A table message holds only table messages, and a rule does not extend to - /// the messages a message holds. A [`CodecStrategy::Table`] rule for a - /// message therefore needs rules for the messages it holds, or a global - /// [`CodecStrategy::Table`]; an exact-path rule fails without them. - /// Setting a message to [`CodecStrategy::Unrolled`] keeps every message that - /// holds it unrolled too, without a warning. + /// A rule does not extend to the messages a message holds: a table message + /// reaches a child that is not a table message through its `Message` impl, + /// so a rule for a message needs no rule for its children. pub codec_strategy_in: Vec<(String, CodecStrategy)>, } @@ -2390,15 +2391,12 @@ pub enum CodeGenWarning { /// [`codec_strategy`](CodeGenConfig::codec_strategy) or by a /// [`codec_strategy_in`](CodeGenConfig::codec_strategy_in) rule, cannot use /// the table and are generated [`CodecStrategy::Unrolled`]. One warning - /// covers the whole run. A message that falls back only because a message - /// it holds is set to `Unrolled` is not counted, because the setting is - /// the reason. + /// covers the whole run. #[non_exhaustive] TableCodecFallbackSummary { /// The number of messages that fell back. fallbacks: usize, - /// The number of messages selected for the table, not counting those - /// kept unrolled only by a message the user set to `Unrolled`. + /// The number of messages selected for the table. selected: usize, /// Why they fell back, most common reason first. reasons: Vec, diff --git a/buffa-codegen/src/table_codec.rs b/buffa-codegen/src/table_codec.rs index dd45c29d..339ce125 100644 --- a/buffa-codegen/src/table_codec.rs +++ b/buffa-codegen/src/table_codec.rs @@ -225,23 +225,35 @@ fn field_entry( match f.ty { Type::TYPE_MESSAGE => { let child = type_path("message")?; - let child_table = table_path(&unshortened_path()?)?; let child_ty = rust_path_to_tokens(&child); + let type_name = field + .type_name + .as_deref() + .ok_or(CodeGenError::MissingField("field.type_name"))?; + // A child with a table is reached through it, and any other, an + // unrolled message, a message of another crate or a well-known + // type, through its `Message` impl. + let child_table = if ctx.uses_table_codec(type_name) { + Some(table_path(&unshortened_path()?)?) + } else { + None + }; let (slot, aux_item) = if f.card == Card::Repeated { + let vt = match &child_table { + Some(table) => quote! { ::buffa::table::RepVt::new::<#child_ty>(&#table) }, + None => quote! { ::buffa::table::RepVt::new_dyn::<#child_ty>() }, + }; ( quote! { ::buffa::alloc::vec::Vec<#child_ty> }, - quote! { - ::buffa::table::Aux::Rep(&::buffa::table::RepVt::new::<#child_ty>(&#child_table)) - }, + quote! { ::buffa::table::Aux::Rep(&#vt) }, ) } else { let slot = classify_field(scope, msg, field, resolver)?.rust_type; - ( - slot.clone(), - quote! { - ::buffa::table::Aux::Msg(&::buffa::table::MsgVt::new::<#slot>(&#child_table)) - }, - ) + let vt = match &child_table { + Some(table) => quote! { ::buffa::table::MsgVt::new::<#slot>(&#table) }, + None => quote! { ::buffa::table::MsgVt::new_dyn::<#slot>() }, + }; + (slot, quote! { ::buffa::table::Aux::Msg(&#vt) }) }; let aux = aux_u16()?; Ok(( diff --git a/buffa-codegen/src/table_plan.rs b/buffa-codegen/src/table_plan.rs index 451c1b8d..530b92bb 100644 --- a/buffa-codegen/src/table_plan.rs +++ b/buffa-codegen/src/table_plan.rs @@ -2,13 +2,13 @@ //! table field looks like. //! //! A message can use the table if the interpreters in `buffa::table` cover -//! every field and every message it holds is a table message too, because a -//! table records the tables of its children. The set of table messages is the -//! largest set of requested, locally eligible messages closed under that -//! rule, found by removing messages until none is left holding a non-table -//! child. +//! every field it has. A message it holds does not have to be a table message: +//! a table reaches a child that has a table through it, and any other child +//! through its `Message` impl, so unrolled messages, messages of other crates +//! and well-known types all work as children. The set of table messages is +//! therefore the requested messages that are locally eligible. -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; use crate::context::CodeGenContext; use crate::features::ResolvedFeatures; @@ -86,21 +86,10 @@ fn type_stem(ty: Type, card: Card) -> Option<&'static str> { #[derive(Clone, Debug)] pub(crate) struct Ineligible { /// The reason in a few words, which the summary warning groups messages - /// by. For a message that holds another that cannot use the table, this - /// includes the other's reason. + /// by. pub(crate) reason: String, /// The reason for this message, naming the field or type. pub(crate) detail: String, - /// The message's own reason if it is the cause of a fallback, and - /// otherwise the cause of the message it holds, which is followed down to - /// the message that cannot use the table itself. - pub(crate) root: String, - /// Whether the fallback follows from a strategy the user chose (a message - /// it holds is set to `Unrolled`), so that it needs no warning. - pub(crate) silent: bool, - /// What to do about it when a rule that names the message exactly asked - /// for the table, if it differs from the general advice. - pub(crate) hint: Option, } /// An [`Ineligible`] whose reason needs no more detail. @@ -109,13 +98,9 @@ fn same(reason: &str) -> Ineligible { } fn ineligible(reason: impl Into, detail: impl Into) -> Ineligible { - let reason = reason.into(); Ineligible { - root: reason.clone(), - reason, + reason: reason.into(), detail: detail.into(), - silent: false, - hint: None, } } @@ -219,8 +204,6 @@ pub(crate) fn table_fields<'a>( /// One message of the run and what the plan needs to know about it. struct Candidate<'a> { fqn: String, - /// The proto paths of the message types of its fields. - children: Vec, fields: Result>, Ineligible>, } @@ -256,18 +239,14 @@ fn collect<'a>( .rust_type_relative(&fqn, package, 0) .is_some_and(|path| path.starts_with("::") || path.starts_with("crate::")); if !is_map_entry && !is_extern { - let mut children = Vec::new(); for f in &msg.field { - match effective_type(ctx, f, &features) { - Type::TYPE_MESSAGE => children.extend(f.type_name.clone()), - Type::TYPE_GROUP => group_types.extend(f.type_name.clone()), - _ => {} + if effective_type(ctx, f, &features) == Type::TYPE_GROUP { + group_types.extend(f.type_name.clone()); } } out.push(Candidate { fields: table_fields(ctx, msg, &fqn, &features), fqn: fqn.clone(), - children, }); } collect( @@ -295,64 +274,10 @@ impl TablePlan { } } -/// Why a message that holds `child` cannot use the table, when `child` has no -/// table. -fn child_without_table( - ctx: &CodeGenContext, - child: &str, - reasons: &HashMap<&str, Ineligible>, - generated: &HashSet<&str>, -) -> Ineligible { - if let Some(held) = reasons.get(child) { - return Ineligible { - reason: format!("holds a message that {}", held.root), - detail: format!( - "it has a field of message type `{child}`, which {}", - held.root - ), - root: held.root.clone(), - silent: held.silent, - hint: held.hint.clone(), - }; - } - if !generated.contains(child) { - return ineligible( - "holds a message that another crate or run generates", - format!("it has a field of message type `{child}`, which is not generated here"), - ); - } - // Not selected: the strategy for it is unrolled, by the user's rule or by - // the global default. - if ctx.codec_strategy_rule(child).is_some() { - Ineligible { - silent: true, - ..ineligible( - "holds a message set to the unrolled codec", - format!( - "it has a field of message type `{child}`, which is set to the unrolled codec" - ), - ) - } - } else { - Ineligible { - hint: Some(format!( - "Select `{child}` as well, and every message it holds (buffa-build: \ - `.codec_strategy_in(CodecStrategy::Table, &[\"{child}\"])`; plugin: \ - `codec_strategy_in={child}=table`)" - )), - ..ineligible( - "holds a message not selected for the table", - format!("it has a field of message type `{child}`, which is not selected for the table codec"), - ) - } - } -} - /// Decide which messages of `files_to_generate` use the table codec. /// /// Returns the plan and a summary warning about the messages that asked for -/// the table and cannot have it, unless the user's own choice of `Unrolled` -/// for a message they hold is the only reason. +/// the table and cannot have it. /// /// # Errors /// @@ -386,118 +311,42 @@ pub(crate) fn plan( &mut group_types, ); } - let generated: HashSet<&str> = candidates.iter().map(|c| c.fqn.as_str()).collect(); // The messages that asked for the table, each with the reason it cannot // have it, if there is one. - let mut reasons: HashMap<&str, Ineligible> = HashMap::new(); - let mut selected: Vec<&Candidate> = Vec::new(); + let mut tables = HashSet::new(); + let mut selected = 0; + let mut errors = Vec::new(); + let mut summary: Vec = Vec::new(); + let mut fallbacks = 0; for c in &candidates { if ctx.codec_strategy(&c.fqn) != CodecStrategy::Table { continue; } - selected.push(c); - if let Err(why) = &c.fields { - reasons.insert(&c.fqn, why.clone()); - } else if group_types.contains(&c.fqn) { - reasons.insert(&c.fqn, same("is the type of a group field")); - } - } - - // Remove every message that holds a child without a table, until none is - // left. - let mut remaining: HashSet<&str> = selected - .iter() - .filter(|c| !reasons.contains_key(c.fqn.as_str())) - .map(|c| c.fqn.as_str()) - .collect(); - loop { - let mut removed = Vec::new(); - for c in selected - .iter() - .filter(|c| remaining.contains(c.fqn.as_str())) - { - // The reason for the first child without a table that the user did - // not choose, if there is one, and otherwise for the first without - // one at all. - let whys: Vec = c - .children - .iter() - .filter(|ch| !remaining.contains(ch.as_str())) - .map(|child| child_without_table(ctx, child, &reasons, &generated)) - .collect(); - if let Some(why) = whys.iter().find(|w| !w.silent).or(whys.first()) { - removed.push((c.fqn.as_str(), why.clone())); - } - } - if removed.is_empty() { - break; - } - for (fqn, why) in removed { - remaining.remove(fqn); - reasons.insert(fqn, why); - } - } - - // A holder removed early may have looked like it fell back only because of - // a message the user set to `Unrolled`, before a message it also holds was - // itself removed for a reason of its own. Look again with the final - // reasons, until no silent holder changes. - loop { - let mut changed = Vec::new(); - for c in selected.iter().filter(|c| { - reasons - .get(c.fqn.as_str()) - .is_some_and(|why| why.silent && !c.children.is_empty()) - }) { - let loud = c - .children - .iter() - .filter(|ch| !remaining.contains(ch.as_str())) - .map(|child| child_without_table(ctx, child, &reasons, &generated)) - .find(|why| !why.silent); - if let Some(why) = loud { - changed.push((c.fqn.as_str(), why)); + selected += 1; + let why = match &c.fields { + Err(why) => why.clone(), + Ok(_) if group_types.contains(&c.fqn) => same("is the type of a group field"), + Ok(_) => { + tables.insert(c.fqn.clone()); + continue; } - } - if changed.is_empty() { - break; - } - reasons.extend(changed); - } - - // A rule that names a message exactly and cannot be honoured is an error, - // and all of them are reported together. The rest are counted by reason in - // one warning, except for the messages whose fallback the user chose. - let mut errors = Vec::new(); - let mut summary: Vec = Vec::new(); - let mut fallbacks = 0; - let mut held_back = 0; - for c in &selected { - let Some(why) = reasons.get(c.fqn.as_str()) else { - continue; }; + // A rule that names a message exactly and cannot be honoured is an + // error, and all of them are reported together. The rest are counted + // by reason in one warning. if let Some((rule, _)) = ctx.codec_strategy_rule(&c.fqn) { if *rule == c.fqn { - let advice = why.hint.clone().unwrap_or_else(|| { - format!( - "Select the unrolled codec for it instead (buffa-build: \ - `.codec_strategy_in(CodecStrategy::Unrolled, &[\"{rule}\"])`; plugin: \ - `codec_strategy_in={rule}=unrolled`), or remove the rule" - ) - }); errors.push(format!( "codec_strategy_in rule '{rule}' selects the table codec for a message that \ - cannot use it: {}. {advice}", + cannot use it: {}. Select the unrolled codec for it instead (buffa-build: \ + `.codec_strategy_in(CodecStrategy::Unrolled, &[\"{rule}\"])`; plugin: \ + `codec_strategy_in={rule}=unrolled`), or remove the rule", why.detail )); continue; } } - if why.silent { - held_back += 1; - continue; - } fallbacks += 1; let entry = match summary.iter().position(|r| r.reason == why.reason) { Some(index) => &mut summary[index], @@ -519,11 +368,9 @@ pub(crate) fn plan( summary.sort_by_key(|reason| std::cmp::Reverse(reason.messages.len())); warnings.push(CodeGenWarning::TableCodecFallbackSummary { fallbacks, - selected: selected.len() - held_back, + selected, reasons: summary, }); } - - let tables = remaining.into_iter().map(str::to_string).collect(); Ok((TablePlan { tables }, warnings)) } diff --git a/buffa-codegen/src/tests/table_codec.rs b/buffa-codegen/src/tests/table_codec.rs index 2366044e..ac3db75c 100644 --- a/buffa-codegen/src/tests/table_codec.rs +++ b/buffa-codegen/src/tests/table_codec.rs @@ -26,7 +26,8 @@ fn scalar(name: &str, number: i32, ty: Type) -> FieldDescriptorProto { /// Package `t` with: /// /// - `Plain`, `Leaf`, and `HasLeaf` (holds a `Leaf`), which can use the table; -/// - `Oneofy` (has a oneof) and `HasOneofy` (holds an `Oneofy`), which cannot; +/// - `Oneofy` (has a oneof), which cannot, and `HasOneofy` (holds an `Oneofy`), +/// which can, because a table message may hold a message that has no table; /// - `Outer` with a nested `Inner`, both plain. fn schema() -> FileDescriptorProto { let mut oneofy = message( @@ -144,22 +145,15 @@ fn the_global_setting_gives_a_table_to_every_message_that_can_use_one() { let (code, warnings) = run(&table_config(CodecStrategy::Table)).unwrap(); assert_eq!( tables(&code), - ["Plain", "Leaf", "HasLeaf", "Outer", "Inner"] + ["Plain", "Leaf", "HasLeaf", "HasOneofy", "Outer", "Inner"] ); - // The other two fall back, and one warning covers the run. + // The one with a oneof falls back, and one warning covers the run. let (counts, reasons) = summary(&warnings); - assert_eq!(counts, (2, 7)); - assert_eq!( - reasons, - [("has a oneof", 1), ("holds a message that has a oneof", 1),] - ); + assert_eq!(counts, (1, 7)); + assert_eq!(reasons, [("has a oneof", 1)]); let text = table_warnings(&warnings)[0].to_string(); - assert!(text.starts_with("2 of 7 messages selected for the table codec")); + assert!(text.starts_with("1 of 7 messages selected for the table codec")); assert!(text.contains("has a oneof (1: .t.Oneofy)"), "{text}"); - assert!( - text.contains("holds a message that has a oneof (1: .t.HasOneofy)"), - "{text}" - ); assert!(text.contains("codec_strategy_in==unrolled"), "{text}"); } @@ -190,6 +184,20 @@ fn a_child_message_is_referenced_through_its_own_table() { ); } +#[test] +fn a_child_without_a_table_is_reached_through_its_message_impl() { + let (code, _) = run(&table_config(CodecStrategy::Table)).unwrap(); + let code = squashed(&code); + // `HasOneofy` is a table message that holds `Oneofy`, which is unrolled. + let holder = code.split("static__BUFFA_TABLE_HasOneofy").nth(1).unwrap(); + let holder = holder.split("impl::buffa::MessageforHasOneofy").next().unwrap(); + assert!( + holder.contains("Aux::Msg(&::buffa::table::MsgVt::new_dyn::<::buffa::MessageField>>())"), + "{holder}" + ); + assert!(!holder.contains("__BUFFA_TABLE_Oneofy"), "{holder}"); +} + #[test] fn a_rule_selects_messages_when_the_global_setting_is_unrolled() { let config = CodeGenConfig { @@ -214,11 +222,11 @@ fn the_last_matching_rule_wins() { }; let (code, _) = run(&config).unwrap(); // `Outer` is unrolled, though its nested message is a table. - assert_eq!(tables(&code), ["Leaf", "HasLeaf", "Inner"]); + assert_eq!(tables(&code), ["Leaf", "HasLeaf", "HasOneofy", "Inner"]); } #[test] -fn an_exact_path_rule_for_a_message_that_holds_an_unrolled_child_is_an_error() { +fn a_message_can_have_a_table_when_it_holds_a_message_set_to_unrolled() { let config = CodeGenConfig { codec_strategy_in: vec![ (".t.HasLeaf".to_string(), CodecStrategy::Table), @@ -226,33 +234,29 @@ fn an_exact_path_rule_for_a_message_that_holds_an_unrolled_child_is_an_error() { ], ..Default::default() }; - // The exact-path rule for `HasLeaf` cannot be honoured: its `Leaf` has no - // table, so this is an error and not a warning. - let err = run(&config).unwrap_err().to_string(); - assert!( - err.contains("codec_strategy_in rule '.t.HasLeaf'") - && err.contains("`.t.Leaf`, which is set to the unrolled codec") - && err.contains("codec_strategy_in=.t.HasLeaf=unrolled"), - "{err}" - ); + let (code, warnings) = run(&config).unwrap(); + assert_eq!(tables(&code), ["HasLeaf"]); + assert!(table_warnings(&warnings).is_empty(), "{warnings:?}"); + assert!(squashed(&code).contains("MsgVt::new_dyn::<")); } #[test] -fn an_exact_path_rule_does_not_select_the_messages_the_message_holds() { - // The global setting stays unrolled, so `Leaf` has not been selected. +fn a_rule_for_a_message_does_not_select_the_messages_it_holds() { + // The global setting stays unrolled, so `Leaf` is not selected, and + // `HasLeaf` reaches it through its `Message` impl. let config = CodeGenConfig { codec_strategy_in: vec![(".t.HasLeaf".to_string(), CodecStrategy::Table)], ..Default::default() }; - let err = run(&config).unwrap_err().to_string(); - assert!( - err.contains("`.t.Leaf`, which is not selected for the table codec") - && err.contains("codec_strategy_in(CodecStrategy::Table, &[\".t.Leaf\"])") - && !err.contains("set to the unrolled"), - "{err}" - ); + let (code, warnings) = run(&config).unwrap(); + assert_eq!(tables(&code), ["HasLeaf"]); + assert!(table_warnings(&warnings).is_empty(), "{warnings:?}"); + let code = squashed(&code); + assert!(code.contains("MsgVt::new_dyn::<"), "{code}"); + assert!(!code.contains("(&__BUFFA_TABLE_Leaf)"), "{code}"); - // A rule for the child as well gives both a table. + // A rule for the child as well gives both a table, and the parent then + // uses the child's. let config = CodeGenConfig { codec_strategy_in: vec![ (".t.HasLeaf".to_string(), CodecStrategy::Table), @@ -263,20 +267,33 @@ fn an_exact_path_rule_does_not_select_the_messages_the_message_holds() { let (code, warnings) = run(&config).unwrap(); assert_eq!(tables(&code), ["Leaf", "HasLeaf"]); assert!(table_warnings(&warnings).is_empty()); + let code = squashed(&code); + assert!(code.contains("(&__BUFFA_TABLE_Leaf)"), "{code}"); + assert!(!code.contains("new_dyn"), "{code}"); } #[test] fn every_exact_path_rule_that_cannot_be_honoured_is_reported_at_once() { + let mut file = schema(); + let mut second = file.message_type[3].clone(); + second.name = Some("Oneofy2".to_string()); + file.message_type.push(second); let config = CodeGenConfig { codec_strategy_in: vec![ (".t.Oneofy".to_string(), CodecStrategy::Table), + (".t.Oneofy2".to_string(), CodecStrategy::Table), (".t.HasOneofy".to_string(), CodecStrategy::Table), ], ..Default::default() }; - let err = run(&config).unwrap_err().to_string(); + let err = generate_with_diagnostics(&[file], &["t.proto".to_string()], &config) + .unwrap_err() + .to_string(); + // `HasOneofy` can use the table, so it is not among them. assert!( - err.contains("rule '.t.Oneofy'") && err.contains("rule '.t.HasOneofy'"), + err.contains("rule '.t.Oneofy'") + && err.contains("rule '.t.Oneofy2'") + && !err.contains("rule '.t.HasOneofy'"), "{err}" ); } @@ -301,30 +318,28 @@ fn a_broad_rule_that_covers_such_a_message_only_warns() { let (code, warnings) = run(&config).unwrap(); assert_eq!( tables(&code), - ["Plain", "Leaf", "HasLeaf", "Outer", "Inner"] + ["Plain", "Leaf", "HasLeaf", "HasOneofy", "Outer", "Inner"] ); // Messages a rule selects are counted like the ones the global setting does. - assert_eq!(summary(&warnings).0, (2, 7)); + assert_eq!(summary(&warnings).0, (1, 7)); } #[test] -fn setting_a_message_to_unrolled_keeps_the_messages_that_hold_it_unrolled_quietly() { +fn setting_a_message_to_unrolled_does_not_affect_the_messages_that_hold_it() { let config = CodeGenConfig { codec_strategy_in: vec![(".t.Leaf".to_string(), CodecStrategy::Unrolled)], ..table_config(CodecStrategy::Table) }; let (code, warnings) = run(&config).unwrap(); - // `HasLeaf` holds the `Leaf` the user set to unrolled: not a table, and - // not in the warning either, which is left with the two that cannot. - assert_eq!(tables(&code), ["Plain", "Outer", "Inner"]); - let (counts, reasons) = summary(&warnings); - assert_eq!(counts, (2, 5)); - assert!( - reasons.iter().all(|(r, _)| !r.contains("set to")), - "{reasons:?}" + // `HasLeaf` holds the `Leaf` the user set to unrolled and has a table + // anyway. The warning counts the messages selected, which leaves out `Leaf`. + assert_eq!( + tables(&code), + ["Plain", "HasLeaf", "HasOneofy", "Outer", "Inner"] ); + assert_eq!(summary(&warnings).0, (1, 6)); - // With the two that cannot set to unrolled as well, nothing is left to say. + // With the one that cannot set to unrolled as well, nothing is left to say. let config = CodeGenConfig { codec_strategy_in: vec![ (".t.Leaf".to_string(), CodecStrategy::Unrolled), @@ -360,7 +375,7 @@ fn a_rule_that_matches_no_message_warns() { } #[test] -fn a_message_type_from_another_crate_is_not_a_table() { +fn a_message_type_from_another_crate_is_reached_through_its_message_impl() { // `HasLeaf.leaf` now names a type mapped to another crate. let mut file = schema(); file.message_type[2].field[0].type_name = Some(".other.Foreign".to_string()); @@ -376,18 +391,19 @@ fn a_message_type_from_another_crate_is_not_a_table() { let (files, warnings) = generate_with_diagnostics(&[file, other], &["t.proto".to_string()], &config).unwrap(); let code = joined(&files); - assert!(!tables(&code).contains(&"HasLeaf".to_string()), "{code}"); + assert!(tables(&code).contains(&"HasLeaf".to_string()), "{code}"); assert!(tables(&code).contains(&"Leaf".to_string())); - let text = table_warnings(&warnings)[0].to_string(); - assert!(text.contains(".t.HasLeaf"), "{text}"); + assert!(table_warnings(&warnings).len() == 1, "{warnings:?}"); + let code = squashed(&code); + let holder = code.split("static__BUFFA_TABLE_HasLeaf").nth(1).unwrap(); assert!( - text.contains("holds a message that another crate or run generates"), - "{text}" + holder.contains("MsgVt::new_dyn::<::buffa::MessageField<::other_crate::Foreign"), + "{holder}" ); } #[test] -fn a_child_mapped_to_another_crate_falls_back_though_this_run_generates_it_too() { +fn a_child_mapped_to_another_crate_is_not_a_table_though_this_run_generates_it_too() { let mut file = schema(); file.message_type[2].field[0].type_name = Some(".other.Foreign".to_string()); let other = FileDescriptorProto { @@ -400,19 +416,18 @@ fn a_child_mapped_to_another_crate_falls_back_though_this_run_generates_it_too() ..table_config(CodecStrategy::Table) }; // Both files are generated, but `HasLeaf` names `::other_crate::Foreign`, - // which has no table here, so this must be a fallback and not an error. - let (files, warnings) = generate_with_diagnostics( + // which has no table here, so it is reached through its `Message` impl and + // its table is not looked up, which would not build. + let (files, _) = generate_with_diagnostics( &[file, other], &["t.proto".to_string(), "other.proto".to_string()], &config, ) .unwrap(); let code = joined(&files); - assert!(!tables(&code).contains(&"HasLeaf".to_string()), "{code}"); + assert!(tables(&code).contains(&"HasLeaf".to_string()), "{code}"); assert!(!tables(&code).contains(&"Foreign".to_string())); - assert!(table_warnings(&warnings)[0] - .to_string() - .contains(".t.HasLeaf")); + assert!(!code.contains("__BUFFA_TABLE_Foreign"), "{code}"); } #[test] @@ -554,29 +569,6 @@ fn the_plan_judges_fields_under_the_same_features_as_the_generator() { assert_eq!(summary(&warnings).0, (2, 2)); } -#[test] -fn a_child_the_user_did_not_choose_does_not_hide_one_that_cannot_use_the_table() { - // `Both` holds `Leaf`, which the user sets to unrolled, and then - // `HasOneofy`, which cannot use the table. The first child alone would - // make the fallback silent, but `Both` falls back for the second too. - let mut file = schema(); - file.message_type.push(message( - "Both", - vec![ - message_field("leaf", 1, ".t.Leaf"), - message_field("o", 2, ".t.HasOneofy"), - ], - )); - let config = CodeGenConfig { - codec_strategy_in: vec![(".t.Leaf".to_string(), CodecStrategy::Unrolled)], - ..table_config(CodecStrategy::Table) - }; - let (_, warnings) = - generate_with_diagnostics(&[file], &["t.proto".to_string()], &config).unwrap(); - let text = table_warnings(&warnings)[0].to_string(); - assert!(text.contains(".t.Both"), "{text}"); -} - #[test] fn the_generated_abi_is_a_literal_that_matches_the_runtime() { // The runtime refuses a table generated for another ABI, so the generator diff --git a/buffa/src/table/mod.rs b/buffa/src/table/mod.rs index 8da60f96..bed8557f 100644 --- a/buffa/src/table/mod.rs +++ b/buffa/src/table/mod.rs @@ -40,6 +40,12 @@ //! group field must not use the table strategy. //! - [`clear`](crate::Message::clear) resets to `Default`, which releases //! allocations that unrolled code keeps. +//! - A child message that has no table, because it is unrolled, generated by +//! another crate or a well-known type, is reached through its +//! [`Message`](crate::Message) impl (see [`MsgVt::new_dyn`]), so it decodes +//! and encodes exactly as it does inside an unrolled parent. Encoding it into +//! a sink that is not a [`BufMut`](crate::bytes::BufMut) stages its bytes in a +//! buffer, which copies them. //! //! # Where the code is compiled //! diff --git a/docs/guide.md b/docs/guide.md index b0117437..9fc80c35 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -1113,13 +1113,14 @@ buffa_build::Config::new() .compile()?; ``` -A table holds only table messages, so a message that holds an unrolled one is unrolled too, and a `codec_strategy_in` rule for a message does not select the messages it holds: with the global setting left at `Unrolled`, select a message and everything it holds. In the example, every message that contains `.wa.Message` stays unrolled, and that usually includes the root message an application encodes; codegen does not warn about a fallback that follows from your own `Unrolled` rule. +A table message may hold any message. It reaches a child that has a table through it, and any other child (a message you set to `Unrolled`, one generated by another crate, or a well-known type such as `Timestamp`) through its `Message` impl, which costs a function call per child. A `codec_strategy_in` rule selects the message it names and the messages nested in it, and does not extend to the messages it holds. In the example, `.wa.Message` stays specialised and the messages that hold it use the table. -The option changes only the binary `Message` implementation. The wire format and the JSON, text, view, and reflection code are the same under both strategies, and a table message encodes to the same bytes and decodes the same accepted input as its unrolled twin. It differs in three ways: +The option changes only the binary `Message` implementation. The wire format and the JSON, text, view, and reflection code are the same under both strategies, and a table message encodes to the same bytes and decodes the same accepted input as its unrolled twin. It differs in four ways: - A field that declares a length past the end of its enclosing message fails at once with `DecodeError::UnexpectedEof`, where unrolled code reads on into the enclosing message and can report a different error for the same rejected input. - The table decodes from one contiguous slice, so a `Buf` that is not contiguous is gathered into one buffer first. `Message::merge_field` on a table message cannot gather, and returns `UnexpectedEof` for such a buffer; only code that calls it directly is affected, such as the default `merge_group`. A message that another crate or another codegen run uses as the type of a group or `DELIMITED` field must therefore stay `Unrolled`. Within one run, codegen keeps the type of a group field unrolled itself. - `clear()` resets a table message to `Default`, so it releases the capacity of its strings and vectors instead of keeping it. +- A child that has no table, encoded into a sink that is not a `BufMut`, such as `Rope`, is staged in a buffer first, so the `bytes` fields inside it are copied and not shared by reference count. These stay unrolled, whatever the setting: @@ -1127,8 +1128,7 @@ These stay unrolled, whatever the setting: - the message type of a group field; - a message that uses the `MessageSet` wire format; - a message with extension ranges, when JSON code is generated and unknown fields are preserved; -- a message with a field of a non-default string, bytes, or collection type, which `use_bytes_type`, `string_type`, `bytes_type`, and `repeated_type` select; -- a message that holds any message that stays unrolled, is not selected for the table, or is generated by another crate, such as a well-known type. +- a message with a field of a non-default string, bytes, or collection type, which `use_bytes_type`, `string_type`, `bytes_type`, and `repeated_type` select. Codegen prints one warning per run that counts the messages that fell back, groups them by reason, and names a few of each. Setting the messages that cause a fallback to `Unrolled` with `codec_strategy_in` silences it. A `codec_strategy_in` rule that selects the table for a message by its exact path, when the message cannot use it, is an error, because the rule asked for something impossible. From a81085855150bf943bec1e09c0861951d11f639b Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 11:47:49 -0700 Subject: [PATCH 03/11] buffa-test: compare table messages holding unrolled, extern and well-known children --- buffa-test/build.rs | 72 ++++++- buffa-test/protos/table_bridge.proto | 57 ++++++ buffa-test/src/lib.rs | 30 +++ buffa-test/src/tests/table_codec.rs | 290 ++++++++++++++++++++++++++- 4 files changed, 440 insertions(+), 9 deletions(-) create mode 100644 buffa-test/protos/table_bridge.proto diff --git a/buffa-test/build.rs b/buffa-test/build.rs index 59cbfee4..8fc2e9ee 100644 --- a/buffa-test/build.rs +++ b/buffa-test/build.rs @@ -1,8 +1,9 @@ /// Compile a schema twice, with its `package ` renamed to `u` and /// generated with the default unrolled codec, and to `t` and generated -/// with `codec_strategy = Table`. A test compares the two codecs on the same -/// schema. `file` names the schema in messages. -fn compile_both_codecs(file: &str, source: &str, base: &str) { +/// with `codec_strategy = Table`, except for the messages in `unrolled` (paths +/// below the package, such as `Hot`), which stay unrolled. A test compares the +/// two codecs on the same schema. `file` names the schema in messages. +fn compile_both_codecs(file: &str, source: &str, base: &str, unrolled: &[&str]) { let out = std::path::PathBuf::from(std::env::var("OUT_DIR").expect("OUT_DIR")); let package = format!("package {base};"); assert!(source.contains(&package), "{file} must declare `{package}`"); @@ -16,17 +17,64 @@ fn compile_both_codecs(file: &str, source: &str, base: &str) { source.replace(&package, &format!("package {base}{suffix};")), ) .expect("write renamed proto"); + let rules: Vec = unrolled + .iter() + .map(|path| format!(".{base}{suffix}.{path}")) + .collect(); buffa_build::Config::new() .files(&[renamed]) .includes(&[&out]) .generate_json(true) .generate_text(true) .codec_strategy(strategy) + .codec_strategy_in(buffa_build::CodecStrategy::Unrolled, &rules) .compile() .unwrap_or_else(|e| panic!("buffa_build failed for {file} ({suffix}): {e}")); } } +/// A package `xe` of table messages, and packages `xfu` (unrolled) and `xft` +/// (table) with messages that hold them, generated in their own runs with `xe` +/// mapped to the crate's `xe` module. A table message then holds messages from +/// another crate, whose table it cannot name. +fn compile_extern_children() { + let out = std::path::PathBuf::from(std::env::var("OUT_DIR").expect("OUT_DIR")); + let leaf = out.join("xe.proto"); + std::fs::write( + &leaf, + "syntax = \"proto3\";\npackage xe;\n\ + message Leaf { int32 x = 1; string s = 2; repeated Leaf kids = 3; }\n", + ) + .expect("write proto"); + buffa_build::Config::new() + .files(&[&leaf]) + .includes(&[&out]) + .codec_strategy(buffa_build::CodecStrategy::Table) + .compile() + .expect("buffa_build failed for xe.proto"); + for (suffix, strategy) in [ + ("u", buffa_build::CodecStrategy::Unrolled), + ("t", buffa_build::CodecStrategy::Table), + ] { + let holder = out.join(format!("xf{suffix}.proto")); + std::fs::write( + &holder, + format!( + "syntax = \"proto3\";\npackage xf{suffix};\nimport \"xe.proto\";\n\ + message Holder {{ xe.Leaf leaf = 1; repeated xe.Leaf leaves = 2; int32 tail = 3; }}\n" + ), + ) + .expect("write proto"); + buffa_build::Config::new() + .files(&[&holder]) + .includes(&[&out]) + .extern_path(".xe", "crate::xe") + .codec_strategy(strategy) + .compile() + .unwrap_or_else(|e| panic!("buffa_build failed for xf{suffix}.proto: {e}")); + } +} + /// Two packages, the second holding messages of the first, compiled three /// ways: unrolled (`xau`, `xbu`), table (`xat`, `xbt`), and table with /// `file_per_package` and `idiomatic_imports` (`xati`, `xbti`), which shortens @@ -167,19 +215,33 @@ fn main() { println!("cargo:rustc-check-cfg=cfg(has_table_codec)"); if rustc_minor() >= 77 { println!("cargo:rustc-cfg=has_table_codec"); - compile_both_codecs("table_codec.proto", &read_proto("table_codec.proto"), "tc"); + compile_both_codecs( + "table_codec.proto", + &read_proto("table_codec.proto"), + "tc", + &[], + ); compile_both_codecs( "table_codec2.proto", &read_proto("table_codec2.proto"), "tc2", + &[], ); compile_both_codecs( "table_codec3.proto", &read_proto("table_codec3.proto"), "tc3", + &[], + ); + compile_both_codecs("the generated wide schema", &wide_proto(), "wide", &[]); + compile_both_codecs( + "table_bridge.proto", + &read_proto("table_bridge.proto"), + "br", + &["Hot"], ); - compile_both_codecs("the generated wide schema", &wide_proto(), "wide"); compile_cross_package(); + compile_extern_children(); compile_table_with_options("table_codec.proto", "tc"); } diff --git a/buffa-test/protos/table_bridge.proto b/buffa-test/protos/table_bridge.proto new file mode 100644 index 00000000..3bfda171 --- /dev/null +++ b/buffa-test/protos/table_bridge.proto @@ -0,0 +1,57 @@ +syntax = "proto3"; + +// build.rs renames this package to `bru` (everything unrolled) and `brt` +// (`codec_strategy = Table`, with `Hot` set to unrolled by a rule), so a test +// can compare the two codecs. In `brt`, `Cold` and `Leaf` are table messages +// and `Hot` is not, so the messages hold each other across the two codecs in +// both directions. +package br; + +import "google/protobuf/any.proto"; +import "google/protobuf/duration.proto"; +import "google/protobuf/empty.proto"; +import "google/protobuf/field_mask.proto"; +import "google/protobuf/struct.proto"; +import "google/protobuf/timestamp.proto"; +import "google/protobuf/wrappers.proto"; + +message Leaf { + int32 x = 1; + string s = 2; + repeated int32 tags = 3; +} + +// Unrolled in `brt`, and holds a table message, and the table message that +// holds it. +message Hot { + int32 a = 1; + Leaf leaf = 2; + Cold back = 3; + repeated Leaf leaves = 4; + bytes blob = 5; +} + +message Cold { + Hot hot = 1; + repeated Hot hots = 2; + Leaf leaf = 3; + repeated Leaf leaves = 4; + int32 tail = 5; +} + +// Well-known types, which no run generates as table messages. +message Wkt { + google.protobuf.Timestamp ts = 1; + google.protobuf.Duration dur = 2; + google.protobuf.Any any = 3; + google.protobuf.Struct st = 4; + google.protobuf.Value val = 5; + google.protobuf.FieldMask mask = 6; + google.protobuf.Int32Value wrapped = 7; + google.protobuf.StringValue wrapped_s = 8; + google.protobuf.Empty empty = 9; + repeated google.protobuf.Timestamp times = 10; + repeated google.protobuf.Any anys = 11; + repeated google.protobuf.Value values = 12; + Cold cold = 13; +} diff --git a/buffa-test/src/lib.rs b/buffa-test/src/lib.rs index 8bc38f2c..34d97b57 100644 --- a/buffa-test/src/lib.rs +++ b/buffa-test/src/lib.rs @@ -1159,6 +1159,36 @@ pub mod tcx { buffa::include_proto!("tcx"); } +// `bru` is `table_bridge.proto` unrolled, and `brt` has the table codec except +// for `Hot`, so tables and unrolled messages hold each other. `xe` is a package +// of table messages that `xfu` (unrolled) and `xft` (table) hold through an +// `extern_path`. +#[allow(clippy::derivable_impls, clippy::match_single_binding)] +#[cfg(has_table_codec)] +pub mod bru { + buffa::include_proto!("bru"); +} +#[forbid(unsafe_code)] +#[allow(clippy::derivable_impls, clippy::match_single_binding)] +#[cfg(has_table_codec)] +pub mod brt { + buffa::include_proto!("brt"); +} +#[forbid(unsafe_code)] +#[cfg(has_table_codec)] +pub mod xe { + buffa::include_proto!("xe"); +} +#[cfg(has_table_codec)] +pub mod xfu { + buffa::include_proto!("xfu"); +} +#[forbid(unsafe_code)] +#[cfg(has_table_codec)] +pub mod xft { + buffa::include_proto!("xft"); +} + // Two packages, the second holding messages of the first: `xau`/`xbu` unrolled, // `xat`/`xbt` with the table codec, and `xti` with the table codec and // `file_per_package` with `idiomatic_imports`, which holds `xati` and `xbti`. diff --git a/buffa-test/src/tests/table_codec.rs b/buffa-test/src/tests/table_codec.rs index 3a59bb55..e31fee31 100644 --- a/buffa-test/src/tests/table_codec.rs +++ b/buffa-test/src/tests/table_codec.rs @@ -386,8 +386,8 @@ fn invalid_utf8_is_rejected() { #[test] fn messages_the_table_cannot_handle_still_work() { - // A oneof, a map, and the messages that hold them are unrolled, and a - // table message may sit next to them. + // A oneof and a map are unrolled, and the messages that hold them are + // tables that reach them through their `Message` impl. let with_oneof = crate::tct::WithOneof { choice: Some(crate::tct::with_oneof::Choice::B("x".into())), c: 4, @@ -761,8 +761,10 @@ fn messages_named_like_what_generated_code_uses_agree() { } #[test] -fn a_table_message_inside_an_unrolled_tree_agrees() { - // `Mixed.inner` is a table in `tct`, and `Mixed` and `HoldsOneof` are not. +fn unrolled_messages_inside_a_table_tree_agree() { + // `Mixed.inner` is a table in `tct`, and so are `Mixed` and `HoldsOneof`, + // which hold `WithOneof` and `WithMap`, which are not, and `WithOneof` + // holds a table message in a oneof variant. let wire = assert_same_codec(&shapes_u::mixed(), &shapes_t::mixed()); assert_same_chained::(&wire); let wire = assert_same_codec( @@ -922,6 +924,8 @@ fn the_messages_the_table_can_handle_use_it() { crate::tct::__BUFFA_TABLE_Entry, crate::tct::__BUFFA_TABLE_Aux, crate::tct::__BUFFA_TABLE_Keywords, + crate::tct::__BUFFA_TABLE_HoldsOneof, + crate::tct::__BUFFA_TABLE_Mixed, crate::tc2t::__BUFFA_TABLE_Req, crate::tc2t::__BUFFA_TABLE_AllRequired, crate::tc2t::__BUFFA_TABLE_AllRepeated, @@ -940,6 +944,11 @@ fn the_messages_the_table_can_handle_use_it() { crate::xti::xati::__BUFFA_TABLE_Wrap, crate::xti::xbti::__BUFFA_TABLE_Holder, crate::xti::xbti::holder::__BUFFA_TABLE_Sub, + crate::brt::__BUFFA_TABLE_Leaf, + crate::brt::__BUFFA_TABLE_Cold, + crate::brt::__BUFFA_TABLE_Wkt, + crate::xe::__BUFFA_TABLE_Leaf, + crate::xft::__BUFFA_TABLE_Holder, ); } @@ -983,3 +992,276 @@ fn messages_held_across_packages_agree_in_every_layout() { wire ); } + +// --------------------------------------------------------------------------- +// Messages that hold messages without a table +// --------------------------------------------------------------------------- + +/// Builds the same values of `table_bridge.proto` in `$m`: `bru` or `brt`. +macro_rules! bridge_samples { + ($name:ident, $m:ident) => { + mod $name { + use crate::$m::{Cold, Hot, Leaf, Wkt}; + use buffa::MessageField; + use buffa_types::google::protobuf::{ + Any, Duration, Empty, FieldMask, Int32Value, StringValue, Struct, Timestamp, Value, + }; + + pub fn leaf(x: i32, s: &str, tags: &[i32]) -> Leaf { + Leaf { + x, + s: s.into(), + tags: tags.to_vec(), + ..Default::default() + } + } + + /// A `Hot`, which holds a `Cold` if `with_back`. + pub fn hot(a: i32, with_back: bool) -> Hot { + Hot { + a, + leaf: MessageField::some(leaf(a, "hot leaf", &[1, 2])), + back: if with_back { + MessageField::some(Cold { + tail: a + 100, + leaf: MessageField::some(leaf(a, "back", &[])), + ..Default::default() + }) + } else { + MessageField::none() + }, + leaves: vec![leaf(a + 1, "l", &[3]), Leaf::default()], + blob: vec![1, 2, 3], + ..Default::default() + } + } + + pub fn cold() -> Cold { + Cold { + hot: MessageField::some(hot(1, true)), + hots: vec![hot(2, false), Hot::default(), hot(3, true)], + leaf: MessageField::some(leaf(4, "c", &[5, 6])), + leaves: vec![leaf(6, "", &[]), Leaf::default()], + tail: 7, + ..Default::default() + } + } + + pub fn wkt() -> Wkt { + let mut st = Struct::new(); + st.insert("k", 1.5); + Wkt { + ts: MessageField::some(Timestamp::from_unix(1_700_000_000, 5)), + dur: MessageField::some(Duration::from_secs_nanos(-3, -4)), + any: MessageField::some(Any::pack( + &Timestamp::from_unix(1, 2), + "type.googleapis.com/google.protobuf.Timestamp", + )), + st: MessageField::some(st), + val: MessageField::some(Value::from("v")), + mask: MessageField::some(FieldMask::from_paths(["a.b", "c"])), + wrapped: MessageField::some(Int32Value::from(0)), + wrapped_s: MessageField::some(StringValue::from("w")), + empty: MessageField::some(Empty::default()), + times: vec![Timestamp::from_unix(1, 0), Timestamp::default()], + anys: vec![Any::default()], + values: vec![Value::from(true), Value::null(), Value::from(2.5)], + cold: MessageField::some(cold()), + ..Default::default() + } + } + } + }; +} + +bridge_samples!(bru_s, bru); +bridge_samples!(brt_s, brt); + +#[test] +fn messages_that_hold_each_other_across_both_codecs_agree() { + // `Cold` is a table and holds `Hot`, which is not and holds a `Cold`. + let wire = assert_same_codec(&bru_s::cold(), &brt_s::cold()); + assert_same_chained::(&wire); + assert_same_on_corrupt_input::(&wire, true); + + // The unrolled message as a root, holding table messages. + let wire = assert_same_codec(&bru_s::hot(9, true), &brt_s::hot(9, true)); + assert_same_chained::(&wire); + assert_same_on_corrupt_input::(&wire, true); +} + +#[test] +fn well_known_types_held_by_a_table_message_agree() { + let wire = assert_same_codec(&bru_s::wkt(), &brt_s::wkt()); + assert_same_chained::(&wire); + assert_same_on_corrupt_input::(&wire, true); + // Set to their defaults, they are still written, as empty records. + let wire = assert_same_codec( + &crate::bru::Wkt { + ts: buffa::MessageField::some(Default::default()), + wrapped: buffa::MessageField::some(Default::default()), + ..Default::default() + }, + &crate::brt::Wkt { + ts: buffa::MessageField::some(Default::default()), + wrapped: buffa::MessageField::some(Default::default()), + ..Default::default() + }, + ); + assert_eq!(wire, [0x0a, 0x00, 0x3a, 0x00]); +} + +#[test] +fn messages_from_another_crate_agree() { + macro_rules! sample { + ($m:ident) => {{ + let leaf = |x: i32, s: &str| crate::xe::Leaf { + x, + s: s.into(), + kids: vec![crate::xe::Leaf::default()], + ..Default::default() + }; + crate::$m::Holder { + leaf: buffa::MessageField::some(leaf(1, "a")), + leaves: vec![leaf(2, "b"), leaf(3, "")], + tail: 4, + ..Default::default() + } + }}; + } + let wire = assert_same_codec(&sample!(xfu), &sample!(xft)); + assert_same_chained::(&wire); + assert_same_on_corrupt_input::(&wire, true); +} + +/// The wire form of `Cold` with `hot` set to a `Hot` with `back` set to a +/// `Cold`, and so on, `pairs` times, innermost empty. +fn cold_hot_chain(pairs: usize) -> Vec { + let wrap = |tag: u8, inner: Vec| { + let mut wire = vec![tag]; + buffa::encoding::encode_varint(inner.len() as u64, &mut wire); + wire.extend(inner); + wire + }; + let mut wire = Vec::new(); + for _ in 0..pairs { + // `Hot.back` (3), then `Cold.hot` (1). + wire = wrap(0x0a, wrap(0x1a, wire)); + } + wire +} + +#[test] +fn nesting_through_both_codecs_is_limited_alike() { + for pairs in [1, 40, 49, 50, 51, 60] { + let wire = cold_hot_chain(pairs); + assert_same_decode::(&wire, false); + } + assert_eq!( + ::decode_from_slice(&cold_hot_chain(60)), + Err(DecodeError::RecursionLimitExceeded) + ); + assert!(::decode_from_slice(&cold_hot_chain(40)).is_ok()); + for limit in [1, 2, 3, 10] { + let wire = cold_hot_chain(6); + let decode = |r: Result, DecodeError>| r; + assert_eq!( + decode( + buffa::DecodeOptions::new() + .with_recursion_limit(limit) + .decode_from_slice::(&wire) + .map(|m| m.encode_to_vec()) + ), + decode( + buffa::DecodeOptions::new() + .with_recursion_limit(limit) + .decode_from_slice::(&wire) + .map(|m| m.encode_to_vec()) + ), + "limit {limit}" + ); + } +} + +#[test] +fn the_element_memory_limit_covers_repeated_children_without_a_table() { + // 1000 empty elements of `Cold.hots` (2), which is unrolled in `brt`, then of + // `Cold.leaves` (4), which is a table. + for tag in [0x12, 0x22] { + let wire: Vec = (0..1000).flat_map(|_| [tag, 0x00]).collect(); + for limit in [100, 10_000, 100_000_000] { + let decode_u = buffa::DecodeOptions::new() + .with_element_memory_limit(limit) + .decode_from_slice::(&wire) + .map(|m| m.encode_to_vec()); + let decode_t = buffa::DecodeOptions::new() + .with_element_memory_limit(limit) + .decode_from_slice::(&wire) + .map(|m| m.encode_to_vec()); + assert_eq!(decode_u, decode_t, "tag {tag:#x}, limit {limit}"); + } + } + let wire: Vec = (0..1000).flat_map(|_| [0x12, 0x00]).collect(); + assert_eq!( + buffa::DecodeOptions::new() + .with_element_memory_limit(100) + .decode_from_slice::(&wire) + .map(|_| ()), + Err(DecodeError::ElementMemoryLimitExceeded) + ); +} + +#[test] +fn a_child_split_across_occurrences_merges_alike() { + // `Cold.hot` (1) twice: {a = 1}, then {leaf = {x = 5}}; and `Cold.leaf` (3). + let wire = [ + 0x0a, 0x02, 0x08, 0x01, // hot { a = 1 } + 0x0a, 0x04, 0x12, 0x02, 0x08, 0x05, // hot { leaf { x = 5 } } + 0x1a, 0x02, 0x08, 0x02, // leaf { x = 2 } + 0x1a, 0x02, 0x12, 0x00, // leaf { s = "" } + ]; + assert_same_decode::(&wire, false); + let merged = ::decode_from_slice(&wire).unwrap(); + let hot = merged.hot.as_option().unwrap(); + assert_eq!((hot.a, hot.leaf.as_option().unwrap().x), (1, 5)); + assert_eq!(merged.leaf.as_option().unwrap().x, 2); +} + +#[test] +fn unknown_fields_in_a_child_without_a_table_are_kept() { + // `Cold.hot` holds `{a = 1, }`. + let wire = [0x0a, 0x05, 0x08, 0x01, 0xa0, 0x06, 0x03]; + assert_same_decode::(&wire, false); + let decoded = ::decode_from_slice(&wire).unwrap(); + assert_eq!( + decoded.hot.as_option().unwrap().__buffa_unknown_fields.len(), + 1 + ); + assert_eq!(decoded.encode_to_vec(), wire); +} + +#[test] +fn a_child_without_a_table_is_encoded_into_every_kind_of_sink() { + let mut cold = brt_s::cold(); + // Enough bytes in a child that has no table to fill several segments. + cold.hots[1].blob = vec![0xab; 64 * 1024]; + let expected = cold.encode_to_vec(); + assert_eq!(expected.len() as u32, cold.encoded_len()); + let mut unrolled = bru_s::cold(); + unrolled.hots[1].blob = vec![0xab; 64 * 1024]; + assert_eq!(unrolled.encode_to_vec(), expected); + + let mut rope = buffa::Rope::new(); + cold.encode(&mut rope); + assert_eq!(&rope.to_contiguous_bytes()[..], &expected[..]); + let mut bytes_mut = buffa::bytes::BytesMut::new(); + cold.encode(&mut bytes_mut); + assert_eq!(&bytes_mut[..], &expected[..]); + // Room for only a few bytes at a time, so the message is staged. + let mut chunked = buffa::bytes::BytesMut::with_capacity(1); + cold.encode_length_delimited(&mut chunked); + let mut framed = Vec::new(); + buffa::encoding::encode_varint(expected.len() as u64, &mut framed); + framed.extend_from_slice(&expected); + assert_eq!(&chunked[..], &framed[..]); +} From e9e95fec78ced5e5de55ff6ef383d9420ee3972d Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 11:59:18 -0700 Subject: [PATCH 04/11] table: format, and add the changelog fragment for children without a table --- .../unreleased/added-20260923-table-holds-any-message.yaml | 3 +++ buffa-codegen/src/tests/table_codec.rs | 5 ++++- buffa-test/src/tests/table_codec.rs | 7 ++++++- buffa/src/table/encode.rs | 2 +- 4 files changed, 14 insertions(+), 3 deletions(-) create mode 100644 .changes/unreleased/added-20260923-table-holds-any-message.yaml diff --git a/.changes/unreleased/added-20260923-table-holds-any-message.yaml b/.changes/unreleased/added-20260923-table-holds-any-message.yaml new file mode 100644 index 00000000..53d699f1 --- /dev/null +++ b/.changes/unreleased/added-20260923-table-holds-any-message.yaml @@ -0,0 +1,3 @@ +kind: Added +body: '**A table message can hold any message** (#NNN, refs #463). Under `CodecStrategy::Table`, a message no longer falls back to the unrolled codec because a message it holds has no table: a child that is set to `Unrolled`, generated by another crate, or a well-known type such as `Timestamp` is encoded, sized, and decoded through its `Message` impl, at the cost of a function call per child. Encoding such a child into a sink that is not a `BufMut`, such as `Rope`, stages it in a buffer, so the `bytes` fields inside it are copied instead of shared by reference count. `CodeGenWarning::TableCodecFallbackSummary` no longer lists messages that hold a message that is not a table, and setting a message to `Unrolled` no longer keeps the messages that hold it unrolled.' +time: 2026-09-23T11:59:12.960844535-07:00 diff --git a/buffa-codegen/src/tests/table_codec.rs b/buffa-codegen/src/tests/table_codec.rs index ac3db75c..f82ad9db 100644 --- a/buffa-codegen/src/tests/table_codec.rs +++ b/buffa-codegen/src/tests/table_codec.rs @@ -190,7 +190,10 @@ fn a_child_without_a_table_is_reached_through_its_message_impl() { let code = squashed(&code); // `HasOneofy` is a table message that holds `Oneofy`, which is unrolled. let holder = code.split("static__BUFFA_TABLE_HasOneofy").nth(1).unwrap(); - let holder = holder.split("impl::buffa::MessageforHasOneofy").next().unwrap(); + let holder = holder + .split("impl::buffa::MessageforHasOneofy") + .next() + .unwrap(); assert!( holder.contains("Aux::Msg(&::buffa::table::MsgVt::new_dyn::<::buffa::MessageField>>())"), "{holder}" diff --git a/buffa-test/src/tests/table_codec.rs b/buffa-test/src/tests/table_codec.rs index e31fee31..2970d22e 100644 --- a/buffa-test/src/tests/table_codec.rs +++ b/buffa-test/src/tests/table_codec.rs @@ -1234,7 +1234,12 @@ fn unknown_fields_in_a_child_without_a_table_are_kept() { assert_same_decode::(&wire, false); let decoded = ::decode_from_slice(&wire).unwrap(); assert_eq!( - decoded.hot.as_option().unwrap().__buffa_unknown_fields.len(), + decoded + .hot + .as_option() + .unwrap() + .__buffa_unknown_fields + .len(), 1 ); assert_eq!(decoded.encode_to_vec(), wire); diff --git a/buffa/src/table/encode.rs b/buffa/src/table/encode.rs index 05039483..fda7f6af 100644 --- a/buffa/src/table/encode.rs +++ b/buffa/src/table/encode.rs @@ -1,5 +1,6 @@ //! The write pass: [`write_to`] and its per-kind arms. +use super::bridge::write_field_value; use super::scalar::Sc; use super::{ Bool, Double, Entry, Fixed32, Fixed64, Float, Int32, Int64, Kind, MessageTable, Sfixed32, @@ -7,7 +8,6 @@ use super::{ REQUIRED, }; use crate::alloc::{string::String, vec::Vec}; -use super::bridge::write_field_value; use crate::encode_sink::PreSized; use crate::encoding::encode_varint; use crate::{types, EncodeSink, SizeCache, UnknownFields}; From 2b0748cea5f1628e53b01cee2ca6464959d74119 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 12:12:21 -0700 Subject: [PATCH 05/11] docs: quote the size of a real schema with the bridge --- docs/guide.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/guide.md b/docs/guide.md index 9fc80c35..390054a7 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -1100,7 +1100,7 @@ Buffa uses a two-pass model to avoid the exponential-time size computation that ### Smaller generated code: `codec_strategy` -By default every generated message contains its own size, write, and merge code, specialised to its fields. `CodecStrategy::Table` replaces it with one static table per message and interpreters in `buffa` that every message shares. On a schema of 334 messages and 3,477 fields, the compiled size at `opt-level = "z"` went from 1,644 KB to 817 KB (measured with oneofs and maps flattened, which the table cannot handle; see [#463](https://github.com/anthropics/buffa/issues/463) for the method). The cost is speed on messages made of many small fields, where encoding takes up to about 3.5 times as long as with the default `CodecStrategy::Unrolled` and decoding up to 1.6 times. Messages dominated by bulk data, such as large strings, bytes, and packed arrays, show no difference. +By default every generated message contains its own size, write, and merge code, specialised to its fields. `CodecStrategy::Table` replaces it with one static table per message and interpreters in `buffa` that every message shares. On a schema of 334 top-level messages and 3,477 fields, the code section of a binary built at `opt-level = "z"` went from 1,670 KB to 1,010 KB, with the 43 messages that have a `oneof` or a `map` still unrolled. The same schema with its oneofs and maps flattened, so that every message can use the table, went from 1,644 KB to 817 KB (see [#463](https://github.com/anthropics/buffa/issues/463) for the method). The cost is speed on messages made of many small fields, where encoding takes up to about 3.5 times as long as with the default `CodecStrategy::Unrolled` and decoding up to 1.6 times. Messages dominated by bulk data, such as large strings, bytes, and packed arrays, show no difference. ```rust,ignore // build.rs From ae5d792f3abfb5e259bf0648d258734c2baf75a9 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 12:40:49 -0700 Subject: [PATCH 06/11] table: rename the message-impl descriptors and correct their docs --- buffa-codegen/src/table_codec.rs | 4 +- buffa-codegen/src/tests/table_codec.rs | 10 +- buffa/src/table/bridge.rs | 28 +++-- buffa/src/table/mod.rs | 28 ++--- buffa/src/table/shape.rs | 22 ++-- buffa/src/table/tests.rs | 137 +++++++++++++++---------- 6 files changed, 136 insertions(+), 93 deletions(-) diff --git a/buffa-codegen/src/table_codec.rs b/buffa-codegen/src/table_codec.rs index 339ce125..fce8a257 100644 --- a/buffa-codegen/src/table_codec.rs +++ b/buffa-codegen/src/table_codec.rs @@ -241,7 +241,7 @@ fn field_entry( let (slot, aux_item) = if f.card == Card::Repeated { let vt = match &child_table { Some(table) => quote! { ::buffa::table::RepVt::new::<#child_ty>(&#table) }, - None => quote! { ::buffa::table::RepVt::new_dyn::<#child_ty>() }, + None => quote! { ::buffa::table::RepVt::new_via_message::<#child_ty>() }, }; ( quote! { ::buffa::alloc::vec::Vec<#child_ty> }, @@ -251,7 +251,7 @@ fn field_entry( let slot = classify_field(scope, msg, field, resolver)?.rust_type; let vt = match &child_table { Some(table) => quote! { ::buffa::table::MsgVt::new::<#slot>(&#table) }, - None => quote! { ::buffa::table::MsgVt::new_dyn::<#slot>() }, + None => quote! { ::buffa::table::MsgVt::new_via_message::<#slot>() }, }; (slot, quote! { ::buffa::table::Aux::Msg(&#vt) }) }; diff --git a/buffa-codegen/src/tests/table_codec.rs b/buffa-codegen/src/tests/table_codec.rs index f82ad9db..3eab4062 100644 --- a/buffa-codegen/src/tests/table_codec.rs +++ b/buffa-codegen/src/tests/table_codec.rs @@ -195,7 +195,7 @@ fn a_child_without_a_table_is_reached_through_its_message_impl() { .next() .unwrap(); assert!( - holder.contains("Aux::Msg(&::buffa::table::MsgVt::new_dyn::<::buffa::MessageField>>())"), + holder.contains("Aux::Msg(&::buffa::table::MsgVt::new_via_message::<::buffa::MessageField>>())"), "{holder}" ); assert!(!holder.contains("__BUFFA_TABLE_Oneofy"), "{holder}"); @@ -240,7 +240,7 @@ fn a_message_can_have_a_table_when_it_holds_a_message_set_to_unrolled() { let (code, warnings) = run(&config).unwrap(); assert_eq!(tables(&code), ["HasLeaf"]); assert!(table_warnings(&warnings).is_empty(), "{warnings:?}"); - assert!(squashed(&code).contains("MsgVt::new_dyn::<")); + assert!(squashed(&code).contains("MsgVt::new_via_message::<")); } #[test] @@ -255,7 +255,7 @@ fn a_rule_for_a_message_does_not_select_the_messages_it_holds() { assert_eq!(tables(&code), ["HasLeaf"]); assert!(table_warnings(&warnings).is_empty(), "{warnings:?}"); let code = squashed(&code); - assert!(code.contains("MsgVt::new_dyn::<"), "{code}"); + assert!(code.contains("MsgVt::new_via_message::<"), "{code}"); assert!(!code.contains("(&__BUFFA_TABLE_Leaf)"), "{code}"); // A rule for the child as well gives both a table, and the parent then @@ -272,7 +272,7 @@ fn a_rule_for_a_message_does_not_select_the_messages_it_holds() { assert!(table_warnings(&warnings).is_empty()); let code = squashed(&code); assert!(code.contains("(&__BUFFA_TABLE_Leaf)"), "{code}"); - assert!(!code.contains("new_dyn"), "{code}"); + assert!(!code.contains("new_via_message"), "{code}"); } #[test] @@ -400,7 +400,7 @@ fn a_message_type_from_another_crate_is_reached_through_its_message_impl() { let code = squashed(&code); let holder = code.split("static__BUFFA_TABLE_HasLeaf").nth(1).unwrap(); assert!( - holder.contains("MsgVt::new_dyn::<::buffa::MessageField<::other_crate::Foreign"), + holder.contains("MsgVt::new_via_message::<::buffa::MessageField<::other_crate::Foreign"), "{holder}" ); } diff --git a/buffa/src/table/bridge.rs b/buffa/src/table/bridge.rs index c3706217..abe532f2 100644 --- a/buffa/src/table/bridge.rs +++ b/buffa/src/table/bridge.rs @@ -1,17 +1,25 @@ //! Message-typed fields whose message is not a table message. //! -//! A table records, for each message field, how to size, write and decode the -//! child. When the child has a table, the interpreters recurse into it. When -//! it has none (it was generated with the unrolled codec, by another crate, or -//! is a well-known type) they call the child's [`Message`] impl through the -//! function pointers of a [`DynVt`], one per operation, compiled once per -//! child type. +//! A table records how to size, write and decode the child of each message +//! field. A child that has a table is reached through it. Any other child, an +//! unrolled message, one generated by another crate or a well-known type, is +//! reached through its [`Message`] impl by the function pointers of a +//! [`DynVt`], one per operation, compiled once per child type. It keeps its +//! own codec, so it writes the same bytes and accepts the same input as it +//! does inside an unrolled parent. //! //! A pointer cannot be generic over the write sink, so the write function -//! takes the [`PreSized`] cursor that every `BufMut` is written through. Any -//! other sink gets the child's bytes from a scratch buffer, which costs one -//! copy of the child and, for a segmented sink such as [`Rope`](crate::Rope), -//! the reference-counted segments of large `bytes` fields inside it. +//! takes the [`PreSized`] cursor, which `Message::encode` and its siblings +//! write every `BufMut` through. Any other sink gets the child from a scratch +//! `Vec` allocated for it, for each element of a repeated field, and copied +//! to the sink: a [`Rope`](crate::Rope), a sink defined outside this crate, +//! and a `BufMut` passed straight to `Message::write_to`. A `Rope` copies the +//! bytes again unless they lie in its backing buffer, and it cannot share +//! the `bytes` fields inside the child by reference count. +//! +//! Decoding reads a child from the slice of the nearest enclosing table +//! message, so the read of a child that overruns is bounded there and not by +//! the whole input. use core::marker::PhantomData; diff --git a/buffa/src/table/mod.rs b/buffa/src/table/mod.rs index bed8557f..00febef9 100644 --- a/buffa/src/table/mod.rs +++ b/buffa/src/table/mod.rs @@ -32,7 +32,11 @@ //! gathered into a buffer first, and a field that declares a length past //! the end of its enclosing message fails at once with //! [`DecodeError::UnexpectedEof`], where unrolled code reads on into the -//! enclosing message and can report a different error. +//! enclosing message and can report a different error. A child reached +//! through its [`Message`](crate::Message) impl is read from the slice of +//! the nearest enclosing table message, so a read that overruns it fails +//! at that message's end, and not where a tree of unrolled messages would +//! notice. //! - [`Table::merge_field`] decodes one field and cannot gather, so it //! returns [`DecodeError::UnexpectedEof`] for a buffer that is not one //! chunk. Only a caller that drives `merge_field` itself is affected, such @@ -40,12 +44,11 @@ //! group field must not use the table strategy. //! - [`clear`](crate::Message::clear) resets to `Default`, which releases //! allocations that unrolled code keeps. -//! - A child message that has no table, because it is unrolled, generated by -//! another crate or a well-known type, is reached through its -//! [`Message`](crate::Message) impl (see [`MsgVt::new_dyn`]), so it decodes -//! and encodes exactly as it does inside an unrolled parent. Encoding it into -//! a sink that is not a [`BufMut`](crate::bytes::BufMut) stages its bytes in a -//! buffer, which copies them. +//! - A child reached through its [`Message`](crate::Message) impl (see +//! [`MsgVt::new_via_message`]) has the same wire format and accepts the same +//! input, but it is staged in a scratch buffer and copied when it is written +//! to any sink other than the cursor that `Message::encode` and its siblings +//! write a [`BufMut`](crate::bytes::BufMut) through. //! //! # Where the code is compiled //! @@ -53,10 +56,10 @@ //! [`BufMut`](crate::bytes::BufMut) through `Message::encode` and its //! siblings are non-generic functions compiled in this crate, at this crate's //! optimisation level, once for all messages. A build can therefore optimise -//! this crate for speed and its own generated code for size. Encoding into a -//! sink that is not a `BufMut`, such as [`Rope`](crate::Rope), and the -//! generic wrappers around decoding are instantiated in the crate that calls -//! them. +//! this crate for speed and its own generated code for size. Encoding into +//! any other sink, such as a [`Rope`](crate::Rope) or a `BufMut` passed +//! straight to `Message::write_to`, and the generic wrappers around +//! decoding are instantiated in the crate that calls them. use core::marker::PhantomData; @@ -486,8 +489,7 @@ impl Table { /// `Bytes*`: `Vec`, `Option>` or `Vec>`; /// - `Enum*`: the storage the entry's [`EnumVt`] was built for; /// - `MsgSingular`: the storage the [`MsgVt`] was built for, and - /// `MsgRepeated`: a `Vec` of the messages the [`RepVt`]'s table - /// describes. + /// `MsgRepeated`: the `Vec` the [`RepVt`] was built for. /// /// `unknown`, if present, must be the offset of a field of type /// `UnknownFields`. The `__table_entry!` macro checks the field types diff --git a/buffa/src/table/shape.rs b/buffa/src/table/shape.rs index b968c14c..b329c495 100644 --- a/buffa/src/table/shape.rs +++ b/buffa/src/table/shape.rs @@ -75,7 +75,8 @@ unsafe fn get_impl(slot: *const u8) -> *const u8 { } impl MsgVt { - /// Describe a field of type `F`, whose messages `table` describes. + /// Describe a field of type `F`, whose message is a table message and + /// `table` is its table. #[must_use] pub const fn new(table: &'static Table) -> Self { Self { @@ -85,12 +86,12 @@ impl MsgVt { } } - /// Describe a field of type `F` whose message has no table here: a - /// message generated with the unrolled codec, by another crate, or one of - /// the well-known types. The child is reached through its [`Message`] - /// impl, so it may use any codec. + /// Describe a field of type `F` whose message is reached through its + /// [`Message`] impl, for a message whose table is not visible here. + /// Prefer [`MsgVt::new`] when it is, because the interpreters then decode + /// the child without a function call. #[must_use] - pub const fn new_dyn() -> Self + pub const fn new_via_message() -> Self where F::Msg: Message, { @@ -161,7 +162,8 @@ unsafe fn parts_impl(slot: *const u8) -> (*const u8, usize) { } impl RepVt { - /// Describe a `Vec` field, whose messages `table` describes. + /// Describe a `Vec` field whose messages are table messages, `table` + /// being their table. #[must_use] pub const fn new(table: &'static Table) -> Self { Self { @@ -173,10 +175,10 @@ impl RepVt { } } - /// Describe a `Vec` field whose messages have no table here; see - /// [`MsgVt::new_dyn`]. + /// Describe a `Vec` field whose messages are reached through their + /// [`Message`] impl; see [`MsgVt::new_via_message`]. #[must_use] - pub const fn new_dyn() -> Self { + pub const fn new_via_message() -> Self { Self { child: Child::of::(), size: core::mem::size_of::(), diff --git a/buffa/src/table/tests.rs b/buffa/src/table/tests.rs index 38094098..dee82f4a 100644 --- a/buffa/src/table/tests.rs +++ b/buffa/src/table/tests.rs @@ -1168,12 +1168,12 @@ static BRIDGED: Table = crate::__table!( ], dense = &dense::<8>(&[1, 2, 3, 4, 5, 6, 7]), aux = [ - Aux::Msg(&MsgVt::new_dyn::>()), - Aux::Rep(&RepVt::new_dyn::()), - Aux::Msg(&MsgVt::new_dyn::>()), - Aux::Rep(&RepVt::new_dyn::()), + Aux::Msg(&MsgVt::new_via_message::>()), + Aux::Rep(&RepVt::new_via_message::()), + Aux::Msg(&MsgVt::new_via_message::>()), + Aux::Rep(&RepVt::new_via_message::()), Aux::Msg(&MsgVt::new::>(&INNER)), - Aux::Msg(&MsgVt::new_dyn::>>()), + Aux::Msg(&MsgVt::new_via_message::>>()), ], unknown = unknown, ); @@ -1297,8 +1297,8 @@ fn a_child_without_a_table_reaches_every_sink() { msg.encode(&mut roomy); assert_eq!(roomy, expected); - // A sink whose chunk is shorter than the message is written through a - // scratch buffer first, and so is every message in a Rope. + // A sink whose chunk is shorter than the message receives the whole + // message from a scratch buffer. let mut small = crate::bytes::BytesMut::with_capacity(1); msg.encode_length_delimited(&mut small); let mut framed = Vec::new(); @@ -1429,55 +1429,86 @@ fn a_length_that_runs_past_a_child_without_a_table_is_an_error() { ); } -#[test] -#[cfg(debug_assertions)] -#[should_panic(expected = "two-pass traversal mismatch")] -fn a_child_that_writes_what_it_did_not_size_is_caught_in_a_scratch_buffer() { - /// Sizes itself as empty and writes a byte. - #[derive(Clone, Default, PartialEq)] - struct Liar; - crate::impl_default_instance!(Liar); - impl Message for Liar { - fn compute_size(&self, _: &mut SizeCache) -> u32 { - 0 - } - fn write_to(&self, _: &mut SizeCache, buf: &mut impl EncodeSink) { +/// Declares `size` bytes and writes `writes` of them. +#[derive(Clone, Default, PartialEq)] +struct Liar { + size: u32, + writes: u8, +} +crate::impl_default_instance!(Liar); + +impl Message for Liar { + fn compute_size(&self, _: &mut SizeCache) -> u32 { + self.size + } + fn write_to(&self, _: &mut SizeCache, buf: &mut impl EncodeSink) { + for _ in 0..self.writes { buf.put_u8(1); } - fn merge_field( - &mut self, - _: Tag, - _: &mut impl Buf, - _: DecodeContext<'_>, - ) -> Result<(), DecodeError> { - unreachable!() - } - fn clear(&mut self) {} } - #[derive(Clone, Default, PartialEq)] - struct HoldsLiar { - liar: MessageField, + fn merge_field( + &mut self, + _: Tag, + _: &mut impl Buf, + _: DecodeContext<'_>, + ) -> Result<(), DecodeError> { + unreachable!() } - static HOLDS_LIAR: Table = crate::__table!( + fn clear(&mut self) {} +} + +#[derive(Clone, Default, PartialEq)] +struct HoldsLiar { + liar: MessageField, +} + +static HOLDS_LIAR: Table = crate::__table!( + HoldsLiar, + abi = ABI, + entries = [crate::__table_entry!( HoldsLiar, - abi = ABI, - entries = [crate::__table_entry!( - HoldsLiar, - liar, - MsgSingular, - 1, - aux = 0, - slot = MessageField - )], - dense = &dense::<2>(&[1]), - aux = [Aux::Msg(&MsgVt::new_dyn::>())], - unknown = none, - ); - table_message!(HoldsLiar, HOLDS_LIAR); - let msg = HoldsLiar { - liar: MessageField::some(Liar), - }; - // Written into a Rope, the child goes through a scratch buffer sized by - // `compute_size`, which its write overruns. - msg.encode(&mut Rope::new()); + liar, + MsgSingular, + 1, + aux = 0, + slot = MessageField + )], + dense = &dense::<2>(&[1]), + aux = [Aux::Msg(&MsgVt::new_via_message::>())], + unknown = none, +); +table_message!(HoldsLiar, HOLDS_LIAR); + +fn holds_liar(size: u32, writes: u8) -> HoldsLiar { + HoldsLiar { + liar: MessageField::some(Liar { size, writes }), + } +} + +#[test] +#[should_panic(expected = "more bytes than compute_size declared")] +fn a_child_that_writes_more_than_it_sized_panics_in_the_scratch_buffer() { + // A Rope is not written through the cursor, so the child is staged in a + // buffer of the size `compute_size` gave, which the write overruns. + holds_liar(0, 1).encode(&mut Rope::new()); +} + +#[test] +#[cfg(debug_assertions)] +#[should_panic(expected = "different byte count than compute_size declared")] +fn a_child_that_writes_less_than_it_sized_panics_in_debug_builds() { + holds_liar(3, 1).encode(&mut Rope::new()); +} + +#[test] +fn a_child_can_be_written_by_calling_write_to_on_a_buffer() { + // `write_to` on a `Vec` is not the pre-sized path `encode` takes, so each + // child is staged and copied. The bytes are the same. + let msg = bridged(); + let mut cache = SizeCache::new(); + let size = msg.compute_size(&mut cache); + let mut out = Vec::new(); + msg.write_to(&mut cache, &mut out); + assert_eq!(out.len(), size as usize); + assert_eq!(out, msg.encode_to_vec()); } From 2c328f4cd242c9a9c01c433a7fb908946cf4cb54 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 12:49:47 -0700 Subject: [PATCH 07/11] codegen: keep the holders of a message with a non-default bytes type unrolled --- ...dded-20260923-table-holds-any-message.yaml | 3 - buffa-codegen/src/table_codec.rs | 25 +- buffa-codegen/src/table_plan.rs | 127 +++++++- buffa-codegen/src/tests/table_codec.rs | 270 +++++++++++++++++- 4 files changed, 395 insertions(+), 30 deletions(-) delete mode 100644 .changes/unreleased/added-20260923-table-holds-any-message.yaml diff --git a/.changes/unreleased/added-20260923-table-holds-any-message.yaml b/.changes/unreleased/added-20260923-table-holds-any-message.yaml deleted file mode 100644 index 53d699f1..00000000 --- a/.changes/unreleased/added-20260923-table-holds-any-message.yaml +++ /dev/null @@ -1,3 +0,0 @@ -kind: Added -body: '**A table message can hold any message** (#NNN, refs #463). Under `CodecStrategy::Table`, a message no longer falls back to the unrolled codec because a message it holds has no table: a child that is set to `Unrolled`, generated by another crate, or a well-known type such as `Timestamp` is encoded, sized, and decoded through its `Message` impl, at the cost of a function call per child. Encoding such a child into a sink that is not a `BufMut`, such as `Rope`, stages it in a buffer, so the `bytes` fields inside it are copied instead of shared by reference count. `CodeGenWarning::TableCodecFallbackSummary` no longer lists messages that hold a message that is not a table, and setting a message to `Unrolled` no longer keeps the messages that hold it unrolled.' -time: 2026-09-23T11:59:12.960844535-07:00 diff --git a/buffa-codegen/src/table_codec.rs b/buffa-codegen/src/table_codec.rs index fce8a257..5fa8cc5d 100644 --- a/buffa-codegen/src/table_codec.rs +++ b/buffa-codegen/src/table_codec.rs @@ -196,11 +196,14 @@ fn field_entry( )) }) }; - let type_path = |what: &str| -> Result { - let type_name = field + let type_name = || { + field .type_name .as_deref() - .ok_or(CodeGenError::MissingField("field.type_name"))?; + .ok_or(CodeGenError::MissingField("field.type_name")) + }; + let type_path = |what: &str| -> Result { + let type_name = type_name()?; ctx.rust_type_relative(type_name, current_package, nesting) .ok_or_else(|| CodeGenError::Other(format!("{what} type '{type_name}' not found"))) }; @@ -208,10 +211,7 @@ fn field_entry( // The table is not among the imports that `idiomatic_imports` shortens // paths with, so its path is built from the unshortened one. let unshortened_path = || -> Result { - let type_name = field - .type_name - .as_deref() - .ok_or(CodeGenError::MissingField("field.type_name"))?; + let type_name = type_name()?; let split = ctx .rust_type_relative_split(type_name, current_package, nesting) .ok_or_else(|| CodeGenError::Other(format!("message type '{type_name}' not found")))?; @@ -226,14 +226,9 @@ fn field_entry( Type::TYPE_MESSAGE => { let child = type_path("message")?; let child_ty = rust_path_to_tokens(&child); - let type_name = field - .type_name - .as_deref() - .ok_or(CodeGenError::MissingField("field.type_name"))?; - // A child with a table is reached through it, and any other, an - // unrolled message, a message of another crate or a well-known - // type, through its `Message` impl. - let child_table = if ctx.uses_table_codec(type_name) { + // A child without a table here is reached through its `Message` + // impl. + let child_table = if ctx.uses_table_codec(type_name()?) { Some(table_path(&unshortened_path()?)?) } else { None diff --git a/buffa-codegen/src/table_plan.rs b/buffa-codegen/src/table_plan.rs index 530b92bb..edb9a77b 100644 --- a/buffa-codegen/src/table_plan.rs +++ b/buffa-codegen/src/table_plan.rs @@ -2,23 +2,23 @@ //! table field looks like. //! //! A message can use the table if the interpreters in `buffa::table` cover -//! every field it has. A message it holds does not have to be a table message: -//! a table reaches a child that has a table through it, and any other child -//! through its `Message` impl, so unrolled messages, messages of other crates -//! and well-known types all work as children. The set of table messages is -//! therefore the requested messages that are locally eligible. +//! every field it has, with one exception that depends on the messages it +//! holds: a child stored with a non-default bytes type would lose its +//! zero-copy decode inside a table message, so the messages that hold one stay +//! unrolled. Any other child is reached through its table or its `Message` +//! impl, so it need not be a table message itself. -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use crate::context::CodeGenContext; use crate::features::ResolvedFeatures; use crate::generated::descriptor::field_descriptor_proto::{Label, Type}; use crate::generated::descriptor::{DescriptorProto, FieldDescriptorProto, FileDescriptorProto}; use crate::impl_message::{ - effective_type, is_explicit_presence_scalar, is_field_packed, is_real_oneof_member, - is_required_field, + effective_type, field_bytes_repr, is_explicit_presence_scalar, is_field_packed, + is_real_oneof_member, is_required_field, map_value_bytes_repr, }; -use crate::message::{find_map_entry, is_closed_enum}; +use crate::message::{find_map_entry, is_closed_enum, map_entry_key_type, map_entry_value_type}; use crate::{CodeGenError, CodeGenWarning, CodecStrategy, TableCodecFallbackReason}; /// The cardinality half of a field's `buffa::table::Kind`. @@ -205,6 +205,66 @@ pub(crate) fn table_fields<'a>( struct Candidate<'a> { fqn: String, fields: Result>, Ineligible>, + /// Whether a `bytes` field or map value of the message has a non-default + /// type. + own_bytes: bool, + /// The proto paths of the message types of its fields: singular, + /// repeated, in a oneof, as a map value, or a group. + holds: Vec, +} + +/// Whether `msg` has a `bytes` field, or a map with `bytes` values, stored +/// as `bytes::Bytes` or a custom type. +fn has_non_default_bytes( + ctx: &CodeGenContext, + msg: &DescriptorProto, + fqn: &str, + features: &ResolvedFeatures, +) -> bool { + let proto_fqn = fqn.trim_start_matches('.'); + msg.field.iter().any(|f| { + let name = f.name.as_deref().unwrap_or(""); + let repr = match find_map_entry(msg, f) { + Some(entry) => map_value_bytes_repr( + ctx, + map_entry_key_type(ctx, entry, features), + map_entry_value_type(ctx, entry, features), + proto_fqn, + name, + ), + None if effective_type(ctx, f, features) == Type::TYPE_BYTES => { + field_bytes_repr(ctx, proto_fqn, name) + } + None => return false, + }; + !repr.is_default() + }) +} + +/// The proto paths of the message types `msg` has fields of. +fn held_messages( + ctx: &CodeGenContext, + msg: &DescriptorProto, + features: &ResolvedFeatures, +) -> Vec { + msg.field + .iter() + .filter_map(|f| { + if let Some(entry) = find_map_entry(msg, f) { + if map_entry_value_type(ctx, entry, features) != Some(Type::TYPE_MESSAGE) { + return None; + } + let value = entry.field.iter().find(|v| v.number == Some(2))?; + return value.type_name.clone(); + } + matches!( + effective_type(ctx, f, features), + Type::TYPE_MESSAGE | Type::TYPE_GROUP + ) + .then(|| f.type_name.clone()) + .flatten() + }) + .collect() } /// Every message of `messages` and the messages nested in them that has a @@ -246,6 +306,8 @@ fn collect<'a>( } out.push(Candidate { fields: table_fields(ctx, msg, &fqn, &features), + own_bytes: has_non_default_bytes(ctx, msg, &fqn, &features), + holds: held_messages(ctx, msg, &features), fqn: fqn.clone(), }); } @@ -261,6 +323,38 @@ fn collect<'a>( } } +/// The messages that hold, directly or through other messages, one that this +/// run generates with a `bytes` field of a non-default type, each with the +/// message it holds that leads there. +/// +/// The table decodes over one contiguous `&[u8]`, where `Buf::copy_to_bytes` +/// copies, so a `Bytes` field of such a child would stop being decoded +/// without a copy. A child of another crate is not a candidate and is not +/// inspected. +fn holders_of_non_default_bytes<'a>(candidates: &'a [Candidate<'_>]) -> HashMap<&'a str, &'a str> { + let mut tainted: HashSet<&str> = candidates + .iter() + .filter(|c| c.own_bytes) + .map(|c| c.fqn.as_str()) + .collect(); + let mut holders = HashMap::new(); + loop { + let before = tainted.len(); + for c in candidates { + if tainted.contains(c.fqn.as_str()) { + continue; + } + if let Some(child) = c.holds.iter().find(|h| tainted.contains(h.as_str())) { + tainted.insert(&c.fqn); + holders.insert(c.fqn.as_str(), child.as_str()); + } + } + if tainted.len() == before { + return holders; + } + } +} + /// The messages generated with the table codec in one run. #[derive(Default)] pub(crate) struct TablePlan { @@ -312,8 +406,11 @@ pub(crate) fn plan( ); } - // The messages that asked for the table, each with the reason it cannot - // have it, if there is one. + let bytes_holders = holders_of_non_default_bytes(&candidates); + + // For each message that asked for the table, the table if it can have + // one, and otherwise the reason it cannot, which an exact rule turns into + // an error and the summary counts. let mut tables = HashSet::new(); let mut selected = 0; let mut errors = Vec::new(); @@ -327,6 +424,14 @@ pub(crate) fn plan( let why = match &c.fields { Err(why) => why.clone(), Ok(_) if group_types.contains(&c.fqn) => same("is the type of a group field"), + Ok(_) if bytes_holders.contains_key(c.fqn.as_str()) => ineligible( + "holds a message with bytes fields of a non-default type", + format!( + "it holds `{}`, which has bytes fields of a non-default type or holds a \ + message that has", + bytes_holders[c.fqn.as_str()] + ), + ), Ok(_) => { tables.insert(c.fqn.clone()); continue; diff --git a/buffa-codegen/src/tests/table_codec.rs b/buffa-codegen/src/tests/table_codec.rs index 3eab4062..698ef1b8 100644 --- a/buffa-codegen/src/tests/table_codec.rs +++ b/buffa-codegen/src/tests/table_codec.rs @@ -201,6 +201,77 @@ fn a_child_without_a_table_is_reached_through_its_message_impl() { assert!(!holder.contains("__BUFFA_TABLE_Oneofy"), "{holder}"); } +#[test] +fn a_repeated_child_without_a_table_is_reached_through_its_message_impl() { + let mut file = schema(); + file.message_type[4].field[0].label = Some(Label::LABEL_REPEATED); + let (files, _) = generate_with_diagnostics( + &[file], + &["t.proto".to_string()], + &table_config(CodecStrategy::Table), + ) + .unwrap(); + let code = squashed(&joined(&files)); + let holder = code.split("static__BUFFA_TABLE_HasOneofy").nth(1).unwrap(); + let holder = holder + .split("impl::buffa::MessageforHasOneofy") + .next() + .unwrap(); + assert!( + holder.contains("Aux::Rep(&::buffa::table::RepVt::new_via_message::())"), + "{holder}" + ); + assert!(holder.contains("Vec"), "{holder}"); +} + +#[test] +fn a_child_in_another_package_is_reached_through_the_path_idiomatic_imports_shortens() { + let other = FileDescriptorProto { + package: Some("a".to_string()), + message_type: vec![message("Cold", vec![scalar("c", 1, Type::TYPE_INT32)])], + ..proto3_file("a.proto") + }; + let holder = FileDescriptorProto { + package: Some("b".to_string()), + dependency: vec!["a.proto".to_string()], + message_type: vec![message( + "Holder", + vec![ + message_field("cold", 1, ".a.Cold"), + repeated_message_field("colds", 2, ".a.Cold"), + ], + )], + ..proto3_file("b.proto") + }; + let config = CodeGenConfig { + codec_strategy_in: vec![(".a.Cold".to_string(), CodecStrategy::Unrolled)], + idiomatic_imports: true, + file_per_package: true, + ..table_config(CodecStrategy::Table) + }; + let (files, warnings) = generate_with_diagnostics( + &[other, holder], + &["a.proto".to_string(), "b.proto".to_string()], + &config, + ) + .unwrap(); + assert!(table_warnings(&warnings).is_empty(), "{warnings:?}"); + let code = squashed(&joined(&files)); + let holder = code.split("static__BUFFA_TABLE_Holder").nth(1).unwrap(); + let holder = holder + .split("impl::buffa::MessageforHolder") + .next() + .unwrap(); + assert!( + holder.contains("MsgVt::new_via_message::>>()"), + "{holder}" + ); + assert!( + holder.contains("RepVt::new_via_message::()"), + "{holder}" + ); +} + #[test] fn a_rule_selects_messages_when_the_global_setting_is_unrolled() { let config = CodeGenConfig { @@ -396,7 +467,7 @@ fn a_message_type_from_another_crate_is_reached_through_its_message_impl() { let code = joined(&files); assert!(tables(&code).contains(&"HasLeaf".to_string()), "{code}"); assert!(tables(&code).contains(&"Leaf".to_string())); - assert!(table_warnings(&warnings).len() == 1, "{warnings:?}"); + assert_eq!(table_warnings(&warnings).len(), 1, "{warnings:?}"); let code = squashed(&code); let holder = code.split("static__BUFFA_TABLE_HasLeaf").nth(1).unwrap(); assert!( @@ -586,3 +657,200 @@ fn the_generated_abi_is_a_literal_that_matches_the_runtime() { ); assert!(!code.contains("abi=::buffa::table::ABI"), "{code}"); } + +// --------------------------------------------------------------------------- +// Messages that hold a message stored with a non-default bytes type +// --------------------------------------------------------------------------- + +const HOLDS_BYTES: &str = "holds a message with bytes fields of a non-default type"; +const CUSTOM_FIELD: &str = "has a field with a custom string, bytes or collection type"; + +fn bytes_field(name: &str, number: i32) -> FieldDescriptorProto { + scalar(name, number, Type::TYPE_BYTES) +} + +fn repeated_message_field(name: &str, number: i32, type_name: &str) -> FieldDescriptorProto { + FieldDescriptorProto { + label: Some(Label::LABEL_REPEATED), + ..message_field(name, number, type_name) + } +} + +/// Package `b` with: +/// +/// - `Blob` (a bytes field, which the rules below give the type `Bytes`) and +/// `PlainBytes` (a bytes field that keeps `Vec`); +/// - `HasBlob`, `HasBlobs` (repeated) and `HoldsHasBlob`, which hold a `Blob` +/// directly, in a list and through `HasBlob`; +/// - `OneofBlob`, which has a oneof member of type `Blob`, and `HoldsOneofBlob` +/// (holds `OneofBlob`); +/// - `MapBlob`, which has a map with `Blob` values, and `HoldsMapBlob`; +/// - `HoldsPlain` (holds `PlainBytes`), `Leaf` and `HoldsLeaf`, which are +/// unaffected. +fn bytes_schema() -> FileDescriptorProto { + let mut oneof_blob = message( + "OneofBlob", + vec![FieldDescriptorProto { + oneof_index: Some(0), + ..message_field("blob", 1, ".b.Blob") + }], + ); + oneof_blob.oneof_decl = vec![OneofDescriptorProto { + name: Some("choice".to_string()), + ..Default::default() + }]; + let mut map_blob = message( + "MapBlob", + vec![repeated_message_field("blobs", 1, ".b.MapBlob.BlobsEntry")], + ); + map_blob.nested_type = vec![DescriptorProto { + name: Some("BlobsEntry".to_string()), + field: vec![ + scalar("key", 1, Type::TYPE_STRING), + message_field("value", 2, ".b.Blob"), + ], + options: (MessageOptions { + map_entry: Some(true), + ..Default::default() + }) + .into(), + ..Default::default() + }]; + FileDescriptorProto { + package: Some("b".to_string()), + message_type: vec![ + message("Blob", vec![bytes_field("data", 1)]), + message("PlainBytes", vec![bytes_field("data", 1)]), + message("HasBlob", vec![message_field("blob", 1, ".b.Blob")]), + message( + "HasBlobs", + vec![repeated_message_field("blobs", 1, ".b.Blob")], + ), + message("HoldsHasBlob", vec![message_field("has", 1, ".b.HasBlob")]), + oneof_blob, + message( + "HoldsOneofBlob", + vec![message_field("o", 1, ".b.OneofBlob")], + ), + map_blob, + message("HoldsMapBlob", vec![message_field("m", 1, ".b.MapBlob")]), + message("HoldsPlain", vec![message_field("p", 1, ".b.PlainBytes")]), + message("Leaf", vec![scalar("x", 1, Type::TYPE_INT32)]), + message("HoldsLeaf", vec![message_field("leaf", 1, ".b.Leaf")]), + ], + ..proto3_file("b.proto") + } +} + +fn run_bytes(config: &CodeGenConfig) -> Result<(String, Vec), CodeGenError> { + let (files, warnings) = + generate_with_diagnostics(&[bytes_schema()], &["b.proto".to_string()], config)?; + Ok((joined(&files), warnings)) +} + +/// The table strategy, with `Blob.data` stored as `Bytes`. +fn blob_config() -> CodeGenConfig { + CodeGenConfig { + bytes_fields: vec![(".b.Blob.data".to_string(), BytesRepr::Bytes)], + ..table_config(CodecStrategy::Table) + } +} + +#[test] +fn a_message_that_holds_a_message_with_a_bytes_type_stays_unrolled() { + let (code, warnings) = run_bytes(&blob_config()).unwrap(); + // The holders of `Blob` fall back, directly, in a list, transitively, and + // through a oneof member and a map value, though the last two also hold + // messages that cannot use the table for other reasons. + assert_eq!( + tables(&code), + ["PlainBytes", "HoldsPlain", "Leaf", "HoldsLeaf"] + ); + let (counts, reasons) = summary(&warnings); + assert_eq!(counts, (8, 12)); + // `OneofBlob` and `MapBlob` fall back for their oneof and map, so they are + // not counted as holders. + assert_eq!( + reasons, + [ + (HOLDS_BYTES, 5), + (CUSTOM_FIELD, 1), + ("has a oneof", 1), + ("has a map field", 1), + ] + ); + let text = table_warnings(&warnings)[0].to_string(); + assert!(text.contains(HOLDS_BYTES), "{text}"); + assert!(text.contains(".b.HasBlob"), "{text}"); +} + +#[test] +fn a_message_with_a_plain_bytes_field_is_unaffected() { + let (code, _) = run_bytes(&blob_config()).unwrap(); + let plain = squashed(&code); + assert!(tables(&code).contains(&"PlainBytes".to_string())); + // `HoldsPlain` reaches `PlainBytes` through its table. + let holder = plain + .split("static__BUFFA_TABLE_HoldsPlain") + .nth(1) + .unwrap(); + assert!(holder.contains("(&__BUFFA_TABLE_PlainBytes)"), "{holder}"); +} + +#[test] +fn a_child_set_to_unrolled_without_bytes_still_lets_its_holder_use_the_table() { + let config = CodeGenConfig { + codec_strategy_in: vec![(".b.Leaf".to_string(), CodecStrategy::Unrolled)], + ..blob_config() + }; + let (code, _) = run_bytes(&config).unwrap(); + assert!(tables(&code).contains(&"HoldsLeaf".to_string()), "{code}"); + assert!(!tables(&code).contains(&"Leaf".to_string())); + assert!(squashed(&code).contains("MsgVt::new_via_message::<")); +} + +#[test] +fn a_child_set_to_unrolled_that_has_a_bytes_type_keeps_its_holder_unrolled() { + // The rule does not decide it: `Blob` is stored as `Bytes` either way. + let config = CodeGenConfig { + codec_strategy_in: vec![(".b.Blob".to_string(), CodecStrategy::Unrolled)], + ..blob_config() + }; + let (code, warnings) = run_bytes(&config).unwrap(); + assert!(!tables(&code).contains(&"HasBlob".to_string()), "{code}"); + assert!(summary(&warnings).1.contains(&(HOLDS_BYTES, 5))); +} + +#[test] +fn a_bytes_type_for_every_field_keeps_the_holders_of_every_bytes_message_unrolled() { + let config = CodeGenConfig { + bytes_fields: vec![(".".to_string(), BytesRepr::Bytes)], + ..table_config(CodecStrategy::Table) + }; + let (code, _) = run_bytes(&config).unwrap(); + // With this rule `PlainBytes` has a `Bytes` field, and so is everything + // that holds it unrolled. + assert_eq!(tables(&code), ["Leaf", "HoldsLeaf"]); +} + +#[test] +fn a_bytes_type_on_another_message_does_not_affect_a_holder() { + let config = CodeGenConfig { + bytes_fields: vec![(".b.PlainBytes.data".to_string(), BytesRepr::Bytes)], + ..table_config(CodecStrategy::Table) + }; + let (code, _) = run_bytes(&config).unwrap(); + assert!(tables(&code).contains(&"HasBlob".to_string()), "{code}"); + assert!(!tables(&code).contains(&"HoldsPlain".to_string())); +} + +#[test] +fn an_exact_path_rule_for_a_holder_of_a_bytes_typed_message_is_an_error() { + let config = CodeGenConfig { + codec_strategy_in: vec![(".b.HasBlob".to_string(), CodecStrategy::Table)], + ..blob_config() + }; + let err = run_bytes(&config).unwrap_err().to_string(); + assert!(err.contains("cannot use it"), "{err}"); + assert!(err.contains("it holds `.b.Blob`"), "{err}"); +} From 609414d370e4f6147f72d232e1b16e57df8efab1 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 12:49:47 -0700 Subject: [PATCH 08/11] buffa-test: cover bytes-typed children and shortened paths to unrolled children --- buffa-test/build.rs | 24 +++++++-- buffa-test/protos/table_bytes.proto | 31 ++++++++++++ buffa-test/protos/table_codec.proto | 9 ++-- buffa-test/src/lib.rs | 7 +++ buffa-test/src/tests/table_codec.rs | 78 ++++++++++++++++++++++++----- 5 files changed, 129 insertions(+), 20 deletions(-) create mode 100644 buffa-test/protos/table_bytes.proto diff --git a/buffa-test/build.rs b/buffa-test/build.rs index 8fc2e9ee..4912c108 100644 --- a/buffa-test/build.rs +++ b/buffa-test/build.rs @@ -79,14 +79,17 @@ fn compile_extern_children() { /// ways: unrolled (`xau`, `xbu`), table (`xat`, `xbt`), and table with /// `file_per_package` and `idiomatic_imports` (`xati`, `xbti`), which shortens /// the paths of types in other packages and so changes what a table path may -/// be. +/// be. `Cold`, which `Holder` holds singly and in a list, stays unrolled in +/// all three, so the table holders reach it through its `Message` impl with +/// the shortened path. fn compile_cross_package() { let out = std::path::PathBuf::from(std::env::var("OUT_DIR").expect("OUT_DIR")); let sources = |suffix: &str| { let dep = format!( "syntax = \"proto3\";\npackage xa{suffix};\n\ message Leaf {{ int32 x = 1; string s = 2; }}\n\ - message Wrap {{ Leaf leaf = 1; repeated Leaf leaves = 2; }}\n" + message Wrap {{ Leaf leaf = 1; repeated Leaf leaves = 2; }}\n\ + message Cold {{ int64 c = 1; string s = 2; }}\n" ); let user = format!( "syntax = \"proto3\";\npackage xb{suffix};\nimport \"xa{suffix}.proto\";\n\ @@ -95,6 +98,8 @@ fn compile_cross_package() { repeated xa{suffix}.Leaf leaves = 2;\n\ xa{suffix}.Wrap wrap = 3;\n\ Sub sub = 4;\n\ + xa{suffix}.Cold cold = 5;\n\ + repeated xa{suffix}.Cold colds = 6;\n\ message Sub {{ xa{suffix}.Leaf l = 1; }}\n\ }}\n" ); @@ -115,7 +120,11 @@ fn compile_cross_package() { let mut config = buffa_build::Config::new() .files(&[dep_path, user_path]) .includes(&[&out]) - .codec_strategy(strategy); + .codec_strategy(strategy) + .codec_strategy_in( + buffa_build::CodecStrategy::Unrolled, + &[format!(".xa{suffix}.Cold")], + ); if idiomatic { let dir = out.join("cross_package_idiomatic"); std::fs::create_dir_all(&dir).expect("create dir"); @@ -242,6 +251,15 @@ fn main() { ); compile_cross_package(); compile_extern_children(); + // `bytes` fields as `bytes::Bytes`, which the messages that hold one + // must keep unrolled. + buffa_build::Config::new() + .files(&["protos/table_bytes.proto"]) + .includes(&["protos/"]) + .codec_strategy(buffa_build::CodecStrategy::Table) + .use_bytes_type() + .compile() + .expect("buffa_build failed for table_bytes.proto"); compile_table_with_options("table_codec.proto", "tc"); } diff --git a/buffa-test/protos/table_bytes.proto b/buffa-test/protos/table_bytes.proto new file mode 100644 index 00000000..67858e8d --- /dev/null +++ b/buffa-test/protos/table_bytes.proto @@ -0,0 +1,31 @@ +syntax = "proto3"; + +// build.rs compiles this once, as package `tbz`, with `codec_strategy = Table` +// and `bytes` fields stored as `bytes::Bytes`. `Blob` has such a field, so it +// is unrolled, and so are `HoldsBlob` and `HoldsBlobs`, which hold it: a table +// decodes from a contiguous slice, where `Bytes` fields could not alias the +// input. `Plain` and `HoldsPlain` have no `bytes` field and are tables. +package tbz; + +message Blob { + bytes data = 1; + repeated bytes chunks = 2; +} + +message HoldsBlob { + Blob blob = 1; + repeated Blob blobs = 2; + int32 tail = 3; +} + +message HoldsBlobs { + HoldsBlob inner = 1; +} + +message Plain { + int32 x = 1; +} + +message HoldsPlain { + Plain plain = 1; +} diff --git a/buffa-test/protos/table_codec.proto b/buffa-test/protos/table_codec.proto index 5ee90e1e..d0981c1b 100644 --- a/buffa-test/protos/table_codec.proto +++ b/buffa-test/protos/table_codec.proto @@ -102,8 +102,9 @@ message Sparse { message Empty {} -// A oneof, a map, and a message that holds one: none can use the table, so -// they stay unrolled when the table is requested. +// A oneof and a map, which the table cannot handle, so `WithOneof` and +// `WithMap` stay unrolled when the table is requested. `HoldsOneof` holds both +// and is a table message, which reaches them through their `Message` impl. message WithOneof { oneof choice { int32 a = 1; @@ -124,8 +125,8 @@ message HoldsOneof { WithMap m = 3; } -// An unrolled message that holds table messages, next to unrolled ones that -// hold a oneof and a map of them. +// A table message that holds table messages (`Inner`, `Nested`) and +// `HoldsOneof`, whose own children are unrolled. message Mixed { Inner inner = 1; HoldsOneof holds = 2; diff --git a/buffa-test/src/lib.rs b/buffa-test/src/lib.rs index 34d97b57..b57d2f7c 100644 --- a/buffa-test/src/lib.rs +++ b/buffa-test/src/lib.rs @@ -1189,6 +1189,13 @@ pub mod xft { buffa::include_proto!("xft"); } +// `tbz` has `bytes` fields stored as `bytes::Bytes`, and the table codec, which +// the messages that hold those fields must not use. +#[cfg(has_table_codec)] +pub mod tbz { + buffa::include_proto!("tbz"); +} + // Two packages, the second holding messages of the first: `xau`/`xbu` unrolled, // `xat`/`xbt` with the table codec, and `xti` with the table codec and // `file_per_package` with `idiomatic_imports`, which holds `xati` and `xbti`. diff --git a/buffa-test/src/tests/table_codec.rs b/buffa-test/src/tests/table_codec.rs index 2970d22e..5ab1028d 100644 --- a/buffa-test/src/tests/table_codec.rs +++ b/buffa-test/src/tests/table_codec.rs @@ -974,6 +974,19 @@ fn messages_held_across_packages_agree_in_every_layout() { l: MessageField::some(leaf(6)), ..Default::default() }), + // Unrolled in the table layouts. + cold: MessageField::some($xa::Cold { + c: 7, + s: "cold".into(), + ..Default::default() + }), + colds: vec![ + $xa::Cold::default(), + $xa::Cold { + c: 8, + ..Default::default() + }, + ], ..Default::default() } }}; @@ -991,6 +1004,8 @@ fn messages_held_across_packages_agree_in_every_layout() { .encode_to_vec(), wire ); + assert_same_chained::(&wire); + assert_same_on_corrupt_input::(&wire, true); } // --------------------------------------------------------------------------- @@ -1164,20 +1179,15 @@ fn nesting_through_both_codecs_is_limited_alike() { assert!(::decode_from_slice(&cold_hot_chain(40)).is_ok()); for limit in [1, 2, 3, 10] { let wire = cold_hot_chain(6); - let decode = |r: Result, DecodeError>| r; assert_eq!( - decode( - buffa::DecodeOptions::new() - .with_recursion_limit(limit) - .decode_from_slice::(&wire) - .map(|m| m.encode_to_vec()) - ), - decode( - buffa::DecodeOptions::new() - .with_recursion_limit(limit) - .decode_from_slice::(&wire) - .map(|m| m.encode_to_vec()) - ), + buffa::DecodeOptions::new() + .with_recursion_limit(limit) + .decode_from_slice::(&wire) + .map(|m| m.encode_to_vec()), + buffa::DecodeOptions::new() + .with_recursion_limit(limit) + .decode_from_slice::(&wire) + .map(|m| m.encode_to_vec()), "limit {limit}" ); } @@ -1270,3 +1280,45 @@ fn a_child_without_a_table_is_encoded_into_every_kind_of_sink() { framed.extend_from_slice(&expected); assert_eq!(&chunked[..], &framed[..]); } + +#[test] +fn a_holder_of_a_bytes_typed_message_is_unrolled_and_decodes_without_copying() { + use crate::tbz::{Blob, HoldsBlob, HoldsBlobs}; + use buffa::bytes::Bytes; + use buffa::MessageField; + // The plan ran on this schema, so `Plain` has a table, and `HoldsBlob` + // and `HoldsBlobs` do not, or their `Bytes` fields would be copied. + let generated = include_str!(concat!(env!("OUT_DIR"), "/table_bytes.rs")); + assert!(generated.contains("static __BUFFA_TABLE_Plain")); + assert!(generated.contains("static __BUFFA_TABLE_HoldsPlain")); + assert!(!generated.contains("__BUFFA_TABLE_Blob")); + assert!(!generated.contains("__BUFFA_TABLE_HoldsBlob")); + + let blob = |fill: u8| Blob { + data: Bytes::from(vec![fill; 64]), + chunks: vec![Bytes::from(vec![fill + 1; 64])], + ..Default::default() + }; + let msg = HoldsBlobs { + inner: MessageField::some(HoldsBlob { + blob: MessageField::some(blob(1)), + blobs: vec![blob(3), blob(5)], + tail: 9, + ..Default::default() + }), + ..Default::default() + }; + let src = Bytes::from(msg.encode_to_vec()); + let range = src.as_ptr() as usize..src.as_ptr() as usize + src.len(); + let aliases = |b: &Bytes| range.contains(&(b.as_ptr() as usize)); + + let decoded = HoldsBlobs::decode(&mut src.clone()).unwrap(); + assert_eq!(decoded, msg); + let inner = decoded.inner.as_option().unwrap(); + let held = inner.blob.as_option().unwrap(); + assert!(aliases(&held.data) && aliases(&held.chunks[0])); + assert!(inner + .blobs + .iter() + .all(|b| aliases(&b.data) && aliases(&b.chunks[0]))); +} From 2d3b68eb35cb6c19973c2416d0372b6207057088 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 12:49:48 -0700 Subject: [PATCH 09/11] docs: state the message-impl child, the sink condition and the bytes limit once --- .../added-20260923-codec-strategy-table.yaml | 2 +- DESIGN.md | 2 +- buffa-build/src/lib.rs | 15 +++--- buffa-codegen/src/lib.rs | 50 ++++++++++++------- docs/guide.md | 16 +++--- 5 files changed, 49 insertions(+), 36 deletions(-) diff --git a/.changes/unreleased/added-20260923-codec-strategy-table.yaml b/.changes/unreleased/added-20260923-codec-strategy-table.yaml index 4186c03e..69b48b4d 100644 --- a/.changes/unreleased/added-20260923-codec-strategy-table.yaml +++ b/.changes/unreleased/added-20260923-codec-strategy-table.yaml @@ -1,4 +1,4 @@ kind: Added body: |- - **Table-driven message codec** (#469, refs #463). `buffa_build::Config::codec_strategy(CodecStrategy::Table)` (plugin option `codec_strategy=table`, `CodeGenConfig::codec_strategy`) generates each message's binary `Message` implementation from a static table and interpreters that every message shares, instead of code specialised to the message's fields. It roughly halves the compiled size of a large schema at `opt-level = "z"`, and messages made of many small fields are slower. `CodecStrategy::Unrolled` stays the default. `codec_strategy_in(strategy, &[paths])` (plugin option `codec_strategy_in==`, repeatable) chooses the strategy for matching messages and the messages nested in them, and the last matching rule wins. The wire format does not change. A message that has a `oneof`, `map`, or group field stays unrolled, and `CodeGenWarning::TableCodecFallbackSummary` counts those; a table message may hold any message, and reaches one that has no table through its `Message` impl; a rule that names such a message by its exact path is an error. The generated code needs Rust 1.77 or later, which `buffa-build` checks, and compiles in a crate with `#![forbid(unsafe_code)]`; see the guide's "Smaller generated code" section for the rest. + **Table-driven message codec** (#469, refs #463). `buffa_build::Config::codec_strategy(CodecStrategy::Table)` (plugin option `codec_strategy=table`, `CodeGenConfig::codec_strategy`) generates each message's binary `Message` implementation from a static table and interpreters that every message shares, instead of code specialised to the message's fields. On a schema the table fully covers it roughly halves the compiled size at `opt-level = "z"`; on the WhatsApp schema, where 43 of 752 messages have a `oneof` or `map` and stay unrolled, the text section is 40% smaller. Messages made of many small fields are slower. `CodecStrategy::Unrolled` stays the default. `codec_strategy_in(strategy, &[paths])` (plugin option `codec_strategy_in==`, repeatable) chooses the strategy for matching messages and the messages nested in them, and the last matching rule wins. The wire format does not change. A message that has a `oneof`, `map`, or group field stays unrolled, and so does a message that holds a message of the same run with a `bytes` field of a non-default type, because the table would copy the `bytes::Bytes` fields that unrolled code shares with the input; `CodeGenWarning::TableCodecFallbackSummary` counts them. A table message may hold any other message, and reaches one that has no table through its `Message` impl, which stages the child in a buffer when it is written to a `Rope` or another sink that is not the cursor `Message::encode` uses, and so copies it. A rule that selects the table, by exact path, for a message that cannot use it is an error. The generated code needs Rust 1.77 or later, which `buffa-build` checks, and compiles in a crate with `#![forbid(unsafe_code)]`; see the guide's "Smaller generated code" section for the rest. time: 2026-09-23T03:10:00+00:00 diff --git a/DESIGN.md b/DESIGN.md index f58c9afa..af27cb12 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -566,7 +566,7 @@ Three decisions shape the runtime: - **The `unsafe` lives in `buffa`.** `__table!` and `__table_entry!` contain the `unsafe` blocks and witness each field's type against its kind, so a table that names the wrong kind for a field does not compile, and generated code compiles under `#![forbid(unsafe_code)]`. `Table::new` also checks the layout constants at compile time. The interpreters run under Miri in CI. - **The interpreters are not generic over the sink or the input where that is avoidable.** `Message::encode` and its siblings write any `BufMut` through one shared, non-generic cursor (`buffa/src/encode_sink.rs`), and decoding runs over a contiguous `&[u8]`, so the interpreters are compiled once in `buffa`, at its `opt-level`, and not once per caller. -- **A child reaches the interpreters through its table or its `Message` impl.** A message field's `Aux` descriptor holds either the child's `MessageTable`, which the interpreters recurse into, or a `DynVt` of three function pointers instantiated for the child type, `compute_size`, `write_to::`, and `merge_length_delimited`, which call the child's `Message` impl (`table/bridge.rs`). The second form serves an unrolled child, a child from another crate, and a well-known type, so a message can use the table whatever it holds. A function pointer cannot be generic over the sink, so the write thunk takes the `PreSized` cursor that every `BufMut` is written through, and any other sink receives the child from a scratch buffer, which costs one copy. The planner in `buffa-codegen` (`table_plan.rs`) selects the messages that asked for the table, drops those the interpreters cannot handle (oneofs, maps, groups and the types of group fields, `MessageSet`, extension ranges with JSON, custom string, bytes, or collection types), and the emitter names a child's static table only if the plan gave the child one. +- **A child reaches the interpreters through its table or its `Message` impl.** A message field's `Aux` descriptor holds either the child's `MessageTable` or a `DynVt` of function pointers instantiated for the child type (`table/bridge.rs`), so a message can use the table whatever it holds. The write pointer takes the `PreSized` cursor because a function pointer cannot be generic over the sink. A pointer per child type and sink type would avoid the copy that other sinks cost, at the price of code in every crate that uses the table. The planner in `buffa-codegen` (`table_plan.rs`) selects the messages that asked for the table and drops those the interpreters cannot handle (oneofs, maps, groups and the types of group fields, `MessageSet`, extension ranges with JSON, custom string, bytes, or collection types). It also drops the holders of a message of the run with a non-default bytes type, transitively, because the table decodes from one contiguous slice and would copy the `bytes::Bytes` fields that unrolled code shares with a `Bytes` input. A child from another crate is not inspected. The emitter names a child's static table only if the plan gave the child one. ### Owned decode: intentional throughput trade-offs diff --git a/buffa-build/src/lib.rs b/buffa-build/src/lib.rs index 4376e2be..bbde39e9 100644 --- a/buffa-build/src/lib.rs +++ b/buffa-build/src/lib.rs @@ -1557,11 +1557,10 @@ impl Config { /// On a schema it fully covers, [`CodecStrategy::Table`] makes the /// compiled size about half as big at `opt-level = "z"`, and it slows /// messages made of many small fields; a message that cannot use it keeps - /// its size. [`CodecStrategy::Table`] has the measurements, says which - /// messages stay unrolled, and lists how a table message behaves - /// differently. This build reports - /// those in one `cargo:warning`. The option never changes the wire - /// format. The generated code needs Rust 1.77 or later, and `compile` + /// its size. [`CodecStrategy::Table`] says which messages stay unrolled and + /// how a table message behaves differently, and this build reports the + /// messages that stay unrolled in one `cargo:warning`. The option never + /// changes the wire format. The generated code needs Rust 1.77 or later, and `compile` /// returns an error on an older compiler when a build script runs it (the /// compiler is read from `RUSTC`). /// @@ -1601,10 +1600,8 @@ impl Config { /// names the message by its exact path. A rule that matches no message /// produces a warning. /// - /// A rule does not extend to the messages a message holds. A table message - /// reaches a child that is not a table message, such as one set to - /// [`CodecStrategy::Unrolled`], through the child's `Message` impl, so - /// keeping a hot message unrolled does not affect the messages that hold it. + /// A rule does not extend to the messages a message holds; see + /// [`CodecStrategy::Table`]. #[must_use] pub fn codec_strategy_in(mut self, strategy: CodecStrategy, paths: &[impl AsRef]) -> Self { for raw in paths.iter().map(AsRef::as_ref) { diff --git a/buffa-codegen/src/lib.rs b/buffa-codegen/src/lib.rs index 403d17e4..1ee2b25f 100644 --- a/buffa-codegen/src/lib.rs +++ b/buffa-codegen/src/lib.rs @@ -1101,24 +1101,39 @@ pub enum CodecStrategy { /// unknown fields are preserved; /// - a message with a field of a non-default string, bytes, or collection /// type, such as `use_bytes_type`, `string_type`, `bytes_type`, and - /// `repeated_type` select. - /// - /// A table message may hold any message. It reaches a child that is a table - /// message through the child's table, and any other child, whether it is - /// [`Unrolled`](Self::Unrolled), generated by another crate, or a - /// well-known type, through the child's `Message` impl. Selecting a message - /// does not select the messages it holds, so a rule for a message and a rule - /// for its children are independent. - /// - /// A table message differs from an unrolled one in four ways. A field that + /// `repeated_type` select; + /// - a message that holds, in a singular, repeated, `oneof`, or map value + /// field, a message of the same run that has a `bytes` field of a + /// non-default type, or that holds one. The table decodes from one + /// contiguous slice, so it would copy `bytes::Bytes` fields that + /// unrolled code decoding from a `Bytes` shares with the input. + /// + /// A table message may hold any other message. It reaches a child that is a + /// table message through the child's table, and any other child, whether + /// it is [`Unrolled`](Self::Unrolled), generated by another crate, or a + /// well-known type, through the child's `Message` impl, which costs a + /// function call per child. Selecting a message does not select the + /// messages it holds, and keeping a child [`Unrolled`](Self::Unrolled) does + /// not keep the messages that hold it unrolled: to keep a whole path + /// specialised, select its holders too. + /// + /// A table message differs from an unrolled one in these ways. A field that /// declares a length past the end of its enclosing message fails at once /// with `DecodeError::UnexpectedEof`, where unrolled code can report a - /// different error for the same rejected input. `clear()` resets the message - /// to its default, which releases the capacity of its strings and vectors. + /// different error for the same rejected input; a child reached through its + /// `Message` impl is read from the slice of the nearest enclosing table + /// message, so it is bounded there. `clear()` resets the message to its + /// default, which releases the capacity of its strings and vectors. /// Decoding gathers a `Buf` that is not contiguous into one buffer first. - /// Encoding a child that has no table into a sink that is not a `BufMut`, - /// such as `Rope`, stages the child in a buffer first, so the `bytes` fields - /// inside it are copied and not shared by reference count. + /// Encoding into any sink other than the cursor that `Message::encode` and + /// its siblings write a `BufMut` through, meaning a `Rope`, a sink defined + /// outside `buffa`, or a `BufMut` passed straight to `Message::write_to`, + /// stages each child reached through its `Message` impl in a scratch + /// buffer, and a `Rope` copies it again and cannot share the `bytes` + /// fields inside it by reference count. Codegen cannot see the fields of a + /// message from another crate, so a table message that holds one with a + /// `bytes` field of a non-default type copies it on decode; set the holder + /// to [`Unrolled`](Self::Unrolled) by hand. /// /// A message that another crate or codegen run uses as the type of a group /// or editions `DELIMITED` field must not be selected, because @@ -1933,9 +1948,8 @@ pub struct CodeGenConfig { /// asked for something impossible. A rule that matches no generated message /// produces a [`CodeGenWarning::CodecStrategyRuleMatchedNothing`]. /// - /// A rule does not extend to the messages a message holds: a table message - /// reaches a child that is not a table message through its `Message` impl, - /// so a rule for a message needs no rule for its children. + /// A rule does not extend to the messages a message holds; see + /// [`CodecStrategy::Table`]. pub codec_strategy_in: Vec<(String, CodecStrategy)>, } diff --git a/docs/guide.md b/docs/guide.md index 390054a7..046dfa88 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -1100,7 +1100,7 @@ Buffa uses a two-pass model to avoid the exponential-time size computation that ### Smaller generated code: `codec_strategy` -By default every generated message contains its own size, write, and merge code, specialised to its fields. `CodecStrategy::Table` replaces it with one static table per message and interpreters in `buffa` that every message shares. On a schema of 334 top-level messages and 3,477 fields, the code section of a binary built at `opt-level = "z"` went from 1,670 KB to 1,010 KB, with the 43 messages that have a `oneof` or a `map` still unrolled. The same schema with its oneofs and maps flattened, so that every message can use the table, went from 1,644 KB to 817 KB (see [#463](https://github.com/anthropics/buffa/issues/463) for the method). The cost is speed on messages made of many small fields, where encoding takes up to about 3.5 times as long as with the default `CodecStrategy::Unrolled` and decoding up to 1.6 times. Messages dominated by bulk data, such as large strings, bytes, and packed arrays, show no difference. +By default every generated message contains its own size, write, and merge code, specialised to its fields. `CodecStrategy::Table` replaces it with one static table per message and interpreters in `buffa` that every message shares. On the WhatsApp schema (`whatsapp.proto` from `waproto`: 334 top-level messages, 752 with the nested ones, 3,477 fields), built with `Box` message fields, no unknown-field preservation, fat LTO, and `panic = "abort"`, the text section of the binary at `opt-level = "z"` went from 1,670 KB to 1,010 KB (−40%). 43 of its 752 messages (23 of 334 top-level) stay unrolled because they have a `oneof` or a `map`. The 1,644 KB to 817 KB figure in [#463](https://github.com/anthropics/buffa/issues/463), which also gives the method, was measured on that schema with its oneofs and maps flattened into plain fields, so that every message could use the table. The cost is speed on messages made of many small fields, where encoding takes up to about 3.5 times as long as with the default `CodecStrategy::Unrolled` and decoding up to 1.6 times. Messages dominated by bulk data, such as large strings, bytes, and packed arrays, show no difference. ```rust,ignore // build.rs @@ -1113,14 +1113,15 @@ buffa_build::Config::new() .compile()?; ``` -A table message may hold any message. It reaches a child that has a table through it, and any other child (a message you set to `Unrolled`, one generated by another crate, or a well-known type such as `Timestamp`) through its `Message` impl, which costs a function call per child. A `codec_strategy_in` rule selects the message it names and the messages nested in it, and does not extend to the messages it holds. In the example, `.wa.Message` stays specialised and the messages that hold it use the table. +A table message may hold any message. It reaches a child that has a table through it, and any other child (a message you set to `Unrolled`, one generated by another crate, or a well-known type such as `Timestamp`) through its `Message` impl, which costs a function call per child. A `codec_strategy_in` rule selects the message it names and the messages nested in it, and does not extend to the messages it holds. In the example, `.wa.Message` and `.wa.Receipt` stay specialised and the messages that hold them use the table. Setting a message to `Unrolled` therefore does not keep the messages that hold it unrolled: to keep a whole path specialised, list its holders too. -The option changes only the binary `Message` implementation. The wire format and the JSON, text, view, and reflection code are the same under both strategies, and a table message encodes to the same bytes and decodes the same accepted input as its unrolled twin. It differs in four ways: +The option changes only the binary `Message` implementation. The wire format and the JSON, text, view, and reflection code are the same under both strategies, and a table message encodes to the same bytes and decodes the same accepted input as its unrolled twin. It differs in these ways: -- A field that declares a length past the end of its enclosing message fails at once with `DecodeError::UnexpectedEof`, where unrolled code reads on into the enclosing message and can report a different error for the same rejected input. +- A field that declares a length past the end of its enclosing message fails at once with `DecodeError::UnexpectedEof`, where unrolled code reads on into the enclosing message and can report a different error for the same rejected input. A child reached through its `Message` impl is read from the slice of the nearest enclosing table message, so it is bounded there. - The table decodes from one contiguous slice, so a `Buf` that is not contiguous is gathered into one buffer first. `Message::merge_field` on a table message cannot gather, and returns `UnexpectedEof` for such a buffer; only code that calls it directly is affected, such as the default `merge_group`. A message that another crate or another codegen run uses as the type of a group or `DELIMITED` field must therefore stay `Unrolled`. Within one run, codegen keeps the type of a group field unrolled itself. - `clear()` resets a table message to `Default`, so it releases the capacity of its strings and vectors instead of keeping it. -- A child that has no table, encoded into a sink that is not a `BufMut`, such as `Rope`, is staged in a buffer first, so the `bytes` fields inside it are copied and not shared by reference count. +- Encoding into any sink other than the cursor that `Message::encode` and its siblings write a `BufMut` through stages each child reached through its `Message` impl in a scratch buffer first. Those sinks are a `Rope`, a sink defined outside `buffa`, and a `BufMut` passed straight to `Message::write_to`. A `Rope` copies the child again and cannot share the `bytes` fields inside it by reference count. +- Codegen cannot see the fields of a message from another crate, so a table message that holds one with a `bytes` field of a non-default type copies it on decode. Set the holder to `Unrolled`. These stay unrolled, whatever the setting: @@ -1128,13 +1129,14 @@ These stay unrolled, whatever the setting: - the message type of a group field; - a message that uses the `MessageSet` wire format; - a message with extension ranges, when JSON code is generated and unknown fields are preserved; -- a message with a field of a non-default string, bytes, or collection type, which `use_bytes_type`, `string_type`, `bytes_type`, and `repeated_type` select. +- a message with a field of a non-default string, bytes, or collection type, which `use_bytes_type`, `string_type`, `bytes_type`, and `repeated_type` select; +- a message that holds, in a singular, repeated, `oneof`, or map value field, a message of the same run that has a `bytes` field of a non-default type, or that holds one. The table decodes from one contiguous slice, so it would copy `bytes::Bytes` fields that unrolled code decoding from a `Bytes` shares with the input. Codegen prints one warning per run that counts the messages that fell back, groups them by reason, and names a few of each. Setting the messages that cause a fallback to `Unrolled` with `codec_strategy_in` silences it. A `codec_strategy_in` rule that selects the table for a message by its exact path, when the message cannot use it, is an error, because the rule asked for something impossible. The table code needs Rust 1.77 or later; `buffa-build` returns an error on an older compiler, and the plugin's output does not compile on one. It contains `unsafe` code, in macros inside `buffa`, so the generated code compiles in a crate with `#![forbid(unsafe_code)]`. The `buffa::table` module the code calls may change in any release, so regenerate the code whenever you update `buffa`; a mismatch is a compile error. -`compute_size`, decoding from a contiguous buffer, and encoding into a `BufMut` are compiled in `buffa`, at the `opt-level` `buffa` is built with. A build that sets `opt-level = "z"` for everything can spend a little size to recover speed with `[profile.release.package.buffa] opt-level = 3`. Encoding into a sink that is not a `BufMut`, such as `Rope`, and the generic wrappers around decoding are compiled in your crate. +`compute_size`, decoding from a contiguous buffer, and encoding into a `BufMut` are compiled in `buffa`, at the `opt-level` `buffa` is built with. A build that sets `opt-level = "z"` for everything can spend a little size to recover speed with `[profile.release.package.buffa] opt-level = 3`. Encoding into any other sink, such as a `Rope`, and the generic wrappers around decoding are compiled in your crate. ### Error handling From fbe36a44f08dfd8d10a2f9113e9ec8b17162e1c0 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 15:00:03 -0700 Subject: [PATCH 10/11] docs: name the bytes copy for messages of other crates, and fix stale wording A table message that holds a message of another crate with a bytes field, such as google.protobuf.Any, copies the field on decode. Say so in the CodecStrategy::Table docs, the guide, DESIGN.md and the runtime docs, and point at the guide for the measurements. Correct the bridge module docs (dangling list, renamed constructors, Rope copy), qualify the size claim, drop the fallback warning's claim that a fallback is larger, and shorten the changelog fragment to a user-level summary. --- .../added-20260923-codec-strategy-table.yaml | 2 +- DESIGN.md | 4 +-- buffa-build/src/lib.rs | 14 +++++----- buffa-codegen/src/lib.rs | 20 +++++++------- buffa-codegen/src/table_plan.rs | 9 ++++--- buffa-test/protos/table_bytes.proto | 9 ++++--- buffa-test/src/lib.rs | 6 ++--- buffa/src/table/bridge.rs | 26 +++++++++++-------- buffa/src/table/mod.rs | 4 ++- buffa/src/table/shape.rs | 7 +++-- docs/guide.md | 12 ++++----- protoc-gen-buffa/src/main.rs | 4 +-- 12 files changed, 65 insertions(+), 52 deletions(-) diff --git a/.changes/unreleased/added-20260923-codec-strategy-table.yaml b/.changes/unreleased/added-20260923-codec-strategy-table.yaml index 69b48b4d..757b250f 100644 --- a/.changes/unreleased/added-20260923-codec-strategy-table.yaml +++ b/.changes/unreleased/added-20260923-codec-strategy-table.yaml @@ -1,4 +1,4 @@ kind: Added body: |- - **Table-driven message codec** (#469, refs #463). `buffa_build::Config::codec_strategy(CodecStrategy::Table)` (plugin option `codec_strategy=table`, `CodeGenConfig::codec_strategy`) generates each message's binary `Message` implementation from a static table and interpreters that every message shares, instead of code specialised to the message's fields. On a schema the table fully covers it roughly halves the compiled size at `opt-level = "z"`; on the WhatsApp schema, where 43 of 752 messages have a `oneof` or `map` and stay unrolled, the text section is 40% smaller. Messages made of many small fields are slower. `CodecStrategy::Unrolled` stays the default. `codec_strategy_in(strategy, &[paths])` (plugin option `codec_strategy_in==`, repeatable) chooses the strategy for matching messages and the messages nested in them, and the last matching rule wins. The wire format does not change. A message that has a `oneof`, `map`, or group field stays unrolled, and so does a message that holds a message of the same run with a `bytes` field of a non-default type, because the table would copy the `bytes::Bytes` fields that unrolled code shares with the input; `CodeGenWarning::TableCodecFallbackSummary` counts them. A table message may hold any other message, and reaches one that has no table through its `Message` impl, which stages the child in a buffer when it is written to a `Rope` or another sink that is not the cursor `Message::encode` uses, and so copies it. A rule that selects the table, by exact path, for a message that cannot use it is an error. The generated code needs Rust 1.77 or later, which `buffa-build` checks, and compiles in a crate with `#![forbid(unsafe_code)]`; see the guide's "Smaller generated code" section for the rest. + **Table-driven message codec** (#469, refs #463). `buffa_build::Config::codec_strategy(CodecStrategy::Table)` (plugin option `codec_strategy=table`, `CodeGenConfig::codec_strategy`) generates each message's binary `Message` implementation from a static table and interpreters that every message shares, instead of code specialised to the message's fields. The compiled code is substantially smaller, and messages made of many small fields are slower. `CodecStrategy::Unrolled` stays the default. `codec_strategy_in(strategy, &[paths])` (plugin option `codec_strategy_in==`, repeatable) chooses the strategy for matching messages and the messages nested in them, and the last matching rule wins. The wire format does not change. Messages the table cannot handle stay unrolled, and `CodeGenWarning::TableCodecFallbackSummary` counts them. A rule that selects the table, by exact path, for a message that cannot use it is an error. The generated code needs Rust 1.77 or later, which `buffa-build` checks, and compiles in a crate with `#![forbid(unsafe_code)]`. The guide's "Smaller generated code" section has the measurements, the messages that stay unrolled and the behavioural differences. time: 2026-09-23T03:10:00+00:00 diff --git a/DESIGN.md b/DESIGN.md index af27cb12..dd8e4f92 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -558,7 +558,7 @@ Within that bound, the shortcuts do not pay off. `merge` does not help: it consu ### 13. Table-Driven Codec (`CodecStrategy::Table`) -The size, write, and merge code of an unrolled message is specialised to its fields. `CodecStrategy::Table` (a per-message option, `Unrolled` by default) replaces it with a static `buffa::table::Table` and a `Message` impl that forwards to interpreters in `buffa::table`. The measurements and the decision to keep `Unrolled` as the default are in [#463](https://github.com/anthropics/buffa/issues/463). +The size, write, and merge code of an unrolled message is specialised to its fields. `CodecStrategy::Table` (a per-message option, `Unrolled` by default) replaces it with a static `buffa::table::Table` and a `Message` impl that forwards to interpreters in `buffa::table`. The decision to keep `Unrolled` as the default is in [#463](https://github.com/anthropics/buffa/issues/463), and the guide's "Smaller generated code" section has the measurements and the list of messages that stay unrolled. A table holds a sorted array of 12-byte entries `{tag, offset, kind, tag_len, aux}`, a dense array that maps field numbers below 64 to entries, and the offset of the unknown-fields slot. `kind` is the field type crossed with its cardinality, so the interpreter dispatches once per field. Message, repeated-message, and enum fields carry a small descriptor (`Aux`) with the accessors that their storage needs, because a `MessageField`, a `Vec`, and an `EnumValue` cannot be read through an offset alone. Offsets come from `core::mem::offset_of!`, so the table needs Rust 1.77 and the generated code refers to it through `buffa::__table!`, which is a compile error on an older compiler. @@ -566,7 +566,7 @@ Three decisions shape the runtime: - **The `unsafe` lives in `buffa`.** `__table!` and `__table_entry!` contain the `unsafe` blocks and witness each field's type against its kind, so a table that names the wrong kind for a field does not compile, and generated code compiles under `#![forbid(unsafe_code)]`. `Table::new` also checks the layout constants at compile time. The interpreters run under Miri in CI. - **The interpreters are not generic over the sink or the input where that is avoidable.** `Message::encode` and its siblings write any `BufMut` through one shared, non-generic cursor (`buffa/src/encode_sink.rs`), and decoding runs over a contiguous `&[u8]`, so the interpreters are compiled once in `buffa`, at its `opt-level`, and not once per caller. -- **A child reaches the interpreters through its table or its `Message` impl.** A message field's `Aux` descriptor holds either the child's `MessageTable` or a `DynVt` of function pointers instantiated for the child type (`table/bridge.rs`), so a message can use the table whatever it holds. The write pointer takes the `PreSized` cursor because a function pointer cannot be generic over the sink. A pointer per child type and sink type would avoid the copy that other sinks cost, at the price of code in every crate that uses the table. The planner in `buffa-codegen` (`table_plan.rs`) selects the messages that asked for the table and drops those the interpreters cannot handle (oneofs, maps, groups and the types of group fields, `MessageSet`, extension ranges with JSON, custom string, bytes, or collection types). It also drops the holders of a message of the run with a non-default bytes type, transitively, because the table decodes from one contiguous slice and would copy the `bytes::Bytes` fields that unrolled code shares with a `Bytes` input. A child from another crate is not inspected. The emitter names a child's static table only if the plan gave the child one. +- **A child reaches the interpreters through its table or its `Message` impl.** A message field's `Aux` descriptor holds either the child's `MessageTable` or a `DynVt` of function pointers instantiated for the child type (`table/bridge.rs`), so a table message can hold any message it is not required to leave unrolled. The write pointer takes the `PreSized` cursor because a function pointer cannot be generic over the sink. A pointer per child type and sink type would avoid the copy that other sinks cost, at the price of code in every crate that uses the table. The planner in `buffa-codegen` (`table_plan.rs`) selects the messages that asked for the table and drops those the interpreters cannot handle. Among them it drops, transitively, the holders of a message of the run with a non-default bytes type, because the table decodes from one contiguous slice and would copy the `bytes::Bytes` fields that unrolled code shares with a `Bytes` input. A child from another crate is not inspected, so a holder of `google.protobuf.Any`, whose `value` is `bytes::Bytes`, keeps its table and copies the payload. ### Owned decode: intentional throughput trade-offs diff --git a/buffa-build/src/lib.rs b/buffa-build/src/lib.rs index bbde39e9..fc37e132 100644 --- a/buffa-build/src/lib.rs +++ b/buffa-build/src/lib.rs @@ -1554,13 +1554,13 @@ impl Config { /// Choose how the binary `Message` implementation of every message is /// generated (default: [`CodecStrategy::Unrolled`]). /// - /// On a schema it fully covers, [`CodecStrategy::Table`] makes the - /// compiled size about half as big at `opt-level = "z"`, and it slows - /// messages made of many small fields; a message that cannot use it keeps - /// its size. [`CodecStrategy::Table`] says which messages stay unrolled and - /// how a table message behaves differently, and this build reports the - /// messages that stay unrolled in one `cargo:warning`. The option never - /// changes the wire format. The generated code needs Rust 1.77 or later, and `compile` + /// On a large schema, [`CodecStrategy::Table`] makes the compiled code + /// about 40% smaller at `opt-level = "z"`, and it slows messages made of + /// many small fields; a message that cannot use it keeps its size. + /// [`CodecStrategy::Table`] says which messages stay unrolled and how a + /// table message behaves differently, and this build reports the messages + /// that stay unrolled in one `cargo:warning`. The option never changes the + /// wire format. The generated code needs Rust 1.77 or later, and `compile` /// returns an error on an older compiler when a build script runs it (the /// compiler is read from `RUSTC`). /// diff --git a/buffa-codegen/src/lib.rs b/buffa-codegen/src/lib.rs index 1ee2b25f..7f91174e 100644 --- a/buffa-codegen/src/lib.rs +++ b/buffa-codegen/src/lib.rs @@ -1086,11 +1086,11 @@ pub enum CodecStrategy { #[default] Unrolled, /// Emit one static table per message and forward `Message` to interpreters - /// shared by every message, in the `buffa` crate. On a schema the table - /// fully covers, it roughly halves the compiled size at - /// `opt-level = "z"`; a message that cannot use it keeps its size. It costs - /// run time on messages made of many small fields. The measurements are in - /// anthropics/buffa#463. + /// shared by every message, in the `buffa` crate. On a large schema it + /// makes the compiled code about 40% smaller at `opt-level = "z"`; a + /// message that cannot use it keeps its size. It costs run time on messages + /// made of many small fields. The guide's "Smaller generated code" section + /// has the measurements. /// /// Not every message can use it. These stay [`Unrolled`](Self::Unrolled): /// @@ -1115,7 +1115,7 @@ pub enum CodecStrategy { /// function call per child. Selecting a message does not select the /// messages it holds, and keeping a child [`Unrolled`](Self::Unrolled) does /// not keep the messages that hold it unrolled: to keep a whole path - /// specialised, select its holders too. + /// specialised, set its holders to `Unrolled` too. /// /// A table message differs from an unrolled one in these ways. A field that /// declares a length past the end of its enclosing message fails at once @@ -1132,8 +1132,10 @@ pub enum CodecStrategy { /// buffer, and a `Rope` copies it again and cannot share the `bytes` /// fields inside it by reference count. Codegen cannot see the fields of a /// message from another crate, so a table message that holds one with a - /// `bytes` field of a non-default type copies it on decode; set the holder - /// to [`Unrolled`](Self::Unrolled) by hand. + /// `bytes` field of a non-default type copies it on decode. The well-known + /// type `google.protobuf.Any` is one, because its `value` is + /// `bytes::Bytes`. To keep the payload shared with a `Bytes` input, set the + /// holder to [`Unrolled`](Self::Unrolled) with `codec_strategy_in`. /// /// A message that another crate or codegen run uses as the type of a group /// or editions `DELIMITED` field must not be selected, because @@ -2574,7 +2576,7 @@ impl core::fmt::Display for CodeGenWarning { write!( f, "{fallbacks} of {selected} messages selected for the table codec use the \ - unrolled codec instead, which works but is larger" + unrolled codec instead" )?; for (i, r) in reasons.iter().enumerate() { let sep = if i == 0 { ": " } else { "; " }; diff --git a/buffa-codegen/src/table_plan.rs b/buffa-codegen/src/table_plan.rs index edb9a77b..3bfe446e 100644 --- a/buffa-codegen/src/table_plan.rs +++ b/buffa-codegen/src/table_plan.rs @@ -3,10 +3,11 @@ //! //! A message can use the table if the interpreters in `buffa::table` cover //! every field it has, with one exception that depends on the messages it -//! holds: a child stored with a non-default bytes type would lose its -//! zero-copy decode inside a table message, so the messages that hold one stay -//! unrolled. Any other child is reached through its table or its `Message` -//! impl, so it need not be a table message itself. +//! holds: a message with a bytes field of a non-default type would lose its +//! zero-copy decode inside a table message, so the messages that hold one, +//! directly or through other messages, stay unrolled. Any other child is +//! reached through its table or its `Message` impl, so it need not be a table +//! message itself. use std::collections::{HashMap, HashSet}; diff --git a/buffa-test/protos/table_bytes.proto b/buffa-test/protos/table_bytes.proto index 67858e8d..96e0347f 100644 --- a/buffa-test/protos/table_bytes.proto +++ b/buffa-test/protos/table_bytes.proto @@ -1,10 +1,11 @@ syntax = "proto3"; // build.rs compiles this once, as package `tbz`, with `codec_strategy = Table` -// and `bytes` fields stored as `bytes::Bytes`. `Blob` has such a field, so it -// is unrolled, and so are `HoldsBlob` and `HoldsBlobs`, which hold it: a table -// decodes from a contiguous slice, where `Bytes` fields could not alias the -// input. `Plain` and `HoldsPlain` have no `bytes` field and are tables. +// and `bytes` fields stored as `bytes::Bytes`. A table decodes from a +// contiguous slice, where `Bytes` fields could not alias the input, so `Blob`, +// which has such fields, is unrolled, and so are `HoldsBlob`, which holds +// `Blob`, and `HoldsBlobs`, which holds `HoldsBlob`. `Plain` and `HoldsPlain` +// have no `bytes` field and are tables. package tbz; message Blob { diff --git a/buffa-test/src/lib.rs b/buffa-test/src/lib.rs index b57d2f7c..3910f88c 100644 --- a/buffa-test/src/lib.rs +++ b/buffa-test/src/lib.rs @@ -1160,7 +1160,7 @@ pub mod tcx { } // `bru` is `table_bridge.proto` unrolled, and `brt` has the table codec except -// for `Hot`, so tables and unrolled messages hold each other. `xe` is a package +// for `Hot`, so table and unrolled messages hold each other. `xe` is a package // of table messages that `xfu` (unrolled) and `xft` (table) hold through an // `extern_path`. #[allow(clippy::derivable_impls, clippy::match_single_binding)] @@ -1189,8 +1189,8 @@ pub mod xft { buffa::include_proto!("xft"); } -// `tbz` has `bytes` fields stored as `bytes::Bytes`, and the table codec, which -// the messages that hold those fields must not use. +// `tbz` has the table codec and `bytes` fields stored as `bytes::Bytes`. The +// messages that have or hold such a field stay unrolled. #[cfg(has_table_codec)] pub mod tbz { buffa::include_proto!("tbz"); diff --git a/buffa/src/table/bridge.rs b/buffa/src/table/bridge.rs index abe532f2..15265f0a 100644 --- a/buffa/src/table/bridge.rs +++ b/buffa/src/table/bridge.rs @@ -1,8 +1,8 @@ //! Message-typed fields whose message is not a table message. //! //! A table records how to size, write and decode the child of each message -//! field. A child that has a table is reached through it. Any other child, an -//! unrolled message, one generated by another crate or a well-known type, is +//! field. A child that has a table is reached through it. Any other child (an +//! unrolled message, one generated by another crate, or a well-known type) is //! reached through its [`Message`] impl by the function pointers of a //! [`DynVt`], one per operation, compiled once per child type. It keeps its //! own codec, so it writes the same bytes and accepts the same input as it @@ -10,16 +10,20 @@ //! //! A pointer cannot be generic over the write sink, so the write function //! takes the [`PreSized`] cursor, which `Message::encode` and its siblings -//! write every `BufMut` through. Any other sink gets the child from a scratch -//! `Vec` allocated for it, for each element of a repeated field, and copied -//! to the sink: a [`Rope`](crate::Rope), a sink defined outside this crate, -//! and a `BufMut` passed straight to `Message::write_to`. A `Rope` copies the -//! bytes again unless they lie in its backing buffer, and it cannot share -//! the `bytes` fields inside the child by reference count. +//! write every `BufMut` through. Any other sink (a [`Rope`](crate::Rope), a +//! sink defined outside this crate, or a `BufMut` passed straight to +//! `Message::write_to`) gets the child from a scratch `Vec` allocated for it, +//! once per element of a repeated field, and the scratch is copied to the +//! sink. A `Rope` therefore copies the child a second time into its tail, and +//! cannot share the `bytes` fields inside it by reference count. //! //! Decoding reads a child from the slice of the nearest enclosing table //! message, so the read of a child that overruns is bounded there and not by -//! the whole input. +//! the whole input. Every `bytes::Bytes` field of the child is copied out of +//! that slice, where unrolled code decoding from a `Bytes` shares it with the +//! input. Code generation keeps the holders of such a message of its own run +//! unrolled, but it cannot see into another crate, so a child from another +//! crate that has a `Bytes` field, such as `google.protobuf.Any`, is copied. use core::marker::PhantomData; @@ -29,8 +33,8 @@ use crate::encoding::encode_varint; use crate::{DecodeContext, DecodeError, EncodeSink, Message, SizeCache}; /// The operations of one child message type, reached through its [`Message`] -/// impl. Built by [`MsgVt::new_dyn`](super::MsgVt::new_dyn) and -/// [`RepVt::new_dyn`](super::RepVt::new_dyn). +/// impl. Built by [`MsgVt::new_via_message`](super::MsgVt::new_via_message) and +/// [`RepVt::new_via_message`](super::RepVt::new_via_message). pub(super) struct DynVt { /// The encoded size of the child, recording nested sizes in the cache. /// diff --git a/buffa/src/table/mod.rs b/buffa/src/table/mod.rs index 00febef9..e8bca0cf 100644 --- a/buffa/src/table/mod.rs +++ b/buffa/src/table/mod.rs @@ -48,7 +48,9 @@ //! [`MsgVt::new_via_message`]) has the same wire format and accepts the same //! input, but it is staged in a scratch buffer and copied when it is written //! to any sink other than the cursor that `Message::encode` and its siblings -//! write a [`BufMut`](crate::bytes::BufMut) through. +//! write a [`BufMut`](crate::bytes::BufMut) through, and its `bytes::Bytes` +//! fields are copied out of the slice it is decoded from, where unrolled +//! code decoding from a `Bytes` shares them with the input. //! //! # Where the code is compiled //! diff --git a/buffa/src/table/shape.rs b/buffa/src/table/shape.rs index b329c495..903bf20c 100644 --- a/buffa/src/table/shape.rs +++ b/buffa/src/table/shape.rs @@ -89,7 +89,9 @@ impl MsgVt { /// Describe a field of type `F` whose message is reached through its /// [`Message`] impl, for a message whose table is not visible here. /// Prefer [`MsgVt::new`] when it is, because the interpreters then decode - /// the child without a function call. + /// the child without a function call. The child is decoded from a slice, + /// so its `bytes::Bytes` fields are copied, where unrolled code decoding + /// from a `Bytes` shares them. #[must_use] pub const fn new_via_message() -> Self where @@ -176,7 +178,8 @@ impl RepVt { } /// Describe a `Vec` field whose messages are reached through their - /// [`Message`] impl; see [`MsgVt::new_via_message`]. + /// [`Message`] impl. Like [`MsgVt::new_via_message`], it copies the + /// `bytes::Bytes` fields of the elements when it decodes. #[must_use] pub const fn new_via_message() -> Self { Self { diff --git a/docs/guide.md b/docs/guide.md index 046dfa88..db95d543 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -216,7 +216,7 @@ The macro pulls in `OUT_DIR/.mod.rs`, which in turn includes the per | `.extern_path(proto, rust)` | — | Map a proto package or a single type to an external Rust path (see below) | | `.exclude_package(pkg)` | — | Drop a proto package (and its sub-packages) from code generation. Useful when directory globbing pulls in option-only packages (e.g. `buf.validate`) that you don't want Rust types for. A leading dot is accepted and stripped. Pair with `.extern_path` if kept files reference types from the excluded package; the generator emits a `cargo:warning` for each such cross-package reference. | | `.type_name_prefix(prefix)` | `""` | Prepend a PascalCase prefix (`[A-Z][A-Za-z0-9]*`; anything else is rejected at generation time) to every generated message/enum type name (`message User` → `struct RpcUser`); modules, oneof enums, extern-mapped types, and the wire format are unaffected. A crate referencing these types via `extern_path` must spell out the prefixed name (`::crate_a::RpcUser`) | -| `.codec_strategy(strategy)` | `Unrolled` | Generate each message's binary `Message` code specialised to its fields (`CodecStrategy::Unrolled`), or from a static table and interpreters shared by every message (`CodecStrategy::Table`), which on a schema it fully covers is about half the compiled size and slower on messages of many small fields; see [Smaller generated code](#smaller-generated-code-codec_strategy) | +| `.codec_strategy(strategy)` | `Unrolled` | Generate each message's binary `Message` code specialised to its fields (`CodecStrategy::Unrolled`), or from a static table and interpreters shared by every message (`CodecStrategy::Table`), which makes a large schema about 40% smaller and is slower on messages of many small fields; see [Smaller generated code](#smaller-generated-code-codec_strategy) | | `.codec_strategy_in(strategy, &[...])` | — | Choose the strategy for matching messages and the messages nested in them (proto-path prefixes; the last matching rule wins), on top of the global setting | | `.use_bytes_type()` | — | Use `bytes::Bytes` for all bytes fields, including `map` values | | `.use_bytes_type_in(&[...])` | — | Use `bytes::Bytes` for matching bytes fields (same `map` rule) | @@ -632,7 +632,7 @@ Passed via `opt:` (works for `remote:` and `local:`): | `idiomatic_enum_aliases=false` | Omit the `UpperCamelCase` associated-const aliases for enum values (`Status::Active`); the `SHOUTY_SNAKE_CASE` variants are unaffected (default: emitted). See [Enums](#enumvaluet--type-safe-open-enums) | | `override_feature_in==:` | Apply a path-scoped editions feature override (currently `enum_type:OPEN`) to the compiled descriptors. Repeatable | | `open_enums_in=` | Shorthand for `override_feature_in==enum_type:OPEN`. Repeatable | -| `codec_strategy=table` | Generate every message's binary `Message` code from a static table and shared interpreters instead of code specialised to its fields (default `unrolled`). The plugin cannot check the compiler version: on Rust before 1.77 the generated code fails to compile. See [Smaller generated code](#smaller-generated-code-codec_strategy) | +| `codec_strategy=table` | Generate each message's binary `Message` code from a static table and shared interpreters instead of code specialised to its fields, for the messages the table can handle (default `unrolled`). The plugin cannot check the compiler version: on Rust before 1.77 the generated code fails to compile. See [Smaller generated code](#smaller-generated-code-codec_strategy) | | `codec_strategy_in==` | Choose `table` or `unrolled` for matching messages and the messages nested in them. Repeatable; leading dot optional; the last matching rule wins | | `unbox_oneof=true` | Store every non-recursive message/group oneof variant inline instead of `Box`. Recursive variants stay boxed. | | `unbox_oneof_in=` | Store matching non-recursive message/group oneof variants inline instead of `Box`. Repeatable; leading dot optional. Use `.` to match all variants. Recursive variants stay boxed for broad matches; exact recursive matches are rejected. | @@ -1100,7 +1100,7 @@ Buffa uses a two-pass model to avoid the exponential-time size computation that ### Smaller generated code: `codec_strategy` -By default every generated message contains its own size, write, and merge code, specialised to its fields. `CodecStrategy::Table` replaces it with one static table per message and interpreters in `buffa` that every message shares. On the WhatsApp schema (`whatsapp.proto` from `waproto`: 334 top-level messages, 752 with the nested ones, 3,477 fields), built with `Box` message fields, no unknown-field preservation, fat LTO, and `panic = "abort"`, the text section of the binary at `opt-level = "z"` went from 1,670 KB to 1,010 KB (−40%). 43 of its 752 messages (23 of 334 top-level) stay unrolled because they have a `oneof` or a `map`. The 1,644 KB to 817 KB figure in [#463](https://github.com/anthropics/buffa/issues/463), which also gives the method, was measured on that schema with its oneofs and maps flattened into plain fields, so that every message could use the table. The cost is speed on messages made of many small fields, where encoding takes up to about 3.5 times as long as with the default `CodecStrategy::Unrolled` and decoding up to 1.6 times. Messages dominated by bulk data, such as large strings, bytes, and packed arrays, show no difference. +By default every generated message contains its own size, write, and merge code, specialised to its fields. `CodecStrategy::Table` replaces it with one static table per message and interpreters in `buffa` that every message shares. On the WhatsApp schema (`whatsapp.proto` from `waproto`: 334 top-level messages, 752 with the nested ones, 3,477 fields), built with `Box` message fields, no unknown-field preservation, fat LTO, and `panic = "abort"`, the text section of the binary at `opt-level = "z"` went from 1,670 KB to 1,010 KB (−40%). 43 of its 752 messages (23 of 334 top-level) stay unrolled because they have a `oneof` or a `map`. [#463](https://github.com/anthropics/buffa/issues/463) describes the method. The cost is speed on messages made of many small fields, where encoding takes up to about 3.5 times as long as with the default `CodecStrategy::Unrolled` and decoding up to 1.6 times. Messages dominated by bulk data, such as large strings, bytes, and packed arrays, show no difference. ```rust,ignore // build.rs @@ -1113,7 +1113,7 @@ buffa_build::Config::new() .compile()?; ``` -A table message may hold any message. It reaches a child that has a table through it, and any other child (a message you set to `Unrolled`, one generated by another crate, or a well-known type such as `Timestamp`) through its `Message` impl, which costs a function call per child. A `codec_strategy_in` rule selects the message it names and the messages nested in it, and does not extend to the messages it holds. In the example, `.wa.Message` and `.wa.Receipt` stay specialised and the messages that hold them use the table. Setting a message to `Unrolled` therefore does not keep the messages that hold it unrolled: to keep a whole path specialised, list its holders too. +Apart from the holders listed below, a table message may hold any message. It reaches a child that has a table through it, and any other child (a message you set to `Unrolled`, one generated by another crate, or a well-known type such as `Timestamp`) through its `Message` impl, which costs a function call per child. A `codec_strategy_in` rule selects the message it names and the messages nested in it, and does not extend to the messages it holds. In the example, `.wa.Message` and `.wa.Receipt` stay specialised and the messages that hold them use the table. Setting a message to `Unrolled` therefore does not keep the messages that hold it unrolled: to keep a whole path specialised, set its holders to `Unrolled` too. The option changes only the binary `Message` implementation. The wire format and the JSON, text, view, and reflection code are the same under both strategies, and a table message encodes to the same bytes and decodes the same accepted input as its unrolled twin. It differs in these ways: @@ -1121,7 +1121,7 @@ The option changes only the binary `Message` implementation. The wire format and - The table decodes from one contiguous slice, so a `Buf` that is not contiguous is gathered into one buffer first. `Message::merge_field` on a table message cannot gather, and returns `UnexpectedEof` for such a buffer; only code that calls it directly is affected, such as the default `merge_group`. A message that another crate or another codegen run uses as the type of a group or `DELIMITED` field must therefore stay `Unrolled`. Within one run, codegen keeps the type of a group field unrolled itself. - `clear()` resets a table message to `Default`, so it releases the capacity of its strings and vectors instead of keeping it. - Encoding into any sink other than the cursor that `Message::encode` and its siblings write a `BufMut` through stages each child reached through its `Message` impl in a scratch buffer first. Those sinks are a `Rope`, a sink defined outside `buffa`, and a `BufMut` passed straight to `Message::write_to`. A `Rope` copies the child again and cannot share the `bytes` fields inside it by reference count. -- Codegen cannot see the fields of a message from another crate, so a table message that holds one with a `bytes` field of a non-default type copies it on decode. Set the holder to `Unrolled`. +- Codegen cannot see the fields of a message from another crate, so a table message that holds one with a `bytes` field of a non-default type copies it on decode. The well-known type `google.protobuf.Any` is one, because its `value` is `bytes::Bytes`. To keep the payload shared with a `Bytes` input, set the holder to `Unrolled` with `codec_strategy_in`. These stay unrolled, whatever the setting: @@ -1130,7 +1130,7 @@ These stay unrolled, whatever the setting: - a message that uses the `MessageSet` wire format; - a message with extension ranges, when JSON code is generated and unknown fields are preserved; - a message with a field of a non-default string, bytes, or collection type, which `use_bytes_type`, `string_type`, `bytes_type`, and `repeated_type` select; -- a message that holds, in a singular, repeated, `oneof`, or map value field, a message of the same run that has a `bytes` field of a non-default type, or that holds one. The table decodes from one contiguous slice, so it would copy `bytes::Bytes` fields that unrolled code decoding from a `Bytes` shares with the input. +- a message that holds, in a singular, repeated, `oneof`, or map value field, a message of the same run that has a `bytes` field of a non-default type, or that holds one. The table decodes from one contiguous slice, so it would copy the `bytes::Bytes` fields that unrolled code decoding from a `Bytes` shares with the input. Codegen prints one warning per run that counts the messages that fell back, groups them by reason, and names a few of each. Setting the messages that cause a fallback to `Unrolled` with `codec_strategy_in` silences it. A `codec_strategy_in` rule that selects the table for a message by its exact path, when the message cannot use it, is an error, because the rule asked for something impossible. diff --git a/protoc-gen-buffa/src/main.rs b/protoc-gen-buffa/src/main.rs index cf62fbbf..da94fee4 100644 --- a/protoc-gen-buffa/src/main.rs +++ b/protoc-gen-buffa/src/main.rs @@ -348,8 +348,8 @@ fn parse_config(params: &str) -> Result { .push(normalize_unbox_oneof_path(value.trim())?); } // `codec_strategy=table` generates the binary `Message` impl of - // every message from a static table and shared interpreters - // (default `unrolled`). Path-scoped rules use the repeatable + // each message the table can handle from a static table and shared + // interpreters (default `unrolled`). Path-scoped rules use the repeatable // `codec_strategy_in==`, whatever the option // order; the last matching rule wins. "codec_strategy" => codegen.codec_strategy = parse_codec_strategy(value)?, From 41f9bc902945f07a416dff3dc9083a86bce238f1 Mon Sep 17 00:00:00 2001 From: Iain McGinniss <309153+iainmcgin@users.noreply.github.com> Date: Wed, 23 Sep 2026 15:00:04 -0700 Subject: [PATCH 11/11] test: cover the bytes rule on chains, cycles, and repeated, map and nested bytes Add codegen tests for a holder declared before the message it holds, cycles with and without a bytes field, repeated bytes, map values, the bytes-key carve-out and a nested message. Add buffa-test checks that the messages set to unrolled really have no table, and a test that pins the copy of an Any payload. --- buffa-codegen/src/tests/table_codec.rs | 171 ++++++++++++++++++++++++- buffa-test/src/tests/table_codec.rs | 75 +++++++++++ 2 files changed, 243 insertions(+), 3 deletions(-) diff --git a/buffa-codegen/src/tests/table_codec.rs b/buffa-codegen/src/tests/table_codec.rs index 698ef1b8..f48633a6 100644 --- a/buffa-codegen/src/tests/table_codec.rs +++ b/buffa-codegen/src/tests/table_codec.rs @@ -413,7 +413,8 @@ fn setting_a_message_to_unrolled_does_not_affect_the_messages_that_hold_it() { ); assert_eq!(summary(&warnings).0, (1, 6)); - // With the one that cannot set to unrolled as well, nothing is left to say. + // Once the message that cannot use the table is set to unrolled too, no + // fallback is left to report. let config = CodeGenConfig { codec_strategy_in: vec![ (".t.Leaf".to_string(), CodecStrategy::Unrolled), @@ -828,8 +829,8 @@ fn a_bytes_type_for_every_field_keeps_the_holders_of_every_bytes_message_unrolle ..table_config(CodecStrategy::Table) }; let (code, _) = run_bytes(&config).unwrap(); - // With this rule `PlainBytes` has a `Bytes` field, and so is everything - // that holds it unrolled. + // With this rule `PlainBytes` has a `Bytes` field, so it stays unrolled and + // so does every message that holds it. assert_eq!(tables(&code), ["Leaf", "HoldsLeaf"]); } @@ -854,3 +855,167 @@ fn an_exact_path_rule_for_a_holder_of_a_bytes_typed_message_is_an_error() { assert!(err.contains("cannot use it"), "{err}"); assert!(err.contains("it holds `.b.Blob`"), "{err}"); } + +// --------------------------------------------------------------------------- +// How far the bytes rule reaches +// --------------------------------------------------------------------------- + +fn repeated_bytes_field(name: &str, number: i32) -> FieldDescriptorProto { + FieldDescriptorProto { + label: Some(Label::LABEL_REPEATED), + ..bytes_field(name, number) + } +} + +/// A message with the one map field `m` whose entries have a key and a value +/// of the given types. +fn message_with_map(name: &str, key: Type, value: Type) -> DescriptorProto { + let mut msg = message( + name, + vec![repeated_message_field("m", 1, &format!(".c.{name}.MEntry"))], + ); + msg.nested_type = vec![DescriptorProto { + name: Some("MEntry".to_string()), + field: vec![scalar("key", 1, key), scalar("value", 2, value)], + options: (MessageOptions { + map_entry: Some(true), + ..Default::default() + }) + .into(), + ..Default::default() + }]; + msg +} + +/// Package `c` with: +/// +/// - `ChainA` holds `ChainB` holds `ChainC` holds `Blob`, declared holder +/// first, so a holder is judged before the message it holds; +/// - `CycleP` and `CycleQ` hold each other, and `CycleQ` has a bytes field, and +/// `CycleX` and `CycleY` hold each other and have none; +/// - `RepBytes` (a repeated bytes field), `MapBytes` (a `map`) +/// and `Outer.Inner` (a nested message), each with a holder; +/// - `BytesKeyMap`, a `map`, whose values keep `Vec` whatever +/// the rule says, and its holder. Protoc rejects a bytes key, so only a +/// hand-built descriptor can have one. +fn taint_schema() -> FileDescriptorProto { + let mut outer = message("Outer", vec![]); + outer.nested_type = vec![message("Inner", vec![bytes_field("data", 1)])]; + FileDescriptorProto { + package: Some("c".to_string()), + message_type: vec![ + message("ChainA", vec![message_field("b", 1, ".c.ChainB")]), + message("ChainB", vec![message_field("c", 1, ".c.ChainC")]), + message("ChainC", vec![message_field("blob", 1, ".c.Blob")]), + message("Blob", vec![bytes_field("data", 1)]), + message("CycleP", vec![message_field("q", 1, ".c.CycleQ")]), + message( + "CycleQ", + vec![message_field("p", 1, ".c.CycleP"), bytes_field("data", 2)], + ), + message("CycleX", vec![message_field("y", 1, ".c.CycleY")]), + message("CycleY", vec![message_field("x", 1, ".c.CycleX")]), + message("RepBytes", vec![repeated_bytes_field("chunks", 1)]), + message("HoldsRepBytes", vec![message_field("r", 1, ".c.RepBytes")]), + message_with_map("MapBytes", Type::TYPE_STRING, Type::TYPE_BYTES), + message("HoldsMapBytes", vec![message_field("m", 1, ".c.MapBytes")]), + message_with_map("BytesKeyMap", Type::TYPE_BYTES, Type::TYPE_BYTES), + message( + "HoldsBytesKeyMap", + vec![message_field("m", 1, ".c.BytesKeyMap")], + ), + outer, + message("HoldsInner", vec![message_field("i", 1, ".c.Outer.Inner")]), + ], + ..proto3_file("c.proto") + } +} + +/// What the plan made of `taint_schema`. +struct Taint { + tables: Vec, + /// The messages that fell back and the messages selected. + counts: (usize, usize), + /// The messages that fell back for holding a message with a bytes type. + holders: usize, +} + +/// The table strategy with every bytes field of `taint_schema` stored as +/// `Bytes`. +fn run_taint() -> Taint { + let config = CodeGenConfig { + bytes_fields: [ + ".c.Blob.data", + ".c.CycleQ.data", + ".c.RepBytes.chunks", + ".c.MapBytes.m", + ".c.BytesKeyMap.m", + ".c.Outer.Inner.data", + ] + .map(|path| (path.to_string(), BytesRepr::Bytes)) + .to_vec(), + ..table_config(CodecStrategy::Table) + }; + let (files, warnings) = + generate_with_diagnostics(&[taint_schema()], &["c.proto".to_string()], &config).unwrap(); + let (counts, reasons) = summary(&warnings); + let holders = reasons + .iter() + .find_map(|&(reason, n)| (reason == HOLDS_BYTES).then_some(n)) + .unwrap_or(0); + Taint { + tables: tables(&joined(&files)), + counts, + holders, + } +} + +#[test] +fn a_chain_of_holders_declared_holder_first_is_unrolled_all_the_way() { + let tables = run_taint().tables; + for name in ["ChainA", "ChainB", "ChainC", "Blob"] { + assert!(!tables.contains(&name.to_string()), "{name}: {tables:?}"); + } +} + +#[test] +fn a_cycle_that_reaches_a_bytes_message_is_unrolled_and_one_that_does_not_is_not() { + let tables = run_taint().tables; + for name in ["CycleP", "CycleQ"] { + assert!(!tables.contains(&name.to_string()), "{name}: {tables:?}"); + } + for name in ["CycleX", "CycleY"] { + assert!(tables.contains(&name.to_string()), "{name}: {tables:?}"); + } +} + +#[test] +fn a_message_holding_repeated_map_or_nested_bytes_is_unrolled() { + let plan = run_taint(); + for name in ["HoldsRepBytes", "HoldsMapBytes", "HoldsInner"] { + assert!( + !plan.tables.contains(&name.to_string()), + "{name}: {:?}", + plan.tables + ); + } + // A message with a plain nested declaration is unaffected. + assert!( + plan.tables.contains(&"Outer".to_string()), + "{:?}", + plan.tables + ); + // The holders counted under the one reason are the three above, the chain + // and the cycle. + assert_eq!(plan.holders, 7); + assert_eq!(plan.counts, (13, 17)); +} + +#[test] +fn a_map_with_a_bytes_key_keeps_vec_values_so_its_holder_uses_the_table() { + let tables = run_taint().tables; + assert!( + tables.contains(&"HoldsBytesKeyMap".to_string()), + "{tables:?}" + ); +} diff --git a/buffa-test/src/tests/table_codec.rs b/buffa-test/src/tests/table_codec.rs index 5ab1028d..2b39cfd1 100644 --- a/buffa-test/src/tests/table_codec.rs +++ b/buffa-test/src/tests/table_codec.rs @@ -1126,6 +1126,81 @@ fn well_known_types_held_by_a_table_message_agree() { assert_eq!(wire, [0x0a, 0x00, 0x3a, 0x00]); } +/// The tests that compare a table message with an unrolled one that it holds +/// mean nothing if the rule that leaves the child unrolled stops matching, so +/// these check the generated code itself. +#[test] +fn the_children_set_to_unrolled_have_no_table_and_their_holders_reach_them_through_the_message_impl( +) { + let out = |file: &str| match file { + "brt" => include_str!(concat!(env!("OUT_DIR"), "/brt.rs")), + "xat" => include_str!(concat!(env!("OUT_DIR"), "/xat.rs")), + "xbt" => include_str!(concat!(env!("OUT_DIR"), "/xbt.rs")), + "xati" => include_str!(concat!(env!("OUT_DIR"), "/cross_package_idiomatic/xati.rs")), + "xbti" => include_str!(concat!(env!("OUT_DIR"), "/cross_package_idiomatic/xbti.rs")), + "xft" => include_str!(concat!(env!("OUT_DIR"), "/xft.rs")), + _ => unreachable!(), + }; + // `Hot` is unrolled, and `Cold`, which holds it, is a table. + let bridge = out("brt"); + assert!(bridge.contains("pub struct Hot ")); + assert!(!bridge.contains("__BUFFA_TABLE_Hot")); + assert!(bridge.contains("static __BUFFA_TABLE_Cold")); + assert!(bridge.contains("new_via_message")); + // `Cold` is unrolled in every layout of the cross-package schema, and + // `Holder` reaches it and the tables of `Leaf` and `Wrap` next to it. + for (dep, user) in [("xat", "xbt"), ("xati", "xbti")] { + assert!(out(dep).contains("pub struct Cold "), "{dep}"); + assert!(!out(dep).contains("__BUFFA_TABLE_Cold"), "{dep}"); + assert!(out(dep).contains("static __BUFFA_TABLE_Leaf"), "{dep}"); + assert!(out(user).contains("new_via_message"), "{user}"); + assert!(out(user).contains("static __BUFFA_TABLE_Holder"), "{user}"); + } + // The messages of `xe` come from another crate, so `Holder` cannot name + // their tables. + assert!(out("xft").contains("new_via_message")); +} + +/// `Any.value` is a `Bytes` in `buffa-types`, which codegen cannot inspect, so +/// a table message that holds an `Any` copies the payload out of the slice it +/// decodes from, where its unrolled twin shares it with a `Bytes` input. The +/// documentation names this, so it is pinned here. +#[test] +fn a_table_message_copies_the_payload_of_an_any_it_holds() { + use buffa::bytes::Bytes; + use buffa::MessageField; + use buffa_types::google::protobuf::Any; + fn any(fill: u8) -> Any { + let mut any = Any::default(); + any.type_url = "type.googleapis.com/x".into(); + any.value = Bytes::from(vec![fill; 64]); + any + } + macro_rules! sample { + ($m:ident) => { + crate::$m::Wkt { + any: MessageField::some(any(1)), + anys: vec![any(2), any(3)], + ..Default::default() + } + }; + } + let wire = assert_same_codec(&sample!(bru), &sample!(brt)); + let src = Bytes::from(wire); + let range = src.as_ptr() as usize..src.as_ptr() as usize + src.len(); + let aliases = |b: &Bytes| range.contains(&(b.as_ptr() as usize)); + + let unrolled = crate::bru::Wkt::decode(&mut src.clone()).unwrap(); + let table = crate::brt::Wkt::decode(&mut src.clone()).unwrap(); + assert_eq!(unrolled.any.as_option().unwrap().value, any(1).value); + assert_eq!(table.any.as_option().unwrap().value, any(1).value); + assert!(aliases(&unrolled.any.as_option().unwrap().value)); + assert!(unrolled.anys.iter().all(|a| aliases(&a.value))); + assert!(!aliases(&table.any.as_option().unwrap().value)); + assert!(table.anys.iter().all(|a| !aliases(&a.value))); + assert_eq!(table.anys, unrolled.anys); +} + #[test] fn messages_from_another_crate_agree() { macro_rules! sample {