Reuse computed type of condition expressions #49881
Conversation
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| @@ -38232,7 +38233,7 @@ namespace ts { | |||
| ? condExpr.right | |||
| : condExpr; | |||
| if (isModuleExportsAccessExpression(location)) return; | |||
| const type = checkTruthinessExpression(location); | |||
| const type = location === condExpr ? condType : checkTruthinessExpression(location); | |||
There was a problem hiding this comment.
If anyone has ideas on how to avoid this checkTruthinessExpression altogether, please let me know.
There was a problem hiding this comment.
Yeah, that seems tricky because it looks like location is either condExpr, and we already have the type, or it is condExpr.right, and then... we haven't visited yet in the trampoline on onOperator, right?
There was a problem hiding this comment.
Indeed, this is my understanding as well. And I don't think that a dynamic programming like approach is worth it here, given the overhead of caching the type of subtrees. Given that the performance issue that manifested itself is mitigated by the current changes I'd consider it fine that we leave this like this, at least until another performance bottleneck is observed.
|
@typescript-bot pack this |
1 similar comment
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at a594c6a. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at a594c6a. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at a594c6a. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at a594c6a. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at a594c6a. You can monitor the build here. |
|
@DanielRosenwasser Here they are:Comparison Report - main..refs/pull/49881/merge [uglify-js]1 of 1 projects failed to build with the old tsc /mnt/ts_downloads/uglify-js/tsconfig.json
|
|
Hey @DanielRosenwasser, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@DanielRosenwasser Here they are:
CompilerComparison Report - main..49881
System
Hosts
Scenarios
TSServerComparison Report - main..49881
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Just a heads up: I'll be on vacation for the next couple of weeks so won't be able to update this PR in the meantime. Feel free to push additional changes if needed, open another PR, or just wait until I'm back. |
| @@ -0,0 +1,5 @@ | |||
| // @strictNullChecks: true | |||
There was a problem hiding this comment.
I wonder if we have a better way of capturing this kind of test, where the thing we're interested in is the execution time.
There was a problem hiding this comment.
Historically we don't - but tests do have a timeout (around 50s), so if you make sure they're large enough to hit the timeout before your change, they're a decent test; they won't capture tiny pretty regressions, but will prevent big ones. We have other tests in this vein.
There was a problem hiding this comment.
Ok then. @JoostK can you make this example much much bigger, so that the test would timeout without the change?
There was a problem hiding this comment.
I will add 4 additional binary ops, which would have brought check times up by a factor of 16 (easily pushing it beyond the 50s threshold)
src/compiler/checker.ts
Outdated
| if (!strictNullChecks) return; | ||
| if (!(getTypeFacts(condType) & TypeFacts.Truthy)) return; |
There was a problem hiding this comment.
Shouldn't this check only be done if location is ultimately condExpr?
There was a problem hiding this comment.
I think you are correct; as this check is indeed also done within helper based on the resolved type. Removing this.
|
@JoostK apologies if you're still on vacation, but any plans on updating this change? If not I guess I could try, but I'm not at all familiar with TS internals... |
JoostK
force-pushed
the
perf/recursive-function-checks
branch
from
a594c6a to
dd5a78b
Compare
|
I have incorporated the feedback in the latest update, PTAL. |
Fixes #49845
This change threads the computed type of conditional expressions into
checkTestingKnownTruthyCallableOrAwaitableTypeto avoid recursively revisiting expression subtrees, resulting in an exponential performance drop. The changed code is similar to how it was before #42835.The test case used to run in ~13s on my laptop before the fix, and now completes instantly again (check time reported as
0.00s)