Skip to content

Conversation

@JoostK
Copy link
Contributor

@JoostK JoostK commented Nov 5, 2020

Fixes #41124
Alternative to #41387.

Flow nodes that optionally provide a flow type will delegate to their antecedent
to determine a flow type using a loop in getFlowTypeOfReference. If these nodes
are marked as shared then their result should be cached for future lookups, but
the registration inside getFlowTypeOfReference did only consider the most-recently
processed flow node. Any flow nodes up to that point that did not provide a type
but were marked as shared would therefore not be cached at all.

This commit keeps track of the first seen shared flow node and ensures that the
result type is cached for that node.

@typescript-bot typescript-bot added For Uncommitted Bug PR for untriaged, rejected, closed or missing bug For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Nov 5, 2020
Flow nodes that optionally provide a flow type will delegate to their antecedent
to determine a flow type using a loop in `getFlowTypeOfReference`. If these nodes
are marked as shared then their result should be cached for future lookups, but
the registration inside `getFlowTypeOfReference` did only consider the most-recently
processed flow node. Any flow nodes up to that point that did not provide a type
but were marked as shared would therefore not be cached at all.

This commit keeps track of the first seen shared flow node and ensures that the
result type is cached for that node.
@JoostK JoostK force-pushed the shared-flow-node-caching branch from 57bb716 to 3cc2032 Compare November 5, 2020 10:06
@sandersn sandersn assigned weswigham and unassigned sheetalkamat Nov 23, 2020
@ahejlsberg
Copy link
Member

@JoostK First, thanks for diagnosing this issue. You are indeed right, we end up not caching in cases where flow nodes are skipped and that's not a good thing. In terms of a fix, I think it can be even simpler than this PR. I will put up an alternative PR and run the full suite of tests on it.

@JoostK
Copy link
Contributor Author

JoostK commented Nov 26, 2020

Closing as resolved in #41665.

@JoostK JoostK closed this Nov 26, 2020
@microsoft microsoft locked as resolved and limited conversation to collaborators Oct 21, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

For Milestone Bug PRs that fix a bug with a specific milestone

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

perf: exponential slowdown in flow analysis

5 participants