From fc08e4bd61c1021c8540caa9876566ed3e13d40f Mon Sep 17 00:00:00 2001 From: Kiryl Mialeshka <8974488+meskill@users.noreply.github.com> Date: Tue, 22 Oct 2024 08:59:20 +0200 Subject: [PATCH] fix(jit): remove excessive plan & field cloning (#3016) Co-authored-by: Sandipsinh Rathod Co-authored-by: Sandipsinh Dilipsinh Rathod <62684960+ssddOnTop@users.noreply.github.com> Co-authored-by: Tushar Mathur Co-authored-by: laststylebender --- Cargo.toml | 4 + benches/bench_synth.rs | 2 +- src/core/jit/context.rs | 18 ++-- src/core/jit/error.rs | 3 + src/core/jit/exec.rs | 12 +-- src/core/jit/exec_const.rs | 27 ++++-- src/core/jit/{common => fixtures}/jp.rs | 8 +- src/core/jit/{common => fixtures}/mod.rs | 0 src/core/jit/{common => fixtures}/posts.json | 0 src/core/jit/{common => fixtures}/users.json | 0 src/core/jit/mod.rs | 2 +- src/core/jit/request.rs | 8 +- src/core/jit/synth/synth.rs | 63 ++++++------- src/core/jit/transform/check_const.rs | 3 +- src/core/jit/transform/check_dedupe.rs | 4 +- src/core/jit/transform/input_resolver.rs | 96 +++++++++++--------- 16 files changed, 135 insertions(+), 115 deletions(-) rename src/core/jit/{common => fixtures}/jp.rs (95%) rename src/core/jit/{common => fixtures}/mod.rs (100%) rename src/core/jit/{common => fixtures}/posts.json (100%) rename src/core/jit/{common => fixtures}/users.json (100%) diff --git a/Cargo.toml b/Cargo.toml index 42d9c17e46..41e5ee0aca 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -270,6 +270,10 @@ debug = false incremental = false overflow-checks = false +[profile.benchmark] +inherits = "release" +debug = true + [profile.release.package.tailcall-cloudflare] strip = true codegen-units = 1 diff --git a/benches/bench_synth.rs b/benches/bench_synth.rs index da69c0a990..1ddad96992 100644 --- a/benches/bench_synth.rs +++ b/benches/bench_synth.rs @@ -1,5 +1,5 @@ use criterion::Criterion; -use tailcall::core::jit::common::JP; +use tailcall::core::jit::fixtures::JP; pub fn bench_synth_nested(c: &mut Criterion) { c.bench_function("synth_nested", |b| { diff --git a/src/core/jit/context.rs b/src/core/jit/context.rs index 5c865b9369..41dddc368e 100644 --- a/src/core/jit/context.rs +++ b/src/core/jit/context.rs @@ -8,21 +8,21 @@ use super::error::*; use super::{Field, OperationPlan, Positioned}; use crate::core::ir::{ResolverContextLike, SelectionField}; -#[derive(Debug, Clone)] -pub struct RequestContext { - plan: OperationPlan, +#[derive(Debug)] +pub struct RequestContext<'a, Input> { + plan: &'a OperationPlan, errors: Arc>>>, } -impl RequestContext { - pub fn new(plan: OperationPlan) -> Self { +impl<'a, Input> RequestContext<'a, Input> { + pub fn new(plan: &'a OperationPlan) -> Self { Self { plan, errors: Arc::new(Mutex::new(vec![])) } } pub fn add_error(&self, new_error: Positioned) { self.errors().push(new_error); } pub fn plan(&self) -> &OperationPlan { - &self.plan + self.plan } pub fn errors(&self) -> MutexGuard>> { self.errors.lock().unwrap() @@ -37,7 +37,7 @@ pub struct Context<'a, Input, Output> { // TODO: remove the args, since they're already present inside the fields and add support for // default values. field: &'a Field, - request: &'a RequestContext, + request: &'a RequestContext<'a, Input>, } impl<'a, Input: Clone, Output> Context<'a, Input, Output> { pub fn new(field: &'a Field, request: &'a RequestContext) -> Self { @@ -136,7 +136,7 @@ mod test { fn test_field() { let plan = setup("query {posts {id title}}").unwrap(); let field = &plan.selection; - let env = RequestContext::new(plan.clone()); + let env = RequestContext::new(&plan); let ctx = Context::::new(&field[0], &env); let expected = as ResolverContextLike>::field(&ctx).unwrap(); insta::assert_debug_snapshot!(expected); @@ -145,7 +145,7 @@ mod test { #[test] fn test_is_query() { let plan = setup("query {posts {id title}}").unwrap(); - let env = RequestContext::new(plan.clone()); + let env = RequestContext::new(&plan); let ctx = Context::new(&plan.selection[0], &env); assert!(ctx.is_query()); } diff --git a/src/core/jit/error.rs b/src/core/jit/error.rs index fdff5ef18f..7a92d1f740 100644 --- a/src/core/jit/error.rs +++ b/src/core/jit/error.rs @@ -51,6 +51,8 @@ pub enum Error { Validation(#[from] ValidationError), #[error("{0}")] ServerError(async_graphql::ServerError), + #[error("Unexpected error")] + Unknown, } impl ErrorExtensions for Error { @@ -61,6 +63,7 @@ impl ErrorExtensions for Error { Error::IR(error) => error.extend(), Error::Validation(error) => error.extend(), Error::ServerError(error) => error.extend(), + Error::Unknown => async_graphql::Error::new(self.to_string()), } } } diff --git a/src/core/jit/exec.rs b/src/core/jit/exec.rs index 7dcb2f7371..0381bb53c4 100644 --- a/src/core/jit/exec.rs +++ b/src/core/jit/exec.rs @@ -18,18 +18,18 @@ type SharedStore = Arc { - ctx: RequestContext, +pub struct Executor<'a, IRExec, Input> { + ctx: RequestContext<'a, Input>, exec: IRExec, } -impl Executor +impl<'a, Input, Output, Exec> Executor<'a, Exec, Input> where Output: for<'b> JsonLike<'b> + Debug + Clone, Input: Clone + Debug, Exec: IRExecutor, { - pub fn new(plan: OperationPlan, exec: Exec) -> Self { + pub fn new(plan: &'a OperationPlan, exec: Exec) -> Self { Self { exec, ctx: RequestContext::new(plan) } } @@ -42,7 +42,7 @@ where store } - pub async fn execute(self, synth: Synth) -> Response { + pub async fn execute(self, synth: Synth<'a, Output>) -> Response { let mut response = Response::new(synth.synthesize()); response.add_errors(self.ctx.errors().clone()); response @@ -53,7 +53,7 @@ where struct ExecutorInner<'a, Input, Output, Error, Exec> { store: SharedStore, ir_exec: &'a Exec, - request: &'a RequestContext, + request: &'a RequestContext<'a, Input>, } impl<'a, Input, Output, Error, Exec> ExecutorInner<'a, Input, Output, Error, Exec> diff --git a/src/core/jit/exec_const.rs b/src/core/jit/exec_const.rs index c407934655..7f08af65df 100644 --- a/src/core/jit/exec_const.rs +++ b/src/core/jit/exec_const.rs @@ -45,10 +45,17 @@ impl ConstValueExecutor { let is_const = self.plan.is_const; // Attempt to skip unnecessary fields - let plan = transform::Skip::new(variables) - .transform(self.plan.clone()) + let Ok(plan) = transform::Skip::new(variables) + .transform(self.plan) .to_result() - .unwrap_or(self.plan); + else { + // this shouldn't actually ever happen + return Response { + data: None, + errors: vec![Positioned::new(Error::Unknown, Pos { line: 0, column: 0 })], + extensions: Default::default(), + }; + }; // Attempt to replace variables in the plan with the actual values // TODO: operation from [ExecutableDocument] could contain definitions for @@ -71,16 +78,16 @@ impl ConstValueExecutor { } }; - // TODO: drop the clones in plan - let exec = ConstValueExec::new(plan.clone(), req_ctx); + let exec = ConstValueExec::new(&plan, req_ctx); + // PERF: remove this particular clone? let vars = request.variables.clone(); - let exe = Executor::new(plan.clone(), exec); + let exe = Executor::new(&plan, exec); let store = exe.store().await; - let synth = Synth::new(plan, store, vars); + let synth = Synth::new(&plan, store, vars); let response = exe.execute(synth).await; // Cache the response if we know the output is always the same - if is_const && self.response.is_none() { + if is_const { self.response = Some(response.clone()); } @@ -89,12 +96,12 @@ impl ConstValueExecutor { } struct ConstValueExec<'a> { - plan: OperationPlan, + plan: &'a OperationPlan, req_context: &'a RequestContext, } impl<'a> ConstValueExec<'a> { - pub fn new(plan: OperationPlan, req_context: &'a RequestContext) -> Self { + pub fn new(plan: &'a OperationPlan, req_context: &'a RequestContext) -> Self { Self { req_context, plan } } diff --git a/src/core/jit/common/jp.rs b/src/core/jit/fixtures/jp.rs similarity index 95% rename from src/core/jit/common/jp.rs rename to src/core/jit/fixtures/jp.rs index c57e9a2ece..e7d44f18e3 100644 --- a/src/core/jit/common/jp.rs +++ b/src/core/jit/fixtures/jp.rs @@ -117,11 +117,11 @@ impl<'a, Value: Deserialize<'a> + Clone + 'a + JsonLike<'a> + std::fmt::Debug> J pub fn synth(&'a self) -> Synth { let ProcessedTestData { posts, users } = self.test_data.to_processed(); - let plan = self.plan.clone(); let vars = self.vars.clone(); - let posts_id = plan.find_field_path(&["posts"]).unwrap().id.to_owned(); - let users_id = plan + let posts_id = self.plan.find_field_path(&["posts"]).unwrap().id.to_owned(); + let users_id = self + .plan .find_field_path(&["posts", "user"]) .unwrap() .id @@ -134,6 +134,6 @@ impl<'a, Value: Deserialize<'a> + Clone + 'a + JsonLike<'a> + std::fmt::Debug> J store }); - Synth::new(plan, store, vars) + Synth::new(&self.plan, store, vars) } } diff --git a/src/core/jit/common/mod.rs b/src/core/jit/fixtures/mod.rs similarity index 100% rename from src/core/jit/common/mod.rs rename to src/core/jit/fixtures/mod.rs diff --git a/src/core/jit/common/posts.json b/src/core/jit/fixtures/posts.json similarity index 100% rename from src/core/jit/common/posts.json rename to src/core/jit/fixtures/posts.json diff --git a/src/core/jit/common/users.json b/src/core/jit/fixtures/users.json similarity index 100% rename from src/core/jit/common/users.json rename to src/core/jit/fixtures/users.json diff --git a/src/core/jit/mod.rs b/src/core/jit/mod.rs index 8c90bb5d21..9f945db599 100644 --- a/src/core/jit/mod.rs +++ b/src/core/jit/mod.rs @@ -14,7 +14,7 @@ mod response; // NOTE: Only used in tests and benchmarks mod builder; -pub mod common; +pub mod fixtures; mod graphql_executor; // Public Exports diff --git a/src/core/jit/request.rs b/src/core/jit/request.rs index 7a4d7d030f..9f5f02306a 100644 --- a/src/core/jit/request.rs +++ b/src/core/jit/request.rs @@ -45,12 +45,14 @@ impl Request { let builder = Builder::new(blueprint, doc); let plan = builder.build(self.operation_name.as_deref())?; - Ok(transform::CheckConst::new() + transform::CheckConst::new() .pipe(transform::CheckDedupe::new()) .transform(plan) .to_result() - // NOTE: Unwrapping because these transformations fail with () error - .unwrap()) + // both transformers are infallible right now + // but we can't just unwrap this in stable rust + // so convert to the Unknown error + .map_err(|_| super::Error::Unknown) } } diff --git a/src/core/jit/synth/synth.rs b/src/core/jit/synth/synth.rs index f2c63d681b..74ea05f868 100644 --- a/src/core/jit/synth/synth.rs +++ b/src/core/jit/synth/synth.rs @@ -8,16 +8,16 @@ use crate::core::scalar; type ValueStore = Store>>; -pub struct Synth { - plan: OperationPlan, +pub struct Synth<'a, Value> { + plan: &'a OperationPlan, store: ValueStore, variables: Variables, } -impl Synth { +impl<'a, Value> Synth<'a, Value> { #[inline(always)] pub fn new( - plan: OperationPlan, + plan: &'a OperationPlan, store: ValueStore, variables: Variables, ) -> Self { @@ -25,7 +25,7 @@ impl Synth { } } -impl<'a, Value> Synth +impl<'a, Value> Synth<'a, Value> where Value: JsonLike<'a> + Clone + std::fmt::Debug, { @@ -233,13 +233,15 @@ mod tests { use async_graphql_value::ConstValue; use serde::{Deserialize, Serialize}; + use super::ValueStore; use crate::core::blueprint::Blueprint; use crate::core::config::{Config, ConfigModule}; use crate::core::jit::builder::Builder; - use crate::core::jit::common::JP; + use crate::core::jit::fixtures::JP; use crate::core::jit::model::{FieldId, Variables}; use crate::core::jit::store::Store; use crate::core::jit::synth::Synth; + use crate::core::jit::OperationPlan; use crate::core::json::JsonLike; use crate::core::valid::Validator; @@ -308,7 +310,10 @@ mod tests { const CONFIG: &str = include_str!("../fixtures/jsonplaceholder-mutation.graphql"); - fn make_store<'a, Value>(query: &str, store: Vec<(FieldId, TestData)>) -> Synth + fn make_store<'a, Value>( + query: &str, + store: Vec<(FieldId, TestData)>, + ) -> (OperationPlan, ValueStore, Variables) where Value: Deserialize<'a> + JsonLike<'a> + Serialize + Clone + std::fmt::Debug, { @@ -343,27 +348,21 @@ mod tests { }); let vars = Variables::new(); - super::Synth::new(plan, store, vars) + (plan, store, vars) } - struct Synths<'a> { - synth_const: Synth, - synth_borrow: Synth>, - } - - impl<'a> Synths<'a> { - fn init(query: &str, store: Vec<(FieldId, TestData)>) -> Self { - let synth_const = make_store::(query, store.clone()); - let synth_borrow = make_store::(query, store.clone()); - Self { synth_const, synth_borrow } - } - fn assert(self) { - let val_const = self.synth_const.synthesize().unwrap(); - let val_const = serde_json::to_string_pretty(&val_const).unwrap(); - let val_borrow = self.synth_borrow.synthesize().unwrap(); - let val_borrow = serde_json::to_string_pretty(&val_borrow).unwrap(); - assert_eq!(val_const, val_borrow); - } + fn assert_synths(query: &str, store: Vec<(FieldId, TestData)>) { + let (plan, value_store, vars) = make_store::(query, store.clone()); + let synth_const = Synth::new(&plan, value_store, vars); + let (plan, value_store, vars) = + make_store::(query, store.clone()); + let synth_borrow = Synth::new(&plan, value_store, vars); + + let val_const = synth_const.synthesize().unwrap(); + let val_const = serde_json::to_string_pretty(&val_const).unwrap(); + let val_borrow = synth_borrow.synthesize().unwrap(); + let val_borrow = serde_json::to_string_pretty(&val_borrow).unwrap(); + assert_eq!(val_const, val_borrow); } #[test] @@ -375,8 +374,7 @@ mod tests { } "#; - let synths = Synths::init(query, store); - synths.assert(); + assert_synths(query, store); } #[test] @@ -388,8 +386,7 @@ mod tests { } "#; - let synths = Synths::init(query, store); - synths.assert(); + assert_synths(query, store); } #[test] @@ -403,8 +400,7 @@ mod tests { posts { id title user { id name } } } "#; - let synths = Synths::init(query, store); - synths.assert(); + assert_synths(query, store); } #[test] @@ -420,8 +416,7 @@ mod tests { users { id name } } "#; - let synths = Synths::init(query, store); - synths.assert(); + assert_synths(query, store); } #[test] diff --git a/src/core/jit/transform/check_const.rs b/src/core/jit/transform/check_const.rs index 6b1d928e3e..fe7ebf427c 100644 --- a/src/core/jit/transform/check_const.rs +++ b/src/core/jit/transform/check_const.rs @@ -1,3 +1,4 @@ +use std::convert::Infallible; use std::marker::PhantomData; use crate::core::ir::model::IR; @@ -31,7 +32,7 @@ pub fn is_const(ir: &IR) -> bool { impl Transform for CheckConst { type Value = OperationPlan; - type Error = (); + type Error = Infallible; fn transform(&self, mut plan: Self::Value) -> Valid { let is_const = plan.iter_dfs().all(|field| match field.ir { diff --git a/src/core/jit/transform/check_dedupe.rs b/src/core/jit/transform/check_dedupe.rs index 878847042a..b5f90ee5ad 100644 --- a/src/core/jit/transform/check_dedupe.rs +++ b/src/core/jit/transform/check_dedupe.rs @@ -1,3 +1,5 @@ +use std::convert::Infallible; + use crate::core::ir::model::IR; use crate::core::jit::OperationPlan; use crate::core::valid::Valid; @@ -29,7 +31,7 @@ fn check_dedupe(ir: &IR) -> bool { impl Transform for CheckDedupe { type Value = OperationPlan; - type Error = (); + type Error = Infallible; fn transform(&self, mut plan: Self::Value) -> Valid { let dedupe = plan.selection.iter().all(|field| { diff --git a/src/core/jit/transform/input_resolver.rs b/src/core/jit/transform/input_resolver.rs index 8c5ab03771..e5645c3713 100644 --- a/src/core/jit/transform/input_resolver.rs +++ b/src/core/jit/transform/input_resolver.rs @@ -1,6 +1,7 @@ use async_graphql_value::{ConstValue, Value}; -use super::super::{Field, OperationPlan, ResolveInputError, Variables}; +use super::super::{Arg, Field, OperationPlan, ResolveInputError, Variables}; +use crate::core::blueprint::Index; use crate::core::json::{JsonLikeOwned, JsonObjectLike}; use crate::core::Type; @@ -50,63 +51,67 @@ where >::Error: std::fmt::Debug, { pub fn resolve_input( - &self, + self, variables: &Variables, ) -> Result, ResolveInputError> { - let new_fields = self + let index = self.plan.index; + let selection = self .plan .selection - .iter() - .map(|field| (*field).clone().try_map(&|value| value.resolve(variables))) - .map(|field| match field { - Ok(field) => { - let field = self.iter_args(field)?; - Ok(field) - } - Err(err) => Err(err), - }) + .into_iter() + .map(|field| field.try_map(&|value| value.resolve(variables))) + // Call `resolve_field` to verify/populate defaults for args + // because the previous map will just try convert values based on + // variables ignoring default values in schema and not checking if arg + // is required TODO: consider changing [Field::try_map] to be able to do + // this check? + .map(|field| Self::resolve_field(&index, field?)) .collect::, _>>()?; - Ok(OperationPlan::new( - self.plan.root_name(), - new_fields, - self.plan.operation_type(), - self.plan.index.clone(), - self.plan.is_introspection_query, - )) + + Ok(OperationPlan { + root_name: self.plan.root_name.to_string(), + operation_type: self.plan.operation_type, + index, + is_introspection_query: self.plan.is_introspection_query, + is_dedupe: self.plan.is_dedupe, + is_const: self.plan.is_const, + selection, + }) } - #[inline(always)] - fn iter_args(&self, mut field: Field) -> Result, ResolveInputError> { + fn resolve_field( + index: &Index, + field: Field, + ) -> Result, ResolveInputError> { + // TODO: should also check and provide defaults for directives let args = field .args .into_iter() - .map(|mut arg| { - let value = self.recursive_parse_arg( + .map(|arg| { + let value = Self::recursive_parse_arg( + index, &field.name, &arg.name, &arg.type_of, &arg.default_value, arg.value, )?; - arg.value = value; - Ok(arg) + Ok(Arg { value, ..arg }) }) .collect::>()?; - field.args = args; - - field.selection = field + let selection = field .selection .into_iter() - .map(|val| self.iter_args(val)) + .map(|field| Self::resolve_field(index, field)) .collect::>()?; - Ok(field) + Ok(Field { args, selection, ..field }) } #[allow(clippy::too_many_arguments)] fn recursive_parse_arg( - &self, + index: &Index, parent_name: &str, arg_name: &str, type_of: &Type, @@ -136,7 +141,7 @@ where return Ok(None); }; - let Some(def) = self.plan.index.get_input_type_definition(type_of.name()) else { + let Some(def) = index.get_input_type_definition(type_of.name()) else { return Ok(Some(value)); }; @@ -148,7 +153,8 @@ where .default_value .clone() .map(|value| Output::try_from(value).expect("The conversion cannot fail")); - let value = self.recursive_parse_arg( + let value = Self::recursive_parse_arg( + index, &parent_name, &arg_field.name, &arg_field.of_type, @@ -160,18 +166,18 @@ where } } } else if let Some(arr) = value.as_array_mut() { - for (index, item) in arr.iter_mut().enumerate() { - let parent_name = format!("{}.{}.{}", parent_name, arg_name, index); - - *item = self - .recursive_parse_arg( - &parent_name, - &index.to_string(), - type_of, - &None, - Some(item.clone()), - )? - .expect("Because we start with `Some`, we will end with `Some`"); + for (i, item) in arr.iter_mut().enumerate() { + let parent_name = format!("{}.{}.{}", parent_name, arg_name, i); + + *item = Self::recursive_parse_arg( + index, + &parent_name, + &i.to_string(), + type_of, + &None, + Some(item.clone()), + )? + .expect("Because we start with `Some`, we will end with `Some`"); } }