Tree shake logical expressions #2098

lukastaegert · 2018-03-29T16:05:23Z

This was inspired by, supersedes and closes #2007 and therefore resolves #2004.
Also resolves #2091.

Originally I wanted to collect some ideas on how to improve #2007 to get it merged before I started implementing larger refactorings to our tree-shaking logic. As it turned out, the changes necessary were a little more than I anticipated so I instead created a generic solution which should now be able to handle all combinations of the now three expressions that support tree-shaking:

sequence expressions (a, b)
logical expressions (a || b, a && b)
conditional expressions (a ? b : c)

For now, tree-shaking will only occur for conditional and logical expressions if a is a literal or calculated from literals, not variables. This is usually the case when using something like https://github.com/rollup/rollup-plugin-replace to set compiler flags. However the new generic solution is designed to easily handle more generic situations as well provided the getValue functionality will be extended at some point (which I decided was out of scope for this PR). This PR is focused on functionality only and not speed, which will be the focus of an upcoming PR. Nevertheless, performance seems to be about on par with the previous version.

This enables tree-shaking things like

// input
function x() {
  console.log('unused');
}

if (false && x()) console.log('ok');

// output: nothing

Also, sequence expressions have been refactored to resolve some lingering bugs that no-one has stumbled upon probably because most people are sensible enough not to use a lot of sequence expressions. In case one of the three expression types is simplified and the result is the callee of a call expression and the context would be changed by the simplification, the result is wrapped in a (0, expression) babel style. This works even in nested situations:

// input
(true, true ? true && x.y : null)()

// output
(0, x.y)()

Also, handling of default exports that are rendered for side-effects only has been refined and synced with these changes.

guybedford

Looks great. The main thing to clarify is the indirect call handling in strict mode that may not be having an effect as far as I can tell.

Also I've been thinking about the tests and it's starting to seem really redundant to have all module format outputs for cases like this that aren't really testing module format output. .Perhaps worth thinking about a new test section that just outputs es modules that can shorten the diffs for unit testing these cases?

guybedford · 2018-03-29T16:18:32Z

test/form/samples/wrap-simplified-expressions/_expected/umd.js

+	(0, wrapper.foo)();
+	(0, wrapper.foo)();
+	(0, wrapper.foo)();
+	(0, wrapper.foo)();


I'm not sure this is true for strict mode actually, are you sure about this? In the following case:

(function () { "use strict"; var a = { b () { console.log(this) } }; a.b(); var c = a.b; c(); (0, c)(); })()

the output I get is a, undefined, undefined, not a, undefined, this.

Not sure what you expect. If c = a.b, then c() and (0, c)() will always have the same output. In non-strict mode it is the global object while in strict mode (which rollup always adds to the output) it is undefined. The important problem is rather the following (with a as defined in your example):

'use strict'; (0, a.b)(); // logs undefined (true && a.b)(); // logs undefined (true ? a.b : a.b)(); // logs undefined (a.b)(); // logs a, so this is not equivalent to any of the above a.b(); // logs a as well

This is one issue that is solved by this transformation. If you remove 'use strict', undefined is just replaced by global.

BTW I added tests both to the form as well as the function section where the latter "prove" the effect of the transformation. I would suggest to play around with these tests.

(Admittedly, I did not trust this myself)

Ah of course, thanks for explaining.

guybedford · 2018-03-29T16:22:06Z

src/ast/nodes/Identifier.ts

 				}
 			}
-
+			if (hasBecomeCallee && name === 'eval') {


In theory eval can be a variable name. It is probably worth adding an extra check here up the scope hierarchy to be sure it isn't.

I think the check is isGlobalVariable just like in CallExpression bindNode.

Usually, doing the (0, ...) transformation does no harm to an identifier except that it is a little inefficient but you are right, we have the information available and should probably go all the way.

Ah, as we always have strict mode, this check is actually unnecessary, cf. here: https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Strict_mode (in "Making eval and arguments simpler")

First, the names eval and arguments can't be bound or assigned in language syntax

Just checked it myself: One cannot create a variable named eval. So any eval must be the global eval and no additional check is necessary. Will add a comment to explain this.

Ah yes strict mode reserves eval, don't know how I forgot that. Hmm in theory we shouldn't need this check in CallExpression then either... I wonder if we could remove thiis.

guybedford · 2018-03-30T10:36:50Z

src/ast/nodes/ConditionalExpression.ts

+			if (this.consequent.includeInBundle()) addedNewNodes = true;
+			if (this.alternate.includeInBundle()) addedNewNodes = true;
+		} else if (testValue ? this.consequent.includeInBundle() : this.alternate.includeInBundle())
+			addedNewNodes = true;


I've always worked with the code style that if the if has curlies, so should the else clause.

guybedford · 2018-03-30T10:40:52Z

src/ast/nodes/ConditionalExpression.ts

