Skip to content

[red-knot] make large-union benchmark slow again #17418

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

Merged
merged 2 commits into from
Apr 16, 2025
Merged

[red-knot] make large-union benchmark slow again #17418

merged 2 commits into from
Apr 16, 2025

Conversation

carljm
Copy link
Contributor

@carljm carljm commented Apr 16, 2025

Summary

Now that we've made the large-unions benchmark fast, let's make it slow again!

This adds a following operation (checking len) on the large union, which is slow, even though building the large union is now fast. (This is also observed in a real-world code sample.) It's slow because for every element of the union, we fetch its __len__ method and check it for compatibility with Sized.

We can make this fast by extending the grouped-types approach, as discussed in #17403, so that we can do this __len__ operation (which is identical for every literal string) just once for all literal strings, instead of once per literal string type in the union.

Until we do that, we can make this acceptably fast again for now by setting a lowish limit on union size, which we can increase in the future when we make it fast. This is what I'll do in the next PR.

Test Plan

cargo bench --bench red_knot

@carljm carljm added the ty Multi-file analysis & type inference label Apr 16, 2025
Copy link
Contributor

github-actions bot commented Apr 16, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link

codspeed-hq bot commented Apr 16, 2025

CodSpeed Performance Report

Merging #17418 will degrade performances by 98.64%

Comparing cjm/unionlimit (91df0e5) with main (a1f3619)

Summary

❌ 1 regressions
✅ 32 untouched benchmarks

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

Benchmarks breakdown

Benchmark BASE HEAD Change
red_knot_micro[many_string_assignments] 78.9 ms 5,816.3 ms -98.64%

Base automatically changed from cjm/bigunions3 to main April 16, 2025 13:55
@carljm carljm enabled auto-merge (squash) April 16, 2025 14:01
@carljm carljm merged commit 5a115e7 into main Apr 16, 2025
21 checks passed
@carljm carljm deleted the cjm/unionlimit branch April 16, 2025 14:05
dcreager added a commit that referenced this pull request Apr 16, 2025
* main: (44 commits)
  [`airflow`] Extend `AIR311` rules (#17422)
  [red-knot] simplify union size limit handling (#17429)
  [`airflow`] Extract `AIR311` from `AIR301` rules (`AIR301`, `AIR311`) (#17310)
  [red-knot] set a size limit on unions of literals (#17419)
  [red-knot] make large-union benchmark slow again (#17418)
  [red-knot] optimize building large unions of literals (#17403)
  [red-knot] Fix comments in type_api.md (#17425)
  [red-knot] Do not assume that `x != 0` if `x` inhabits `~Literal[0]` (#17370)
  [red-knot] make large-union benchmark more challenging (#17416)
  [red-knot] Acknowledge that `T & anything` is assignable to `T` (#17413)
  Update Rust crate clap to v4.5.36 (#17381)
  Raise syntax error when `\` is at end of file (#17409)
  [red-knot] Add regression tests for narrowing constraints cycles (#17408)
  [red-knot] Add some knowledge of `__all__` to `*`-import machinery (#17373)
  Update taiki-e/install-action digest to be7c31b (#17379)
  Update Rust crate mimalloc to v0.1.46 (#17382)
  Update PyO3/maturin-action action to v1.49.1 (#17384)
  Update Rust crate anyhow to v1.0.98 (#17380)
  dependencies: switch from `chrono` to `jiff`
  Update Rust crate bstr to v1.12.0 (#17385)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants