From d6931d687e3c952b7e2067edea44be59d7ba5e81 Mon Sep 17 00:00:00 2001 From: David Frank Date: Wed, 21 Aug 2024 10:24:59 +0000 Subject: [PATCH 1/4] Optimize the performance of deserializing types containing options The recoverable_visit_some method needs to clone the Deserializer in order to backtrack in case of errors. This cloning is a very expensive operation which this PR optimizes. 1. Wrap the TypeEnv in an Rc. Since the TypeEnv is a heavy object and is barely updated after the Deserializer is constructed, it makes sense to keep a single instance of it so cloning becomes cheap. 2. Make the happy path in recoverable_visit_some as cheap as possible by recursing on self instead of the clone. In case of an error, we reset the state from the clone. --- rust/candid/src/de.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/rust/candid/src/de.rs b/rust/candid/src/de.rs index afe5b14a..bd175025 100644 --- a/rust/candid/src/de.rs +++ b/rust/candid/src/de.rs @@ -18,6 +18,7 @@ use byteorder::{LittleEndian, ReadBytesExt}; use serde::de::{self, Visitor}; use std::fmt::Write; use std::{collections::VecDeque, io::Cursor, mem::replace}; +use std::rc::Rc; const MAX_TYPE_LEN: i32 = 500; @@ -59,7 +60,7 @@ impl<'de> IDLDeserialize<'de> { env: &TypeEnv, expected_type: &Type, ) -> Result { - self.de.table.merge(env)?; + Rc::make_mut(&mut self.de.table).merge(env)?; self.de.is_untyped = true; self.deserialize_with_type(expected_type.clone()) } @@ -298,7 +299,7 @@ macro_rules! check_recursion { #[derive(Clone)] struct Deserializer<'de> { input: Cursor<&'de [u8]>, - table: TypeEnv, + table: Rc, types: VecDeque<(usize, Type)>, wire_type: Type, expect_type: Type, @@ -322,7 +323,7 @@ impl<'de> Deserializer<'de> { let (env, types) = header.to_types()?; Ok(Deserializer { input: reader, - table: env, + table: env.into(), types: types.into_iter().enumerate().collect(), wire_type: TypeInner::Unknown.into(), expect_type: TypeInner::Unknown.into(), @@ -604,14 +605,16 @@ impl<'de> Deserializer<'de> { // This is safe, because the visitor either impl Copy or is zero sized let v = unsafe { std::ptr::read(&visitor) }; let mut self_clone = self.clone(); - match v.visit_some(&mut self_clone) { + match v.visit_some(&mut *self) { Ok(v) => { - *self = self_clone; Ok(v) } Err(Error::Subtype(_)) => { - // Remember the backtracking cost - self.config = self_clone.config; + *self = Self { + // Remember the backtracking cost + config: self.config.clone(), + ..self_clone + }; self.add_cost(10)?; self.deserialize_ignored_any(serde::de::IgnoredAny)?; visitor.visit_none() From da2bf69547514443c9054a7ac5e33c45f3e4cc1b Mon Sep 17 00:00:00 2001 From: David Frank Date: Wed, 21 Aug 2024 10:30:20 +0000 Subject: [PATCH 2/4] fix imports --- rust/candid/src/de.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/rust/candid/src/de.rs b/rust/candid/src/de.rs index bd175025..f5c2f995 100644 --- a/rust/candid/src/de.rs +++ b/rust/candid/src/de.rs @@ -17,8 +17,7 @@ use binread::BinRead; use byteorder::{LittleEndian, ReadBytesExt}; use serde::de::{self, Visitor}; use std::fmt::Write; -use std::{collections::VecDeque, io::Cursor, mem::replace}; -use std::rc::Rc; +use std::{collections::VecDeque, io::Cursor, mem::replace, rc::Rc}; const MAX_TYPE_LEN: i32 = 500; From 47ee9d6d8ed562a0d086baf627364b8e4bbfe4b6 Mon Sep 17 00:00:00 2001 From: David Frank Date: Wed, 21 Aug 2024 10:35:49 +0000 Subject: [PATCH 3/4] Formatting --- rust/candid/src/de.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/rust/candid/src/de.rs b/rust/candid/src/de.rs index f5c2f995..4a91b4ef 100644 --- a/rust/candid/src/de.rs +++ b/rust/candid/src/de.rs @@ -605,9 +605,7 @@ impl<'de> Deserializer<'de> { let v = unsafe { std::ptr::read(&visitor) }; let mut self_clone = self.clone(); match v.visit_some(&mut *self) { - Ok(v) => { - Ok(v) - } + Ok(v) => Ok(v), Err(Error::Subtype(_)) => { *self = Self { // Remember the backtracking cost From f47c80a925d1a7bebba7702939fdc0834627e6ec Mon Sep 17 00:00:00 2001 From: David Frank Date: Wed, 21 Aug 2024 10:40:11 +0000 Subject: [PATCH 4/4] Linter --- rust/candid/src/de.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/candid/src/de.rs b/rust/candid/src/de.rs index 4a91b4ef..33bb9529 100644 --- a/rust/candid/src/de.rs +++ b/rust/candid/src/de.rs @@ -603,7 +603,7 @@ impl<'de> Deserializer<'de> { } // This is safe, because the visitor either impl Copy or is zero sized let v = unsafe { std::ptr::read(&visitor) }; - let mut self_clone = self.clone(); + let self_clone = self.clone(); match v.visit_some(&mut *self) { Ok(v) => Ok(v), Err(Error::Subtype(_)) => {