significant performance degradation with v7.x

[alexlamsl]

• Version: v7.7.3
• Platform: Windows 10 x64

mishoo/UglifyJS#1624 (comment)
mishoo/UglifyJS#1624 (comment)

Steps to reproduce:

git clone https://github.com/mishoo/UglifyJS2.git
cd UglifyJS2
npm install
node test/benchmark.js -mc warnings=false,passes=3

Timings for math.js

Version	Duration
v0.10.48	20.907 s
v0.12.18	21.407 s
v4.8.0	21.907 s
v6.10.0	26.047 s
v7.7.3	46.203 s

[mscdex]

Just briefly looking over the code and judging by the profiler output, it looks like the extensive use of instanceof has one of the highest tick counts, which can be a perf killer in hot paths. There are also a bunch of miscellaneous perf hits I see throughout the code base that may also contribute to bad performance in general (or at least in V8).

I also see a lot of deopts happening when running the benchmarks.

[kzc]

More likely that V8 in Node 7 does not handle deep recursion as well as past versions.

Repro:

npm install uglify-js@2.8.14

wget https://cdnjs.cloudflare.com/ajax/libs/mathjs/3.9.0/math.js

$ /usr/bin/time node773 node_modules/.bin/uglifyjs math.js -mc warnings=0 | shasum
       22.12 real        22.60 user         0.13 sys
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

$ /usr/bin/time node690 node_modules/.bin/uglifyjs math.js -mc warnings=0 | shasum
       12.69 real        13.22 user         0.16 sys
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

$ /usr/bin/time node591 node_modules/.bin/uglifyjs math.js -mc warnings=0 | shasum
       10.21 real        10.48 user         0.14 sys
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

$ /usr/bin/time node421 node_modules/.bin/uglifyjs math.js -mc warnings=0 | shasum
       10.71 real        11.01 user         0.14 sys
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

$ /usr/bin/time node0_12_9 node_modules/.bin/uglifyjs math.js -mc warnings=0 | shasum
        9.92 real        10.40 user         0.15 sys
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

$ /usr/bin/time node0_10_41 node_modules/.bin/uglifyjs math.js -mc warnings=0 | shasum
       10.50 real        10.36 user         0.14 sys
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

[billouboq]

I can see performance regression with jshttp/etag benchmark too : https://github.com/jshttp/etag

Node 7.7.3 : ~380k req/s
Node 6.10 : ~440k req/s
Node 4.8.0 : ~450k req/s
Node 0.12.18 : ~360k req/s
Node 0.10.48 : ~430k req/s

[kzc]

TurboFan helps somewhat:

$ /usr/bin/time node773 --turbo node_modules/.bin/uglifyjs math.js -mc warnings=0 | shasum
       14.41 real        16.36 user         0.15 sys
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

but still significantly slower than Node 0.10.x, 0.12.x, 4.x, 5.x

[ChALkeR]

/cc @nodejs/v8

[bnoordhuis]

I also see a lot of deopts happening when running the benchmarks.

I see that too. It's written in a dispatch-y style that doesn't cater to V8's strengths.

I think the difference between versions can be explained by V8 implementing ES6 semantics more closely. A lot of CPU time is spent in instanceof checks, method invocation and property lookups; those all became more expensive with the new semantics. I don't see anything we (node.js) have direct control over.

[mikeal]

Can someone advise whether these are temporary performance degradations while ES6 support is added or whether these are more or less permanent degradations to performance due to new features being added to the language?

This is the kind of feedback we need to push into the standards process. If permanent, more of these kinds of changes should be pushed back against.

[kzc]

uglify-js is a widely used module downloaded millions of times per week. Its code style has not changed for years. Its use of instanceof is perfectly valid javascript. It ran fine with all versions of Node up to and including 4.x. Node 6.x shows a 20% decrease in performance, and Node 7.x shows a 2X decrease in performance (1.4X decrease in performance with --turbo).

