[red-knot] Further optimize *-import visibility constraints#17375
[red-knot] Further optimize *-import visibility constraints#17375AlexWaygood merged 3 commits intomainfrom
*-import visibility constraints#17375Conversation
|
|
3% speedup on the cold benchmark 🥳 https://codspeed.io/astral-sh/ruff/branches/alex%2Foptimize-star-imports |
There was a problem hiding this comment.
Thank you, this looks correct! Great that we can reclaim even more performance.
self.scope_start_visibility tracks the visibility of x_new = <unbound> bindings of symbols that are not yet defined (symbols that are already defined, track visibility of their own x_defined = <unbound> binding). This is important when we model something like
if True:
x = 1
else:
# At this point in control flow, we have a scope_start_visibility
# of `~[True]`, which we'll use for the `x = <unbound>` binding
# that will be implicitly created for this control flow branch
# when we merge control flow with the `if` branch below.
# a use of `x` here sees `x = 1` with visibility `[True]` and
# `x = <unbound>` with visibility `~[True]`.The important thing is that scope_start_visibility returns to AlwaysTrue after this if-else branch. x can not be unbound, but new symbols would still be unbound. You can basically ask: if I would see a use of has_not_been_bound_yet at this point in control flow, would I still be able to see the implicit has_not_been_bound_yet = <unbound> binding at the scope start? And the answer for this example is yes. Things could be different if there would be terminal statements inside the branches:
def _():
if True:
return
# scope_start_visibility would be AlwaysFalse hereFor the star-import construct that we're modeling, the scope-start-visibility should not be affected (after control flow has merged), since there are no terminal statements inside. A use of has_not_been_bound_yet after this construct would have the same visibility of the scope start than before:
if <condition>:
from module import X
else:
# empty
|
|
||
| #[must_use = "A `*`-import visibility constraint must always be negated after it is added"] | ||
| pub(super) fn record_star_import_visibility_constraint( | ||
| pub(super) fn record_and_negate_star_import_visibility_constraint( |
There was a problem hiding this comment.
I think it would be useful to future-us to add a doc comment to this method explaining what it is doing and why it is safe to do it, in the limited cases where we use this function.
There was a problem hiding this comment.
Added some docs! Sadly this PR is no longer net-negative in terms of whether it adds or removes lines of code, but so it goes 🥲
Would appreciate a quick review from you or @sharkdp to double-check everything in the doc-comment is accurate!
|
I think there are probably some optimisations we could also make for the "slow path" where the But I don't know if it's worth the complexity cost to make that optimisation, since I think it would require adding more APIs to the use-def map that would only be used in this one place, and we probably don't hit the slow path very often at all. |
sharkdp
left a comment
sharkdp
left a comment
There was a problem hiding this comment.
The new comment looks great, thank you
* main: (31 commits) [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) [red-knot] Further optimize `*`-import visibility constraints (#17375) [red-knot] Minor 'member_lookup_with_policy' fix (#17407) [red-knot] Initial support for `dataclass`es (#17353) Sync vendored typeshed stubs (#17402) [red-knot] improve function/bound method type display (#17294) [red-knot] Move relation methods from `CallableType` to `Signature` (#17365) [syntax-errors] `await` outside async functions (#17363) [red-knot] optimize is_subtype_of for literals (#17394) [red-knot] add a large-union-of-string-literals benchmark (#17393) Update pre-commit dependencies (#17383) [red-knot] mypy_primer: Fail job on panic or internal errors (#17389) [red-knot] Document limitations of diagnostics-silencing in unreachable code (#17387) [red-knot] detect unreachable attribute assignments (#16852) ...
3 participants
Summary
Fixes #17322. Remove any need for snapshotting when applying visibility constraints to
*imports. This also ends up simplifying the code!Test Plan
Existing tests all pass. I'd appreciate a careful review, though. Is it definitely safe to remove these calls, as I am doing currently in the PR?
E.g. the
UseDefMapBuilder::negate_star_import_visibility_constraintmethod onmainhas this call:and
UseDefMapBuilder::mergehas this call:Both calls are currently removed in this PR for the "fast path" for newly added
*-import definitions, sinceUseDefMapBuilder::negate_star_import_visibility_constraintis removed as part of this PR, andUseDefMapBuilder::mergeis no longer called in the fast path. If it's not safe to remove these calls from the fast path, is there a test I could add that passes onmain, but fails with them removed?