Tree-shake prototype method calls on literals by lukastaegert · Pull Request #1916 · rollup/rollup

lukastaegert · 2018-01-27T15:13:08Z

Finally found some time to do some programming of my own!

This PR will resolve most of the examples listed in Rollup does not strip anything with a chained function #1824. The remaining ones rely on the global Math object being properly typed which is beyond the scope of this PR.
It will also resolve the initial example given in Recognize pure instance methods #673. The workaround described later will not be resolved, though, as this again will require changes to the global variable handling.

With this PR, Rollup will recognise all standard prototype methods on string, number, boolean, array and object literals. This includes methods that call one of their arguments such as Array.map, for which side-effect detection is in place, and mutating methods like Array.sort for which it is taken care that all calls will be included if the mutated variable is included. This will also work across variables as long as those variables are not reassigned. For example, this will be removed:

// input
const uncoolChars = 'MX';
const str = 'My String X';
const wrapper = {str};
const coolString = wrapper.str
  .split('')
  .filter(char => uncoolChars.indexOf(char) === -1)
  .join('')
  .trim();

// output: nothing

However if a side-effect is introduced in the filter function, e.g. .filter(char => console.log(char) || true), then everything will be retained. This also includes mutations of included variables:

// input
const chars = [ 'A', 'B', 'C' ];
const localObj = {};
const exportedObj = {};

chars.forEach( char => {
	localObj[ char ] = char;
	exportedObj[ char ] = char;
} );
export { exportedObj };

// output
const chars = [ 'A', 'B', 'C' ];
const exportedObj = {};

chars.forEach( char => {
	exportedObj[ char ] = char;
} );

export { exportedObj };

However if we remove the export of exportedObject, then nothing is retained.

Assumptions: As always with Rollup's tree-shaking, we assume that all builtin methods behave according to their specification. This also includes the assumption that e.g. .toString, even if overridden, does not have any side-effects.

Possible future extensions: With this in place, it should also not be too difficult to start adding return types (i.e. string, boolean, number, object, array or unknown) to builtin global functions (except that it will be a lot of work...). Furthermore, a similar logic might be applied to handle global functions that call their arguments, e.g. Promise.resolve(myFunc) could be removed if calling myFunc does not have a side-effect.

guybedford

👍 nice work!

I ran some quick tests and it seems to reduce the build size of rollup itself which is a good start, although just marginally.

Interestingly there seems to be a performance improvement with this change - I wonder if that's due to these acting as better stop conditions, or some other random noise.

guybedford · 2018-01-28T15:22:27Z

 	withNew: boolean;
 	args?: (ExpressionEntity | SpreadElement)[];
-	caller: CallExpressionType;
+	caller: Object;


Is there any way to keep this as a more descriptive type? This analysis code can be quite complex to follow for users new to the codebase, so maintaining transparent types where possible would be useful in aiding understanding.

Actually the only use of caller is to identify if the corresponding CallExpression has already been encountered in this run. Which does not exist in the case of implicit calls be builtins.
Would it be OK to instead rename caller to callIdentifier to make it more clear this is only an identification label?

guybedford · 2018-01-28T15:30:30Z

