[ty] cache Type::is_redundant_with#20477
Conversation
Diagnostic diff on typing conformance testsChanges were detected when running ty on typing conformance tests--- old-output.txt 2025-10-16 11:31:13.724737373 +0000
+++ new-output.txt 2025-10-16 11:31:17.072755647 +0000
@@ -1,5 +1,5 @@
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/ef9f932/src/function/execute.rs:402:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_type_statement.py`: `PEP695TypeAliasType < 'db >::value_type_(Id(d017)): execute: too many cycle iterations`
-fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/ef9f932/src/function/execute.rs:402:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(16443)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/ef9f932/src/function/execute.rs:402:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_type_statement.py`: `PEP695TypeAliasType < 'db >::value_type_(Id(d417)): execute: too many cycle iterations`
+fatal[panic] Panicked at /home/runner/.cargo/git/checkouts/salsa-e6f3bb7c2a062968/ef9f932/src/function/execute.rs:402:17 when checking `/home/runner/work/ruff/ruff/typing/conformance/tests/aliases_typealiastype.py`: `infer_definition_types(Id(16c43)): execute: too many cycle iterations`
_directives_deprecated_library.py:15:31: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `int`
_directives_deprecated_library.py:30:26: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `str`
_directives_deprecated_library.py:36:41: error[invalid-return-type] Function always implicitly returns `None`, which is not assignable to return type `Self@__add__` |
CodSpeed Performance ReportMerging #20477 will improve performances by 12.52%Comparing Summary
Benchmarks breakdown
Footnotes
|
This comment was marked as outdated.
This comment was marked as outdated.
89b9a18 to
3c75a77
Compare
|
(Rebasing this on main to see the benchmark results) |
|
I took a look at the mypy primer hang for pandas-stubs and I'm starting to believe that this is the same as astral-sh/ty#1111. The cycle head is I've been able to reproduce the hang on almost every run using But I haven't yet been able to reproduce it after pulling in #20515 What I don't understand is why this is only happening when ty runs multi-threaded. Is it just because the cycle nesting is different and, depending on the nesting, the cycle converges faster? Or is there a multithreaded bug my hacky "re-use nested fixpoint values" implementation only happens to bypass the problematic code? |
|
I added some logging to the The MRO for Name("DatetimeIndex"): Iteration 122 vs 123 https://www.diffchecker.com/EJa9XVe0/ I suspect what's happening is that, depending on the entry query, the In fact, the query does panic fairly quickly. I added a few logging statements to Salsa and the output for a run where ty hangs is: which confirms that there's still a multi threaded bug in Salsa when a query participating in a cycle panics (as discussed here awslabs/shuttle#193 (comment)). For now, I suggest that we add some tracing logs to Salsa similar to what I did so that this becomes easier to discover (and doesn't require a 3h debugging session). I'll spend some time today to see if I can figure out why Salsa ends up hanging if there's a panic but I don't plan on spending too much time on it because astral-sh/ty#1111 seems more important than fixing something that improves our debugging experience. |
|
The fix that I've in mind for astral-sh/ty#1111 might help here because we would iterate all cycles at once, rather than having many nested cycles. I think that could help because I suspect that whether we run into this issue depends on how the |
|
I bumped the Salsa version to a temporary commit that contains the upstream fix and mypy primer is now failing with: I'll consider this "solved" from the salsa side (except for the optimisation discussed in astral-sh/ty#1111 which I expect to help here). @carljm ping me if you want me to look into anything else. |
|
65cf8dc to
5ee2420
Compare
|
|
Nice, mypy primer now completes with the new salsa version... The test failure is somewhat concerning. Is this new due to the salsa update? |
|
TODO: Address |
Nope, the test was failing before my update commit. @carljm maybe something you could take a look at (I've no idea what's happening there 😆 ) |
The There's also a panic in recursive protocol tests, though :-| |
|
My biggest concern with this PR is the memory increase - memo metadata = ~89MB
+ memo metadata = ~103MBThat's pretty substantial and can easily be a few GB for large projects (I suspect it uses around as much memory as Could we limit the caching scope? Would it even make sense to cache |
I expect this would be worse for memory usage, as the return type of It may be interesting to look at this again after #20602 and only cache |
These test failures should be fixed by #20653. We had some test output that depended on the ordering of Salsa IDs |
Caching |
c1695e1 to
8ef874d
Compare
|
Hmm, I did a rebase and one test is now stack overflowing. @carljm do you remember if this already happened before? I don't think it's something I should be able to mess up with salsa. Or did I mess up the rebase? Details |
|
The memory usage now looks better but it also isn't a perf improvement anymore |
it might be because Carl now needs to cache |
Pretty sure that both of these changes are because union simplification now uses Not sure about the stack overflow. I wouldn't expect this PR in and of itself to cause a stack overflow. |
but the mdtest you've added as part of this PR causes us to overflow on |
Oh yes, of course! Totally forgot I'd added that test in this PR. |
8ef874d to
ad69224
Compare
|
I switched the PR to cache There does also seem to be a new fuzzer failure on seed 102, though. |
|
This PR resolves some hangs. So I'm fine accepting the memory regression, and we can look into adding support for within a revision LRU to salsa (for queries that return a cloneable value). |
|
The fuzzer failure should be fixed with the latest salsa version (of my fixpoint rewrite PR) |
ad69224 to
081f95a
Compare
|
Added a tracking issue astral-sh/ty#1367 |
…rable * origin/main: Don't use codspeed or depot runners in CI jobs on forks (#20894) [ty] cache Type::is_redundant_with (#20477) Fix run-away for mutually referential instance attributes (#20645) [ty] Limit shown import paths to at most 5 unless ty runs with `-v` (#20912) [ty] Use field-specifier return type as the default type for the field (#20915) [ty] Do not assume that `field`s have a default value (#20914) [ty] Fix match pattern value narrowing to use equality semantics (#20882) Update setup instructions for Zed 0.208.0+ (#20902) Move TOML indent size config (#20905) [syntax-errors]: implement F702 as semantic syntax error (#20869) [ty] Heterogeneous unpacking support for unions (#20377) [ty] refactor `Place` (#20871) Auto-accept snapshot changes as part of typeshed-sync PRs (#20892) [`airflow`] Add warning to `airflow.datasets.DatasetEvent` usage (`AIR301`) (#20551) [`flake8-pyi`] Fix operator precedence by adding parentheses when needed (`PYI061`) (#20508) [`pyupgrade`] Fix false negative for `TypeVar` with default argument in `non-pep695-generic-class` (`UP046`) (#20660) Update parser snapshots (#20893) Fix syntax error false positives for escapes and quotes in f-strings (#20867)
…nt-sets * dcreager/non-non-inferable: Don't use codspeed or depot runners in CI jobs on forks (#20894) [ty] cache Type::is_redundant_with (#20477) Fix run-away for mutually referential instance attributes (#20645) [ty] Limit shown import paths to at most 5 unless ty runs with `-v` (#20912) [ty] Use field-specifier return type as the default type for the field (#20915) [ty] Do not assume that `field`s have a default value (#20914) [ty] Fix match pattern value narrowing to use equality semantics (#20882) Update setup instructions for Zed 0.208.0+ (#20902) Move TOML indent size config (#20905) [syntax-errors]: implement F702 as semantic syntax error (#20869) [ty] Heterogeneous unpacking support for unions (#20377) [ty] refactor `Place` (#20871) Auto-accept snapshot changes as part of typeshed-sync PRs (#20892) [`airflow`] Add warning to `airflow.datasets.DatasetEvent` usage (`AIR301`) (#20551) [`flake8-pyi`] Fix operator precedence by adding parentheses when needed (`PYI061`) (#20508) [`pyupgrade`] Fix false negative for `TypeVar` with default argument in `non-pep695-generic-class` (`UP046`) (#20660) Update parser snapshots (#20893) Fix syntax error false positives for escapes and quotes in f-strings (#20867)
Summary
TODO
Test Plan
Added test that stack overflowed before this PR.