Perf regression: emit time

[amcasey]

Several of the perf tests show an emit time regression since 4.2, but I focused on angular, since it seemed the clearest.

Regressions are at 68b0323 (aka #42676) and 7751ecb (aka #35877). Cumulative effect is 10-15%.

[rbuckton]

#42676 didn't seem to have any major regressions when I tested it, although it does have some impact (namely a full walk of the tree prior to emit), but there's really no other easy way to fix substitutions requiring parenthesization (other than not doing it and just adding excess parentheses around everything added via substitution).

#35877 is an ECMAScript conformance change that affects imports and necessitates adding new nodes to the tree for every call to an imported function. I'm surprised the impact there is so heavy, but unfortunately its another case where I'm not certain what we could do differently other than make the transformation optional via a flag. We opted not to do the flag, but also not to do the transform for namespaces and only external modules.

You mentioned these are the times from Angular. What version of NodeJS is this from?

[amcasey]

@rbuckton I believe that's the node 14 chart, but I saw changes across versions.

[DanielRosenwasser]

@rbuckton it looks like #42676 is not strictly necessary - it could be backed out and while the emitted code won't always be canonical, it will use a conservative but cheap check that can win back half of the regression. I would vote that we revert unless we can avoid the tree walk.

#35877 - well, that seems more unclear to me.

[rbuckton]

@rbuckton it looks like #42676 is not strictly necessary - it could be backed out and while the emitted code won't always be canonical, it will use a conservative but cheap check that can win back half of the regression. I would vote that we revert unless we can avoid the tree walk.

I'd like to investigate that one further as there were some other changes that were part of that change (such as the reusable trampoline for binary expressions) that might be difficult to untangle. I considered other mechanisms for handling the parenthesization cleanly, but they were all much more complicated implementation wise. The main reason I introduced this change was to prevent us from running into further one-off parenthesization-related issues. I may have some other ideas that could work, such as "pushing" a parenthesization rule prior to emitting a child node, and having substitution check for this rule on the way out. The downside being the need to ensure both the node factory and the printer remain in sync.

#35877 - well, that seems more unclear to me.

That PR fixes a longstanding issue and has support from members of TC39, rolling it back would be unfortunate. When we discussed it as a team in a design meeting, one of the important points was that anyone using --module esnext and a bundler like Webpack+Babel was already getting this output, so we were matching the ecosystem. I might also want to deep-dive into this PR to determine if there's a way to reduce the performance cost.

Perhaps it might be feasible to mark the PropertyAccessExpression resulting from substitution of an imported Identifier with an EmitFlag, and have the printer wrap the identifier in a (void 0, ...). Another possible solution would be to create a single, reusable void 0 expression to reduce allocations.

[DanielRosenwasser]

@rbuckton I will opt for us to back out the changes by tomorrow since we haven't gotten a flag in yet.

[rbuckton]

For some reason the performance dashboard isn't showing perf numbers for recent CI builds. I'm looking into that. The fix in #43652 should have significantly improved both regressions (since one was exacerbated by the other), though I'm still looking for further optimizations.