-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[ty] Cache ClassType::nearest_disjoint_base
#22065
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
Conversation
Typing conformance resultsNo changes detected ✅ |
|
Merging this PR will improve performance by 4.21%
Performance Changes
Comparing Footnotes
|
|
Hm running this again locally (due to the lack of difference on Pydantic in the CodSpeed report), I can't reproduce the performance difference on Pydantic. I'll poke around some more. |
|
Seems like it could be worth it just for the memory reduction reported by mypy_primer (though I don't fully understand why this would reduce memory usage) |
|
It could make sense to run ty locally with One reason why memory usage might be reduced is that this PR deduplicates some salsa metadata because queries calling |
0c6b89e to
b6ff245
Compare
|
After rebasing on |
We round the the memory usage numbers to get more stable results. However, this also means that not all changes show in the memory usage report. You can run ty locally with |
|
I'm doing that, and the diff in sphinx went away :p I also can't see a diff in Prefect locally. |
b6ff245 to
4a5c711
Compare
|
I ran ty with the environment variable On On this PR (after rebasing it on So, this PR does not lead to an especially significant memory reduction when run on a large project -- but it is still a reduction in memory usage. Given the performance improvement reported by codspeed, and the fact that we know that this method is very hot, I think this PR is very worthwhile. Here are the full Salsa reports. mainThis PR |
4a5c711 to
d83e0dc
Compare
|
@zanieb any reason why this is still in draft? Looks landable to me :-) |
I think this is a bit more nuanced. Codspeed does report that performance regresses for some benchmarks. The memory improvement is nice but I expect it to be less (or total memory usage might even increase) if we reduce the metadata that salsa tracks, e.g. by salsa-rs/salsa#1045 Because of that, I would suggest we don't give too much weight to the memory improvement and make the merge decision solely based on the perf improvement. |
For sure. The main point I wanted to make is that (unlike many instances where we add caches), this doesn't appear to lead to any memory-usage regression (at least not right now). There are only two reported performance regressions (vs seven reported improvements)... but I suppose it is a bit concerning that there is a 3% regression reported on the multithreaded benchmark. Most real-world uses of ty are going to invoke it with multithreading enabled, and the regression on that benchmark could indicate that the cache is leading to lock contention between threads, I suppose? I guess it could be worth also running the ty_benchmark script to see if any regressions are reported there. |
|
I merged |
d83e0dc to
a1780c9
Compare
|
It was just in draft because I wasn't particularly confident it did anything and didn't want to distract ya'll :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems worth it to me given 4-7% speedups locally on pandas with ty_benchmark, no reported regressions locally with ty_benchmark, consistent speedups on most Codspeed benchmarks, and no significant impact on memory usage. But I will defer to @MichaReiser
|
I ran |
Pulled out of #22052
Locally, this improved performance on Pydantic by 5%, avoiding the regressions in the referenced pull request.