-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[red-knot] type narrowing #12706
[red-knot] type narrowing #12706
Conversation
CodSpeed Performance ReportMerging #12706 will degrade performances by 13.78%Comparing Summary
Benchmarks breakdown
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work and the documentation is excellent.
We may be able to remove quiet a bit of code from BitSet
by using smallvec (if we want?) and we probably clone the narrowing constraints on every query run which we should avoid ;)
crates/red_knot_python_semantic/src/semantic_index/use_def/bitset.rs
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/src/semantic_index/use_def/bitset.rs
Outdated
Show resolved
Hide resolved
crates/red_knot_python_semantic/src/semantic_index/use_def/bitset.rs
Outdated
Show resolved
Hide resolved
@MichaReiser Thanks for the excellent review! I think I've addressed all the comments. Lots of changes, so you may want to take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only skimmed through. Thanks for addressing all the feedback and we're down to a 13% perf regression :)
crates/red_knot_python_semantic/src/semantic_index/use_def/bitset.rs
Outdated
Show resolved
Hide resolved
} | ||
|
||
/// Merge two [`SymbolState`] and return the result. | ||
pub(super) fn merge(a: &SymbolState, b: &SymbolState) -> SymbolState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think we could make use of it but it would probably complicate the logic a bit because you need two versions of push
and a into_iter
version for constraints
For example, couldn't we avoid cloning the constraints and instead take them right from the symbol state?
Hmm, I made a reply comment about the "taking ownership in merge" thing but now I don't see it in the UI anywhere. Anyway the upshot was that you're right, I was able to do that; it didn't help the benchmarks but I left it in anyway. Going to go ahead and merge this, but if you see anything in those latest changes that looks dumb, don't hesitate to comment and I'll fix! |
Extend the
UseDefMap
to also track which constraints (provided by e.g.if
tests) apply to each visible definition.Uses a custom
BitSet
andBitSetArray
to track which constraints apply to which definitions, while keeping data inline as much as possible.