[ty] increase the limit on the number of elements in a non-recursively defined literal union#21683
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
CodSpeed Performance ReportMerging #21683 will degrade performances by 9.91%Comparing Summary
Benchmarks breakdown
Footnotes
|
|
Hmm, for example, when inspecting the following code, I see a clear performance improvement locally, but the change on codspeed seems less clear: class Counter:
def __init__(self: "Counter"):
self.count = 0
def increment(self: "Counter"):
self.count = self.count + 1
reveal_type(Counter().count) # revealed: Unknown | intmain: # release
$ hyperfine -i "ty check widen.py"
Benchmark 1: ty check widen.py
Time (mean ± σ): 103.3 ms ± 8.6 ms [User: 56.9 ms, System: 45.0 ms]
Range (min … max): 96.4 ms … 136.6 ms 22 runs
# debug
$ hyperfine -i "ty check widen.py"
Benchmark 1: ty check widen.py
Time (mean ± σ): 899.9 ms ± 6.2 ms [User: 849.7 ms, System: 51.9 ms]
Range (min … max): 889.4 ms … 910.3 ms 10 runs# release
$ hyperfine -i "ty check widen.py"
Benchmark 1: ty check widen.py
Time (mean ± σ): 59.6 ms ± 9.2 ms [User: 26.5 ms, System: 37.8 ms]
Range (min … max): 52.5 ms … 95.7 ms 31 runs
# debug
$ hyperfine -i "ty check widen.py"
Benchmark 1: ty check widen.py
Time (mean ± σ): 267.2 ms ± 4.1 ms [User: 222.2 ms, System: 55.6 ms]
Range (min … max): 261.6 ms … 276.5 ms 10 runs |
|
Conversely, the apparent performance degradation is due to increasing The performance impact of changing 190: https://codspeed.io/astral-sh/ruff/runs/compare/6929edd91e804897641888e6..6929fa9ed77b771deb1a9244 |
…e performance diff" This reverts commit 961c578.
There might just not be enough very large unions so that, in the grand picture, the improvement is lost in noise |
|
I think this is ready for review. Here are some comments: The performance degradation of codspeed(many_string_assignments) is entirely expected. There is no clear standard for what the new upper limit for literal unions should be, but I've set it to 256 for now. |
carljm
left a comment
There was a problem hiding this comment.
This is great, thank you!
* origin/main: (41 commits) [ty] Carry generic context through when converting class into `Callable` (#21798) [ty] Add more tests for renamings (#21810) [ty] Minor improvements to `assert_type` diagnostics (#21811) [ty] Add some attribute/method renaming test cases (#21809) Update mkdocs-material to 9.7.0 (Insiders now free) (#21797) Remove unused whitespaces in test cases (#21806) [ty] fix panic when instantiating a type variable with invalid constraints (#21663) [ty] fix build failure caused by conflicts between #21683 and #21800 (#21802) [ty] do nothing with `store_expression_type` if `inner_expression_inference_state` is `Get` (#21718) [ty] increase the limit on the number of elements in a non-recursively defined literal union (#21683) [ty] normalize typevar bounds/constraints in cycles (#21800) [ty] Update completion eval to include modules [ty] Add modules to auto-import [ty] Add support for module-only import requests [ty] Refactor auto-import symbol info [ty] Clarify the use of `SymbolKind` in auto-import [ty] Redact ranking of completions from e2e LSP tests [ty] Tweaks tests to use clearer language [ty] Update evaluation results [ty] Make auto-import ignore symbols in modules starting with a `_` ...
Summary
Closes astral-sh/ty#957
As explained in astral-sh/ty#957, literal union types for recursively defined values can be widened early to speed up the convergence of fixed-point iterations.
This PR achieves this by embedding a marker in
UnionTypethat distinguishes whether a value is recursively defined.This also allows us to identify values that are not recursively defined, so I've increased the limit on the number of elements in a literal union type for such values.
Edit: while this PR doesn't provide the significant performance improvement initially hoped for, it does have the benefit of allowing the number of elements in a literal union to be raised above the salsa limit, and indeed mypy_primer results revealed that a literal union of 220 elements was actually being used.
Test Plan
call/union.mdhas been updated