Avoid redundant cloning of executable definition ASTs

Reviewed By: scotthovestadt

Differential Revision: D94779922

fbshipit-source-id: 3fb6f5130b5b69b5c46929509303b13958b5af88

Author: tyao1

compiler/crates/dependency-analyzer/src/ast.rs

@@ -23,14 +23,18 @@ pub struct ReachableAst {
     pub base_fragment_names: FragmentDefinitionNameSet,
 }
 
-/// Get all definitions that are reachable from project definitions
+/// Get all definitions that are reachable from project definitions.
+///
+/// Takes borrowed slices to avoid cloning all definitions upfront. Only clones
+/// into the result Vec once, saving ~222K deep clones for large projects with
+/// base definitions.
 pub fn get_reachable_ast(
-    project_definitions: Vec<ExecutableDefinition>,
-    base_definitions: Vec<ExecutableDefinition>,
+    project_definitions: &[ExecutableDefinition],
+    base_definitions: &[ExecutableDefinition],
 ) -> ReachableAst {
     if base_definitions.is_empty() {
         return ReachableAst {
-            definitions: project_definitions,
+            definitions: project_definitions.to_vec(),
             base_fragment_names: Default::default(),
         };
     }
@@ -42,30 +46,31 @@ pub fn get_reachable_ast(
     // Preprocess all base fragment definitions
     // Skipping operations because project definitions can't reference base operations
     for base_definition in base_definitions {
-        match &base_definition {
-            ExecutableDefinition::Fragment(fragment) => {
-                let name = FragmentDefinitionName(fragment.name.value);
-                assert!(
-                    base_definitions_map.insert(name, base_definition).is_none(),
-                    "get_reachable_ast called on graph with duplicate definition of `{name}`"
-                )
-            }
-            ExecutableDefinition::Operation(_) => {}
+        if let ExecutableDefinition::Fragment(fragment) = base_definition {
+            let name = FragmentDefinitionName(fragment.name.value);
+            assert!(
+                base_definitions_map
+                    .insert(name, base_definition.clone())
+                    .is_none(),
+                "get_reachable_ast called on graph with duplicate definition of `{name}`"
+            );
         }
     }
 
-    let mut result = project_definitions.clone();
+    // Clone project definitions into result once. We iterate the borrowed slice
+    // directly below for selection visiting, avoiding a second clone.
+    let mut result = project_definitions.to_vec();
 
     // Find references from project definitions -> base definitions
     for definition in project_definitions {
         let selections = match definition {
-            ExecutableDefinition::Operation(definition) => definition.selections,
-            ExecutableDefinition::Fragment(definition) => definition.selections,
+            ExecutableDefinition::Operation(definition) => &definition.selections,
+            ExecutableDefinition::Fragment(definition) => &definition.selections,
         };
         visit_selections(
             &base_definitions_map,
             &mut reachable_base_asts,
-            &selections,
+            selections,
             false,
         )
     }

compiler/crates/dependency-analyzer/tests/ast.rs

@@ -17,15 +17,15 @@ pub async fn transform_fixture(fixture: &Fixture<'_>) -> Result<String, String>
 
     let source_location = SourceLocationKey::standalone(fixture.file_name);
     let definitions = parse_executable(parts[0], source_location).unwrap();
-    let base_definitions = parts
+    let base_definitions: Vec<ExecutableDefinition> = parts
         .iter()
         .skip(1)
         .flat_map(|part| parse_executable(part, source_location).unwrap().definitions)
         .collect();
     let ReachableAst {
         definitions: result,
         base_fragment_names,
-    } = get_reachable_ast(definitions.definitions, base_definitions);
+    } = get_reachable_ast(&definitions.definitions, &base_definitions);
 
     let mut texts = result
         .into_iter()

compiler/crates/relay-compiler/src/build_project/project_asts.rs

@@ -39,16 +39,17 @@ pub fn get_project_asts(
     graphql_asts: &FnvHashMap<ProjectName, GraphQLAsts>,
     project_config: &ProjectConfig,
 ) -> Result<ProjectAstData, BuildProjectError> {
+    let empty: &[ExecutableDefinition] = &[];
     let project_asts = graphql_asts
         .get(&project_config.name)
         .map(|asts| asts.get_all_executable_definitions())
-        .unwrap_or_default();
+        .unwrap_or(empty);
     let (base_project_asts, base_definition_names) = match project_config.base {
         Some(base_project_name) => {
             let base_project_asts = graphql_asts
                 .get(&base_project_name)
                 .map(|asts| asts.get_all_executable_definitions())
-                .unwrap_or_default();
+                .unwrap_or(empty);
             let base_definition_names = base_project_asts
                 .iter()
                 // TODO(T64459085): Figure out what to do about unnamed (anonymous) operations
@@ -61,17 +62,17 @@ pub fn get_project_asts(
                 .collect::<ExecutableDefinitionNameSet>();
             (base_project_asts, base_definition_names)
         }
-        None => (Vec::new(), Default::default()),
+        None => (empty, Default::default()),
     };
-    find_duplicates(&project_asts, &base_project_asts).map_err(|errors| {
+    find_duplicates(project_asts, base_project_asts).map_err(|errors| {
         BuildProjectError::ValidationErrors {
             errors,
             project_name: project_config.name,
         }
     })?;
 
     let mut base_resolver_fragment_asts =
-        find_base_resolver_fragment_asts(schema, &base_definition_names, &base_project_asts);
+        find_base_resolver_fragment_asts(schema, &base_definition_names, base_project_asts);
 
     let ReachableAst {
         mut definitions,

compiler/crates/relay-compiler/src/graphql_asts.rs

@@ -35,12 +35,16 @@ use crate::utils::get_parser_features;
 
 /// A collection of GraphQL abstract syntax trees (ASTs) for a set of files.
 ///
-/// This struct contains a map of file paths to their corresponding GraphQL ASTs,
-/// as well as sets of pending and removed definition names.
+/// Stores definitions both per-file (for per-file lookup) and in a pre-flattened
+/// Vec (for efficient access to all definitions). `get_all_executable_definitions`
+/// returns a borrowed slice instead of cloning ~222K definitions each time.
 #[derive(Debug)]
 pub struct GraphQLAsts {
     /// A map of file paths to their corresponding GraphQL ASTs.
     asts: FnvHashMap<PathBuf, Vec<ExecutableDefinition>>,
+    /// Pre-flattened Vec of all definitions across all files. Avoids expensive
+    /// per-call cloning in `get_all_executable_definitions`.
+    all_definitions: Vec<ExecutableDefinition>,
     /// Names of fragments and operations that are updated or created.
     pub pending_definition_names: ExecutableDefinitionNameSet,
     /// Names of fragments and operations that are deleted.
@@ -55,8 +59,8 @@ impl GraphQLAsts {
         self.asts.get(file_path)
     }
 
-    pub fn get_all_executable_definitions(&self) -> Vec<ExecutableDefinition> {
-        self.asts.values().flatten().cloned().collect()
+    pub fn get_all_executable_definitions(&self) -> &[ExecutableDefinition] {
+        &self.all_definitions
     }
 
     pub fn from_graphql_sources_map(
@@ -198,8 +202,13 @@ impl GraphQLAsts {
         }
 
         if syntax_errors.is_empty() {
+            // Pre-flatten all definitions to avoid cloning on every call to
+            // get_all_executable_definitions(). This trades memory for avoiding
+            // ~222K deep clones per build on the intern project.
+            let all_definitions = asts.values().flatten().cloned().collect();
             Ok(Self {
                 asts,
+                all_definitions,
                 pending_definition_names,
                 removed_definition_names,
             })