[ty] Short circuit ReachabilityConstraints::analyze_single for dynamic types#19867
[ty] Short circuit ReachabilityConstraints::analyze_single for dynamic types#19867MichaReiser merged 3 commits intomainfrom
ReachabilityConstraints::analyze_single for dynamic types#19867Conversation
MichaReiser
requested review from
AlexWaygood,
dcreager and
sharkdp
as code owners
MichaReiser
added
performance
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
CodSpeed WallTime Performance ReportMerging #19867 will not alter performanceComparing Summary
|
|
Nice, this is even a win for non concurrent runs. All credit goes to @carljm for the much better fix than what I came up with |
carljm
left a comment
carljm
left a comment
There was a problem hiding this comment.
Perf results look amazing! Hopefully they persist once we get the logic right 😆 (I expect they will)
| // doesn't help much. | ||
| // See <https://github.com/astral-sh/ty/issues/968>. | ||
| if matches!(ty, Type::Dynamic(_)) { | ||
| return Truthiness::AlwaysTrue.negate_if(!predicate.is_positive); |
There was a problem hiding this comment.
Sorry, I gave you the wrong info here, I had the sense of this constraint reversed in my head. This needs to return Truthiness::AlwaysFalse, not Truthiness::AlwaysTrue! I suspect the mypy-primer run is taking so long because there are now tons more bogus diagnostics 😆
The fact that no test failed on this change also suggests we need an explicit mdtest like this:
from typing import Never, Any
def f(func: Any) -> Never:
func()Where an invalid-return-type is expected (because the call to func() is not terminal). That test fails on this PR currently.
| return Truthiness::AlwaysTrue.negate_if(!predicate.is_positive); | |
| return Truthiness::AlwaysFalse.negate_if(!predicate.is_positive); |
There was a problem hiding this comment.
Don't worry, it does. The most congested lock is now for IntersectionType:
And the wall clock time on the large project on my linux machine goes from 42s .. to 16s which is rather ridiculous
|
|
Okay, the single threaded benchmarks don't improve by that much anymore. But it's still a huge improvement for multi threaded workloads (with many cores... not what github actions provides) |
* main: Don't cache files with diagnostics (#19869) [ty] support recursive type aliases (#19805) [ty] Remove unsafe `salsa::Update` implementations in `tuple.rs` (#19880) [ty] Function argument inlay hints (#19269) [ty] Remove Salsa interning for `TypedDictType` (#19879) Fix `lint.future-annotations` link (#19876) [ty] Reduce memory usage of `TupleSpec` and `TupleType` (#19872) [ty] Track heap usage of salsa structs (#19790) Update salsa to pull in tracked struct changes (#19843) [ty] simplify CycleDetector::visit signature (#19873) [ty] use interior mutability in type visitors (#19871) [ty] Fix tool name is None when no ty path is given in ty_benchmark (#19870) [ty] Remove `Type::Tuple` (#19669) [ty] Short circuit `ReachabilityConstraints::analyze_single` for dynamic types (#19867)
4 participants
Summary
Fixes astral-sh/ty#968
This reduces the wall time for a large project from ~24s to ~19s. I plan to do another perf recording to verify that the lock congestion is entirely gone but I first need to walk the dog :)
See inline comment for more details.
Test Plan
cargo nextest