+		valueOf: returnsNumber
+	}),
+	string: assembleMemberDescriptions({
+		charAt: returnsString,


I can't believe you skipped string.blink() 😜

I KNOW. Especially since I just checked that even Node supports it! Should I change that?

I would personally be deeply upset if my codebase wasn't able to tree-shake my pure blink-rendering code.

guybedford · 2018-01-28T15:32:17Z

+		toLowerCase: returnsString,
+		toString: returnsString,
+		toUpperCase: returnsString,
+		trim: returnsString,


Maybe add trimLeft and trimRight.

I wanted to but MDN was warning me about it:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/trimLeft

Ah, ok sure, didn't see those.

guybedford · 2018-01-28T15:39:30Z

+		lastIndexOf: returnsNumber,
+		map: callsArgReturnsArray,
+		reduce: callsArgReturnsUnknown,
+		reduceRight: callsArgReturnsUnknown,


Could reverse be added here as returnsArray?

Also would it be possible to support shift, push, pop and unshift, sort and splice?

Unfortunately, mutating methods are much more difficult. The reason is this:
If we want to check if e.g. .reverse() can be called on a variable, we check if this variable has a unique value, e.g. in

const x = [1, 2, 3]

x would have a unique value, while in

let x = [1, 2, 3]; x = [3, 4, 5];

the value is not unique i.e. we are checking for reassignments. In case of a reassignment, we just internally mark the variable as "reassigned" and treat it like undefined since otherwise (i.e. if we were tracking all assignments), there would be a possibly exponential performance degradation (learned this the hard way).

Now if there is a unique value, we continue asking the original expression if it supports .reverse() until we hit the ArrayLiteral. But the algorithm no longer knows what the original variable was or any variables in between. So we do not know if any of those variables are already included in the bundle. Which is a problem because in that situation, the call would have the side-effect of mutating an included variable.

I guess its possible to find a solution for this but for now I just wanted to focus on the low-hanging fruit.

Makes sense.

guybedford · 2018-01-28T15:41:57Z

+		slice: returnsArray,
+		some: callsArgReturnsBoolean,
+		toLocaleString: returnsString,
+		toString: returnsString


Is the assumption that these can work because we know we specifically have an array of literal values so there are no toString side effects?

If these cases work, surely join would work too then?

But if we don't actually know what we are calling toString on, I would argue we should remove these.

Ah, I see, did not consider this meta-effect. So I guess yes, we could consider removing it. Which in the case of join would really be a pity. Or we act under the assumption that toString never has a side-effect. Might be reasonable to do as we also assume that builtins have not been modified. Even if a toString method is provided, I would assume that people would not rely on it having side-effects. I may be proven wrong but unless you know a specific package that relies on this, I would argue to maintain this assumption until someone encounters an issue here.
What do you think?

toString should always be pure definitely, even if it is overridden. So it's the edge case of the edge case that catches yes... and we have assumptions like this all over for builtins so I suppose it is consistent with the assumptions of the project. It may be worth documenting somewhere these kinds of assumptions so that users can easily determine if Rollup is right for their codebase before hitting odd bugs like these though. Personally I find projects that clearly state their assumptions incredibly reassuring as you can fully appreciate the decisions you are making with it.

guybedford · 2018-01-28T15:48:31Z

+		hasEffectsWhenCalledAtPath: path => {
+			if (path.length === 1) {
+				const subPath = path[0];
+				return isUnknownKey(subPath) || !literalMembers[type][subPath];


literalMembers[type] can be hoisted out of this function.

I was hoping so, too. Unfortunately there is some circular dependency between the data structures in this file (which is the main reason I moved everything into the same file). I.e., createUnknownLiteral is called the first time before we hit the definition of literalMembers. If you have a suggestion how to untangle these that would be cool but I fear it will need to remain like this for now.

It sounds like the simplest fix may just be to flatten UNKNOWN_LITERALS into UNKNOWN_LITERAL_NUMBER, UNKNOWN_LITERAL_BOOLEAN and UNKNOWN_LITERAL_STRING. The collection object could still exist fine if lookups are needed in other places.

guybedford · 2018-01-28T15:54:09Z

+		propertyIsEnumerable: returnsBoolean,
+		toLocaleString: returnsString,
+		toString: returnsString,
+		valueOf: returnsUnknown


Another thing - it could be worth having these on the other literal types as well as they all extend from object too.

guybedford · 2018-01-28T17:07:11Z

Another thing I was wondering here - should array expressions containing spread literals be deoptimized, or does the code handling spread already do that? Since those iterators can have side effects.

lukastaegert · 2018-01-28T17:51:39Z

Thanks for the quick feedback. I don't know if I manage to put in the changes today but I will add them at the latest tomorrow morning. About spread elements: Those are currently heavily deoptimized i.e. using a spread element is always a side-effect, you can try it in the REPL! Certainly not ideal, might be worthwhile to revisit once we add object spread elements.

guybedford · 2018-01-28T18:14:29Z

+	number: createUnknownLiteral('number'),
+	string: createUnknownLiteral('string')
+};
+const returnsBoolean: RawMemberDescription = { value: { returns: UNKNOWN_LITERALS.boolean, callsArgs: null } };


(then these would become UNKNOWN_LITERAL_BOOLEAN etc..

guybedford · 2018-01-28T18:23:21Z

That's all my feedback on this one - will be back online week of 4th of Feb here 😁

lukastaegert · 2018-01-28T19:58:47Z

Awesome, thanks & have fun!

lukastaegert · 2018-01-29T05:39:10Z

Ok, updating this will take a moment longer, but since you're on vacation I guess this does not matter much :)

As it turned out after explaining to you in great length why we cannot include them, mutating array methods are actually NOT a problem because nested calls on included objects are always included anyway. Such a handy heuristic! So gonna include those + some regression tests for other things that came up in the review just to be safe.

matter.

access and correct toString identification

* Add inherited object members to all literals

lukastaegert requested a review from guybedford January 27, 2018 15:13

guybedford approved these changes Jan 28, 2018

View reviewed changes

guybedford reviewed Jan 28, 2018

View reviewed changes

lukastaegert added this to the 0.56.0 milestone Feb 8, 2018

lukastaegert added 7 commits February 11, 2018 11:19

Treeshake pure literal prototype functions

853fc53

Tree-shake array members that call their first argument

51eed57

Rename caller => callIdentifier to make clear the data type does not

30c7e08

matter.

Split up literal types for slightly better performance due to easier

141bdd7

access and correct toString identification

* Add mutator functions to array

6e4e055

* Add inherited object members to all literals

Use correct option name in missing option warning

1779287

Fix most deprecation warnings in tests

67a7131

lukastaegert force-pushed the literal-prototypes branch from c471107 to 67a7131 Compare February 11, 2018 10:19

lukastaegert merged commit 67a7131 into master Feb 15, 2018

lukastaegert deleted the literal-prototypes branch February 15, 2018 07:44

Uh oh!

Uh oh!

Conversation

lukastaegert commented Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guybedford commented Jan 28, 2018

Uh oh!

lukastaegert commented Jan 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guybedford commented Jan 28, 2018

Uh oh!

lukastaegert commented Jan 28, 2018

Uh oh!

lukastaegert commented Jan 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lukastaegert commented Jan 27, 2018 •

edited

Loading