Rollup version 2.53.2 uses substantially more memory than 2.53.1

[dasa]

Rollup Version

v2.53.2

Operating System (or Browser)

macOS

Node Version (if applicable)

14.17.3

Link To Reproduction

N/A

Expected Behaviour

Memory usage remains similar between patch releases.

Actual Behaviour

For my project, v2.53.1 peaks at about 3.3 GB memory for the node process as reported by the macOS Activity Monitor. Rollup v2.53.2 peaks at about 4.8 GB.

[kzc]

Cannot reproduce maximum resident memory size increase between v2.53.1 and v2.53.2 on macOS building the latest version of rollup from sources (ea96ab0), but it is evident that node v14 uses considerably more memory than node v12 or node v16:

node-v12 + rollup v2.53.1 -> avg 22.10 real, 1136295936 max res

$ for i in 1 2 3 4; do /usr/bin/time -l node-v12.13.0 dist-v2.53.1/bin/rollup --config rollup.config.ts --configPlugin typescript 2>&1 | egrep 'real|maximum' | xargs echo ; done
22.60 real 44.39 user 2.71 sys 1156284416 maximum resident set size
22.03 real 43.45 user 2.73 sys 1120350208 maximum resident set size
21.88 real 42.68 user 2.73 sys 1157246976 maximum resident set size
21.89 real 42.63 user 2.71 sys 1111302144 maximum resident set size

node-v14 + rollup v2.53.1 -> avg 21.96 real, 1423980544 max res

$ for i in 1 2 3 4; do /usr/bin/time -l node-v14.16.0 dist-v2.53.1/bin/rollup --config rollup.config.ts --configPlugin typescript 2>&1 | egrep 'real|maximum' | xargs echo ; done
22.17 real 46.22 user 2.80 sys 1451900928 maximum resident set size
21.94 real 45.30 user 2.77 sys 1406828544 maximum resident set size
21.89 real 44.84 user 2.69 sys 1420095488 maximum resident set size
21.83 real 44.98 user 2.66 sys 1417097216 maximum resident set size

node-v16 + rollup v2.53.1 -> avg 22.15 real, 1081286656 max res

$ for i in 1 2 3 4; do /usr/bin/time -l node-v16.1.0 dist-v2.53.1/bin/rollup --config rollup.config.ts --configPlugin typescript 2>&1 | egrep 'real|maximum' | xargs echo ; done
22.30 real 45.66 user 2.82 sys 1058729984 maximum resident set size
22.23 real 46.18 user 2.84 sys 1149878272 maximum resident set size
22.12 real 46.00 user 2.84 sys 1059414016 maximum resident set size
21.93 real 45.58 user 2.82 sys 1057124352 maximum resident set size

node-v12 + rollup v2.53.2 -> avg 22.10 real, 1143720960 max res

$ for i in 1 2 3 4; do /usr/bin/time -l node-v12.13.0 dist-v2.53.2/bin/rollup --config rollup.config.ts --configPlugin typescript 2>&1 | egrep 'real|maximum' | xargs echo ; done
22.05 real 42.88 user 2.72 sys 1143865344 maximum resident set size
21.96 real 43.34 user 2.72 sys 1118474240 maximum resident set size
22.20 real 43.72 user 2.70 sys 1162633216 maximum resident set size
22.20 real 42.96 user 2.72 sys 1149911040 maximum resident set size

node-v14 + rollup v2.53.2 -> avg 22.02 real, 1413613568 max res

$ for i in 1 2 3 4; do /usr/bin/time -l node-v14.16.0 dist-v2.53.2/bin/rollup --config rollup.config.ts --configPlugin typescript 2>&1 | egrep 'real|maximum' | xargs echo ; done
22.11 real 45.43 user 2.77 sys 1413652480 maximum resident set size
22.11 real 45.26 user 2.65 sys 1411878912 maximum resident set size
21.96 real 44.93 user 2.68 sys 1417183232 maximum resident set size
21.88 real 44.77 user 2.66 sys 1411739648 maximum resident set size

node-v16 + rollup v2.53.2 -> avg 22.06 real, 1065199616 max res

$ for i in 1 2 3 4; do /usr/bin/time -l node-v16.1.0 dist-v2.53.2/bin/rollup --config rollup.config.ts --configPlugin typescript 2>&1 | egrep 'real|maximum' | xargs echo ; done
22.00 real 45.84 user 2.82 sys 1075097600 maximum resident set size
22.24 real 45.91 user 2.84 sys 1065652224 maximum resident set size
21.95 real 46.02 user 2.86 sys 1057013760 maximum resident set size
22.06 real 45.96 user 2.83 sys 1063034880 maximum resident set size

[sosukesuzuki]

Maybe prettier/prettier has same problem. See prettier/prettier#11217. The CI has failed because of memory problem when building flow-parser with Rollup 2.53.2.

Also I've created the repository to reproduce the bug https://github.com/sosukesuzuki-sandbox/rollup-2.53.2-bug-reproduce

[kzc]

Thanks for the succinct reproduction.

The problem was isolated to 7c014fb. It occurs with rollup 2.53.2 even if --no-treeshake is specified.

The following patch will avoid the JavaScript heap out of memory failure:

--- a/src/ast/scopes/TrackingScope.ts
+++ b/src/ast/scopes/TrackingScope.ts
@@ -14,6 +14,6 @@ export default class TrackingScope extends BlockScope {
                isHoisted: boolean
        ): LocalVariable {
                this.hoistedDeclarations.push(identifier);
-               return super.addDeclaration(identifier, context, init, isHoisted);
+               return this.parent.addDeclaration(identifier, context, init, isHoisted);
        }
 }

at the expense of a test regression. That code was changed to allow var declarations in block-less if statements to run correctly.

Using rollup 2.53.2:

$ echo 'if (Math.random() === -123) var x=1; console.log(x ? "FAIL" : "PASS");' | rollup --silent
if (Math.random() === -123) var x=1; console.log(x ? "FAIL" : "PASS");

$ echo 'if (Math.random() === -123) var x=1; console.log(x ? "FAIL" : "PASS");' | rollup --silent | node
PASS

Regression can be seen in rollup 2.53.2 with the patch above:

$ echo 'if (Math.random() === -123) var x=1; console.log(x ? "FAIL" : "PASS");' | rollup --silent
console.log("FAIL" );

The line in question in src/ast/scopes/TrackingScope.ts was an attempt to have rollup process the example code above in the same manner as if it had braces around the var declaration:

$ rollup -v
rollup v2.53.1

$ echo 'if (Math.random() === -123){ var x=1; } console.log(x ? "FAIL" : "PASS");' | rollup --silent
if (Math.random() === -123){ var x=1; } console.log(x ? "FAIL" : "PASS");

$ echo 'if (Math.random() === -123) var x=1; console.log(x ? "FAIL" : "PASS");' | rollup --silent
console.log("FAIL" );

[kzc]

Here's a fix for the heap memory issue with no test regressions. Please try it out.

--- a/src/ast/scopes/TrackingScope.ts
+++ b/src/ast/scopes/TrackingScope.ts
@@ -1,6 +1,10 @@
 import { AstContext } from '../../Module';
+import BlockStatement from '../nodes/BlockStatement';
 import Identifier from '../nodes/Identifier';
+import VariableDeclaration from '../nodes/VariableDeclaration';
+import VariableDeclarator from '../nodes/VariableDeclarator';
 import { ExpressionEntity } from '../nodes/shared/Expression';
+import { NodeBase } from '../nodes/shared/Node';
 import LocalVariable from '../variables/LocalVariable';
 import BlockScope from './BlockScope';
 
@@ -14,6 +18,18 @@ export default class TrackingScope extends BlockScope {
                isHoisted: boolean
        ): LocalVariable {
                this.hoistedDeclarations.push(identifier);
-               return super.addDeclaration(identifier, context, init, isHoisted);
+
+               let parent, grandparent;
+               if (
+                       init instanceof NodeBase &&
+                       (parent = init.parent) instanceof VariableDeclarator &&
+                       (grandparent = parent.parent) instanceof VariableDeclaration &&
+                       grandparent.kind === 'var' &&
+                       !(grandparent.parent instanceof BlockStatement)
+               ) {
+                       return super.addDeclaration(identifier, context, init, isHoisted);
+               }
+
+               return this.parent.addDeclaration(identifier, context, init, isHoisted);
        }
 }

Latest git (ea96ab0) with patch:

$ /usr/bin/time rollup flow_parser.js --silent | shasum
        4.60 real         6.30 user         0.29 sys
8c13ffb299b68f95b9c4103ef274f52ff8d86591  -

which is comparable with rollup v2.53.1:

$ rollup -v
rollup v2.53.1

$ /usr/bin/time rollup flow_parser.js --silent | shasum
        4.61 real         6.34 user         0.29 sys
8c13ffb299b68f95b9c4103ef274f52ff8d86591  -

@lukastaegert I'll leave this with you. I wasn't able to construct a reduced test case. Perhaps you can create a more elegant fix for block-less var declarations with init expressions.

[kzc]

By the way, the flow-parser package does indeed have occurrences of block-less var declarations with init expressions. Here's the first one:

function
EE(b){if(rN.Uint8Array)var
c=new(rN.Uint8Array)(b.l);else
var
c=new
Array(b.l);var
e=b.c,d=e.length,a=0;for(;a<d;a++)c[a]=e.charCodeAt(a);for(d=b.l;a<d;a++)c[a]=0;b.c=c;b.t=4;return c}

function EE(b) {
	if (rN.Uint8Array) var c = new rN.Uint8Array(b.l);
	else var c = new Array(b.l);
	var e = b.c,
		d = e.length,
		a = 0;
	for (; a < d; a++) c[a] = e.charCodeAt(a);
	for (d = b.l; a < d; a++) c[a] = 0;
	b.c = c;
	b.t = 4;
	return c;
}

The package just happened to avoid the corner case in the test case above.

[lukastaegert]

Looking into this now

[lukastaegert]

So at the core of the problem we have that in some cases, up to 76000 variables are hoisted across a single if-statement. The nature of the tracking scope is that I "remembers" which variables are hoisted across it. The problem is that the change in question implicitly used a "double declaration" to mark those variables as "untracked". However those double declarations where also picked up by tracking scopes further out, blowing up some arrays tremendously.

I think the answer here must be to question the use of double declarations for this purpose in general, I will think about something better. I think the problem existed to some limited extent even before the version in question.

[kzc]

Although the fix in #4181 (comment) works for #4181, another large heap size increase not addressed by the patch has existed since 6260def - even with --no-treeshake. The following test case causes the rollup CLI and the rollup REPL in browsers to fail since v2.52.3 (not to be confused with v2.53.2):

switch(x){ default: var x=1;
switch(x){ default: var x=2;
switch(x){ default: var x=3;
switch(x){ default: var x=4;
switch(x){ default: var x=5;
switch(x){ default: var x=6;
switch(x){ default: var x=7;
switch(x){ default: var x=8;
switch(x){ default: var x=9;
switch(x){ default: var x=10;
switch(x){ default: var x=11;
switch(x){ default: var x=12;
switch(x){ default: var x=13;
switch(x){ default: var x=14;
switch(x){ default: var x=15;
switch(x){ default: var x=16;
switch(x){ default: var x=17;
switch(x){ default: var x=18;
switch(x){ default: var x=19;
switch(x){ default: var x=20;
switch(x){ default: var x=21;
switch(x){ default: var x=22;
switch(x){ default: var x=23;
switch(x){ default: var x=24;
switch(x){ default: var x=25;
switch(x){ default: var x=26;
switch(x){ default: var x=27;
switch(x){ default: var x=28;
switch(x){ default: var x=29;
switch(x){ default: var x=30;
}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}
console.log(x);

The flow-parser package has deeply nested blocks similar to this test case that was generated by js_of_ocaml 3.7.1 according to a comment in its source code.

[kzc]

So at the core of the problem we have that in some cases, up to 76000 variables are hoisted across a single if-statement

There's that, but there may be some O(n^n) scope complexity thing going on as evidenced by the test case above with just 30 declarations of the same var.

[lukastaegert]

I have created a tentative fix here #4183, maybe this example can serve as a good test case.

[lukastaegert]

Looks good: https://rollupjs.org/repl/?pr=4183&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMnN3aXRjaCh4KSU3QiUyMGRlZmF1bHQlM0ElMjB2YXIlMjB4JTNEMSUzQiU1Q25zd2l0Y2goeCklN0IlMjBkZWZhdWx0JTNBJTIwdmFyJTIweCUzRDIlM0IlNUNuc3dpdGNoKHgpJTdCJTIwZGVmYXVsdCUzQSUyMHZhciUyMHglM0QzJTNCJTVDbnN3aXRjaCh4KSU3QiUyMGRlZmF1bHQlM0ElMjB2YXIlMjB4JTNENCUzQiU1Q25zd2l0Y2goeCklN0IlMjBkZWZhdWx0JTNBJTIwdmFyJTIweCUzRDUlM0IlNUNuc3dpdGNoKHgpJTdCJTIwZGVmYXVsdCUzQSUyMHZhciUyMHglM0Q2JTNCJTVDbnN3aXRjaCh4KSU3QiUyMGRlZmF1bHQlM0ElMjB2YXIlMjB4JTNENyUzQiU1Q25zd2l0Y2goeCklN0IlMjBkZWZhdWx0JTNBJTIwdmFyJTIweCUzRDglM0IlNUNuc3dpdGNoKHgpJTdCJTIwZGVmYXVsdCUzQSUyMHZhciUyMHglM0Q5JTNCJTVDbnN3aXRjaCh4KSU3QiUyMGRlZmF1bHQlM0ElMjB2YXIlMjB4JTNEMTAlM0IlNUNuc3dpdGNoKHgpJTdCJTIwZGVmYXVsdCUzQSUyMHZhciUyMHglM0QxMSUzQiU1Q25zd2l0Y2goeCklN0IlMjBkZWZhdWx0JTNBJTIwdmFyJTIweCUzRDEyJTNCJTVDbnN3aXRjaCh4KSU3QiUyMGRlZmF1bHQlM0ElMjB2YXIlMjB4JTNEMTMlM0IlNUNuc3dpdGNoKHgpJTdCJTIwZGVmYXVsdCUzQSUyMHZhciUyMHglM0QxNCUzQiU1Q25zd2l0Y2goeCklN0IlMjBkZWZhdWx0JTNBJTIwdmFyJTIweCUzRDE1JTNCJTVDbnN3aXRjaCh4KSU3QiUyMGRlZmF1bHQlM0ElMjB2YXIlMjB4JTNEMTYlM0IlNUNuc3dpdGNoKHgpJTdCJTIwZGVmYXVsdCUzQSUyMHZhciUyMHglM0QxNyUzQiU1Q25zd2l0Y2goeCklN0IlMjBkZWZhdWx0JTNBJTIwdmFyJTIweCUzRDE4JTNCJTVDbnN3aXRjaCh4KSU3QiUyMGRlZmF1bHQlM0ElMjB2YXIlMjB4JTNEMTklM0IlNUNuc3dpdGNoKHgpJTdCJTIwZGVmYXVsdCUzQSUyMHZhciUyMHglM0QyMCUzQiU1Q25zd2l0Y2goeCklN0IlMjBkZWZhdWx0JTNBJTIwdmFyJTIweCUzRDIxJTNCJTVDbnN3aXRjaCh4KSU3QiUyMGRlZmF1bHQlM0ElMjB2YXIlMjB4JTNEMjIlM0IlNUNuc3dpdGNoKHgpJTdCJTIwZGVmYXVsdCUzQSUyMHZhciUyMHglM0QyMyUzQiU1Q25zd2l0Y2goeCklN0IlMjBkZWZhdWx0JTNBJTIwdmFyJTIweCUzRDI0JTNCJTVDbnN3aXRjaCh4KSU3QiUyMGRlZmF1bHQlM0ElMjB2YXIlMjB4JTNEMjUlM0IlNUNuc3dpdGNoKHgpJTdCJTIwZGVmYXVsdCUzQSUyMHZhciUyMHglM0QyNiUzQiU1Q25zd2l0Y2goeCklN0IlMjBkZWZhdWx0JTNBJTIwdmFyJTIweCUzRDI3JTNCJTVDbnN3aXRjaCh4KSU3QiUyMGRlZmF1bHQlM0ElMjB2YXIlMjB4JTNEMjglM0IlNUNuc3dpdGNoKHgpJTdCJTIwZGVmYXVsdCUzQSUyMHZhciUyMHglM0QyOSUzQiU1Q25zd2l0Y2goeCklN0IlMjBkZWZhdWx0JTNBJTIwdmFyJTIweCUzRDMwJTNCJTVDbiU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU3RCU1Q25jb25zb2xlLmxvZyh4KSUzQiUyMiUyQyUyMmlzRW50cnklMjIlM0F0cnVlJTdEJTVEJTJDJTIyb3B0aW9ucyUyMiUzQSU3QiUyMmZvcm1hdCUyMiUzQSUyMmVzJTIyJTJDJTIybmFtZSUyMiUzQSUyMm15QnVuZGxlJTIyJTJDJTIyYW1kJTIyJTNBJTdCJTIyaWQlMjIlM0ElMjIlMjIlN0QlMkMlMjJnbG9iYWxzJTIyJTNBJTdCJTdEJTdEJTJDJTIyZXhhbXBsZSUyMiUzQW51bGwlN0Q=

[lukastaegert]

@kzc Thanks a lot for the triage work and concise test case!

[lukastaegert]

So to sum up what the root of the problem was:

• whenever a variable was hoisted across a block scope, Rollup added a fake second hoisted declaration to trigger a logic that would later deoptimize this variable
• when a variable was hoisted across several block scopes, this effect was exponentiated as the fake declarations would themselves trigger additional fake declarations. The number 70000 I mentioned above was likely the result of this effect
• the changes in 2.53.2 did not cause the issue but rather amplified it by triggering this logic for a lot of additional scopes

So in hindsight, the use of fake declarations was easy but probably not the wises of choices. The fix now imperatively sets a flag to deoptimize the variable without using an additional declaration.

[kzc]

when a variable was hoisted across several block scopes, this effect was exponentiated as the fake declarations would themselves trigger additional fake declarations

fwiw, this is of no consequence, but here's an amusing test case solved by #4183. Only a single var declaration in a deeply nested scope was necessary to exhaust process memory. Perhaps it'd be useful to add as a test.

$ cat oom.js
{{{{{{{{{{{{{{{{{{{{{{{{{{{{{{
var x;
}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}
console.log(x = "PASS");

$ rollup -v
rollup v2.53.1

$ rollup oom.js --silent
...
FATAL ERROR: invalid array length Allocation failed - JavaScript heap out of memory

$ rollup -v
rollup v2.53.3

$ /usr/bin/time -l rollup oom.js --silent
console.log("PASS");
        0.14 real         0.10 user         0.01 sys
  30023680  maximum resident set size

Again, this is not important - feel free to ignore:

--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7 +7 @@
-* Solve an issue that could lead to severe memory issues and crashes when there are a lot of hoisted variables (#4183)
+* Solve severe memory issues and crashes with `var` declarations in deeply nested scopes (#4183)