Skip to content

More reliable mechanism for _this/_super simplification - #56130

Merged
Ron Buckton (rbuckton) merged 4 commits into
mainfrom
fix-55646
Oct 24, 2023
Merged

More reliable mechanism for _this/_super simplification#56130
Ron Buckton (rbuckton) merged 4 commits into
mainfrom
fix-55646

Conversation

@rbuckton

Copy link
Copy Markdown
Contributor

This changes the somewhat fragile way we handled the simplification of constructor body statements when transforming super() that became even more fragile in TS 5.2. These simplifications were intended to slim down transformed constructor bodies when a temporary variable like _this may not be necessary:

// givenclassCextendsBase{constructor(){super();}}// without simplificationvarC=(function(_super){functionC(){var_this=this;_this=_super.call(this)||this;return_this;}returnC;})(Base);// with simplificaitonvarC=(function(_super){functionC(){return_super.call(this)||this;}returnC;})(Base);

However, when using was introduced in TS 5.2, the simplification mechanism became more complicated as the super() statements we would normally look for would be moved inside of a try..catch..finally block if the constructor contained a using declaration.

Rather than try to handle specific super() simplifications when initially visiting the constructor body, this PR instead performs these simplifications in a second pass after the initial transformation completes. This results in a mechanism that is more reliable than the prior approach as it only performs these simplifications with complete knowledge of the transformed result..

Fixes#55646
Fixes#55637

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

TypeScript Bot (@typescript-bot) perf test

@typescript-bot

TypeScript Bot (typescript-bot) commented Oct 16, 2023

Copy link
Copy Markdown
Contributor

Heya Ron Buckton (@rbuckton), I've started to run the regular perf test suite on this PR at 21a697d. You can monitor the build here.

Update: The results are in!

@typescript-bot

Copy link
Copy Markdown
Contributor

Ron Buckton (@rbuckton)
The results of the perf run you requested are in!

Here they are:

Compiler

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Angular - node (v18.15.0, x64)
Memory used295,079k (± 0.02%)295,145k (± 0.01%)+66k (+ 0.02%)295,098k295,207kp=0.020 n=6
Parse Time2.62s (± 0.92%)2.64s (± 0.52%)~2.61s2.65sp=0.506 n=6
Bind Time0.84s (± 1.17%)0.84s (± 1.06%)~0.83s0.85sp=0.798 n=6
Check Time8.07s (± 0.31%)8.08s (± 0.22%)~8.05s8.10sp=0.463 n=6
Emit Time7.05s (± 0.19%)7.09s (± 0.28%)+0.04s (+ 0.61%)7.06s7.12sp=0.008 n=6
Total Time18.58s (± 0.19%)18.65s (± 0.22%)+0.07s (+ 0.36%)18.60s18.72sp=0.010 n=6
Compiler-Unions - node (v18.15.0, x64)
Memory used193,542k (± 1.65%)191,631k (± 1.24%)~190,642k196,499kp=1.000 n=6
Parse Time1.34s (± 0.82%)1.35s (± 1.46%)~1.32s1.38sp=0.611 n=6
Bind Time0.73s (± 0.00%)0.73s (± 0.00%)~0.73s0.73sp=1.000 n=6
Check Time9.13s (± 0.27%)9.12s (± 0.30%)~9.09s9.16sp=0.629 n=6
Emit Time2.64s (± 0.37%)2.63s (± 0.44%)~2.61s2.64sp=0.209 n=6
Total Time13.84s (± 0.26%)13.83s (± 0.17%)~13.80s13.86sp=0.464 n=6
Monaco - node (v18.15.0, x64)
Memory used347,292k (± 0.00%)347,355k (± 0.01%)+63k (+ 0.02%)347,313k347,421kp=0.005 n=6
Parse Time2.45s (± 0.31%)2.45s (± 0.17%)~2.45s2.46sp=1.000 n=6
Bind Time0.94s (± 0.87%)0.94s (± 0.55%)~0.94s0.95sp=0.114 n=6
Check Time6.90s (± 0.49%)6.91s (± 0.44%)~6.87s6.95sp=0.376 n=6
Emit Time4.03s (± 0.51%)4.04s (± 0.40%)~4.02s4.07sp=0.255 n=6
Total Time14.32s (± 0.34%)14.35s (± 0.19%)~14.31s14.38sp=0.260 n=6
TFS - node (v18.15.0, x64)
Memory used302,543k (± 0.01%)302,578k (± 0.01%)~302,539k302,637kp=0.092 n=6
Parse Time2.01s (± 0.77%)2.00s (± 0.42%)~1.99s2.01sp=0.078 n=6
Bind Time1.00s (± 1.36%)1.00s (± 1.04%)~0.98s1.01sp=0.557 n=6
Check Time6.26s (± 0.31%)6.27s (± 0.48%)~6.24s6.32sp=0.514 n=6
Emit Time3.55s (± 0.58%)3.59s (± 0.45%)+0.03s (+ 0.94%)3.56s3.60sp=0.022 n=6
Total Time12.83s (± 0.34%)12.85s (± 0.25%)~12.82s12.91sp=0.226 n=6
material-ui - node (v18.15.0, x64)
Memory used470,501k (± 0.00%)470,498k (± 0.00%)~470,479k470,512kp=0.423 n=6
Parse Time2.58s (± 0.75%)2.58s (± 0.67%)~2.56s2.60sp=1.000 n=6
Bind Time0.99s (± 0.55%)1.00s (± 0.89%)~0.99s1.01sp=0.341 n=6
Check Time16.62s (± 0.34%)16.59s (± 0.36%)~16.53s16.67sp=0.421 n=6
Emit Time0.00s (± 0.00%)0.00s (± 0.00%)~0.00s0.00sp=1.000 n=6
Total Time20.20s (± 0.34%)20.17s (± 0.26%)~20.11s20.25sp=0.627 n=6
xstate - node (v18.15.0, x64)
Memory used512,610k (± 0.01%)512,649k (± 0.01%)~512,579k512,720kp=0.471 n=6
Parse Time3.27s (± 0.30%)3.26s (± 0.27%)~3.25s3.27sp=0.082 n=6
Bind Time1.55s (± 0.41%)1.55s (± 0.58%)~1.54s1.56sp=1.000 n=6
Check Time2.86s (± 0.58%)2.88s (± 0.69%)~2.86s2.91sp=0.171 n=6
Emit Time0.08s (± 4.99%)0.08s (± 0.00%)~0.08s0.08sp=0.405 n=6
Total Time7.76s (± 0.28%)7.76s (± 0.26%)~7.74s7.80sp=0.935 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)
  • xstate - node (v18.15.0, x64)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

