New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix binder performance regression #31142
Conversation
@typescript-bot perf test |
Heya @rbuckton, I've started to run the perf test suite on this PR at 57a8ee1. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
@rbuckton Here they are:Comparison Report - master..31142
System
Hosts
Scenarios
|
This change significantly improves bind time across the board. The emit time changes don't make much sense as far as I can tell, so I'm not certain if they are relevant. Nothing I changed should affect emit time, so it is likely some other deoptimization specific to Node 6.5.0 (x64), which I'm not sure we should care about since it exits LTS maintenance in 3 days on 4/30/2019. |
Wow. That's significant. Definitely some node 6 specific deopt. |
|
||
|
||
==== tests/cases/conformance/es2019/globalThisCollision.js (1 errors) ==== | ||
var globalThis; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpit: an even better test would be var globalThis = window
or var globalThis = this
since that's the likely polyfill code we'll see in JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its necessary since that doesn't effect the outcome of the test, this is purely testing that var globalThis
in the global scope results in our collision error message.
Was this a case of objects of the same type not sharing the same hidden class? |
Effectively, yes. By setting some expando properties early on a Symbol, v8 chose a completely different set of operations to optimize. Deoptimization traces showed that we ended up with about 50% more eager deoptimizations due to |
When we introduced
globalThis
support, we inadvertently regressed binder performance by over 100%. This regression appears to be due to an early change to the shape of ourSymbol
type prior to binder being called.