Benchmark Multi-File Compiler DBs

[erikwrede]

Description

In some local benchmarks, I have noticed a slow-down when the compiler db fills up. While some side effects of a full db are expected, I would've anticipated a much lower impact of overall executable count on individual file validation.

It is especially interesting that this even occurred when the executables all had the exact same content, as this might indicate some caching problems in salsa.

I'll try to investigate further and provide some benchmarks to reproduce. In the meantime: is there any way to drop documents from the db other than cloning the entire database?

Edit: I added benchmarks below. As seen in the graph, most of the performance problems seem to be caused by a long delay between two iterations at ~1-2k iterations. Is this maybe caused by additional memory allocation?

simple_query_compiler_with_single_file_validation                                                                             
                        time:   [25.527 µs 25.649 µs 25.799 µs]
Found 16 outliers among 100 measurements (16.00%)
  6 (6.00%) low severe
  3 (3.00%) low mild
  3 (3.00%) high mild
  4 (4.00%) high severe

simple_query_compiler_with_multi_file_validation                                                                             
                        time:   [552.60 µs 822.02 µs 1.0782 ms]
                        change: [-47.065% -22.119% +11.327%] (p = 0.17 > 0.05)
                        No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
  12 (12.00%) high severe

Benchmark code

fn bench_simple_query_compiler_with_single_file_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_single_file_validation", move |b| {
        b.iter(|| {
            let mut compiler = ApolloCompiler::new();
            compiler.add_type_system(schema, "schema.graphql");
            let executable_id = compiler.add_executable(query, "query.graphql");
            compiler.db.validate_executable(executable_id)
        })
    });
}

fn bench_simple_query_compiler_with_multi_file_validation(c: &mut Criterion) {

    let query = include_str!("testdata/simple_query.graphql");
    let schema = include_str!("testdata/simple_schema.graphql");

    let mut compiler = ApolloCompiler::new();
    compiler.add_type_system(schema, "schema.graphql");
    compiler.validate();
    c.bench_function("simple_query_compiler_with_multi_file_validation", move |b| {
        b.iter(|| {
            let executable_id = compiler.add_executable(query, "query.graphql");
            compiler.db.validate_executable(executable_id)
        })
    });
}

criterion_group!(
    benches,
    bench_simple_query_compiler_with_single_file_validation,
    bench_simple_query_compiler_with_multi_file_validation
);
criterion_main!(benches);

[lrlna]

Oooofff this screams "bug" for sure!

As we investigate, you can use sweep outdated values:

        use salsa::SweepStrategy;

        compiler.db.sweep_all(SweepStrategy::discard_outdated());

It is especially interesting that this even occurred when the executables all had the exact same content, as this might indicate some caching problems in salsa.

I don't think it's a caching problem in salsa, I feel like it's more of an issue in how we construct and update inputs. The outcome makes me think some sort of input internally has changed, which is why another instance of a storage was created. No input should have changed if the executables stayed the same.

Thank you for doing all this investigative work and reporting issues, by the way! Super helpful 🙏

[erikwrede]

Great to hear that 😊

I've played around with invoking the sweep strategy and now the response time increase seems to be polynomial-ish. The huge jump at around 1000 queries is removed by that, but the sweep strategy adds additional overhead to each operation that exceeds the computation costs for small queries.

Also tried profiling for some time, but haven't been able to pinpoint the results to that spot where response times jump yet, especially when I want usable traces, because a lot is optimized away in release or obfuscated behind salsa's logic. The pure add_executable call doesn't show any performance degradation over time.

By the way, what do you think about adding a criterion action to the Workflows: https://github.com/boa-dev/criterion-compare-action
Alternatively CodSpeed looks interesting, too.

It's not the most accurate indicator due to actions runner load variations, but it might still be a great tool to track high level impact of features and changes.

[SimonSapin]

I believe this is no longer relevant in apollo-compiler 1.0. The public API no longer has compiler objects or databases, so it’s not possible to have many schemas or documents in the same database. (It is possible to have a single schema parsed from multiple files.) Salsa is still used internally but we’ll gradually port validation code to no longer use it.

Please let us know if you still find something suspicious!

[erikwrede]

@SimonSapin thanks for all the follow-ups in the issue threads! I've been on 0.9 for some time and haven't looked into the public API changes since. Will get back when I re-check with 1.0.

Out of curiosity: Can you share (or point me to an explaining issue) why you are moving away from salsa? 😊

[SimonSapin]

In short: caching is hard to make both effective and correct.

Everything that apollo-compiler 0.x keeps in Salsa databases distributes roughly into:

• Results of parsing. When the input string change, this needs to be recomputed. When the input string doesn’t change, avoiding needless reparsing happens "naturally" when the API has users manipulating self-contained documents. A queryable database automatic dependency tracking doesn’t bring much.
• Trivial access to components of the above, for example "find a type definition by its name". Does not benefit from caching. Having data structures contain a name-keyed hash maps in the first place is better than doing linear scans and caching their results in hash maps on the side.
• Diagnostics created by validation. These contain source location information, to allow printing line numbers. If location is not part of the cache key, printing a diagnostic from the cache may print outdated and incorrect line numbers. If it's part of the cache key, changing anything in the input likely changes the location (encoded as byte offset from the start of the file) of everything that comes after it, so all diagnostics there will be cache misses.

All in all, apollo-compiler 0.x spent CPU time hashing nested data structures to use them as cache keys, but did not often save time by using cached results. Additionally, Salsa databases being !Sync is an obstacle in a multi-threaded server like Apollo Router. And adding inputs to a database to then refer to them by ID was a less convenient user API compared to manipulating self-contained Schema and ExecutableDocument Rust types.

It’s possible we’ll reintroduce some form of caching later, but it’ll take some careful design. Though my opinion is that parsing and validation don’t feel like they should be very computationally expensive, so I’d rather spend engineering time making them faster.