Can Google V8 maintainers take a look at this and suggest v8 options that bring performance back to the Node 4.x level? Uglify is 100% CPU bound and would benefit from unconditionally generating machine code at startup and never deoptimizing as v8 once did.

[jasnell]

@mscdex ... is it just me or has instanceof performance downgraded significantly in V8 recently? It seems to be much slower now for some reason

[bmeurer]

That seems interesting. instanceof did indeed get a serious performance drop with ES2015, due to the addition of Symbol.hasInstance and the (observable) lookup of it. This invalidated quite a couple of V8's optimizations for instanceof.

[fhinkel]

Thanks for the benchmarks! instanceof uses the @@hasInstance symbol since ES2015, so we had to change the implementation 😞 .

Can you check the performance with Node.js master or even better, with v8 master? Here's a today's binary (works on ubuntu or build yourself).

[fhinkel]

Ah, @bmeurer beat me to it 😄 . Is the optimization still relying on nothing setting the symbol?

[bmeurer]

@fhinkel No that optimization was invalid, thanks to ES2015 proxies... ES2015 made it really challenging to have fast general case instanceof. :-(

[bnoordhuis]

@bmeurer We float v8/v8@08377af to mitigate #9634. Should we revert that?

(EDIT: Or perhaps I misunderstood your comment?)

[bmeurer]

@bnoordhuis That would make it even worse.

[bmeurer]

Not exactly sure what's up with Node v7, but current Node LKGR (Mar 20th) with Ignition+TurboFan definitely beats v4, 11.825s vs. 12.553s. Even w/ CS+FCG via --noturbo, we still get pretty decent performance.

$ time ~/Applications/node-v4.2.4-linux-x64/bin/node bin/uglifyjs math.js -mc warnings=0 | shasum
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

real    0m12.553s
user    0m14.068s
sys     0m0.300s
$ time ~/Applications/node-v6.10.0-linux-x64/bin/node bin/uglifyjs math.js -mc warnings=0 | shasum
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

real    0m14.264s
user    0m14.486s
sys     0m0.627s
$ time ~/Applications/node-v7.7.3-linux-x64/bin/node bin/uglifyjs math.js -mc warnings=0 | shasum
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

real    0m25.617s
user    0m25.824s
sys     0m0.513s
$ time ~/Applications/node-vee-eight-lkgr/bin/node --noturbo bin/uglifyjs math.js -mc warnings=0 | shasum
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

real    0m14.979s
user    0m15.729s
sys     0m0.376s
$ time ~/Applications/node-vee-eight-lkgr/bin/node bin/uglifyjs math.js -mc warnings=0 | shasum
e2c6d55b77068d607b0d6d4abb7884cf766cc09a  -

real    0m11.825s
user    0m13.224s
sys     0m0.350s

[bmeurer]

@bnoordhuis Oh, I think it might be because Node v7 is missing crbug.com/2511223003 and the follow-up fixes for instanceof in TurboFan. @fhinkel's backmerge was only about the Crankshaft side of things.

[bmeurer]

@bnoordhuis @fhinkel ...looking at Node v7 with --print-opt-code makes it obvious: There are various functions with heavy use of instanceof being sent to TurboFan w/o having the appropriate fix for the Symbol.hasInstance protector invalidation in Node.

[fhinkel]

Yes, we had discussed that the turbofan commit was not a good candidate to be cherry-picked.

[bnoordhuis]

On the bright side, node 8.0 ships with V8 5.7 and will have the fix.

[Fishrock123]

If this is just for v7.x, at this point in the cycle I think it is just best to swallow it if the backport seems iffy to do.

[bnoordhuis]

I agree. I'll close this out.

[mhart]

This benchmark seems like it might be a good candidate for https://benchmarking.nodejs.org/

[mhdawson]

@mhart can you open an issue in nodejs/benchmarking to suggest that ?