Skip to content

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jul 9, 2025

Summary

A full equivalence check at the top of Type::has_relation_to() is unnecessary. All our mdtests, and all our property tests, pass if it is replaced with a much cheaper naive equality check. The reason for this is that equivalence is generally the same as equality for atomic types (types that cannot contain other types); and "non-atomic" types (types that can contain other types within them) all have comprehensive subtyping logic lower down in the Type::has_relation_to() method. For every non-atomic type in our model currently, our subtyping logic is sufficiently generalized to also cover equivalent types; there's no need to check for equivalence separately.

Up till now, the inefficiency here hasn't mattered that much, because comparing two nominal types for equivalence is generally pretty cheap. (It is not cheap for large unions, large intersections or large tuples (or combinations of the three), but these are all somewhat rare.) Unfortunately, however, comparing two structural types for equivalence can be extremely expensive: determining whether two protocols are equivalent can necessitate a full structural check between the two underlying interfaces. That means that this PR yields a 7% speedup when checking DateType, which has some large protocols in it. We also see 1% speedups on a lot of other micro- and macro-benchmarks for ty, and no slowdowns: https://codspeed.io/astral-sh/ruff/branches/alex%2Fremove-equivalent-check?runnerMode=Instrumentation.

Test plan

  • Existing tests all pass
  • QUICKCHECK_TESTS=100000 cargo test --release -p ty_python_semantic -- --ignored types::property_tests::stable passes
  • Clean mypy_primer report

@AlexWaygood AlexWaygood added performance Potential performance improvement ty Multi-file analysis & type inference labels Jul 9, 2025
Copy link
Contributor

github-actions bot commented Jul 9, 2025

mypy_primer results

No ecosystem changes detected ✅
No memory usage changes detected ✅

Copy link

codspeed-hq bot commented Jul 9, 2025

CodSpeed Instrumentation Performance Report

Merging #19230 will improve performances by 7.33%

Comparing alex/remove-equivalent-check (6c9d9fb) with main (35a33f0)

Summary

⚡ 1 improvements
✅ 39 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
DateType 263.6 ms 245.6 ms +7.33%

@AlexWaygood AlexWaygood marked this pull request as ready for review July 9, 2025 12:15
@AlexWaygood AlexWaygood changed the title [ty] Remove expensive and unnecessary equivalence check from the top of Type::has_relation_to() [ty] Optimize protocol subtyping by removing expensive and unnecessary equivalence check from the top of Type::has_relation_to() Jul 9, 2025
Copy link
Contributor

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! Thank you

@AlexWaygood AlexWaygood merged commit 59aa869 into main Jul 10, 2025
37 checks passed
@AlexWaygood AlexWaygood deleted the alex/remove-equivalent-check branch July 10, 2025 08:42
UnboundVariable pushed a commit to UnboundVariable/ruff that referenced this pull request Jul 11, 2025
…re_help

* 'main' of https://github.com/astral-sh/ruff:
  Add simple integration tests for all output formats (astral-sh#19265)
  [`flake8-return`] Fix false-positive for variables used inside nested functions in `RET504`  (astral-sh#18433)
  [ty] Add a `--quiet` mode (astral-sh#19233)
  Treat form feed as valid whitespace before a line continuation (astral-sh#19220)
  [ty] Make `check_file` a salsa query (astral-sh#19255)
  [ty] Consolidate submodule resolving code between `types.rs` and `ide_support.rs` (astral-sh#19256)
  [ty] Remove countme from salsa-structs (astral-sh#19257)
  [ty] Improve and document equivalence for module-literal types (astral-sh#19243)
  [ty] Optimize protocol subtyping by removing expensive and unnecessary equivalence check from the top of `Type::has_relation_to()` (astral-sh#19230)
  [ty] Ecosystem analyzer: parallelize, fix race condition (astral-sh#19252)
  [ty] Add completion kind to playground (astral-sh#19251)
  [ty] Deploy ecosystem diff to Cloudflare pages (astral-sh#19234)
  [ty] Add semantic token provider to playground (astral-sh#19232)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Potential performance improvement ty Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants