Skip to content

[red-knot] Update salsa#17320

Merged
carljm merged 3 commits intomainfrom
micha/update-salsa
Apr 9, 2025
Merged

[red-knot] Update salsa#17320
carljm merged 3 commits intomainfrom
micha/update-salsa

Conversation

@MichaReiser
Copy link
Member

Summary

Update Salsa to pull in salsa-rs/salsa#788 which fixes the, by now, famous access to field whilst the value is being initialized.

This PR also re-enables all tests that previously triggered the panic.

Test Plan

cargo test

@MichaReiser MichaReiser added the ty Multi-file analysis & type inference label Apr 9, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 2025

mypy_primer results

No ecosystem changes detected ✅

@codspeed-hq
Copy link

codspeed-hq bot commented Apr 9, 2025

CodSpeed Performance Report

Merging #17320 will not alter performance

Comparing micha/update-salsa (09a8741) with main (144484d)

Summary

✅ 32 untouched benchmarks

@MichaReiser
Copy link
Member Author

Lol... Codspeed is confused. It thinks I acknowledged this regression a month ago.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 9, 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 force-pushed the micha/update-salsa branch 2 times, most recently from f9383df to 0e8ac9e Compare April 9, 2025 17:17
@MichaReiser
Copy link
Member Author

It looks like the main reason for the regression is that we now see more deep_verified_memo calls compared to before. I'm a bit confused about this because the old code should already always have reached the deep_verify_memo (unless it panicked). The only thing I can think of is that we now run deep_verify_memo for provisional memos where we always returned Changed before... but wouldn't that lead to a perf improvement instead of a regression?

@MichaReiser MichaReiser marked this pull request as ready for review April 9, 2025 17:31
AlexWaygood

This comment was marked as resolved.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@dcreager dcreager left a comment

Choose a reason for hiding this comment

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

🎉 Thanks Micha!

@MichaReiser
Copy link
Member Author

I think I may have identified the reason for the perf regression... salsa-rs/salsa#789

@MichaReiser
Copy link
Member Author

TODO: Update the commit after my other PR is merged

@MichaReiser
Copy link
Member Author

Yes, this was it. Now it's neutral.

@MichaReiser
Copy link
Member Author

I'll merge this tomorrow. The salsa merge queue is too slow. If anyone wants this sooner. Feel free to update the Cargo.tomls with the commit hash from the salsa repo (tip of head)

@carljm carljm merged commit 9f6913c into main Apr 9, 2025
23 checks passed
@carljm carljm deleted the micha/update-salsa branch April 9, 2025 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants