Refactor tree-shaking rendering by lukastaegert · Pull Request #1949 · rollup/rollup

lukastaegert · 2018-02-07T06:54:05Z

This PR is now ready to be reviewed.

To try it out, install rollup via

npm install rollup/rollup#refactor-treeshake-rendering

Issues resolved by this PR:

resolves Comments and whitespace preserved unnecessarily #911: Always removes comments together even with functions even at the beginning of a module (while preserving the possibility to have a banner comment)
resolves Partially removed default export produces invalid syntax #1772: Default exported objects that are not actually used but contain side-effects are now properly wrapped in parentheses as expression statements
resolves treeshake build bug for not used empty function #1831: Unused variables are removed properly in more situations
resolves Multiple Empty Lines in Combination with Babel #1848: Removed statements no longer leave unwanted white-space
resolves Dropped comment before default export #1850: Default exported expressions retain annotation comments even when they are rewritten for other output formats
resolves rollup doesn't strip const with comma operator #1937: Unused variables are removed properly in more situations
resolves [bug] Redefining live function & class bindings #1943: Ids of exported functions are not replaced with a reference to the exports object for certain outputs

This will refactor core parts of the rendering logic especially with regard to tree-shaking and comment-handling. Previously, tree-shaking was managed by the rendering logic of the Statement base node, i.e. nodes that were not included would remove themselves from the tree. This often lead to unwanted white-space and sup-optimal results with regard to comments.

Instead, the parent node is now responsible for not rendering its children based on their .included flag.

To that end, modules, block scopes and switch cases now use a new optimized statement border detection algorithm. For block statements, it will e.g. look like this:

// input

{ // retained comment
  // removed leading comment
  removedStatement; // removed trailing comment
  // retained leading comment
  removedStatement; // retained trailing comment  
} // retained comment

// output

{ // retained comment
  // retained leading comment
  removedStatement; // retained trailing comment  
} // retained comment

i.e.

if there are no statements on the first line of the block statement, all comments here are retained with the block statement, otherwise they are retained/removed together with the first statement
if there are no statements on the same line and after another statement, all comments here are retained with the leading statement, otherwise they are retained/removed together with the next statement
all other comments not on the same line as a statement are retained/removed together with the next statement (this is important for e.g. annotations)
line-breaks inside comments are ignored

For modules this means that any comment starting on the first line of the module will be retained as a header comment while all subsequent comments belong to the first statement.

Also, the logic for variable declarations was thoroughly rewritten to provide, in my opinion, the best possible output in most situations, e.g.

// input

var /* removed leading comment */ removed1 = 1, // removed trailing comment
  // retained leading comment
  retained = 2, // retained trailing comment
  // removed leading comment
  removed1 = 3;

console.log(retained);

// output

var // retained leading comment
  retained = 2; // retained trailing comment

console.log(retained);

This also affects the way named export declarations are rendered.

The only thing I am not 100% happy about is the complexity of the variable declaration rendering function. Any suggestions for e.g. extracting functionality here are very welcome.

guybedford · 2018-02-07T07:44:34Z

Great work! Happy to review when ready.

exceptions for IfStatements and ExportDefaultDeclarations for now

old logic but can leave trailing comments and line-breaks as the SwitchCase length provided by acorn does not include those)

* Skip unnecessary calculations

of comments

* Move responsibility to suppress semicolon to parent

next—even though it can produce less nice code—as otherwise, it was not guaranteed each single line comment was properly closed

before a line-break with the previous declarator

lukastaegert · 2018-02-11T14:48:26Z

From my side this is ready for review! I hope I did the system bindings correctly as I rewrote this logic simply by looking at the previous output 😳

guybedford · 2018-02-11T14:52:35Z

Sure, will get to it.

Just testing #1937, if I alter the example to use destructuring I'm getting the statement removed incorrectly:

const FONT_HINGE_SIZE = 0.07;
const BACK_HINGE_SIZE = 0.16;

function PointerGauge () {
	const { backHinge, frontHinge } = { backHinge: BACK_HINGE_SIZE, frontHinge: FONT_HINGE_SIZE };

	return [
		//backHinge,
		fontHinge
	]
}

export default PointerGauge;

strips the whole destructuring statement above.

guybedford · 2018-02-11T14:53:44Z

Ahh, no that was just spelling - seems to be working fine.

guybedford

Looks good, I really like the direction this rendering logic is going.

In a test on a large codebase I'm getting about a 5 - 10% slowdown, do you think we could try and focus on some performance aspects here for landing?

I've added some suggestions to reduce allocations, but will have to see if that does much.

I have tested on Windows here and all is fine, but even so it may be worth also checking that all of the line break logic correctly handles \r characters too, as it seems quite \n specific currently.

guybedford · 2018-02-11T15:08:02Z

+export default class DoWhileStatement extends NodeBase {
 	type: NodeType.DoWhileStatement;
-	body: StatementNode;
+	body: Node;


Are you sure we need to get rid of the concept of a StatementNode here? Quite nice to match the spec.

It serves no purpose but I can put in StatementNode as an alias for Node so that it is there if we need it in the future.

Ahh originally I had the compound ESTree types as representing a union of their members so there was some typing benefit here on the generic interfaces. But I remember now you refactored the Node methods to no longer rely on these union properties, which is cleaner anyway. But perhaps nice to at least keep some level of the semantic if it turns out there are places where this typing information might be useful to reconsider in future.

guybedford · 2018-02-11T15:13:30Z

+	const declarationEnd = findFirstOccurrenceOutsideComment(code, declarationKeyword) + declarationKeyword.length;
+	code = code.slice(declarationEnd);
+	code = code.slice(0, findFirstOccurrenceOutsideComment(code, '{'));
+	const generatorStarPos = findFirstOccurrenceOutsideComment(code, '*');


Minor performance suggestion here, although its probably trivial, but instead of creating a new string, the findFirstOccurrenceOutsideComment method could take an offset argument like the standard string indexOf function.

To my shame I must admit I was not actually aware of the second parameter of String.indexOf 😳Yes, of course it makes sense to use it here and in other places. I see how I can rewrite this to reduce the number of slices.

guybedford · 2018-02-11T15:14:44Z

+	let commentStart, commentLength, lineBreakPos;
+	while (true) {
+		commentStart = code.indexOf('/');
+		lineBreakPos = (~commentStart ? code.slice(0, commentStart) : code).indexOf(searchString);


Similarly these slice calls could be refactored to use an index into the indexOf method.

guybedford · 2018-02-11T15:20:55Z

+	code = code.slice(declarationEnd);
+	code = code.slice(0, findFirstOccurrenceOutsideComment(code, '{'));
+	const generatorStarPos = findFirstOccurrenceOutsideComment(code, '*');
+	if (~generatorStarPos) {


I'm not sure I can handle this convention anymore :P

When using binary operators like this in JavaScript, it means the 64-bit JavaScript float is converted into a 32-bit twos-complement integer with rounding, then the not operation is applied.

In addition these can fail for large numbers when the 32 bit limit gets reached, which can still be in the safe operating range of the integer values of a 64 bit float (although the ranges are well out of what Rollup will ever see).

generatorStarPos === 0 is explicit and a single comparison operation in the final assembly over all the above multi-step complexity.

I must admit I do not really like this either but running all different versions of checking that indexOf (or something similar) returned a result through jsben.ch on Chrome, the tilde operator was always fastest

Ok, running with a larger sample set and a slightly different test, the tilde operator actually seems to be on par with !== -1 so I'll use that.

guybedford · 2018-02-11T15:29:06Z

+	let commentStart, commentLength, lineBreakPos;
+	while (true) {
+		commentStart = code.indexOf('/');
+		lineBreakPos = (~commentStart ? code.slice(0, commentStart) : code).indexOf(searchString);


Is this really lineBreakPos or rather searchPos?

guybedford · 2018-02-11T15:48:09Z

-			this.declaration.render(code, options);
+	render (code: MagicString, options: RenderOptions, { start, end }: NodeRenderOptions = {}) {
+		if (this.declaration === null) {
+			code.remove(start || this.start, end || this.end);


When will start not be defined here?

guybedford · 2018-02-11T15:50:46Z

@@ -63,9 +59,7 @@ export default class BlockStatement extends StatementBase {

 	render (code: MagicString, options: RenderOptions) {


I wonder if we should make start and end mandatory arguments for all render calls, defaulting to start = this.start?

guybedford · 2018-02-11T15:51:27Z

-	render (code: MagicString, _options: RenderOptions) {
-		code.remove(this.leadingCommentStart || this.start, this.next || this.end);
+	render (code: MagicString, _options: RenderOptions, { start, end }: NodeRenderOptions = {}) {
+		code.remove(start || this.start, end || this.end);


Having start and end as arguments with defaults would avoid these scenarios too.

As defaults work with object patterns as well, this is certainly an improvement

guybedford · 2018-02-11T15:56:12Z

+			this.end - (code.original[this.end - 1] === ';' ? 1 : 0)
+		);
+		let renderedContentEnd;
+		if (/\n\s*$/.test(code.slice(this.start, separatedNodes[0].start))) {


guybedford · 2018-02-11T16:08:24Z

+			}
+			leadingString = '';
+			nextSeparatorString = '';
+			if (isIdentifier(node.id) && isReassignedPartOfExportsObject(node.id.variable)) {


Perhaps isReassignedAsExportsMember? Object as a term isn't descriptive enough for me to immediately think of a member expression form without further prompting.

lukastaegert · 2018-02-12T06:26:39Z

I am pretty sure the reason for the worse performance is that now each statement in every module and block statement has its boundaries calculated though only very few really need this. This was my naive implementation but seeing this, I will try to change the algorithm to only calculate boundaries when they are actually needed (since not running code is always the best performance improvement...).

* Get rid of unnecessary defaults * Replace tilde operator

* Rename noSemicolon -> isStatement

lukastaegert · 2018-02-13T07:52:00Z

Ok, I did not do everything yet (especially line-breaks still assume "\n") but

the render helpers now all use start indices
boundary detection is skipped if a boundary is not needed

I expect especially the latter to provide a very noticeable performance boost.
@guybedford Could you check your codebase again and tell me if there still is a performance degradation?

guybedford · 2018-02-13T09:19:39Z

With the more accurate timing I'm only seeing a slowdown now from around 260ms for render to 340ms in a large tree.

The string search times seems to make sense in that well-written string search algorithms offer better than O(n) complexity, while taking an initial hit in additional mechanics setup.

I've created a PR to get the performance timings working again. Perhaps these can be watched closely as any further changes are made... so hard to speculate here.

lukastaegert · 2018-02-13T09:46:21Z

Nice to hear that render is still rather fast. I have got some ideas on how to skip boundary detection for variable declarations when not necessary that should further noticeably speed up rendering. Let's see what we get to in the end.

guybedford · 2018-02-14T08:49:40Z

The last commit seems to have added a good 20ms or so speedup. Funny how it doesn't look like it does much.

The performance costs seem pretty negligible at this point so count me approved... just worth making a habit of checking it for all compiler work I think.

guybedford · 2018-02-14T08:52:46Z

 		return addedNewNodes;
 	}
+
+	render (code: MagicString, options: RenderOptions) {


Should this render function not have a third argument? Just wondering how much the polymorphism might bite us in performance as predictable singular argument signatures are important to optimizations.

But for each SwitchCase, the signature is the same. As far as I know, call signatures are more important. Might be reasonable to consider adding "undefined" as a third parameter when calling render instead. But maybe we wait until we got rollup fit for the V8 web tooling benchmark so that we can talk to the actual experts here.

Since a switch case is a statement I was thinking of a call site like the render call from renderStatementList, although I see this is distinguished by needsBoundaries now.

* Properly handle \r\n line-breaks

lukastaegert · 2018-02-14T09:19:25Z

Ok, this is again ready for review. I added a few smaller improvements i.e. charCodeAt instead of direct character access and just counting up an index instead of searching in some situations. Most importantly, the expensive variable declaration rendering algorithm will be skipped if none of its transformations are necessary (which is probably the reason for the 20ms speedup).

As for line-breaks, I decided to ignore the uncommon "\r"-only line-breaks (the result will still work, it will just not be as pretty). As for "\r\n" line-breaks, those should have already worked in most situations out of the box except in situations where we remove a line-break. This is now accounted for.

guybedford · 2018-02-14T09:46:14Z

One super minor nit in testing:

// comment
export var p = 5; /*asdf1*/ switch (test) {
		// comment
	default:
		'asdf'; //asdf
}
/* asdf2 */

Returns export var p = 5;/*asdf*/ losing the spacing before the first block comment.

guybedford · 2018-02-14T09:50:24Z

-		// discover this module's imports and exports
-		let lastNode: Node;
-
+		enhance(this.ast, this, this.dynamicImports);


If you're no longer using this.comments does that mean this property can be removed entirely, including the use of the onComment hook?

Just tested this and the answer is yes :)

Unfortunately not as it is still required to remove source map comments. This, however, is the only reason why we keep this.

Ahh. Can you point me to the code?

Maybe at some point we should implement our own algorithm to scan for and remove those comments.

Ah I see. A sourceMappingURL comment can only be a block comment so it's fine to do something custom. Also it can be based on lastIndexOf as a backwards search through the string would be quicker.

Here's the code I have used for this previously - https://github.com/systemjs/builder/blob/master/lib/sourcemaps.js#L12.

Looking at this example, maybe the issue is not that trivial and we should probably rather trust acorns parsing here and leave it as it is:

rollup/test/form/samples/removes-existing-sourcemap-comments/main.js

Line 3 in 22aa1fb

var str = `

Otherwise we might break the sourceMap handling of other libraries.

Here's v8's method to do this - https://github.com/v8/v8/blob/8d38c15e04598f9e48e6230327da0e41a2445957/src/inspector/search-util.cc#L16

Note that it doesn't use a lexer at all.

I think it would be much simpler to make this code its own method and deprecate this.comments entirely. But happy to make this a separate PR too if you prefer.

Thinking about this further detection is a less stringent problem than removal - and that’s what I got confused here. It makes sense that Rollup has to hold to a higher standard for removal not to break code.

Perhaps we can inline the removal into the parsing code without separately gathering the comments at least.

guybedford · 2018-02-14T09:55:44Z

Including the removal of the this.comments tracking on module, the performance seems just about completely equal now to me.

Good to merge with those changes.

guybedford · 2018-02-14T15:05:03Z

Just to be clear, this seems good to land anytime to me, given that the points mentioned are no longer relevant.

lukastaegert · 2018-02-14T15:35:04Z

Nice! Depending on what else is lying around here, I guess I'll try to assemble a release tomorrow!

lukastaegert mentioned this pull request Feb 11, 2018

Bug of compile (1, eval) #1793

Closed

lukastaegert added 17 commits February 11, 2018 11:21

Make BlockStatements take care of removing their children

4b9dfc3

Extract BlockStatement rendering to be used for Program nodes with

2436ee1

exceptions for IfStatements and ExportDefaultDeclarations for now

Use new logic for switch statements (which works much better than the

48f4683

old logic but can leave trailing comments and line-breaks as the SwitchCase length provided by acorn does not include those)

* Properly handle multi-line comments after statements

152eb29

* Skip unnecessary calculations

Refactor and simplify default export rendering

eae7d96

Clean up code, improve performance and resolve #1850

c345fb5

Resolve #1772

7781b5f

Remove "statement" concept and other remains of old positioning logic

698f3a2

When checking for symbols other than line-breaks, check for both kinds

a395faf

of comments

First draft for variable declarations

de9acd0

* Simplify declaration rendering logic

ed77a00

* Move responsibility to suppress semicolon to parent

Prevent trailing white-space

d2720f6

Remove line-breaks after declaration key-word

6bb3829

Resolve #1937

2a570a2

Resolve #1831

3588a4a

Resolve #1943

9ff220f

Include line-breaks with the previous statement instead of the

6b30972

next—even though it can produce less nice code—as otherwise, it was not guaranteed each single line comment was properly closed

lukastaegert force-pushed the refactor-treeshake-rendering branch from e6d1a29 to 6b30972 Compare February 11, 2018 10:21

lukastaegert added 2 commits February 11, 2018 13:55

Rework declaration handling to associate comments after a comma but

103e16f

before a line-break with the previous declarator

Fix TypeScript test

377fe90

lukastaegert requested a review from guybedford February 11, 2018 14:36

lukastaegert changed the title ~~[WIP] Refactor tree-shaking rendering~~ Refactor tree-shaking rendering Feb 11, 2018

guybedford reviewed Feb 11, 2018

View reviewed changes

* Improve some names

495f97a

* Get rid of unnecessary defaults * Replace tilde operator

lukastaegert added 4 commits February 12, 2018 08:26

* Reintroduce statement concept as an alias

acaea3d

* Rename noSemicolon -> isStatement

Reduce number of necessary slices by supplying start indices

d45a74c

Only calculate boundaries if necessary

adeebc2

Store flags for AST nodes directly on the prototype

8ecadf2

Improve variable declaration performance

114149f

guybedford approved these changes Feb 14, 2018

View reviewed changes

guybedford reviewed Feb 14, 2018

View reviewed changes

* Use charCodeAt where possible to improve performance

22aa1fb

* Properly handle \r\n line-breaks

guybedford reviewed Feb 14, 2018

View reviewed changes

lukastaegert added this to the 0.56.0 milestone Feb 14, 2018

Slightly clean up source map removal

bb8b81d

lukastaegert merged commit bb8b81d into master Feb 15, 2018

lukastaegert deleted the refactor-treeshake-rendering branch February 15, 2018 07:45

		@@ -63,9 +59,7 @@ export default class BlockStatement extends StatementBase {

		render (code: MagicString, options: RenderOptions) {

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

guybedford commented Feb 7, 2018

Uh oh!

lukastaegert commented Feb 11, 2018

Uh oh!

guybedford commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guybedford commented Feb 11, 2018

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!

lukastaegert commented Feb 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

guybedford commented Feb 13, 2018

Uh oh!

lukastaegert commented Feb 13, 2018

Uh oh!

guybedford commented Feb 14, 2018

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!

lukastaegert commented Feb 14, 2018

Uh oh!

guybedford commented Feb 14, 2018

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

lukastaegert commented Feb 7, 2018 •

edited

Loading

guybedford commented Feb 11, 2018 •

edited

Loading

lukastaegert commented Feb 13, 2018 •

edited

Loading