Skip to content

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
@codspeed-hq
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 ty Multi-file analysis & type inference label Feb 24, 2025
@github-actions
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 ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants