From aca14bebc25622c096f818fd60acd2a8f8468103 Mon Sep 17 00:00:00 2001 From: Lee Danilek Date: Thu, 10 Oct 2024 17:59:21 -0400 Subject: [PATCH] ModuleMetadata parse args and returns lazily (#30589) Some function have enormous arg validators, which can take a while to parse. And we parse these whenever we look up a ModuleMetadata, which happens whenever the module gets imported by another module. Avoid bad performance on imports by leaving the arg and return validators as unparsed JSON strings. GitOrigin-RevId: 8b503b565db8ea38cb9be4b927025baf13607873 --- crates/isolate/src/environment/analyze.rs | 28 +-- .../src/environment/helpers/validation.rs | 11 +- crates/isolate/src/tests/analyze.rs | 192 +++++++++--------- crates/model/src/modules/module_versions.rs | 76 +++++-- crates/node_executor/src/executor.rs | 6 +- crates/node_executor/src/local.rs | 32 +-- 6 files changed, 190 insertions(+), 155 deletions(-) diff --git a/crates/isolate/src/environment/analyze.rs b/crates/isolate/src/environment/analyze.rs index a81c27f0..870cf104 100644 --- a/crates/isolate/src/environment/analyze.rs +++ b/crates/isolate/src/environment/analyze.rs @@ -671,28 +671,28 @@ fn udf_analyze( && fn_canon_path.as_str() == module_path.as_str() { // Source map is valid; proceed with mapping in original source map - functions.push(AnalyzedFunction { - name: canonicalized_name.clone(), - pos: Some(AnalyzedSourcePosition { + functions.push(AnalyzedFunction::new( + canonicalized_name.clone(), + Some(AnalyzedSourcePosition { path: fn_canon_path, start_lineno: token.get_src_line(), start_col: token.get_src_col(), }), udf_type, - visibility: visibility.clone(), - args: args.clone(), - returns: returns.clone(), - }); + visibility.clone(), + args.clone(), + returns.clone(), + )?); } else { // If there is no valid source map, push a function without a position - functions.push(AnalyzedFunction { - name: canonicalized_name.clone(), - pos: None, + functions.push(AnalyzedFunction::new( + canonicalized_name.clone(), + None, udf_type, - visibility: visibility.clone(), - args: args.clone(), - returns: returns.clone(), - }); + visibility.clone(), + args.clone(), + returns.clone(), + )?); // Log reason for fallback if fn_canon_path.as_str() != module_path.as_str() { diff --git a/crates/isolate/src/environment/helpers/validation.rs b/crates/isolate/src/environment/helpers/validation.rs index 91be2700..18a75c6d 100644 --- a/crates/isolate/src/environment/helpers/validation.rs +++ b/crates/isolate/src/environment/helpers/validation.rs @@ -374,7 +374,7 @@ impl ValidatedPathAndArgs { let returns_validator = if path.udf_path.is_system() { ReturnsValidator::Unvalidated } else { - analyzed_function.returns.clone() + analyzed_function.returns()? }; match ValidatedPathAndArgs::new_inner( @@ -443,10 +443,11 @@ impl ValidatedPathAndArgs { let table_mapping = &tx.table_mapping().namespace(path.component.into()); // If the UDF has an args validator, check that these args match. - let args_validation_error = - analyzed_function - .args - .check_args(&args, table_mapping, &virtual_system_mapping())?; + let args_validation_error = analyzed_function.args()?.check_args( + &args, + table_mapping, + &virtual_system_mapping(), + )?; if let Some(error) = args_validation_error { return Ok(Err(JsError::from_message(format!( diff --git a/crates/isolate/src/tests/analyze.rs b/crates/isolate/src/tests/analyze.rs index a54d1fa2..08801338 100644 --- a/crates/isolate/src/tests/analyze.rs +++ b/crates/isolate/src/tests/analyze.rs @@ -263,11 +263,11 @@ async fn test_analyze_function(rt: TestRuntime) -> anyhow::Result<()> { assert_eq!( &Vec::from(analyzed_module.functions.clone()), &[ - AnalyzedFunction { - name: "throwsError".parse()?, + AnalyzedFunction::new( + "throwsError".parse()?, // Don't check line numbers since those change on every `convex/server` // change. - pos: Some(AnalyzedSourcePosition { + Some(AnalyzedSourcePosition { path: "sourceMaps.js".parse()?, start_lineno: analyzed_module.functions[0] .pos @@ -276,14 +276,14 @@ async fn test_analyze_function(rt: TestRuntime) -> anyhow::Result<()> { .start_lineno, start_col: analyzed_module.functions[0].pos.as_ref().unwrap().start_col, }), - udf_type: UdfType::Query, - visibility: Some(Visibility::Public), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, - AnalyzedFunction { - name: "throwsErrorInDep".parse()?, - pos: Some(AnalyzedSourcePosition { + UdfType::Query, + Some(Visibility::Public), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, + AnalyzedFunction::new( + "throwsErrorInDep".parse()?, + Some(AnalyzedSourcePosition { path: "sourceMaps.js".parse()?, start_lineno: analyzed_module.functions[1] .pos @@ -292,41 +292,41 @@ async fn test_analyze_function(rt: TestRuntime) -> anyhow::Result<()> { .start_lineno, start_col: analyzed_module.functions[1].pos.as_ref().unwrap().start_col, }), - udf_type: UdfType::Query, - visibility: Some(Visibility::Public), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, + UdfType::Query, + Some(Visibility::Public), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, ], ); let source_mapped = analyzed_module.source_mapped.unwrap(); assert_eq!( &Vec::from(source_mapped.functions), &[ - AnalyzedFunction { - name: "throwsError".parse()?, - pos: Some(AnalyzedSourcePosition { + AnalyzedFunction::new( + "throwsError".parse()?, + Some(AnalyzedSourcePosition { path: "sourceMaps.js".parse()?, start_lineno: 21, start_col: analyzed_module.functions[0].pos.as_ref().unwrap().start_col, }), - udf_type: UdfType::Query, - visibility: Some(Visibility::Public), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, - AnalyzedFunction { - name: "throwsErrorInDep".parse()?, - pos: Some(AnalyzedSourcePosition { + UdfType::Query, + Some(Visibility::Public), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, + AnalyzedFunction::new( + "throwsErrorInDep".parse()?, + Some(AnalyzedSourcePosition { path: "sourceMaps.js".parse()?, start_lineno: 27, start_col: analyzed_module.functions[1].pos.as_ref().unwrap().start_col, }), - udf_type: UdfType::Query, - visibility: Some(Visibility::Public), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, + UdfType::Query, + Some(Visibility::Public), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, ], ); Ok(()) @@ -353,100 +353,100 @@ async fn test_analyze_internal_function(rt: TestRuntime) -> anyhow::Result<()> { assert_eq!( &Vec::from(analyzed_module.functions.clone()), &[ - AnalyzedFunction { - name: "myInternalQuery".parse()?, + AnalyzedFunction::new( + "myInternalQuery".parse()?, // Don't check line numbers since those change on every `convex/server` // change. - pos: analyzed_module.functions[0].pos.clone(), - udf_type: UdfType::Query, - visibility: Some(Visibility::Internal), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, - AnalyzedFunction { - name: "publicQuery".parse()?, + analyzed_module.functions[0].pos.clone(), + UdfType::Query, + Some(Visibility::Internal), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, + AnalyzedFunction::new( + "publicQuery".parse()?, // Don't check line numbers since those change on every `convex/server` // change. - pos: analyzed_module.functions[1].pos.clone(), - udf_type: UdfType::Query, - visibility: Some(Visibility::Public), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, - AnalyzedFunction { - name: "myInternalMutation".parse()?, + analyzed_module.functions[1].pos.clone(), + UdfType::Query, + Some(Visibility::Public), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, + AnalyzedFunction::new( + "myInternalMutation".parse()?, // Don't check line numbers since those change on every `convex/server` // change. - pos: analyzed_module.functions[2].pos.clone(), - udf_type: UdfType::Mutation, - visibility: Some(Visibility::Internal), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, - AnalyzedFunction { - name: "publicMutation".parse()?, + analyzed_module.functions[2].pos.clone(), + UdfType::Mutation, + Some(Visibility::Internal), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, + AnalyzedFunction::new( + "publicMutation".parse()?, // Don't check line numbers since those change on every `convex/server` // change. - pos: analyzed_module.functions[3].pos.clone(), - udf_type: UdfType::Mutation, - visibility: Some(Visibility::Public), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, + analyzed_module.functions[3].pos.clone(), + UdfType::Mutation, + Some(Visibility::Public), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, ], ); let source_mapped = analyzed_module.source_mapped.unwrap(); assert_eq!( &Vec::from(source_mapped.functions.clone()), &[ - AnalyzedFunction { - name: "myInternalQuery".parse()?, - pos: Some(AnalyzedSourcePosition { + AnalyzedFunction::new( + "myInternalQuery".parse()?, + Some(AnalyzedSourcePosition { path: "internal.js".parse()?, start_lineno: 16, start_col: analyzed_module.functions[0].pos.as_ref().unwrap().start_col, }), - udf_type: UdfType::Query, - visibility: Some(Visibility::Internal), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, - AnalyzedFunction { - name: "publicQuery".parse()?, - pos: Some(AnalyzedSourcePosition { + UdfType::Query, + Some(Visibility::Internal), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, + AnalyzedFunction::new( + "publicQuery".parse()?, + Some(AnalyzedSourcePosition { path: "internal.js".parse()?, start_lineno: 19, start_col: analyzed_module.functions[1].pos.as_ref().unwrap().start_col, }), - udf_type: UdfType::Query, - visibility: Some(Visibility::Public), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, - AnalyzedFunction { - name: "myInternalMutation".parse()?, - pos: Some(AnalyzedSourcePosition { + UdfType::Query, + Some(Visibility::Public), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, + AnalyzedFunction::new( + "myInternalMutation".parse()?, + Some(AnalyzedSourcePosition { path: "internal.js".parse()?, start_lineno: 23, start_col: analyzed_module.functions[2].pos.as_ref().unwrap().start_col, }), - udf_type: UdfType::Mutation, - visibility: Some(Visibility::Internal), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, - AnalyzedFunction { - name: "publicMutation".parse()?, - pos: Some(AnalyzedSourcePosition { + UdfType::Mutation, + Some(Visibility::Internal), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, + AnalyzedFunction::new( + "publicMutation".parse()?, + Some(AnalyzedSourcePosition { path: "internal.js".parse()?, start_lineno: 26, start_col: analyzed_module.functions[3].pos.as_ref().unwrap().start_col, }), - udf_type: UdfType::Mutation, - visibility: Some(Visibility::Public), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, + UdfType::Mutation, + Some(Visibility::Public), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, ], ); Ok(()) diff --git a/crates/model/src/modules/module_versions.rs b/crates/model/src/modules/module_versions.rs index 118b2212..ff81df5a 100644 --- a/crates/model/src/modules/module_versions.rs +++ b/crates/model/src/modules/module_versions.rs @@ -247,8 +247,56 @@ pub struct AnalyzedFunction { pub pos: Option, pub udf_type: UdfType, pub visibility: Option, - pub args: ArgsValidator, - pub returns: ReturnsValidator, + + // Leave args and returns unparsed to avoid performance overhead in common + // case of reading ModuleMetadata without needing to validate the function. + + // JSON-serialized ArgsValidator + pub args_str: Option, + // JSON-serialized ReturnsValidator + pub returns_str: Option, +} + +impl AnalyzedFunction { + pub fn new( + name: FunctionName, + pos: Option, + udf_type: UdfType, + visibility: Option, + args: ArgsValidator, + returns: ReturnsValidator, + ) -> anyhow::Result { + let args_json = JsonValue::try_from(args)?; + let returns_json = JsonValue::try_from(returns)?; + Ok(Self { + name, + pos, + udf_type, + visibility, + args_str: Some(serde_json::to_string(&args_json)?), + returns_str: Some(serde_json::to_string(&returns_json)?), + }) + } + + pub fn args(&self) -> anyhow::Result { + match &self.args_str { + Some(args) => { + let deserialized_value: JsonValue = serde_json::from_str(args)?; + ArgsValidator::try_from(deserialized_value) + }, + None => Ok(ArgsValidator::Unvalidated), + } + } + + pub fn returns(&self) -> anyhow::Result { + match &self.returns_str { + Some(returns) => { + let deserialized_value: JsonValue = serde_json::from_str(returns)?; + ReturnsValidator::try_from(deserialized_value) + }, + None => Ok(ReturnsValidator::Unvalidated), + } + } } impl HeapSize for AnalyzedFunction { @@ -276,15 +324,13 @@ impl TryFrom for SerializedAnalyzedFunction { type Error = anyhow::Error; fn try_from(f: AnalyzedFunction) -> anyhow::Result { - let args_json = JsonValue::try_from(f.args)?; - let returns_json = JsonValue::try_from(f.returns)?; Ok(Self { name: f.name.to_string(), pos: f.pos.map(TryFrom::try_from).transpose()?, udf_type: f.udf_type.to_string(), visibility: f.visibility, - args: Some(serde_json::to_string(&args_json)?), - returns: Some(serde_json::to_string(&returns_json)?), + args: f.args_str, + returns: f.returns_str, }) } } @@ -298,20 +344,8 @@ impl TryFrom for AnalyzedFunction { pos: f.pos.map(AnalyzedSourcePosition::try_from).transpose()?, udf_type: f.udf_type.parse()?, visibility: f.visibility, - args: match f.args { - Some(args) => { - let deserialized_value: JsonValue = serde_json::from_str(&args)?; - ArgsValidator::try_from(deserialized_value)? - }, - None => ArgsValidator::Unvalidated, - }, - returns: match f.returns { - Some(returns) => { - let deserialized_value: JsonValue = serde_json::from_str(&returns)?; - ReturnsValidator::try_from(deserialized_value)? - }, - None => ReturnsValidator::Unvalidated, - }, + args_str: f.args, + returns_str: f.returns, }) } } @@ -600,7 +634,7 @@ mod tests { // Should parse as `visibility: None`, and `args: Unvalidated`. assert_eq!(function.visibility, None); - assert_eq!(function.args, ArgsValidator::Unvalidated); + assert_eq!(function.args()?, ArgsValidator::Unvalidated); Ok(()) } } diff --git a/crates/node_executor/src/executor.rs b/crates/node_executor/src/executor.rs index a90c193e..aa2a6757 100644 --- a/crates/node_executor/src/executor.rs +++ b/crates/node_executor/src/executor.rs @@ -456,14 +456,14 @@ impl Actions { .name .parse() .map_err(|e| invalid_function_name_error(&e))?; - functions.push(AnalyzedFunction { - name: function_name, + functions.push(AnalyzedFunction::new( + function_name, pos, udf_type, visibility, args, returns, - }); + )?); } // Sort by line number where source position of None compares least diff --git a/crates/node_executor/src/local.rs b/crates/node_executor/src/local.rs index 50837daa..08dc11c9 100644 --- a/crates/node_executor/src/local.rs +++ b/crates/node_executor/src/local.rs @@ -883,30 +883,30 @@ export { hello, internalHello }; assert_eq!( Vec::from(modules[&path].functions.clone()), &[ - AnalyzedFunction { - name: "hello".parse()?, - pos: Some(AnalyzedSourcePosition { + AnalyzedFunction::new( + "hello".parse()?, + Some(AnalyzedSourcePosition { path: "static_node_source.js".parse()?, start_lineno: 28, start_col: modules[&path].functions[0].pos.as_ref().unwrap().start_col, }), - udf_type: UdfType::Action, - visibility: Some(Visibility::Public), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, - AnalyzedFunction { - name: "internalHello".parse()?, - pos: Some(AnalyzedSourcePosition { + UdfType::Action, + Some(Visibility::Public), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, + AnalyzedFunction::new( + "internalHello".parse()?, + Some(AnalyzedSourcePosition { path: "static_node_source.js".parse()?, start_lineno: 31, start_col: modules[&path].functions[1].pos.as_ref().unwrap().start_col, }), - udf_type: UdfType::Action, - visibility: Some(Visibility::Internal), - args: ArgsValidator::Unvalidated, - returns: ReturnsValidator::Unvalidated, - }, + UdfType::Action, + Some(Visibility::Internal), + ArgsValidator::Unvalidated, + ReturnsValidator::Unvalidated, + )?, ] ); Ok(())