Refactor expression evaluator #57955

dragomirtitian · 2024-03-27T14:38:17Z

Refactor expression evaluator to sit outside of the checker.

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.

jakebailey · 2024-03-27T16:05:18Z

@typescript-bot perf test this

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

jakebailey · 2024-03-27T16:06:58Z

src/compiler/checker.ts

+    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 · 2024-03-27T16:39:04Z

@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.

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?

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.

DanielRosenwasser

Seems like a very simple change.

Refactor expression evaluator

468203b

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

jakebailey reviewed Mar 27, 2024

View reviewed changes

Remove onNumericLiteral as that check is already performed through …

5dd509e

…checkExpression.

DanielRosenwasser requested a review from weswigham March 27, 2024 17:29

weswigham approved these changes Mar 27, 2024

View reviewed changes

DanielRosenwasser requested a review from ahejlsberg March 27, 2024 17:44

DanielRosenwasser approved these changes Mar 27, 2024

View reviewed changes

weswigham merged commit fd388f7 into microsoft:main Mar 27, 2024
25 checks passed

jakebailey mentioned this pull request Mar 27, 2024

Use evaluator for isolatedModules enum restrictions #57966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor expression evaluator #57955

Refactor expression evaluator #57955

dragomirtitian commented Mar 27, 2024

typescript-bot commented Mar 27, 2024

jakebailey commented Mar 27, 2024

typescript-bot commented Mar 27, 2024 •

edited

Loading

jakebailey Mar 27, 2024

dragomirtitian Mar 27, 2024

dragomirtitian Mar 27, 2024

typescript-bot commented Mar 27, 2024

tsc

tsserver

startup

weswigham left a comment

DanielRosenwasser commented Mar 27, 2024

DanielRosenwasser left a comment

Refactor expression evaluator #57955

Refactor expression evaluator #57955

Conversation

dragomirtitian commented Mar 27, 2024

typescript-bot commented Mar 27, 2024

jakebailey commented Mar 27, 2024

typescript-bot commented Mar 27, 2024 • edited Loading

jakebailey Mar 27, 2024

Choose a reason for hiding this comment

dragomirtitian Mar 27, 2024

Choose a reason for hiding this comment

dragomirtitian Mar 27, 2024

Choose a reason for hiding this comment

typescript-bot commented Mar 27, 2024

tsc

tsserver

startup

weswigham left a comment

Choose a reason for hiding this comment

DanielRosenwasser commented Mar 27, 2024

DanielRosenwasser left a comment

Choose a reason for hiding this comment

typescript-bot commented Mar 27, 2024 •

edited

Loading