Skip to content

Conversation

@MichaReiser
Copy link
Member

@MichaReiser MichaReiser commented Jun 28, 2025

Summary

This PR updates Salsa to pull in Ibraheems multithreading improvements

salsa-rs/salsa#921

Performance

A small regression for single-threaded benchmarks is expected because papaya is slightly slower than a Mutex<FxHashMap> in the uncontested case (~10%). However, this shouldn't matter as much in practice because:

  1. Salsa has a fast-path when only using 1 DB instance which is the common case in production. This fast-path is not impacted by the changes but we measure the slow paths in our benchmarks (because we use multiple db instances)
  2. Fixing the 10x slowdown for the congested case (multi threading) outweights the downsides of a 10% perf regression for single threaded use cases, especially considering that ty is heavily multi threaded.

Test Plan

cargo test

@MichaReiser MichaReiser added internal An internal refactor or improvement ty Multi-file analysis & type inference labels Jun 28, 2025
@MichaReiser MichaReiser requested a review from ibraheemdev June 28, 2025 20:13
@MichaReiser
Copy link
Member Author

MichaReiser commented Jun 28, 2025

@ibraheemdev can you take over this PR. It seems something broke the heap size feature upstream (maybe salsa-rs/salsa#927?)

Edit: I pinned the commit to before said commit for now but that also means that the papaya commit is missing too. We should add a test in upstream salsa that catches the regression and fix it before merging this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jun 28, 2025

mypy_primer results

Changes were detected when running on open source projects
async-utils (https://github.com/mikeshardmind/async-utils)
- TOTAL MEMORY USAGE: ~49MB
+ TOTAL MEMORY USAGE: ~45MB

beartype (https://github.com/beartype/beartype)
- TOTAL MEMORY USAGE: ~88MB
+ TOTAL MEMORY USAGE: ~97MB

discord.py (https://github.com/Rapptz/discord.py)
-     memo fields = ~189MB
+     memo fields = ~207MB

alerta (https://github.com/alerta/alerta)
- TOTAL MEMORY USAGE: ~117MB
+ TOTAL MEMORY USAGE: ~106MB

isort (https://github.com/pycqa/isort)
- TOTAL MEMORY USAGE: ~80MB
+ TOTAL MEMORY USAGE: ~88MB

paasta (https://github.com/yelp/paasta)
-     memo fields = ~156MB
+     memo fields = ~171MB

django-stubs (https://github.com/typeddjango/django-stubs)
-     memo fields = ~156MB
+     memo fields = ~142MB

static-frame (https://github.com/static-frame/static-frame)
-     memo fields = ~304MB
+     memo fields = ~334MB

openlibrary (https://github.com/internetarchive/openlibrary)
-     memo fields = ~171MB
+     memo fields = ~189MB

rotki (https://github.com/rotki/rotki)
-     memo fields = ~445MB
+     memo fields = ~490MB

scikit-learn (https://github.com/scikit-learn/scikit-learn)
- TOTAL MEMORY USAGE: ~717MB
+ TOTAL MEMORY USAGE: ~652MB

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 28, 2025

CodSpeed Instrumentation Performance Report

Merging #19020 will degrade performances by 7.34%

Comparing micha/salsa-update (baee7c3) with main (ebc70a4)

Summary

❌ 1 regressions
✅ 38 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
ty_micro[many_tuple_assignments] 128.9 ms 139.2 ms -7.34%

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 28, 2025

CodSpeed WallTime Performance Report

Merging #19020 will improve performances by ×8.5

Comparing micha/salsa-update (baee7c3) with main (ebc70a4)

Summary

⚡ 1 improvements
✅ 7 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
multithreaded[pydantic] 2,947.7 ms 346.6 ms ×8.5

@github-actions
Copy link
Contributor

github-actions bot commented Jun 28, 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
Copy link
Member Author

Hmm, the memory usage changes here must be noise?

@ibraheemdev ibraheemdev force-pushed the micha/salsa-update branch from d609605 to baee7c3 Compare July 2, 2025 13:36
@ibraheemdev ibraheemdev marked this pull request as ready for review July 2, 2025 13:36
@ibraheemdev ibraheemdev merged commit e9b5ea7 into main Jul 2, 2025
35 of 36 checks passed
@ibraheemdev ibraheemdev deleted the micha/salsa-update branch July 2, 2025 13:55
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