tsserver

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
Compiler-UnionsTSServer - node (v18.15.0, x64)
Req 1 - updateOpen2,372ms (± 1.62%)2,393ms (± 1.42%)~2,336ms2,423msp=0.229 n=6
Req 2 - geterr5,376ms (± 1.70%)5,339ms (± 1.42%)~5,282ms5,439msp=0.575 n=6
Req 3 - references327ms (± 0.63%)330ms (± 1.71%)~323ms337msp=0.375 n=6
Req 4 - navto275ms (± 1.29%)276ms (± 1.27%)~270ms280msp=0.627 n=6
Req 5 - completionInfo count1,356 (± 0.00%)1,356 (± 0.00%)~1,3561,356p=1.000 n=6
Req 5 - completionInfo83ms (± 7.98%)82ms (± 8.15%)~76ms91msp=0.935 n=6
CompilerTSServer - node (v18.15.0, x64)
Req 1 - updateOpen2,474ms (± 0.99%)2,493ms (± 0.65%)~2,466ms2,514msp=0.173 n=6
Req 2 - geterr4,183ms (± 1.48%)4,106ms (± 1.96%)~4,042ms4,210msp=0.173 n=6
Req 3 - references334ms (± 1.14%)340ms (± 1.61%)~333ms345msp=0.059 n=6
Req 4 - navto283ms (± 0.48%)285ms (± 0.78%)~283ms289msp=0.145 n=6
Req 5 - completionInfo count1,518 (± 0.00%)1,518 (± 0.00%)~1,5181,518p=1.000 n=6
Req 5 - completionInfo79ms (± 6.33%)82ms (± 8.53%)~74ms89msp=0.546 n=6
xstateTSServer - node (v18.15.0, x64)
Req 1 - updateOpen2,598ms (± 0.38%)2,581ms (± 0.50%)-18ms (- 0.67%)2,562ms2,598msp=0.045 n=6
Req 2 - geterr1,711ms (± 2.24%)1,716ms (± 3.06%)~1,638ms1,773msp=0.936 n=6
Req 3 - references110ms (± 7.77%)113ms (±10.10%)~105ms129msp=1.000 n=6
Req 4 - navto359ms (± 0.37%)359ms (± 0.29%)~358ms360msp=0.457 n=6
Req 5 - completionInfo count2,073 (± 0.00%)2,073 (± 0.00%)~2,0732,073p=1.000 n=6
Req 5 - completionInfo304ms (± 1.37%)308ms (± 1.32%)~304ms315msp=0.126 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)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Startup

Comparison Report - baseline..pr
MetricbaselineprDeltaBestWorstp-value
tsc-startup - node (v18.15.0, x64)
Execution time152.42ms (± 0.18%)152.45ms (± 0.16%)~151.43ms155.17msp=0.282 n=600
tsserver-startup - node (v18.15.0, x64)
Execution time227.68ms (± 0.14%)227.71ms (± 0.15%)~226.63ms231.04msp=0.891 n=600
tsserverlibrary-startup - node (v18.15.0, x64)
Execution time228.40ms (± 0.16%)228.55ms (± 0.19%)+0.15ms (+ 0.07%)226.64ms235.53msp=0.001 n=600
typescript-startup - node (v18.15.0, x64)
Execution time228.80ms (± 0.18%)228.88ms (± 0.17%)~227.25ms234.95msp=0.064 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)
BenchmarkNameIterations
Currentpr6
Baselinebaseline6

Developer Information:

Download Benchmarks

@@ -136,8 +136,7 @@ var D = /** @class */ (function (_super) {
__extends(D, _super);
function D() {
var _this = _super.call(this, 1, 2) || this;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional to leave this unused variable behind? I guess it's harmless?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and no. It's intentional in that the conditions for the simplification that elides var _this = isn't met. We could change the rules, in the future if necessary, but I don't want to spend too much time processing these simplifications during emit, which are primarily only to improve readability when debugging unmapped outputs, so I've generally opted for more coarse-grained rules.

@rbuckton

Copy link
Copy Markdown
ContributorAuthor

I'm going to make a small update to make updatePropertyName public to align with #56194

@typescript-bot

Copy link
Copy Markdown
Contributor

Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page.

Also, please make sure Daniel Rosenwasser (@DanielRosenwasser) and Ryan Cavanaugh (@RyanCavanaugh) are aware of the changes, just as a heads up.

@rbuckton
Ron Buckton (rbuckton) deleted the fix-55646 branch October 24, 2023 01:01
@microsoftMicrosoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Author: TeamFor Milestone BugPRs that fix a bug with a specific milestone

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect ES5 transpilation for try/catch + super Incorrect ES5 transpilation for 'this' usage in lambda as a constructor default parameter

3 participants

@rbuckton@typescript-bot@jakebailey