From: Matt Corallo Date: Sun, 29 Aug 2021 05:26:39 +0000 (+0000) Subject: Drop writer size hinting/message vec preallocation X-Git-Tag: v0.0.101~14^2~3 X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=07b674ecb1b92028ba830e8de056d669e50f3752;p=rust-lightning Drop writer size hinting/message vec preallocation In order to avoid significant malloc traffic, messages previously explicitly stated their serialized length allowing for Vec preallocation during the message serialization pipeline. This added some amount of complexity in the serialization code, but did avoid some realloc() calls. Instead, here, we drop all the complexity in favor of a fixed 2KiB buffer for all message serialization. This should not only be simpler with a similar reduction in realloc() traffic, but also may reduce heap fragmentation by allocating identically-sized buffers more often. --- diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 88826d65..835061fe 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -94,9 +94,6 @@ impl Writer for VecWriter { self.0.extend_from_slice(buf); Ok(()) } - fn size_hint(&mut self, size: usize) { - self.0.reserve_exact(size); - } } struct TestChainMonitor { diff --git a/fuzz/src/chanmon_deser.rs b/fuzz/src/chanmon_deser.rs index 933930cf..c3193034 100644 --- a/fuzz/src/chanmon_deser.rs +++ b/fuzz/src/chanmon_deser.rs @@ -18,9 +18,6 @@ impl Writer for VecWriter { self.0.extend_from_slice(buf); Ok(()) } - fn size_hint(&mut self, size: usize) { - self.0.reserve_exact(size); - } } #[inline] diff --git a/fuzz/src/msg_targets/utils.rs b/fuzz/src/msg_targets/utils.rs index 82fa739f..6325f3bf 100644 --- a/fuzz/src/msg_targets/utils.rs +++ b/fuzz/src/msg_targets/utils.rs @@ -13,13 +13,9 @@ use lightning::util::ser::Writer; pub struct VecWriter(pub Vec); impl Writer for VecWriter { fn write_all(&mut self, buf: &[u8]) -> Result<(), ::std::io::Error> { - assert!(self.0.capacity() >= self.0.len() + buf.len()); self.0.extend_from_slice(buf); Ok(()) } - fn size_hint(&mut self, size: usize) { - self.0.reserve_exact(size); - } } // We attempt to test the strictest behavior we can for a given message, however, some messages @@ -43,6 +39,7 @@ macro_rules! test_msg { msg.write(&mut w).unwrap(); assert_eq!(w.0.len(), p); + assert_eq!(msg.serialized_length(), p); assert_eq!(&r.into_inner()[..p], &w.0[..p]); } } @@ -60,6 +57,7 @@ macro_rules! test_msg_simple { if let Ok(msg) = <$MsgType as Readable>::read(&mut r) { let mut w = VecWriter(Vec::new()); msg.write(&mut w).unwrap(); + assert_eq!(msg.serialized_length(), w.0.len()); let msg = <$MsgType as Readable>::read(&mut ::std::io::Cursor::new(&w.0)).unwrap(); let mut w_two = VecWriter(Vec::new()); @@ -82,6 +80,7 @@ macro_rules! test_msg_exact { let mut w = VecWriter(Vec::new()); msg.write(&mut w).unwrap(); assert_eq!(&r.into_inner()[..], &w.0[..]); + assert_eq!(msg.serialized_length(), w.0.len()); } } } @@ -99,6 +98,7 @@ macro_rules! test_msg_hole { let mut w = VecWriter(Vec::new()); msg.write(&mut w).unwrap(); let p = w.0.len() as usize; + assert_eq!(msg.serialized_length(), p); assert_eq!(w.0.len(), p); assert_eq!(&r.get_ref()[..$hole], &w.0[..$hole]); diff --git a/lightning/src/chain/transaction.rs b/lightning/src/chain/transaction.rs index 502eb895..0219ebbe 100644 --- a/lightning/src/chain/transaction.rs +++ b/lightning/src/chain/transaction.rs @@ -75,7 +75,7 @@ impl OutPoint { } } -impl_writeable!(OutPoint, 0, { txid, index }); +impl_writeable!(OutPoint, { txid, index }); #[cfg(test)] mod tests { diff --git a/lightning/src/ln/features.rs b/lightning/src/ln/features.rs index d1f6b89d..8f251d84 100644 --- a/lightning/src/ln/features.rs +++ b/lightning/src/ln/features.rs @@ -392,7 +392,6 @@ impl InitFeatures { /// Writes all features present up to, and including, 13. pub(crate) fn write_up_to_13(&self, w: &mut W) -> Result<(), io::Error> { let len = cmp::min(2, self.flags.len()); - w.size_hint(len + 2); (len as u16).write(w)?; for i in (0..len).rev() { if i == 0 { @@ -584,12 +583,6 @@ impl Features { (byte & unknown_features) != 0 }) } - - /// The number of bytes required to represent the feature flags present. This does not include - /// the length bytes which are included in the serialized form. - pub(crate) fn byte_count(&self) -> usize { - self.flags.len() - } } impl Features { @@ -702,7 +695,6 @@ impl Features { impl Writeable for Features { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(self.flags.len() + 2); (self.flags.len() as u16).write(w)?; for f in self.flags.iter().rev() { // Swap back to big-endian f.write(w)?; @@ -835,7 +827,7 @@ mod tests { #[test] fn convert_to_context_with_unknown_flags() { // Ensure the `from` context has fewer known feature bytes than the `to` context. - assert!(InvoiceFeatures::known().byte_count() < NodeFeatures::known().byte_count()); + assert!(InvoiceFeatures::known().flags.len() < NodeFeatures::known().flags.len()); let invoice_features = InvoiceFeatures::known().set_unknown_feature_optional(); assert!(invoice_features.supports_unknown_bits()); let node_features: NodeFeatures = invoice_features.to_context(); diff --git a/lightning/src/ln/msgs.rs b/lightning/src/ln/msgs.rs index 5b49f43b..34ccc8ad 100644 --- a/lightning/src/ln/msgs.rs +++ b/lightning/src/ln/msgs.rs @@ -1053,10 +1053,7 @@ impl Readable for OptionalField { } -impl_writeable_len_match!(AcceptChannel, { - {AcceptChannel{ shutdown_scriptpubkey: OptionalField::Present(ref script), .. }, 270 + 2 + script.len()}, - {_, 270} - }, { +impl_writeable!(AcceptChannel, { temporary_channel_id, dust_limit_satoshis, max_htlc_value_in_flight_msat, @@ -1074,7 +1071,7 @@ impl_writeable_len_match!(AcceptChannel, { shutdown_scriptpubkey }); -impl_writeable!(AnnouncementSignatures, 32+8+64*2, { +impl_writeable!(AnnouncementSignatures, { channel_id, short_channel_id, node_signature, @@ -1083,7 +1080,6 @@ impl_writeable!(AnnouncementSignatures, 32+8+64*2, { impl Writeable for ChannelReestablish { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(if let OptionalField::Present(..) = self.data_loss_protect { 32+2*8+33+32 } else { 32+2*8 }); self.channel_id.write(w)?; self.next_local_commitment_number.write(w)?; self.next_remote_commitment_number.write(w)?; @@ -1121,7 +1117,6 @@ impl Readable for ChannelReestablish{ impl Writeable for ClosingSigned { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 8 + 64 + if self.fee_range.is_some() { 1+1+ 2*8 } else { 0 }); self.channel_id.write(w)?; self.fee_satoshis.write(w)?; self.signature.write(w)?; @@ -1145,40 +1140,36 @@ impl Readable for ClosingSigned { } } -impl_writeable!(ClosingSignedFeeRange, 2*8, { +impl_writeable!(ClosingSignedFeeRange, { min_fee_satoshis, max_fee_satoshis }); -impl_writeable_len_match!(CommitmentSigned, { - { CommitmentSigned { ref htlc_signatures, .. }, 32+64+2+htlc_signatures.len()*64 } - }, { +impl_writeable!(CommitmentSigned, { channel_id, signature, htlc_signatures }); -impl_writeable_len_match!(DecodedOnionErrorPacket, { - { DecodedOnionErrorPacket { ref failuremsg, ref pad, .. }, 32 + 4 + failuremsg.len() + pad.len() } - }, { +impl_writeable!(DecodedOnionErrorPacket, { hmac, failuremsg, pad }); -impl_writeable!(FundingCreated, 32+32+2+64, { +impl_writeable!(FundingCreated, { temporary_channel_id, funding_txid, funding_output_index, signature }); -impl_writeable!(FundingSigned, 32+64, { +impl_writeable!(FundingSigned, { channel_id, signature }); -impl_writeable!(FundingLocked, 32+33, { +impl_writeable!(FundingLocked, { channel_id, next_per_commitment_point }); @@ -1202,10 +1193,7 @@ impl Readable for Init { } } -impl_writeable_len_match!(OpenChannel, { - { OpenChannel { shutdown_scriptpubkey: OptionalField::Present(ref script), .. }, 319 + 2 + script.len() }, - { _, 319 } - }, { +impl_writeable!(OpenChannel, { chain_hash, temporary_channel_id, funding_satoshis, @@ -1227,54 +1215,47 @@ impl_writeable_len_match!(OpenChannel, { shutdown_scriptpubkey }); -impl_writeable!(RevokeAndACK, 32+32+33, { +impl_writeable!(RevokeAndACK, { channel_id, per_commitment_secret, next_per_commitment_point }); -impl_writeable_len_match!(Shutdown, { - { Shutdown { ref scriptpubkey, .. }, 32 + 2 + scriptpubkey.len() } - }, { +impl_writeable!(Shutdown, { channel_id, scriptpubkey }); -impl_writeable_len_match!(UpdateFailHTLC, { - { UpdateFailHTLC { ref reason, .. }, 32 + 10 + reason.data.len() } - }, { +impl_writeable!(UpdateFailHTLC, { channel_id, htlc_id, reason }); -impl_writeable!(UpdateFailMalformedHTLC, 32+8+32+2, { +impl_writeable!(UpdateFailMalformedHTLC, { channel_id, htlc_id, sha256_of_onion, failure_code }); -impl_writeable!(UpdateFee, 32+4, { +impl_writeable!(UpdateFee, { channel_id, feerate_per_kw }); -impl_writeable!(UpdateFulfillHTLC, 32+8+32, { +impl_writeable!(UpdateFulfillHTLC, { channel_id, htlc_id, payment_preimage }); -impl_writeable_len_match!(OnionErrorPacket, { - { OnionErrorPacket { ref data, .. }, 2 + data.len() } - }, { +impl_writeable!(OnionErrorPacket, { data }); impl Writeable for OnionPacket { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(1 + 33 + 20*65 + 32); self.version.write(w)?; match self.public_key { Ok(pubkey) => pubkey.write(w)?, @@ -1301,7 +1282,7 @@ impl Readable for OnionPacket { } } -impl_writeable!(UpdateAddHTLC, 32+8+8+32+4+1366, { +impl_writeable!(UpdateAddHTLC, { channel_id, htlc_id, amount_msat, @@ -1312,7 +1293,6 @@ impl_writeable!(UpdateAddHTLC, 32+8+8+32+4+1366, { impl Writeable for FinalOnionHopData { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 8 - (self.total_msat.leading_zeros()/8) as usize); self.payment_secret.0.write(w)?; HighZeroBytesDroppedVarInt(self.total_msat).write(w) } @@ -1328,7 +1308,6 @@ impl Readable for FinalOnionHopData { impl Writeable for OnionHopData { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(33); // Note that this should never be reachable if Rust-Lightning generated the message, as we // check values are sane long before we get here, though its possible in the future // user-generated messages may hit this. @@ -1429,7 +1408,6 @@ impl Readable for OnionHopData { impl Writeable for Ping { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(self.byteslen as usize + 4); self.ponglen.write(w)?; vec![0u8; self.byteslen as usize].write(w)?; // size-unchecked write Ok(()) @@ -1451,7 +1429,6 @@ impl Readable for Ping { impl Writeable for Pong { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(self.byteslen as usize + 2); vec![0u8; self.byteslen as usize].write(w)?; // size-unchecked write Ok(()) } @@ -1471,7 +1448,6 @@ impl Readable for Pong { impl Writeable for UnsignedChannelAnnouncement { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(2 + 32 + 8 + 4*33 + self.features.byte_count() + self.excess_data.len()); self.features.write(w)?; self.chain_hash.write(w)?; self.short_channel_id.write(w)?; @@ -1499,10 +1475,7 @@ impl Readable for UnsignedChannelAnnouncement { } } -impl_writeable_len_match!(ChannelAnnouncement, { - { ChannelAnnouncement { contents: UnsignedChannelAnnouncement {ref features, ref excess_data, ..}, .. }, - 2 + 32 + 8 + 4*33 + features.byte_count() + excess_data.len() + 4*64 } - }, { +impl_writeable!(ChannelAnnouncement, { node_signature_1, node_signature_2, bitcoin_signature_1, @@ -1512,13 +1485,10 @@ impl_writeable_len_match!(ChannelAnnouncement, { impl Writeable for UnsignedChannelUpdate { fn write(&self, w: &mut W) -> Result<(), io::Error> { - let mut size = 64 + self.excess_data.len(); let mut message_flags: u8 = 0; if let OptionalField::Present(_) = self.htlc_maximum_msat { - size += 8; message_flags = 1; } - w.size_hint(size); self.chain_hash.write(w)?; self.short_channel_id.write(w)?; self.timestamp.write(w)?; @@ -1557,17 +1527,13 @@ impl Readable for UnsignedChannelUpdate { } } -impl_writeable_len_match!(ChannelUpdate, { - { ChannelUpdate { contents: UnsignedChannelUpdate {ref excess_data, ref htlc_maximum_msat, ..}, .. }, - 64 + 64 + excess_data.len() + if let OptionalField::Present(_) = htlc_maximum_msat { 8 } else { 0 } } - }, { +impl_writeable!(ChannelUpdate, { signature, contents }); impl Writeable for ErrorMessage { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 2 + self.data.len()); self.channel_id.write(w)?; (self.data.len() as u16).write(w)?; w.write_all(self.data.as_bytes())?; @@ -1594,7 +1560,6 @@ impl Readable for ErrorMessage { impl Writeable for UnsignedNodeAnnouncement { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(76 + self.features.byte_count() + self.addresses.len()*38 + self.excess_address_data.len() + self.excess_data.len()); self.features.write(w)?; self.timestamp.write(w)?; self.node_id.write(w)?; @@ -1677,10 +1642,7 @@ impl Readable for UnsignedNodeAnnouncement { } } -impl_writeable_len_match!(NodeAnnouncement, <=, { - { NodeAnnouncement { contents: UnsignedNodeAnnouncement { ref features, ref addresses, ref excess_address_data, ref excess_data, ..}, .. }, - 64 + 76 + features.byte_count() + addresses.len()*(NetAddress::MAX_LEN as usize + 1) + excess_address_data.len() + excess_data.len() } - }, { +impl_writeable!(NodeAnnouncement, { signature, contents }); @@ -1724,7 +1686,6 @@ impl Writeable for QueryShortChannelIds { // Calculated from 1-byte encoding_type plus 8-bytes per short_channel_id let encoding_len: u16 = 1 + self.short_channel_ids.len() as u16 * 8; - w.size_hint(32 + 2 + encoding_len as usize); self.chain_hash.write(w)?; encoding_len.write(w)?; @@ -1752,7 +1713,6 @@ impl Readable for ReplyShortChannelIdsEnd { impl Writeable for ReplyShortChannelIdsEnd { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 1); self.chain_hash.write(w)?; self.full_information.write(w)?; Ok(()) @@ -1787,7 +1747,6 @@ impl Readable for QueryChannelRange { impl Writeable for QueryChannelRange { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 4 + 4); self.chain_hash.write(w)?; self.first_blocknum.write(w)?; self.number_of_blocks.write(w)?; @@ -1838,7 +1797,6 @@ impl Readable for ReplyChannelRange { impl Writeable for ReplyChannelRange { fn write(&self, w: &mut W) -> Result<(), io::Error> { let encoding_len: u16 = 1 + self.short_channel_ids.len() as u16 * 8; - w.size_hint(32 + 4 + 4 + 1 + 2 + encoding_len as usize); self.chain_hash.write(w)?; self.first_blocknum.write(w)?; self.number_of_blocks.write(w)?; @@ -1869,7 +1827,6 @@ impl Readable for GossipTimestampFilter { impl Writeable for GossipTimestampFilter { fn write(&self, w: &mut W) -> Result<(), io::Error> { - w.size_hint(32 + 4 + 4); self.chain_hash.write(w)?; self.first_timestamp.write(w)?; self.timestamp_range.write(w)?; diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs index 34bcda3a..e6a06b56 100644 --- a/lightning/src/ln/peer_handler.rs +++ b/lightning/src/ln/peer_handler.rs @@ -721,7 +721,7 @@ impl P /// Append a message to a peer's pending outbound/write buffer, and update the map of peers needing sends accordingly. fn enqueue_message(&self, peer: &mut Peer, message: &M) { - let mut buffer = VecWriter(Vec::new()); + let mut buffer = VecWriter(Vec::with_capacity(2048)); wire::write(message, &mut buffer).unwrap(); // crash if the write failed let encoded_message = buffer.0; diff --git a/lightning/src/util/ser.rs b/lightning/src/util/ser.rs index 0b5036bd..73f3b4dc 100644 --- a/lightning/src/util/ser.rs +++ b/lightning/src/util/ser.rs @@ -35,18 +35,13 @@ use util::byte_utils::{be48_to_array, slice_to_be48}; /// serialization buffer size pub const MAX_BUF_SIZE: usize = 64 * 1024; -/// A trait that is similar to std::io::Write but has one extra function which can be used to size -/// buffers being written into. -/// An impl is provided for any type that also impls std::io::Write which simply ignores size -/// hints. +/// A simplified version of std::io::Write that exists largely for backwards compatibility. +/// An impl is provided for any type that also impls std::io::Write. /// /// (C-not exported) as we only export serialization to/from byte arrays instead pub trait Writer { /// Writes the given buf out. See std::io::Write::write_all for more fn write_all(&mut self, buf: &[u8]) -> Result<(), io::Error>; - /// Hints that data of the given size is about the be written. This may not always be called - /// prior to data being written and may be safely ignored. - fn size_hint(&mut self, size: usize); } impl Writer for W { @@ -54,8 +49,6 @@ impl Writer for W { fn write_all(&mut self, buf: &[u8]) -> Result<(), io::Error> { ::write_all(self, buf) } - #[inline] - fn size_hint(&mut self, _size: usize) { } } pub(crate) struct WriterWriteAdaptor<'a, W: Writer + 'a>(pub &'a mut W); @@ -82,10 +75,6 @@ impl Writer for VecWriter { self.0.extend_from_slice(buf); Ok(()) } - #[inline] - fn size_hint(&mut self, size: usize) { - self.0.reserve_exact(size); - } } /// Writer that only tracks the amount of data written - useful if you need to calculate the length @@ -97,8 +86,6 @@ impl Writer for LengthCalculatingWriter { self.0 += buf.len(); Ok(()) } - #[inline] - fn size_hint(&mut self, _size: usize) {} } /// Essentially std::io::Take but a bit simpler and with a method to walk the underlying stream diff --git a/lightning/src/util/ser_macros.rs b/lightning/src/util/ser_macros.rs index 5178732c..b229383b 100644 --- a/lightning/src/util/ser_macros.rs +++ b/lightning/src/util/ser_macros.rs @@ -231,39 +231,18 @@ macro_rules! decode_tlv_stream { } macro_rules! impl_writeable { - ($st:ident, $len: expr, {$($field:ident),*}) => { + ($st:ident, {$($field:ident),*}) => { impl ::util::ser::Writeable for $st { fn write(&self, w: &mut W) -> Result<(), $crate::io::Error> { - if $len != 0 { - w.size_hint($len); - } - #[cfg(any(test, feature = "fuzztarget"))] - { - // In tests, assert that the hard-coded length matches the actual one - if $len != 0 { - let mut len_calc = ::util::ser::LengthCalculatingWriter(0); - $( self.$field.write(&mut len_calc).expect("No in-memory data may fail to serialize"); )* - assert_eq!(len_calc.0, $len); - assert_eq!(self.serialized_length(), $len); - } - } $( self.$field.write(w)?; )* Ok(()) } #[inline] fn serialized_length(&self) -> usize { - if $len == 0 || cfg!(any(test, feature = "fuzztarget")) { - let mut len_calc = 0; - $( len_calc += self.$field.serialized_length(); )* - if $len != 0 { - // In tests, assert that the hard-coded length matches the actual one - assert_eq!(len_calc, $len); - } else { - return len_calc; - } - } - $len + let mut len_calc = 0; + $( len_calc += self.$field.serialized_length(); )* + return len_calc; } } @@ -276,59 +255,6 @@ macro_rules! impl_writeable { } } } -macro_rules! impl_writeable_len_match { - ($struct: ident, $cmp: tt, ($calc_len: expr), {$({$match: pat, $length: expr}),*}, {$($field:ident),*}) => { - impl Writeable for $struct { - fn write(&self, w: &mut W) -> Result<(), $crate::io::Error> { - let len = match *self { - $($match => $length,)* - }; - w.size_hint(len); - #[cfg(any(test, feature = "fuzztarget"))] - { - // In tests, assert that the hard-coded length matches the actual one - let mut len_calc = ::util::ser::LengthCalculatingWriter(0); - $( self.$field.write(&mut len_calc).expect("No in-memory data may fail to serialize"); )* - assert!(len_calc.0 $cmp len); - assert_eq!(len_calc.0, self.serialized_length()); - } - $( self.$field.write(w)?; )* - Ok(()) - } - - #[inline] - fn serialized_length(&self) -> usize { - if $calc_len || cfg!(any(test, feature = "fuzztarget")) { - let mut len_calc = 0; - $( len_calc += self.$field.serialized_length(); )* - if !$calc_len { - assert_eq!(len_calc, match *self { - $($match => $length,)* - }); - } - return len_calc - } - match *self { - $($match => $length,)* - } - } - } - - impl ::util::ser::Readable for $struct { - fn read(r: &mut R) -> Result { - Ok(Self { - $($field: Readable::read(r)?),* - }) - } - } - }; - ($struct: ident, $cmp: tt, {$({$match: pat, $length: expr}),*}, {$($field:ident),*}) => { - impl_writeable_len_match!($struct, $cmp, (true), { $({ $match, $length }),* }, { $($field),* }); - }; - ($struct: ident, {$({$match: pat, $length: expr}),*}, {$($field:ident),*}) => { - impl_writeable_len_match!($struct, ==, (false), { $({ $match, $length }),* }, { $($field),* }); - } -} /// Write out two bytes to indicate the version of an object. /// $this_version represents a unique version of a type. Incremented whenever the type's diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 64b88acb..379f72a8 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -52,9 +52,6 @@ impl Writer for TestVecWriter { self.0.extend_from_slice(buf); Ok(()) } - fn size_hint(&mut self, size: usize) { - self.0.reserve_exact(size); - } } pub struct TestFeeEstimator {