Skip outer expressions when checking for super keyword in binder #20164
Conversation
src/compiler/binder.ts
Outdated
| function isSuperOrSuperProperty(node: Node, kind: SyntaxKind) { | ||
| switch (kind) { | ||
| function isSuperOrSuperProperty(node: Node) { | ||
| node = skipOuterExpressions(node); |
There was a problem hiding this comment.
@rbuckton According to the commit log, kind was being passed in here to avoid some kind of polymorphism. If that's the case; would it make sense to continue having isSuperOrSuperProperty take the kind and node directly, and move the call to skipOuterExpressions into computeCallExpression, or is what I have here (which is more idiomatic considering what we do elsewhere) fine?
There was a problem hiding this comment.
This was an optimization introduced after reviewing deoptimization data via IRHydra. By only digging into node when kind indicates a property or element access, we avoid a number of soft-deoptimization hits from looking at both node.kind and node.expression in the same function.
The most efficient approach which doesn't require traversing the subtree tree again would be to have a TransformFlags.ContainsSuperOrSuperProperty. We would then add the flag for SuperKeyword and for a property or element access whose expression is SuperKeyword and reset the flag for all nodes except ParenthesizedExpression, AsExpression, TypeAssertionExpression, or PartiallyEmittedExpression so that it doesn't leak out of the context where it needs to matter.
Then we only need to interrogate subtreeFlags for the answer, which avoids polymorphism and walking the subtree.
If we are running out of TransformFlags then it doesn't matter where we put it, since the cost of deoptimization is probably negligible compared to the cost of skipOuterExpressions (which likely ends up deoptimized in any case since it is highly polymorphic).
There was a problem hiding this comment.
We have two unused bits left in TransformFlags; 27 and 28, so we can do it. But even as is the next new emit feature that seriously needs transform flags (like, say, proper lexical arguments) is already in for a bad time what with there not being enough flags. But I suppose that's a future problem.
src/compiler/binder.ts
Outdated
| function isSuperOrSuperProperty(node: Node, kind: SyntaxKind) { | ||
| switch (kind) { | ||
| function isSuperOrSuperProperty(node: Node) { | ||
| node = skipOuterExpressions(node); |
There was a problem hiding this comment.
This was an optimization introduced after reviewing deoptimization data via IRHydra. By only digging into node when kind indicates a property or element access, we avoid a number of soft-deoptimization hits from looking at both node.kind and node.expression in the same function.
The most efficient approach which doesn't require traversing the subtree tree again would be to have a TransformFlags.ContainsSuperOrSuperProperty. We would then add the flag for SuperKeyword and for a property or element access whose expression is SuperKeyword and reset the flag for all nodes except ParenthesizedExpression, AsExpression, TypeAssertionExpression, or PartiallyEmittedExpression so that it doesn't leak out of the context where it needs to matter.
Then we only need to interrogate subtreeFlags for the answer, which avoids polymorphism and walking the subtree.
If we are running out of TransformFlags then it doesn't matter where we put it, since the cost of deoptimization is probably negligible compared to the cost of skipOuterExpressions (which likely ends up deoptimized in any case since it is highly polymorphic).
weswigham
force-pushed
the
super-access-call-fix
branch
from
d769ebf to
34d4206
Compare
|
@rbuckton Done. This now uses transform flag aggregations for tracking subtrees that contain a |
|
@rbuckton any more comments? |
* origin/master: (114 commits) LEGO: check in for master to temporary branch. LEGO: check in for master to temporary branch. Enable substitution for object literal shorthand property assignments in the system transform (microsoft#21106) Skip outer expressions when checking for super keyword in binder (microsoft#20164) Group intersection code in getSimplifiedIndexedAccessType Use filter instead of unnecessary laziness Remove mapped types to never from intersections Handle jsconfig.json in fourslash tests (microsoft#16484) Add a `getFullText()` helper method to `IScriptSnapshot` (microsoft#21155) Test Diff and Omit Keep string index from intersections in base constraint of index type LEGO: check in for master to temporary branch. Fix bug: get merged symbol of symbol.parent before checking against module symbol (microsoft#21147) Do not trigger the failed lookup location invalidation for creation of program emit files Handles microsoft#20934 Add test where emit of the file results in invalidating resolution and hence invoking recompile Parse comment on @Property tag and use as documentation comment (microsoft#21119) Update baselines push/popTypeResolution for circular base constraints LEGO: check in for master to temporary branch. Test:incorrect mapped type doesn't infinitely recur ...
This adds transform flags for tracking
Superand property accesses/element accesses which containSuper; enabling us to not need to search the subtree while binding these expressions, but still know if the subtree contains thesuperreference that would indicate that the call expression needs to be sent through the ES2015 transformer (#20125). Additionally, this enabled me to shift which compute function had responsibility for setting the captures lexical this flag, allowing me to correct our behavior in a few cases (#21081).Fixes #20125
Fixes #21081