Skip to content
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

Experiment with tracking which entities actually may be narrowed #51525

Open
DanielRosenwasser opened this issue Nov 14, 2022 · 4 comments
Open
Assignees
Labels
Domain: Performance Reports of unusually slow behavior Experimentation Needed Someone needs to try this out to see what happens Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

In Pyright, narrowing can be avoided by keeping track of which entities actually get narrowed within a given scope. This could save a good amount of time by avoiding redundant walks up the control flow graph just to discover nothing interesting a given variable or property.

I'm not sure if Pyright also tracks the first location at which an entity may be narrowed, but that information could also be used to signal when a narrowing walk needs to stop.

This would come at the cost of some memory overhead, but we'd need to experiment to see what the trade-offs are.

@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript In Discussion Not yet reached consensus Domain: Performance Reports of unusually slow behavior labels Nov 14, 2022
@jakebailey
Copy link
Member

jakebailey commented Nov 14, 2022

I think the code in question for pyright was introduced in:

@fatcerberus
Copy link

fatcerberus commented Nov 14, 2022

Wait, does TS walk the control flow graph backwards to discover narrowings? I always figured the graph looked something like x = string | number -> if (true) x = string -> else x = number and the compiler would know exactly which part of the tree it was in at the time it encountered a reference to x so it could just look up the current type directly.

@DanielRosenwasser
Copy link
Member Author

It does walk backwards. If it walked forwards, it would have to retain the context of all potential narrowing operations (and for all potential types we'd want to narrow from - which is occasionally necessary for things like narrowing constraints of generics rather than the generics themselves). That wouldn't work in our system where checking an expression should be as lazy as possible.

Anyway, in line with that, there are definitely opportunities for us to optimize here.

@fatcerberus
Copy link

I guess that makes sense now that I think about it - there really aren't any "set the type to string" style narrowings - even typeof x === 'string' can narrow to never if the original type doesn't overlap with string. Otherwise you could probably do some kind of short-circuiting logic.

@RyanCavanaugh RyanCavanaugh added Experimentation Needed Someone needs to try this out to see what happens and removed In Discussion Not yet reached consensus labels Nov 16, 2022
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Nov 16, 2022
weswigham added a commit to weswigham/TypeScript that referenced this issue May 30, 2023
…n relevant references

Fixes microsoft#51525

This requied reworking how we compare references so we could equate them with a `set` - that change, in and of itself,
may actually also be perf positive (since we now get to cache the walk over the reference's AST), but both changes together
is somewhere between neutral and 4% gains in local testing, depending on the suite. I'll need to see if this bears out on
the much slower CI box, though, since 4% locally is < 0.1s, which is well in the realm of "OS scheduler variance" for some of these
tests on my box. Do note, unlike pyright, which in the linked issue's change, calculates the references used within a loop
during binding, we calculate it late on-demand and cache it in the checker. This is basically required for us, since we equate
references using the identity of the resolved symbol of the root node of the reference (which... may be a bit circular with
expression checking!).
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label May 30, 2023
@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Performance Reports of unusually slow behavior Experimentation Needed Someone needs to try this out to see what happens Fix Available A PR has been opened for this issue Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript
Projects
None yet
6 participants