+	render(
+		code: MagicString,
+		options: RenderOptions,
+		{ hasBecomeCallee, hasBecomeStatement }: NodeRenderOptions = BLANK


Just wondering if we could simplify this interface and if we passed through the parent itself as a consistent third argument and then deduced these from that?

Eg a isStatementOfParent(this, parent) check, although perhaps that is unnecessary repeated work.

I guess some of this could be handled via a parameter renderedParent. Might also come in handy once we start inlining expressions. Will experiment a little with this.

guybedford · 2018-03-30T10:41:54Z

src/ast/nodes/ConditionalExpression.ts

+			branchToRetain.render(code, options, {
+				hasBecomeStatement,
+				hasBecomeCallee:
+					hasBecomeCallee || (isCallExpression(this.parent) && this.parent.callee === this),


This seems to be acting on old parent information, which would then simplify to eg isCallExpression(curParent) && curParent.callee === this?

guybedford · 2018-03-30T10:42:12Z

src/ast/nodes/ConditionalExpression.ts

+				hasBecomeStatement,
+				hasBecomeCallee:
+					hasBecomeCallee || (isCallExpression(this.parent) && this.parent.callee === this),
+				hasDifferentParent: true


Similarly this would reduce to parent !== this.parent?

guybedford · 2018-03-30T11:04:12Z

test/form/samples/wrap-simplified-expressions/_expected/umd.js

+
+	// Indirectly invoked eval is executed in the global scope
+	function testEval() {
+		console.log((0, eval)('this'));


Indirect eval still holds though of course.

lukastaegert · 2018-03-30T19:43:31Z

Perhaps worth thinking about a new test section that just outputs es modules that can shorten the diffs for unit testing these cases?

Have been thinking about something like this myself for some time. Either a new section or we let the form test runner check if output.format is provided and if so, it should just create and check the corresponding output. Would also have the advantage of enabling tests that check problems only with specific outputs.

My favourite would be to not add a new section but

enable the format check as outlined
enable folders to group the samples to make it easier to find tests that handle certain topics

guybedford · 2018-03-30T21:23:02Z

enable the format check as outlined
enable folders to group the samples to make it easier to find tests that handle certain topics

This sounds nice to me :) Folders could perhaps have an options.js default template that applies in that subfolder precedence.

lukastaegert · 2018-03-30T21:44:09Z

This sounds nice to me :) Folders could perhaps have an options.js default template that applies in that subfolder precedence.

Very good idea! Thus, sub-folders can really become coherent test suites that cover certain situations with little boilerplate. Hope to find some time to look into this for the next PR (but probably not for this one just yet).

lukastaegert · 2018-03-30T21:51:59Z

Ok, this is now ready for another review. I actually took up your suggestion and made the logic now much more generic and future-proof (think: large-scale expression inlining):

There are now two NodeRenderOptions that control the process:
- renderedParent?: Node: Only present if the rendered parent is different from the actual parent (to avoid overhead)
- fieldOfRenderedParent?: string: Indicates on which field of the parent the child will be rendered (e.g. to detect if an expression has become a callee)

I also found and fixed another bug when e.g. an object expression (which always need round brackets when rendered as a statement) becomes a statement due to tree-shaking simplifications.

guybedford

Very nice to see the simplifications, glad it could work out!

Only worry here is the repeated work in lookups for parent and field information if this approach does start to scale out more. Interested to hear your thoughts on this, but this is trickier to implement I know, although you did mention you're looking into performance next, so maybe can come as part of that.

guybedford · 2018-03-30T22:29:38Z

src/ast/nodes/CallExpression.ts

 import { ObjectPath } from '../values';

+export function isCallExpression(node: Node | { type?: string }): node is CallExpression {


This seems to be defined but not actually used?

Is removed👍

guybedford · 2018-03-30T22:31:37Z

src/ast/nodes/CallExpression.ts

@@ -1,14 +1,17 @@
 import CallOptions from '../CallOptions';
 import ExecutionPathOptions from '../ExecutionPathOptions';
 import SpreadElement from './SpreadElement';
-import { isGlobalVariable } from '../variables/GlobalVariable';


There seems to be one other use of this in TaggedTemplateExpression. If doing both, that actually removes all usages of this function entirely as well, although can be left to tree shaking too :)

guybedford · 2018-03-30T22:34:05Z

src/ast/nodes/Identifier.ts

+				name === 'eval' &&
+				renderedParent &&
+				renderedParent.type === 'CallExpression' &&
+				fieldOfRenderedParent === 'callee'


Nice! Slightly shorter names might be renderParent and renderParentField, but perhaps these are more descriptive anyway.

guybedford · 2018-03-30T22:35:01Z

src/ast/nodes/SequenceExpression.ts

+			let firstStart = 0,
+				lastEnd,
+				includedNodes = 0;
+			for (const { node, start, end } of getCommaSeparatedNodesWithBoundaries(


When I wrote getCommaSeparatedNodesWithBoundaries, I always hoped I could use it not only for variable declarations but also for sequence expressions. So should you decide to write your entire app using sequence expressions, you now get really nice comment handling 😉

guybedford · 2018-03-30T22:42:34Z

src/utils/renderHelpers.ts

+
+export function childIsStatement(parent: { type?: string }) {
+	return (
+		parent.type === 'Program' || parent.type === 'ExpressionStatement' // e.g. default exports rendered for side-effects only


I take it these will expand over time?

An alternative here might be Program.prototype.isStatementContainer = true and ExpressionStatement.prototype.isStatementContainer = true where NodeBase.prototype.isStatementContainer = false.

Actually not. In fact at the moment if an expression is simplified so that it becomes a statement, there will always be an expression statement left as the actual parent except for simplified default exports. Changed this now to get rid of this exception.

guybedford · 2018-03-30T22:47:48Z

src/utils/renderHelpers.ts

+		if (Array.isArray(value)) {
+			for (const nestedValue of value) {
+				if (nestedValue === child) return key;
+			}


This does worry me a little - eg unnecessary lookup for long sequence expressions. This information is available to the render call itself, so we could just enforce it being passed for all render calls getting us back to a monomorphic render function, but I know that is a lot of work and may not even be itself desirable for performance (although maybe it would...).

If I remember correctly there are only two node types with more than one array field - template elements and tagged template elements, so that knowledge could permit an optimization here, but perhaps worth seeing if it becomes a bottleneck. I just prefer to avoid unnecessary loops that is all, and the keys loop is already unnecessary in theory due to this being parent contextual information the render function could have passed.

Would running Rollup on uglify code that is rewritten to use long sequence expressions hit this loop?

At the moment, expression tree-shaking is powerful but very rare, and the field will only be determined if the child is actually simplified. I do not like adding this information for every render simply because it is totally unnecessary 98% of the time. The only information currently used is the fact if it is a callee so I simplified it accordingly and removed the loop. Should we require more information at some point, we can certainly revisit that.

TrySound · 2018-03-31T14:08:43Z

bin/src/run/loadConfigFile.ts

@@ -4,7 +4,8 @@ import * as rollup from 'rollup';
 import batchWarnings from './batchWarnings';
 import relativeId from '../../../src/utils/relativeId';
 import { handleError, stderr } from '../logging';
-import { InputOptions, OutputChunk } from '../../../src/rollup/index';
+import { InputOptions } from '../../../src/rollup/index';
+import { Bundle } from '../../../src/rollup';


rollup/index?

I guess the /index is unnecessary due to some TypeScript weirdness (which mirrors a node weirdness) 😉

Ok, it seems removing the /index does not really play nice without our setup so I'll leave it.

lukastaegert · 2018-03-31T14:14:11Z

Posted another version to check out @guybedford.

guybedford

Yeah, does seem better to use more specific render options. Can always reconsider more general ones in future too.

…ain situations

fix issues around simplified object expressions

* Simplify rendered parent logic to remove unnecessary checks * Fix some weird type issues

guybedford · 2018-04-04T15:35:45Z

I've just hit the (0, eval) bug in the latest SystemJS release with the Rollup build... great timing on this one :)

lukastaegert requested a review from guybedford March 29, 2018 16:05

kzc mentioned this pull request Mar 29, 2018

Mangling module import and export bindings mishoo/UglifyJS#3026

Closed

guybedford requested changes Mar 30, 2018

View reviewed changes

guybedford approved these changes Mar 30, 2018

View reviewed changes

lukastaegert force-pushed the tree-shake-logical-expressions branch from 3c8771a to 80cfff0 Compare March 30, 2018 21:44

guybedford reviewed Mar 30, 2018

View reviewed changes

TrySound reviewed Mar 31, 2018

View reviewed changes

guybedford approved these changes Apr 1, 2018

View reviewed changes

lukastaegert added 6 commits April 1, 2018 18:10

Tree-shake and simplify logical expressions

6d341db

Wrap simplified logical, conditional and sequence expressions in cert…

0ec43cb

…ain situations

Resolve #2091

67b4833

Implement more generic logic for handling changed parent relations,

a9f05f9

fix issues around simplified object expressions

Remove unnecessary check for global eval

b969cd4

* Remove some unused helpers

b047228

* Simplify rendered parent logic to remove unnecessary checks * Fix some weird type issues

lukastaegert force-pushed the tree-shake-logical-expressions branch from 2ffea8f to b047228 Compare April 1, 2018 16:13

lukastaegert added this to the 0.58.0 milestone Apr 1, 2018

lukastaegert merged commit b047228 into master Apr 1, 2018

lukastaegert added the Fixed pending release label Apr 1, 2018

lukastaegert deleted the tree-shake-logical-expressions branch April 1, 2018 16:21

lukastaegert removed the fixed pending release label Apr 16, 2018

		import { ObjectPath } from '../values';

		export function isCallExpression(node: Node \| { type?: string }): node is CallExpression {

Tree shake logical expressions #2098

Tree shake logical expressions #2098

Conversation

lukastaegert commented Mar 29, 2018

guybedford left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guybedford Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert commented Mar 30, 2018

guybedford commented Mar 30, 2018

lukastaegert commented Mar 30, 2018

lukastaegert commented Mar 30, 2018

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert commented Mar 31, 2018

guybedford left a comment

Choose a reason for hiding this comment

guybedford commented Apr 4, 2018

guybedford left a comment •

edited

Loading

guybedford Mar 29, 2018 •

edited

Loading