Improve control flow type caching #35332
Conversation
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 2d19e83. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 2d19e83. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 2d19e83. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 2d19e83. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@ahejlsberg Here they are:Comparison Report - master..35332
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
| flowLoopStart = flowLoopCount; | ||
| flowTypeCache = undefined; |
There was a problem hiding this comment.
Since we do it in two places, can we capture this "execute in new flow cache context" pattern into it's own helper function?
There was a problem hiding this comment.
I think it is fine as is.
| @@ -27332,7 +27324,7 @@ namespace ts { | |||
|
|
|||
| function checkDeclarationInitializer(declaration: HasExpressionInitializer) { | |||
| const initializer = getEffectiveInitializer(declaration)!; | |||
| const type = getTypeOfExpression(initializer, /*cache*/ true); | |||
| const type = getQuickTypeOfExpression(initializer) || checkExpressionCached(initializer); | |||
There was a problem hiding this comment.
IIRC, using the cache flag here was due to a perf issue, not a correctness one, and the caching was a correctness trade-off - using the per-loop cache is probably better, if possible.
There was a problem hiding this comment.
No, we need to call checkExpressionCached here because it resets the flow loop state to ensure everything is computed from scratch before the type is permanently cached. The getTypeOfExpression cache is purely there to help performance, but there is no guarantee that the same type will be returned every time.
|
Tests look good (all errors are preexisting conditions) and performance shows a slight positive effect. |
| */ | ||
| function getTypeOfExpression(node: Expression, cache?: boolean) { | ||
| function getTypeOfExpression(node: Expression) { |
There was a problem hiding this comment.
We should document that we probably only expect this to be called into from flow analysis now - using it elsewhere is probably a mistake. I know we have a call into it from the node builder and services layers, too, but both of those should probably just be using checkExpressionCached.
There was a problem hiding this comment.
Seems good-ish. That we're completely disabling the cache (and not using a temporary cache for the duration of the loop analysis) makes me wonder if something like
while (true) {
while (true) {
if (choice === choiceOne) {}
}
}would still have bad perf by avoiding the cache on the inner loop...
This PR improves performance of
getTypeOfExpressionby caching results whenever control flow analysis was required to compute them. Unlike our existing caching schemes, this implementation works even during control flow analysis of loops: The cache is saved and cleared before starting analysis of a loop back edge (which may produce incomplete types), and then restored when analysis completes.Fixes #35205.