Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster accumulators #615

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

MichaReiser
Copy link
Contributor

@MichaReiser MichaReiser commented Nov 17, 2024

This PR implements faster accumulators.

Accumulators are relatively costly today because collecting the accumulated values requires iterating over the queries' entire dependency tree.
This cost is negligible with a cold DB but adds up for very deep trees.

For example, adding accumulators to Ruff resulted in a 30% perf regression astral-sh/ruff#14116

This PR adds a new field to AccumulatedMap that tracks whether any dependency of the current query has any accumulated values. It checks that field when collecting the accumulated values, allowing it to skip entire subtrees.

This approach should make accumulated values nearly "free" in the common cases where there are no or only very few diagnostics.

Test Plan

I added a benchmark and I verified that the perf regression in the linked Ruff PR is mostly gone after using this salsa version.

Running the benchmark locally gives me

     Running benches\accumulator.rs (target\release\deps\accumulator-51d27e7934278de6.exe)
accumulator             time:   [666.27 µs 676.29 µs 688.11 µs]
                        change: [-32.104% -30.905% -29.682%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
  1 (1.00%) high mild
  11 (11.00%) high severe

where the baseline is main

Copy link

netlify bot commented Nov 17, 2024

Deploy Preview for salsa-rs canceled.

Name Link
🔨 Latest commit 76079d2
🔍 Latest deploy log https://app.netlify.com/sites/salsa-rs/deploys/674f18c81e639b0008aa6168

@@ -17,6 +22,16 @@ impl AccumulatedMap {
.accumulate(value);
}

/// Marks the accumulator result that some nested query has some accumulated values.
pub fn add_dependency_with_accumulated_values(&mut self) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the naming at all but struggled to come up with something better.

Copy link

codspeed-hq bot commented Nov 17, 2024

CodSpeed Performance Report

Merging #615 will not alter performance

Comparing MichaReiser:micha/faster-accumulators (76079d2) with master (b7331d9)

Summary

✅ 8 untouched benchmarks
🆕 1 new benchmarks

Benchmarks breakdown

Benchmark master MichaReiser:micha/faster-accumulators Change
🆕 accumulator N/A 5.1 ms N/A

@MichaReiser MichaReiser marked this pull request as ready for review November 17, 2024 18:10
Copy link
Contributor

@davidbarsky davidbarsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving modulo one nit. I'll let you decide whether to merge or not.

src/accumulator/accumulated_map.rs Outdated Show resolved Hide resolved
@MichaReiser MichaReiser added this pull request to the merge queue Dec 3, 2024
Merged via the queue into salsa-rs:master with commit e68679b Dec 3, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants