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

Update Salsa #16338

Merged
merged 1 commit into from
Feb 24, 2025
Merged

Update Salsa #16338

merged 1 commit into from
Feb 24, 2025

Conversation

MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Feb 24, 2025

Update Salsa to fix stale query results for multi-argument queries where one argument is a tracked struct.

The performance regression isn't unexpected. #15763 introduced coarse grained dependencies and the version used in that branch removed adding dependencies for any tracked fields (not even to the tracked struct itself). It turned out, that this was incorrect in the case where tracked structs (and there IDs) get reused. The upstream fix now records a dependency on the tracked struct. This is still better than without coarse grained dependencies where salsa recorded a dependency for each tracked field but it isn't free (but required for correctness).

@MichaReiser MichaReiser added the internal An internal refactor or improvement label Feb 24, 2025
Copy link

codspeed-hq bot commented Feb 24, 2025

CodSpeed Performance Report

Merging #16338 will degrade performances by 5.29%

Comparing micha/update-salsa (5295d99) with main (5eaf225)

Summary

❌ 1 (👁 1) regressions
✅ 31 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
👁 red_knot_check_file[incremental] 5.5 ms 5.8 ms -5.29%

@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Feb 24, 2025
Copy link
Contributor

github-actions bot commented Feb 24, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@MichaReiser MichaReiser marked this pull request as ready for review February 24, 2025 08:44
@MichaReiser MichaReiser merged commit 81a5765 into main Feb 24, 2025
21 checks passed
@MichaReiser MichaReiser deleted the micha/update-salsa branch February 24, 2025 08:44
dcreager added a commit that referenced this pull request Feb 25, 2025
* main: (38 commits)
  [red-knot] Use arena-allocated association lists for narrowing constraints (#16306)
  [red-knot] Rewrite `Type::try_iterate()` to improve type inference and diagnostic messages (#16321)
  Add issue templates (#16213)
  Normalize inconsistent markdown headings in docstrings (#16364)
  [red-knot] Better diagnostics for method calls (#16362)
  [red-knot] Add argfile and windows glob path support (#16353)
  [red-knot] Handle pipe-errors gracefully (#16354)
  Rename `venv-path` to `python` (#16347)
  [red-knot] Fixup some formatting in `infer.rs` (#16348)
  [red-knot] Restrict visibility of more things in `class.rs` (#16346)
  [red-knot] Add diagnostic for class-object access to pure instance variables (#16036)
  Add `per-file-target-version` option (#16257)
  [PLW1507] Mark fix unsafe (#16343)
  [red-knot] Add a test to ensure that `KnownClass::try_from_file_and_name()` is kept up to date (#16326)
  Extract class and instance types (#16337)
  Re-order changelog entries for 0.9.7 (#16344)
  [red-knot] Add support for `@classmethod`s (#16305)
  Update Salsa (#16338)
  Update Salsa part 1 (#16340)
  Upgrade Rust toolchain to 1.85.0 (#16339)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants