Avoid calling libsecp serialization fns when calculating length

When writing out libsecp256k1 objects during serialization in a
TLV, we potentially calculate the TLV length twice before
performing the actual serialization (once when calculating the
total TLV-stream length and once when calculating the length of the
secp256k1-object-containing TLV). Because the lengths of secp256k1
objects is a constant, we'd ideally like LLVM to entirely optimize
out those calls and simply know the expected length. However,
without cross-language LTO, there is no way for LLVM to verify that
there are no side-effects of the calls to libsecp256k1, leaving
LLVM with no way to optimize them out.

This commit adds a new method to `Writeable` which returns the
length of an object once serialized. It is implemented by default
using `LengthCalculatingWriter` (which LLVM generally optimizes out
for Rust objects) and overrides it for libsecp256k1 objects.

As of this commit, on an Intel 2687W v3, the serialization
benchmarks take:

test routing::network_graph::benches::read_network_graph  ... bench: 2,035,402,164 ns/iter (+/- 1,855,357)
test routing::network_graph::benches::write_network_graph ... bench: 308,235,267 ns/iter (+/- 140,202)

Author: TheBlueMatt

lightning/src/util/ser.rs

@@ -19,7 +19,7 @@ use core::cmp;
 
 use bitcoin::secp256k1::Signature;
 use bitcoin::secp256k1::key::{PublicKey, SecretKey};
-use bitcoin::secp256k1::constants::{PUBLIC_KEY_SIZE, COMPACT_SIGNATURE_SIZE};
+use bitcoin::secp256k1::constants::{PUBLIC_KEY_SIZE, SECRET_KEY_SIZE, COMPACT_SIGNATURE_SIZE};
 use bitcoin::blockdata::script::Script;
 use bitcoin::blockdata::transaction::{OutPoint, Transaction, TxOut};
 use bitcoin::consensus;
@@ -185,6 +185,16 @@ pub trait Writeable {
 		msg.0[..2].copy_from_slice(&(len as u16 - 2).to_be_bytes());
 		msg.0
 	}
+
+	/// Gets the length of this object after it has been serialized. This can be overridden to
+	/// optimize cases where we prepend an object with its length.
+	// Note that LLVM optimizes this away in most cases! Check that it isn't before you override!
+	#[inline]
+	fn serialized_length(&self) -> usize {
+		let mut len_calc = LengthCalculatingWriter(0);
+		self.write(&mut len_calc).expect("No in-memory data may fail to serialize");
+		len_calc.0
+	}
 }
 
 impl<'a, T: Writeable> Writeable for &'a T {
@@ -561,6 +571,10 @@ impl Writeable for PublicKey {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
 		self.serialize().write(w)
 	}
+	#[inline]
+	fn serialized_length(&self) -> usize {
+		PUBLIC_KEY_SIZE
+	}
 }
 
 impl Readable for PublicKey {
@@ -575,15 +589,19 @@ impl Readable for PublicKey {
 
 impl Writeable for SecretKey {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
-		let mut ser = [0; 32];
+		let mut ser = [0; SECRET_KEY_SIZE];
 		ser.copy_from_slice(&self[..]);
 		ser.write(w)
 	}
+	#[inline]
+	fn serialized_length(&self) -> usize {
+		SECRET_KEY_SIZE
+	}
 }
 
 impl Readable for SecretKey {
 	fn read<R: Read>(r: &mut R) -> Result<Self, DecodeError> {
-		let buf: [u8; 32] = Readable::read(r)?;
+		let buf: [u8; SECRET_KEY_SIZE] = Readable::read(r)?;
 		match SecretKey::from_slice(&buf) {
 			Ok(key) => Ok(key),
 			Err(_) => return Err(DecodeError::InvalidValue),
@@ -610,6 +628,10 @@ impl Writeable for Signature {
 	fn write<W: Writer>(&self, w: &mut W) -> Result<(), ::std::io::Error> {
 		self.serialize_compact().write(w)
 	}
+	#[inline]
+	fn serialized_length(&self) -> usize {
+		COMPACT_SIGNATURE_SIZE
+	}
 }
 
 impl Readable for Signature {
@@ -666,9 +688,7 @@ impl<T: Writeable> Writeable for Option<T> {
 		match *self {
 			None => 0u8.write(w)?,
 			Some(ref data) => {
-				let mut len_calc = LengthCalculatingWriter(0);
-				data.write(&mut len_calc).expect("No in-memory data may fail to serialize");
-				BigSize(len_calc.0 as u64 + 1).write(w)?;
+				BigSize(data.serialized_length() as u64 + 1).write(w)?;
 				data.write(w)?;
 			}
 		}

lightning/src/util/ser_macros.rs

@@ -10,7 +10,7 @@
 macro_rules! encode_tlv {
 	($stream: expr, {$(($type: expr, $field: expr)),*}, {$(($optional_type: expr, $optional_field: expr)),*}) => { {
 		#[allow(unused_imports)]
-		use util::ser::{BigSize, LengthCalculatingWriter};
+		use util::ser::BigSize;
 		// Fields must be serialized in order, so we have to potentially switch between optional
 		// fields and normal fields while serializing. Thus, we end up having to loop over the type
 		// counts.
@@ -30,19 +30,15 @@ macro_rules! encode_tlv {
 			$(
 				if i == $type {
 					BigSize($type).write($stream)?;
-					let mut len_calc = LengthCalculatingWriter(0);
-					$field.write(&mut len_calc)?;
-					BigSize(len_calc.0 as u64).write($stream)?;
+					BigSize($field.serialized_length() as u64).write($stream)?;
 					$field.write($stream)?;
 				}
 			)*
 			$(
 				if i == $optional_type {
 					if let Some(ref field) = $optional_field {
 						BigSize($optional_type).write($stream)?;
-						let mut len_calc = LengthCalculatingWriter(0);
-						field.write(&mut len_calc)?;
-						BigSize(len_calc.0 as u64).write($stream)?;
+						BigSize(field.serialized_length() as u64).write($stream)?;
 						field.write($stream)?;
 					}
 				}
@@ -59,18 +55,16 @@ macro_rules! encode_varint_length_prefixed_tlv {
 		{
 			$(
 				BigSize($type).write(&mut len)?;
-				let mut field_len = LengthCalculatingWriter(0);
-				$field.write(&mut field_len)?;
-				BigSize(field_len.0 as u64).write(&mut len)?;
-				len.0 += field_len.0;
+				let field_len = $field.serialized_length();
+				BigSize(field_len as u64).write(&mut len)?;
+				len.0 += field_len;
 			)*
 			$(
 				if let Some(ref field) = $optional_field {
 					BigSize($optional_type).write(&mut len)?;
-					let mut field_len = LengthCalculatingWriter(0);
-					field.write(&mut field_len)?;
-					BigSize(field_len.0 as u64).write(&mut len)?;
-					len.0 += field_len.0;
+					let field_len = field.serialized_length();
+					BigSize(field_len as u64).write(&mut len)?;
+					len.0 += field_len;
 				}
 			)*
 		}