Refactor expression evaluator by dragomirtitian · Pull Request #57955 · microsoft/TypeScript

Titian Cernicova-Dragomir (dragomirtitian) · 2024-03-27T14:38:17Z

Refactor expression evaluator to sit outside of the checker.

TypeScript Bot (typescript-bot) · 2024-03-27T14:38:23Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Jake Bailey (jakebailey) · 2024-03-27T16:05:18Z

TypeScript Bot (@typescript-bot) perf test this

TypeScript Bot (typescript-bot) · 2024-03-27T16:05:20Z

Starting jobs; this comment will be updated as builds start and complete.

Command	Status	Results
`perf test this`	✅ Started	👀 Results

Jake Bailey (jakebailey) · 2024-03-27T16:06:58Z

+ var evaluate = createEvaluator({
+ evaluateElementAccessExpression,
+ evaluateEntityNameExpression,
+ onNumericLiteral: checkGrammarNumericLiteral,


It feels strange to have this callback. This only needs the AST node and nothing else. Can we just walk the nodes like other grammar checks?

I was trying to introduce as little extra overhead as possible, I agree that callback is kinds of the odd one out, but without an extra walk I'm not sure how we could ensure that check is performed in the same way.
The check does appear to happen in other places though. Enum members should go through checkEnumMember which will call checkGrammarNumericLiteral during expression checking. Similarly for template literals. So the check might be redundant for a full check. I'll test and see.

Just tested .. removing the check from here does not fail any test 😕. I do think it ends up being called through other paths.

TypeScript Bot (typescript-bot) · 2024-03-27T16:39:04Z

Jake Bailey (@jakebailey)
The results of the perf run you requested are in!

Here they are:

tsc

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{Angular - node (v18.15.0, x64)}
_{Memory used}	_{295,484k (± 0.00%)}	_{295,498k (± 0.01%)}	_~	_295,465k	_295,539k	_{p=0.298 n=6}
_{Parse Time}	_{3.19s (± 0.24%)}	_{3.19s (± 0.71%)}	_~	_3.17s	_3.23s	_{p=0.564 n=6}
_{Bind Time}	_{0.99s (± 0.52%)}	_{1.00s (± 0.84%)}	_~	_0.99s	_1.01s	_{p=0.923 n=6}
_{Check Time}	_{9.68s (± 0.27%)}	_{9.69s (± 0.36%)}	_~	_9.63s	_9.73s	_{p=0.747 n=6}
_{Emit Time}	_{8.37s (± 0.14%)}	_{8.37s (± 0.43%)}	_~	_8.32s	_8.42s	_{p=0.627 n=6}
_{Total Time}	_{22.23s (± 0.14%)}	_{22.23s (± 0.20%)}	_~	_22.19s	_22.31s	_{p=0.747 n=6}
_{Compiler-Unions - node (v18.15.0, x64)}
_{Memory used}	_{192,583k (± 0.78%)}	_{192,046k (± 0.08%)}	_~	_191,913k	_192,315k	_{p=0.936 n=6}
_{Parse Time}	_{1.66s (± 0.99%)}	_{1.65s (± 0.89%)}	_~	_1.63s	_1.67s	_{p=0.871 n=6}
_{Bind Time}	_{0.88s (± 0.59%)}	_{0.88s (± 0.72%)}	_~	_0.87s	_0.89s	_{p=0.386 n=6}
_{Check Time}	_{11.21s (± 0.74%)}	_{11.27s (± 0.89%)}	_~	_11.11s	_11.36s	_{p=0.298 n=6}
_{Emit Time}	_{3.14s (± 0.87%)}	_{3.14s (± 0.47%)}	_~	_3.11s	_3.15s	_{p=1.000 n=6}
_{Total Time}	_{16.89s (± 0.53%)}	_{16.93s (± 0.72%)}	_~	_16.74s	_17.05s	_{p=0.336 n=6}
_{Monaco - node (v18.15.0, x64)}
_{Memory used}	_{347,443k (± 0.01%)}	_{347,451k (± 0.00%)}	_~	_347,431k	_347,472k	_{p=0.423 n=6}
_{Parse Time}	_{3.69s (± 1.58%)}	_{3.67s (± 1.51%)}	_~	_3.57s	_3.73s	_{p=0.574 n=6}
_{Bind Time}	_{1.39s (± 1.18%)}	_{1.38s (± 1.00%)}	_~	_1.35s	_1.39s	_{p=0.341 n=6}
_{Check Time}	_{10.25s (± 0.21%)}	_{10.24s (± 0.50%)}	_~	_10.16s	_10.30s	_{p=1.000 n=6}
_{Emit Time}	_{6.02s (± 0.54%)}	_{6.02s (± 0.47%)}	_~	_5.99s	_6.07s	_{p=0.686 n=6}
_{Total Time}	_{21.34s (± 0.29%)}	_{21.31s (± 0.41%)}	_~	_21.18s	_21.42s	_{p=0.630 n=6}
_{TFS - node (v18.15.0, x64)}
_{Memory used}	_{302,734k (± 0.01%)}	_{302,751k (± 0.01%)}	_~	_302,709k	_302,800k	_{p=0.378 n=6}
_{Parse Time}	_{2.40s (± 1.10%)}	_{2.40s (± 1.38%)}	_~	_2.36s	_2.43s	_{p=0.936 n=6}
_{Bind Time}	_{1.19s (± 0.43%)}	_{1.22s (± 0.80%)}	_{+0.03s (+ 2.37%)}	_1.21s	_1.23s	_{p=0.004 n=6}
_{Check Time}	_{7.46s (± 0.36%)}	_{7.48s (± 0.40%)}	_~	_7.44s	_7.53s	_{p=0.805 n=6}
_{Emit Time}	_{4.27s (± 0.76%)}	_{4.28s (± 0.60%)}	_~	_4.25s	_4.32s	_{p=0.570 n=6}
_{Total Time}	_{15.32s (± 0.25%)}	_{15.38s (± 0.37%)}	_~	_15.32s	_15.48s	_{p=0.072 n=6}
_{material-ui - node (v18.15.0, x64)}
_{Memory used}	_{509,981k (± 0.01%)}	_{509,965k (± 0.01%)}	_~	_509,923k	_510,005k	_{p=0.575 n=6}
_{Parse Time}	_{3.92s (± 0.19%)}	_{3.93s (± 0.37%)}	_~	_3.91s	_3.95s	_{p=0.117 n=6}
_{Bind Time}	_{1.46s (± 0.67%)}	_{1.46s (± 0.71%)}	_~	_1.45s	_1.48s	_{p=0.452 n=6}
_{Check Time}	_{25.31s (± 0.17%)}	_{25.41s (± 0.32%)}	_~	_25.30s	_25.48s	_{p=0.052 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{30.69s (± 0.14%)}	_{30.80s (± 0.27%)}	_{+0.11s (+ 0.36%)}	_30.68s	_30.88s	_{p=0.029 n=6}
_{mui-docs - node (v18.15.0, x64)}
_{Memory used}	_{1,737,822k (± 0.00%)}	_{1,737,828k (± 0.00%)}	_~	_1,737,795k	_1,737,850k	_{p=0.575 n=6}
_{Parse Time}	_{7.78s (± 0.25%)}	_{7.79s (± 0.70%)}	_~	_7.71s	_7.86s	_{p=0.687 n=6}
_{Bind Time}	_{2.79s (± 0.32%)}	_{2.79s (± 0.74%)}	_~	_2.76s	_2.81s	_{p=0.367 n=6}
_{Check Time}	_{66.71s (± 0.42%)}	_{66.60s (± 0.15%)}	_~	_66.51s	_66.77s	_{p=0.378 n=6}
_{Emit Time}	_{0.16s (± 3.29%)}	_{0.16s (± 2.58%)}	_~	_0.15s	_0.16s	_{p=0.595 n=6}
_{Total Time}	_{77.44s (± 0.35%)}	_{77.34s (± 0.14%)}	_~	_77.17s	_77.48s	_{p=0.378 n=6}
_{self-build-src - node (v18.15.0, x64)}
_{Memory used}	_{2,392,034k (± 0.03%)}	_{2,392,685k (± 0.03%)}	_~	_2,391,537k	_2,393,515k	_{p=0.128 n=6}
_{Parse Time}	_{6.06s (± 0.50%)}	_{6.07s (± 0.66%)}	_~	_6.01s	_6.12s	_{p=0.574 n=6}
_{Bind Time}	_{2.26s (± 1.03%)}	_{2.25s (± 0.92%)}	_~	_2.23s	_2.29s	_{p=0.413 n=6}
_{Check Time}	_{39.67s (± 0.50%)}	_{39.53s (± 0.34%)}	_~	_39.43s	_39.78s	_{p=0.298 n=6}
_{Emit Time}	_{3.15s (± 1.09%)}	_{3.18s (± 1.35%)}	_~	_3.14s	_3.26s	_{p=0.297 n=6}
_{Total Time}	_{51.15s (± 0.35%)}	_{51.04s (± 0.25%)}	_~	_50.91s	_51.27s	_{p=0.378 n=6}
_{self-compiler - node (v18.15.0, x64)}
_{Memory used}	_{415,053k (± 0.01%)}	_{415,056k (± 0.01%)}	_~	_415,014k	_415,119k	_{p=0.873 n=6}
_{Parse Time}	_{4.17s (± 0.65%)}	_{4.11s (± 0.65%)}	_{-0.06s (- 1.32%)}	_4.08s	_4.14s	_{p=0.020 n=6}
_{Bind Time}	_{1.58s (± 0.96%)}	_{1.59s (± 1.66%)}	_~	_1.55s	_1.62s	_{p=0.808 n=6}
_{Check Time}	_{22.38s (± 0.38%)}	_{22.29s (± 0.43%)}	_~	_22.16s	_22.42s	_{p=0.092 n=6}
_{Emit Time}	_{1.68s (± 1.78%)}	_{1.68s (± 1.99%)}	_~	_1.64s	_1.72s	_{p=1.000 n=6}
_{Total Time}	_{29.81s (± 0.28%)}	_{29.67s (± 0.41%)}	_~	_29.51s	_29.85s	_{p=0.054 n=6}
_{vscode - node (v18.15.0, x64)}
_{Memory used}	_{2,892,649k (± 0.00%)}	_{2,892,623k (± 0.00%)}	_~	_2,892,494k	_2,892,679k	_{p=0.575 n=6}
_{Parse Time}	_{12.87s (± 0.33%)}	_{12.86s (± 0.25%)}	_~	_12.82s	_12.91s	_{p=0.570 n=6}
_{Bind Time}	_{4.12s (± 1.17%)}	_{4.11s (± 0.33%)}	_~	_4.09s	_4.13s	_{p=0.742 n=6}
_{Check Time}	_{71.33s (± 0.43%)}	_{71.15s (± 0.45%)}	_~	_70.70s	_71.45s	_{p=0.422 n=6}
_{Emit Time}	_{19.99s (± 7.78%)}	_{19.20s (± 0.23%)}	_~	_19.13s	_19.27s	_{p=0.108 n=6}
_{Total Time}	_{108.30s (± 1.68%)}	_{107.31s (± 0.32%)}	_~	_106.84s	_107.69s	_{p=0.128 n=6}
_{webpack - node (v18.15.0, x64)}
_{Memory used}	_{408,368k (± 0.01%)}	_{408,443k (± 0.02%)}	_~	_408,385k	_408,618k	_{p=0.128 n=6}
_{Parse Time}	_{3.88s (± 0.47%)}	_{3.89s (± 0.48%)}	_~	_3.87s	_3.92s	_{p=0.869 n=6}
_{Bind Time}	_{1.69s (± 0.99%)}	_{1.69s (± 1.06%)}	_~	_1.67s	_1.71s	_{p=1.000 n=6}
_{Check Time}	_{16.71s (± 0.34%)}	_{16.77s (± 0.35%)}	_~	_16.69s	_16.86s	_{p=0.230 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{22.29s (± 0.25%)}	_{22.34s (± 0.23%)}	_~	_22.27s	_22.42s	_{p=0.170 n=6}
_{xstate - node (v18.15.0, x64)}
_{Memory used}	_{513,092k (± 0.02%)}	_{513,089k (± 0.01%)}	_~	_512,988k	_513,124k	_{p=0.630 n=6}
_{Parse Time}	_{3.28s (± 0.31%)}	_{3.28s (± 0.33%)}	_~	_3.27s	_3.30s	_{p=0.863 n=6}
_{Bind Time}	_{1.57s (± 0.62%)}	_{1.57s (± 0.40%)}	_~	_1.56s	_1.58s	_{p=0.733 n=6}
_{Check Time}	_{2.85s (± 1.17%)}	_{2.85s (± 0.68%)}	_~	_2.83s	_2.88s	_{p=0.570 n=6}
_{Emit Time}	_{0.07s (± 5.69%)}	_{0.07s (± 0.00%)}	_~	_0.07s	_0.07s	_{p=0.405 n=6}
_{Total Time}	_{7.77s (± 0.46%)}	_{7.77s (± 0.29%)}	_~	_7.75s	_7.80s	_{p=0.517 n=6}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

Angular - node (v18.15.0, x64)
Compiler-Unions - node (v18.15.0, x64)
Monaco - node (v18.15.0, x64)
TFS - node (v18.15.0, x64)
material-ui - node (v18.15.0, x64)
mui-docs - node (v18.15.0, x64)
self-build-src - node (v18.15.0, x64)
self-compiler - node (v18.15.0, x64)
vscode - node (v18.15.0, x64)
webpack - node (v18.15.0, x64)
xstate - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

tsserver

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{Compiler-UnionsTSServer - node (v18.15.0, x64)}
_{Req 1 - updateOpen}	_{2,374ms (± 0.90%)}	_{2,371ms (± 0.35%)}	_~	_2,361ms	_2,381ms	_{p=1.000 n=6}
_{Req 2 - geterr}	_{5,612ms (± 0.39%)}	_{5,635ms (± 0.41%)}	_~	_5,604ms	_5,667ms	_{p=0.173 n=6}
_{Req 3 - references}	_{317ms (± 0.24%)}	_{318ms (± 0.33%)}	_~	_316ms	_319ms	_{p=0.611 n=6}
_{Req 4 - navto}	_{273ms (± 0.15%)}	_{272ms (± 0.00%)}	_{-1ms (- 0.43%)}	_272ms	_272ms	_{p=0.002 n=6}
_{Req 5 - completionInfo count}	_{1,357 (± 0.00%)}	_{1,357 (± 0.00%)}	_~	_1,357	_1,357	_{p=1.000 n=6}
_{Req 5 - completionInfo}	_{87ms (± 3.74%)}	_{87ms (± 0.47%)}	_{-0ms (- 0.19%)}	_87ms	_88ms	_{p=0.047 n=6}
_{CompilerTSServer - node (v18.15.0, x64)}
_{Req 1 - updateOpen}	_{2,504ms (± 0.42%)}	_{2,505ms (± 0.44%)}	_~	_2,493ms	_2,522ms	_{p=0.872 n=6}
_{Req 2 - geterr}	_{4,145ms (± 0.25%)}	_{4,134ms (± 0.13%)}	_{-12ms (- 0.28%)}	_4,129ms	_4,141ms	_{p=0.030 n=6}
_{Req 3 - references}	_{333ms (± 0.27%)}	_{333ms (± 0.45%)}	_~	_332ms	_336ms	_{p=0.933 n=6}
_{Req 4 - navto}	_{298ms (± 0.14%)}	_{296ms (± 0.93%)}	_~	_293ms	_298ms	_{p=0.054 n=6}
_{Req 5 - completionInfo count}	_{1,519 (± 0.00%)}	_{1,519 (± 0.00%)}	_~	_1,519	_1,519	_{p=1.000 n=6}
_{Req 5 - completionInfo}	_{73ms (± 0.56%)}	_{73ms (± 0.56%)}	_~	_72ms	_73ms	_{p=0.218 n=6}
_{xstateTSServer - node (v18.15.0, x64)}
_{Req 1 - updateOpen}	_{2,635ms (± 0.20%)}	_{2,632ms (± 0.26%)}	_~	_2,619ms	_2,638ms	_{p=0.575 n=6}
_{Req 2 - geterr}	_{1,740ms (± 2.59%)}	_{1,744ms (± 2.53%)}	_~	_1,686ms	_1,796ms	_{p=0.936 n=6}
_{Req 3 - references}	_{115ms (±10.96%)}	_{126ms (± 1.64%)}	_~	_122ms	_128ms	_{p=0.332 n=6}
_{Req 4 - navto}	_{370ms (± 2.39%)}	_{368ms (± 0.64%)}	_~	_366ms	_371ms	_{p=1.000 n=6}
_{Req 5 - completionInfo count}	_{2,079 (± 0.00%)}	_{2,079 (± 0.00%)}	_~	_2,079	_2,079	_{p=1.000 n=6}
_{Req 5 - completionInfo}	_{307ms (± 1.64%)}	_{306ms (± 2.03%)}	_~	_301ms	_316ms	_{p=0.872 n=6}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

CompilerTSServer - node (v18.15.0, x64)
Compiler-UnionsTSServer - node (v18.15.0, x64)
xstateTSServer - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

startup

Comparison Report - baseline..pr

_Metric	_baseline	_pr	_Delta	_Best	_Worst	_p-value
_{tsc-startup - node (v18.15.0, x64)}
_{Execution time}	_{153.27ms (± 0.19%)}	_{153.29ms (± 0.19%)}	_~	_152.20ms	_158.35ms	_{p=0.376 n=600}
_{tsserver-startup - node (v18.15.0, x64)}
_{Execution time}	_{226.55ms (± 0.14%)}	_{226.41ms (± 0.18%)}	_{-0.14ms (- 0.06%)}	_224.79ms	_231.70ms	_{p=0.000 n=600}
_{tsserverlibrary-startup - node (v18.15.0, x64)}
_{Execution time}	_{221.80ms (± 0.16%)}	_{221.81ms (± 0.16%)}	_~	_220.48ms	_225.00ms	_{p=0.929 n=600}
_{typescript-startup - node (v18.15.0, x64)}
_{Execution time}	_{221.48ms (± 0.17%)}	_{221.28ms (± 0.15%)}	_{-0.19ms (- 0.09%)}	_219.98ms	_225.87ms	_{p=0.000 n=600}

System info unknown

Hosts

node (v18.15.0, x64)

Scenarios

tsc-startup - node (v18.15.0, x64)
tsserver-startup - node (v18.15.0, x64)
tsserverlibrary-startup - node (v18.15.0, x64)
typescript-startup - node (v18.15.0, x64)

_Benchmark	_Name	_Iterations
_Current	_pr	₆
_Baseline	_baseline	₆

Developer Information:

Download Benchmarks

…checkExpression.

Wesley Wigham (weswigham)

Just a philosophy question here: utilities.ts is a now 10,000 line long file of what is essentially uncategorized helper functions. Since we refer to it as "the expression evaluator", does that imply it's standalone enough that it should warrant its' own file, rather than getting tossed in alongside the other uncategorized helper functions?

Daniel Rosenwasser (DanielRosenwasser) · 2024-03-27T17:43:53Z

does that imply it's standalone enough that it should warrant its' own file, rather than getting tossed in alongside the other uncategorized helper functions?

Given how small it is and how often new files get created, I think it's probably fine in utilities.ts - at least for now.

Daniel Rosenwasser (DanielRosenwasser)

Seems like a very simple change.

Refactor expression evaluator
468203b

TypeScript Bot (typescript-bot) added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Mar 27, 2024

Jake Bailey (jakebailey) reviewed Mar 27, 2024
View reviewed changes

Remove onNumericLiteral as that check is already performed through …
5dd509e
…checkExpression.

Daniel Rosenwasser (DanielRosenwasser) requested a review from Wesley Wigham (weswigham)March 27, 2024 17:29

Wesley Wigham (weswigham) approved these changes Mar 27, 2024
View reviewed changes

Daniel Rosenwasser (DanielRosenwasser) requested a review from Anders Hejlsberg (ahejlsberg)March 27, 2024 17:44

Daniel Rosenwasser (DanielRosenwasser) approved these changes Mar 27, 2024
View reviewed changes

Wesley Wigham (weswigham) merged commit fd388f7 into microsoft:mainMar 27, 2024

Jake Bailey (jakebailey) mentioned this pull request Mar 27, 2024
Use evaluator for isolatedModules enum restrictions #57966
Merged

Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 16, 2025

Navigation Menu

Uh oh!

Refactor expression evaluator - #57955

Refactor expression evaluator#57955
Wesley Wigham (weswigham) merged 2 commits into
microsoft:mainfrom
bloomberg:refactor-evaluator

Titian Cernicova-Dragomir (dragomirtitian) commented Mar 27, 2024

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024

Uh oh!

Jake Bailey (jakebailey) commented Mar 27, 2024

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024•
edited
Loading

Uh oh!

Jake Bailey (jakebailey)Mar 27, 2024

Uh oh!

Titian Cernicova-Dragomir (dragomirtitian)Mar 27, 2024

Uh oh!

Titian Cernicova-Dragomir (dragomirtitian)Mar 27, 2024

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024

tsc

tsserver

startup

Uh oh!

Wesley Wigham (weswigham) left a comment

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) commented Mar 27, 2024

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

Titian Cernicova-Dragomir (dragomirtitian) commented Mar 27, 2024

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024

Uh oh!

Jake Bailey (jakebailey) commented Mar 27, 2024

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jake Bailey (jakebailey)Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

Titian Cernicova-Dragomir (dragomirtitian)Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

Titian Cernicova-Dragomir (dragomirtitian)Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

TypeScript Bot (typescript-bot) commented Mar 27, 2024

tsc

tsserver

startup

Uh oh!

Wesley Wigham (weswigham) left a comment

Choose a reason for hiding this comment

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) commented Mar 27, 2024

Uh oh!

Daniel Rosenwasser (DanielRosenwasser) left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

TypeScript Bot (typescript-bot) commented Mar 27, 2024•
edited
Loading