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
get-convex · Oct 10, 2024 · aca14be · aca14be
1 parent 7909fac
commit aca14be
Show file tree

Hide file tree

Showing 6 changed files with 190 additions and 155 deletions.
diff --git a/crates/isolate/src/environment/analyze.rs b/crates/isolate/src/environment/analyze.rs
@@ -671,28 +671,28 @@ fn udf_analyze<RT: Runtime>(
             && 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
@@ -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
@@ -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(())