Remove Salsa database for 1.2× ~ 2× validation speed (#838)

Author: SimonSapin

crates/apollo-compiler/CHANGELOG.md

@@ -17,6 +17,34 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ## Maintenance
 ## Documentation-->
 
+# [x.x.x] (unreleased) - 2024-mm-dd
+
+## BREAKING
+
+- **Move `NodeLocation` and `FileId` from `apollo_compiler::validation` to the crate root - [SimonSapin], [pull/838]**
+
+## Maintenance
+
+- **Remove Salsa database for 1.2× ~ 2× validation speed - [SimonSapin], [pull/838]**
+  We were not taking advantage of caching it provides.
+  Additionally, validation uses `Schema` and `ExecutableDocument` directly
+  rather than converting them to AST.
+  ```
+  $ cargo bench --bench multi-source
+  supergraph parse_and_validate
+                        time:   [398.09 µs 399.32 µs 400.82 µs]
+                        change: [-19.966% -19.418% -18.864%] (p = 0.00 < 0.05)
+                        Performance has improved.
+  simple_query parse_and_validate
+                        time:   [12.097 µs 12.104 µs 12.113 µs]
+                        change: [-52.467% -52.282% -52.109%] (p = 0.00 < 0.05)
+                        Performance has improved.
+  ```
+
+[SimonSapin]: https://github.com/SimonSapin]
+[pull/838]: https://github.com/apollographql/apollo-rs/pull/838
+
+
 # [1.0.0-beta.13](https://crates.io/crates/apollo-compiler/1.0.0-beta.13) - 2024-02-14
 
 > This release includes a critical fix to overflow protection in validation.

crates/apollo-compiler/Cargo.toml

@@ -22,7 +22,6 @@ apollo-parser = { path = "../apollo-parser", version = "0.7.6" }
 ariadne = { version = "0.4.0", features = ["auto-color"] }
 indexmap = "2.0.0"
 rowan = "0.15.5"
-salsa = "0.16.1"
 serde = { version = "1.0", features = ["derive"] }
 serde_json_bytes = { version = "0.2.2", features = ["preserve_order"] }
 thiserror = "1.0.31"

crates/apollo-compiler/benches/multi_source.rs

@@ -1,59 +1,43 @@
+use apollo_compiler::ast::Document;
 use apollo_compiler::ExecutableDocument;
 use apollo_compiler::Schema;
 use criterion::*;
 
-fn compile(schema: &str, query: &str) {
-    let schema = Schema::parse_and_validate(schema, "schema.graphql").unwrap();
-    let doc = ExecutableDocument::parse(&schema, query, "query.graphql").unwrap();
+fn parse_ast(schema: &str, query: &str) {
+    let schema = Document::parse(schema, "schema.graphql").unwrap();
+    let doc = Document::parse(query, "query.graphql").unwrap();
     black_box((schema, doc));
 }
 
-fn compile_and_validate(schema: &str, query: &str) {
+fn parse_and_validate(schema: &str, query: &str) {
     let schema = Schema::parse_and_validate(schema, "schema.graphql").unwrap();
     let doc = ExecutableDocument::parse_and_validate(&schema, query, "query.graphql").unwrap();
     black_box((schema, doc));
 }
 
-fn bench_simple_query_compiler(c: &mut Criterion) {
+fn bench_simple_query(c: &mut Criterion) {
     let query = include_str!("testdata/simple_query.graphql");
     let schema = include_str!("testdata/simple_schema.graphql");
 
-    c.bench_function("simple_query_compiler", move |b| {
-        b.iter(|| compile(schema, query))
+    c.bench_function("simple_query parse_ast", move |b| {
+        b.iter(|| parse_ast(schema, query))
     });
-}
-
-fn bench_simple_query_compiler_with_validation(c: &mut Criterion) {
-    let query = include_str!("testdata/simple_query.graphql");
-    let schema = include_str!("testdata/simple_schema.graphql");
-
-    c.bench_function("simple_query_compiler_with_validation", move |b| {
-        b.iter(|| compile_and_validate(schema, query))
+    c.bench_function("simple_query parse_and_validate", move |b| {
+        b.iter(|| parse_and_validate(schema, query))
     });
 }
 
-fn bench_compiler_supergraph(c: &mut Criterion) {
+fn bench_supergraph(c: &mut Criterion) {
     let schema = include_str!("testdata/supergraph.graphql");
     let query = include_str!("testdata/supergraph_query.graphql");
 
-    c.bench_function("supergraph_compiler", move |b| {
-        b.iter(|| compile(schema, query))
+    c.bench_function("supergraph parse_ast", move |b| {
+        b.iter(|| parse_ast(schema, query))
     });
-}
-fn bench_compiler_supergraph_with_validation(c: &mut Criterion) {
-    let schema = include_str!("testdata/supergraph.graphql");
-    let query = include_str!("testdata/supergraph_query.graphql");
-
-    c.bench_function("supergraph_compiler_with_validation", move |b| {
-        b.iter(|| compile_and_validate(schema, query))
+    c.bench_function("supergraph parse_and_validate", move |b| {
+        b.iter(|| parse_and_validate(schema, query))
     });
 }
 
-criterion_group!(
-    benches,
-    bench_compiler_supergraph,
-    bench_compiler_supergraph_with_validation,
-    bench_simple_query_compiler,
-    bench_simple_query_compiler_with_validation
-);
+criterion_group!(benches, bench_supergraph, bench_simple_query);
 criterion_main!(benches);

crates/apollo-compiler/src/ast/from_cst.rs

@@ -1,8 +1,8 @@
 use crate::ast;
 use crate::ast::Document;
 use crate::validation::FileId;
-use crate::validation::NodeLocation;
 use crate::Node;
+use crate::NodeLocation;
 use crate::SourceMap;
 use apollo_parser::cst;
 use apollo_parser::cst::CstNode;

crates/apollo-compiler/src/ast/impls.rs

@@ -470,51 +470,30 @@ impl DirectiveDefinition {
 impl SchemaDefinition {
     serialize_method!();
 }
-impl Extensible for SchemaDefinition {
-    type Extension = SchemaExtension;
-}
 
 impl ScalarTypeDefinition {
     serialize_method!();
 }
-impl Extensible for ScalarTypeDefinition {
-    type Extension = ScalarTypeExtension;
-}
 
 impl ObjectTypeDefinition {
     serialize_method!();
 }
-impl Extensible for ObjectTypeDefinition {
-    type Extension = ObjectTypeExtension;
-}
 
 impl InterfaceTypeDefinition {
     serialize_method!();
 }
-impl Extensible for InterfaceTypeDefinition {
-    type Extension = InterfaceTypeExtension;
-}
 
 impl UnionTypeDefinition {
     serialize_method!();
 }
-impl Extensible for UnionTypeDefinition {
-    type Extension = UnionTypeExtension;
-}
 
 impl EnumTypeDefinition {
     serialize_method!();
 }
-impl Extensible for EnumTypeDefinition {
-    type Extension = EnumTypeExtension;
-}
 
 impl InputObjectTypeDefinition {
     serialize_method!();
 }
-impl Extensible for InputObjectTypeDefinition {
-    type Extension = InputObjectTypeExtension;
-}
 
 impl SchemaExtension {
     serialize_method!();
@@ -1840,62 +1819,3 @@ impl fmt::Debug for InvalidNameError {
         fmt::Display::fmt(self, f)
     }
 }
-
-impl<T: Extensible> TypeWithExtensions<T> {
-    /// Iterate over elements of the base definition and its extensions.
-    pub fn iter_all<'a, Item, DefIter, ExtIter>(
-        &'a self,
-        mut map_definition: impl FnMut(&'a Node<T>) -> DefIter + 'a,
-        map_extension: impl FnMut(&'a Node<T::Extension>) -> ExtIter + 'a,
-    ) -> impl Iterator<Item = Item> + 'a
-    where
-        Item: 'a,
-        DefIter: Iterator<Item = Item> + 'a,
-        ExtIter: Iterator<Item = Item> + 'a,
-    {
-        map_definition(&self.definition).chain(self.extensions.iter().flat_map(map_extension))
-    }
-}
-
-macro_rules! iter_extensible_method {
-    ($property:ident, $ty:ty) => {
-        pub fn $property(&self) -> impl Iterator<Item = &'_ $ty> + '_ {
-            self.iter_all(
-                |definition| definition.$property.iter(),
-                |extension| extension.$property.iter(),
-            )
-        }
-    };
-}
-
-impl TypeWithExtensions<SchemaDefinition> {
-    iter_extensible_method!(directives, Node<Directive>);
-    iter_extensible_method!(root_operations, Node<(OperationType, NamedType)>);
-}
-
-impl TypeWithExtensions<ObjectTypeDefinition> {
-    iter_extensible_method!(directives, Node<Directive>);
-    iter_extensible_method!(fields, Node<FieldDefinition>);
-    iter_extensible_method!(implements_interfaces, NamedType);
-}
-
-impl TypeWithExtensions<InterfaceTypeDefinition> {
-    iter_extensible_method!(directives, Node<Directive>);
-    iter_extensible_method!(fields, Node<FieldDefinition>);
-    iter_extensible_method!(implements_interfaces, NamedType);
-}
-
-impl TypeWithExtensions<UnionTypeDefinition> {
-    iter_extensible_method!(directives, Node<Directive>);
-    iter_extensible_method!(members, NamedType);
-}
-
-impl TypeWithExtensions<EnumTypeDefinition> {
-    iter_extensible_method!(directives, Node<Directive>);
-    iter_extensible_method!(values, Node<EnumValueDefinition>);
-}
-
-impl TypeWithExtensions<InputObjectTypeDefinition> {
-    iter_extensible_method!(directives, Node<Directive>);
-    iter_extensible_method!(fields, Node<InputValueDefinition>);
-}

crates/apollo-compiler/src/ast/mod.rs

@@ -33,7 +33,6 @@
 
 use crate::Node;
 use crate::NodeStr;
-use std::collections::HashMap;
 
 pub(crate) mod from_cst;
 pub(crate) mod impls;
@@ -363,28 +362,3 @@ pub struct FloatValue(String);
 #[derive(Clone, Eq, PartialEq)]
 #[non_exhaustive]
 pub struct FloatOverflowError {}
-
-/// Trait implemented by extensible type definitions, to associate the extension type with the base
-/// definition type.
-pub(crate) trait Extensible {
-    type Extension;
-}
-
-#[derive(Debug, Clone, Hash, PartialEq, Eq)]
-pub(crate) struct TypeWithExtensions<T: Extensible> {
-    pub definition: Node<T>,
-    pub extensions: Vec<Node<T::Extension>>,
-}
-
-// TODO(@goto-bus-stop): may have to do Arc<TypeWithExtensions> as we need to clone
-// it for salsa reasons. OR pass (object|scalar|etc, Name) tuples to the salsa queries.
-#[derive(Debug, PartialEq, Eq)]
-pub(crate) struct TypeSystem {
-    pub schema: TypeWithExtensions<SchemaDefinition>,
-    pub objects: HashMap<Name, TypeWithExtensions<ObjectTypeDefinition>>,
-    pub scalars: HashMap<Name, TypeWithExtensions<ScalarTypeDefinition>>,
-    pub interfaces: HashMap<Name, TypeWithExtensions<InterfaceTypeDefinition>>,
-    pub unions: HashMap<Name, TypeWithExtensions<UnionTypeDefinition>>,
-    pub enums: HashMap<Name, TypeWithExtensions<EnumTypeDefinition>>,
-    pub input_objects: HashMap<Name, TypeWithExtensions<InputObjectTypeDefinition>>,
-}