New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance regression of instanceof in v7 and master #9634

Closed
mcollina opened this Issue Nov 16, 2016 · 32 comments

Comments

Projects
None yet
@mcollina
Member

mcollina commented Nov 16, 2016

  • Version: v7 and master
  • Platform: Mac
  • Subsystem:

instanceof checks has become almost 100 times slower in Node v7+ (and current master)

var r = /hello/

console.time('instanceof RegExp')
for (var i = 0; i < 100000000; i++) {
  r instanceof RegExp
}
console.timeEnd('instanceof RegExp')

var o = {}

console.time('instanceof Object')
for (var i = 0; i < 100000000; i++) {
  o instanceof Object
}
console.timeEnd('instanceof Object')

In node v6:

$ node instanceof.js
instanceof RegExp: 133.519ms
instanceof Object: 134.572ms

In node v7:

$ node instanceof.js
instanceof RegExp: 9858.616ms
instanceof Object: 9839.696ms

I know this is a problem in V8, but I think it's good to track it here as well.

v8 issue: https://bugs.chromium.org/p/v8/issues/detail?id=5640

cc @fhinkel

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Nov 16, 2016

Member

Could you also create a V8 bug and link to it here ?

Member

targos commented Nov 16, 2016

Could you also create a V8 bug and link to it here ?

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Nov 16, 2016

Member

@targos done, thanks.

Member

mcollina commented Nov 16, 2016

@targos done, thanks.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Nov 16, 2016

Member

I cannot reproduce this regression in Chrome 54.

Member

targos commented Nov 16, 2016

I cannot reproduce this regression in Chrome 54.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Nov 16, 2016

Member

Now that I test it... neither can I, and in theory they use the same v8 version.
Maybe there is a patch to backport to the v8 version we are using.

Member

mcollina commented Nov 16, 2016

Now that I test it... neither can I, and in theory they use the same v8 version.
Maybe there is a patch to backport to the v8 version we are using.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 16, 2016

Member

Found the cause: commit 2a4b068 from last September, cc @addaleax.

Chrome 54 ships the exact same version as node.js master, 5.4.500.41, and I can confirm that it's fast in Chrome and d8 (./configure --enable-d8) but not in Node.js.

I checked with perf(1) and it showed that V8 was spending a phenomenal amount of time in v8::internal::Object::InstanceOf(), more specifically the code path that checks for the presence of a @@hasInstance method. Here is a minimal test case:

function F() {}
const hasInstance = Function.prototype[Symbol.hasInstance];
Object.defineProperty(F, Symbol.hasInstance, { value: hasInstance });
for (var o = {}, i = 0; i < 1e8; i++) o instanceof Object;

Basically, if V8 sees that @@hasInstance is used, it starts emitting conservative slow path code.

Member

bnoordhuis commented Nov 16, 2016

Found the cause: commit 2a4b068 from last September, cc @addaleax.

Chrome 54 ships the exact same version as node.js master, 5.4.500.41, and I can confirm that it's fast in Chrome and d8 (./configure --enable-d8) but not in Node.js.

I checked with perf(1) and it showed that V8 was spending a phenomenal amount of time in v8::internal::Object::InstanceOf(), more specifically the code path that checks for the presence of a @@hasInstance method. Here is a minimal test case:

function F() {}
const hasInstance = Function.prototype[Symbol.hasInstance];
Object.defineProperty(F, Symbol.hasInstance, { value: hasInstance });
for (var o = {}, i = 0; i < 1e8; i++) o instanceof Object;

Basically, if V8 sees that @@hasInstance is used, it starts emitting conservative slow path code.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Nov 16, 2016

Member

Oh no! :(

We have some possible solutions:

a. revert that fix, but probably it's semver-major. If we could do it semver-minor it will be the best solution.
b. wait until there is a patch from upstream V8, and apply that.

It would be nice to a quick post-mortem on this one. How can we spot those before releasing?

Member

mcollina commented Nov 16, 2016

Oh no! :(

We have some possible solutions:

a. revert that fix, but probably it's semver-major. If we could do it semver-minor it will be the best solution.
b. wait until there is a patch from upstream V8, and apply that.

It would be nice to a quick post-mortem on this one. How can we spot those before releasing?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 16, 2016

Member

Huh… yeah, I did not see that coming.

Member

addaleax commented Nov 16, 2016

Huh… yeah, I did not see that coming.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 16, 2016

Member

(sorry, mis-clicked and posted the comment too early)

How can we spot those before releasing?

  • Does V8 have benchmarks for this kind of thing? Is running them inside Node helpful?
  • Would people from the V8 team have spotted that this is problematic?
  • @mcollina I’m curious – how did you spot this?
Member

addaleax commented Nov 16, 2016

(sorry, mis-clicked and posted the comment too early)

How can we spot those before releasing?

  • Does V8 have benchmarks for this kind of thing? Is running them inside Node helpful?
  • Would people from the V8 team have spotted that this is problematic?
  • @mcollina I’m curious – how did you spot this?
@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Nov 16, 2016

Member

@mcollina I’m curious – how did you spot this?

I found out that https://github.com/mcollina/bloomrun was 10 times slower in v7 rather than v6. It boiled down to instanceof, I did not do a git bisect on core, and I should have (that's awesome @bnoordhuis!!!).

Member

mcollina commented Nov 16, 2016

@mcollina I’m curious – how did you spot this?

I found out that https://github.com/mcollina/bloomrun was 10 times slower in v7 rather than v6. It boiled down to instanceof, I did not do a git bisect on core, and I should have (that's awesome @bnoordhuis!!!).

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 16, 2016

Member

Truthfully, I didn't bisect. I had a pretty good idea from looking at the perf data where the problem came from and reverting the commit confirmed that. :-)

Member

bnoordhuis commented Nov 16, 2016

Truthfully, I didn't bisect. I had a pretty good idea from looking at the perf data where the problem came from and reverting the commit confirmed that. :-)

@bmeurer

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Nov 17, 2016

Member

I think I can come up with a small(ish) mitigation for the problem. Let me have a look.

Member

bmeurer commented Nov 17, 2016

I think I can come up with a small(ish) mitigation for the problem. Let me have a look.

kisg pushed a commit to paul99/v8mips that referenced this issue Nov 17, 2016

[crankshaft] No need to rely on the @@hasInstance protector.
In Crankshaft we can actually do an abstract interpretation of the
@@hasInstance lookup when optimizing instanceof and then use the
normal machinery to protect the result instead of relying on the
global @@hasInstance protector cell for optimizations.

This recovers the 100x performance drop in Node.js v7 reported in
nodejs/node#9634. This patch should be
easily back-mergable to Node.js v7.

BUG=v8:5640
R=yangguo@chromium.org,franzih@chromium.org

Review-Url: https://codereview.chromium.org/2504263004
Cr-Commit-Position: refs/heads/master@{#41059}

@mcollina mcollina changed the title from Performance regression in instanceof in v7 and master to Performance regression of instanceof in v7 and master Nov 17, 2016

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Nov 17, 2016

Member

@bmeurer I can confirm the patch works as expected, and it's fairly straightforward to apply.

Member

mcollina commented Nov 17, 2016

@bmeurer I can confirm the patch works as expected, and it's fairly straightforward to apply.

@bmeurer

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Nov 17, 2016

Member

@mcollina Awesome. I'm currently fixing the issue in TurboFan as well. The patch is slightly more involved there for a couple of reasons. But you should be fine with the Crankshaft fix for 99% of the cases.

Member

bmeurer commented Nov 17, 2016

@mcollina Awesome. I'm currently fixing the issue in TurboFan as well. The patch is slightly more involved there for a couple of reasons. But you should be fine with the Crankshaft fix for 99% of the cases.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Nov 17, 2016

Member

I'm currently fixing the issue in TurboFan as well.

About that, I wonder: is TurboFan enabled by default in V8? If so, how do we know what parts of it are shipped without a flag? I'm asking because I think I never saw a [compiling method ... using TurboFan] message without the --turbo flag and things are usually slower with the flag enabled.

Member

targos commented Nov 17, 2016

I'm currently fixing the issue in TurboFan as well.

About that, I wonder: is TurboFan enabled by default in V8? If so, how do we know what parts of it are shipped without a flag? I'm asking because I think I never saw a [compiling method ... using TurboFan] message without the --turbo flag and things are usually slower with the flag enabled.

@bmeurer

This comment has been minimized.

Show comment
Hide comment
@bmeurer

bmeurer Nov 17, 2016

Member

It is for language features that Crankshaft cannot deal with, i.e. try-catch, try-finally, for-of, etc.

Member

bmeurer commented Nov 17, 2016

It is for language features that Crankshaft cannot deal with, i.e. try-catch, try-finally, for-of, etc.

@ChALkeR ChALkeR added the performance label Nov 17, 2016

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Nov 17, 2016

Member

Would this have been caught by an octane run ? If so it might make sense to set that up as one of our nightly benchmarks. Now that we have our own v8 branch were all of the changes applied I'm guessing it should re relatively easy to do that.

Member

mhdawson commented Nov 17, 2016

Would this have been caught by an octane run ? If so it might make sense to set that up as one of our nightly benchmarks. Now that we have our own v8 branch were all of the changes applied I'm guessing it should re relatively easy to do that.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 17, 2016

Member

I did a quick check with the EarleyBoyer benchmark, it has a lot of instanceof checks. Before/after numbers when run in node (not d8):

EarleyBoyer: 5219  # master
EarleyBoyer: 24491  # with 2a4b068 reverted

So an unequivocal 'yes' in this particular case. :-)

Member

bnoordhuis commented Nov 17, 2016

I did a quick check with the EarleyBoyer benchmark, it has a lot of instanceof checks. Before/after numbers when run in node (not d8):

EarleyBoyer: 5219  # master
EarleyBoyer: 24491  # with 2a4b068 reverted

So an unequivocal 'yes' in this particular case. :-)

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Nov 17, 2016

Member

Will look at getting octane added as part of our nightly benchmarks in nodejs/benchmarking#75.

Member

mhdawson commented Nov 17, 2016

Will look at getting octane added as part of our nightly benchmarks in nodejs/benchmarking#75.

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Nov 17, 2016

Member

Good idea!

Member

mcollina commented Nov 17, 2016

Good idea!

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Nov 18, 2016

Member

The issue is marked as fixed in v8. Can we backport the fix here? What is the process for v8 patches and backports?

Member

mcollina commented Nov 18, 2016

The issue is marked as fixed in v8. Can we backport the fix here? What is the process for v8 patches and backports?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Nov 18, 2016

Member

@fhinkel any chance for it to be backported to V8 5.4 or 5.5 ?

Member

targos commented Nov 18, 2016

@fhinkel any chance for it to be backported to V8 5.4 or 5.5 ?

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Nov 18, 2016

Member

@natorion Can we backport performance improvements to 5.4 or only bug fixes?

If we can't backport it to V8, we can always cherry-pick the commits to Node. We probably want to wait a few days though, in case there are any issues with the commits.

Member

fhinkel commented Nov 18, 2016

@natorion Can we backport performance improvements to 5.4 or only bug fixes?

If we can't backport it to V8, we can always cherry-pick the commits to Node. We probably want to wait a few days though, in case there are any issues with the commits.

@natorion

This comment has been minimized.

Show comment
Hide comment
@natorion

natorion Nov 21, 2016

For 5.4 it is probably to late. For 5.5 it should be fine (the CS part) as this is still in beta. Need to check though. What is the benefit for Node if this is merged to 5.5 though?

BTW, this also needs to be fixed on 5.6.

natorion commented Nov 21, 2016

For 5.4 it is probably to late. For 5.5 it should be fine (the CS part) as this is still in beta. Need to check though. What is the benefit for Node if this is merged to 5.5 though?

BTW, this also needs to be fixed on 5.6.

@fhinkel

This comment has been minimized.

Show comment
Hide comment
@fhinkel

fhinkel Nov 21, 2016

Member

Should we wait until #9697 has landed?

Member

fhinkel commented Nov 21, 2016

Should we wait until #9697 has landed?

@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina Nov 21, 2016

Member

We need to get this into node v7.

Member

mcollina commented Nov 21, 2016

We need to get this into node v7.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 21, 2016

Member

We will probably upgrade to V8 5.5 during the v7 release cycle if we can work out the kinks, see #9618.

Member

bnoordhuis commented Nov 21, 2016

We will probably upgrade to V8 5.5 during the v7 release cycle if we can work out the kinks, see #9618.

@addaleax addaleax referenced this issue Nov 22, 2016

Closed

deps: update V8 to 5.4.500.43 #9697

2 of 2 tasks complete

addaleax added a commit to addaleax/node that referenced this issue Nov 22, 2016

deps: cherry-pick 08377af from v8 upstream
Orignial commit message:

[crankshaft] No need to rely on the @@hasInstance protector.

In Crankshaft we can actually do an abstract interpretation of the
@@hasInstance lookup when optimizing instanceof and then use the
normal machinery to protect the result instead of relying on the
global @@hasInstance protector cell for optimizations.

This recovers the 100x performance drop in Node.js v7 reported in
nodejs#9634. This patch should be
easily back-mergable to Node.js v7.

BUG=v8:5640
R=yangguo@chromium.org,franzih@chromium.org

Committed: https://crrev.com/08377af957b1602396ebf3e11ae000f15ed09333
Cr-Commit-Position: refs/heads/master@{#41059}

Fixes: nodejs#9634

@addaleax addaleax referenced this issue Nov 22, 2016

Closed

deps: cherry-pick 08377af from v8 upstream #9730

3 of 3 tasks complete

@fhinkel fhinkel closed this in b5453ee Dec 4, 2016

addaleax added a commit that referenced this issue Dec 5, 2016

deps: cherry-pick 08377af from v8 upstream
Orignial commit message:

[crankshaft] No need to rely on the @@hasInstance protector.

In Crankshaft we can actually do an abstract interpretation of the
@@hasInstance lookup when optimizing instanceof and then use the
normal machinery to protect the result instead of relying on the
global @@hasInstance protector cell for optimizations.

This recovers the 100x performance drop in Node.js v7 reported in
#9634. This patch should be
easily back-mergable to Node.js v7.

BUG=v8:5640
R=yangguo@chromium.org,franzih@chromium.org

Committed: https://crrev.com/08377af957b1602396ebf3e11ae000f15ed09333
Cr-Commit-Position: refs/heads/master@{#41059}

PR-URL: #9730
Fixes: #9634
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jmdarling added a commit to jmdarling/node that referenced this issue Dec 8, 2016

deps: cherry-pick 08377af from v8 upstream
Orignial commit message:

[crankshaft] No need to rely on the @@hasInstance protector.

In Crankshaft we can actually do an abstract interpretation of the
@@hasInstance lookup when optimizing instanceof and then use the
normal machinery to protect the result instead of relying on the
global @@hasInstance protector cell for optimizations.

This recovers the 100x performance drop in Node.js v7 reported in
nodejs#9634. This patch should be
easily back-mergable to Node.js v7.

BUG=v8:5640
R=yangguo@chromium.org,franzih@chromium.org

Committed: https://crrev.com/08377af957b1602396ebf3e11ae000f15ed09333
Cr-Commit-Position: refs/heads/master@{#41059}

PR-URL: nodejs#9730
Fixes: nodejs#9634
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

cscott added a commit to cscott/domino that referenced this issue Jan 26, 2017

Upgrade `should` to avoid performance problems in node 7.x.
Node 7.x contains a performance regression which makes certain
`instanceof` tests 100x slower:
  nodejs/node#9634

Upgrading `should` from 8.3.0 to 11.1.2 makes this performance
problem go away when running our tests, for reasons I don't
fully understand, but it seems to be related to:
  shouldjs/should.js@0737171
so perhaps the root of the performance problem is actually
`should.not`, not the `instanceof` regression.
@bengl

This comment has been minimized.

Show comment
Hide comment
@bengl

bengl Apr 21, 2017

Member

It looks like this might have regressed back to a 50(ish)x slowdown for RegExp on the Node 8 nightlies, and for non-builtins (which I added a test for in @mcollina's original example code), it seems to be a 10(ish)x slowdown on both Node 7 and Node 8 nightlies. (Should I open a new issue?)

System: Linux 4.9.11 x86_64

$ cat instanceof.js 
var r = /hello/

console.time('instanceof RegExp')
for (var i = 0; i < 100000000; i++) {
  r instanceof RegExp
}
console.timeEnd('instanceof RegExp')

var o = {}

console.time('instanceof Object')
for (var i = 0; i < 100000000; i++) {
  o instanceof Object
}
console.timeEnd('instanceof Object')

function F () {}
var f = new F

console.time('instanceof F')
for (var i = 0; i < 100000000; i++) {
  f instanceof F
}
console.timeEnd('instanceof F')
$ ~/.nvm/versions/node/v6.10.2/bin/node instanceof.js 
instanceof RegExp: 127.840ms
instanceof Object: 129.514ms
instanceof F: 635.539ms
$ ~/.nvm/versions/node/v7.9.0/bin/node instanceof.js 
instanceof RegExp: 163.946ms
instanceof Object: 120.818ms
instanceof F: 6786.285ms
$ ~/.nvm/versions/node/v8.0.0-nightly2017042158066d16d5/bin/node instanceof.js 
instanceof RegExp: 7501.615ms
instanceof Object: 121.429ms
instanceof F: 7219.541ms
Member

bengl commented Apr 21, 2017

It looks like this might have regressed back to a 50(ish)x slowdown for RegExp on the Node 8 nightlies, and for non-builtins (which I added a test for in @mcollina's original example code), it seems to be a 10(ish)x slowdown on both Node 7 and Node 8 nightlies. (Should I open a new issue?)

System: Linux 4.9.11 x86_64

$ cat instanceof.js 
var r = /hello/

console.time('instanceof RegExp')
for (var i = 0; i < 100000000; i++) {
  r instanceof RegExp
}
console.timeEnd('instanceof RegExp')

var o = {}

console.time('instanceof Object')
for (var i = 0; i < 100000000; i++) {
  o instanceof Object
}
console.timeEnd('instanceof Object')

function F () {}
var f = new F

console.time('instanceof F')
for (var i = 0; i < 100000000; i++) {
  f instanceof F
}
console.timeEnd('instanceof F')
$ ~/.nvm/versions/node/v6.10.2/bin/node instanceof.js 
instanceof RegExp: 127.840ms
instanceof Object: 129.514ms
instanceof F: 635.539ms
$ ~/.nvm/versions/node/v7.9.0/bin/node instanceof.js 
instanceof RegExp: 163.946ms
instanceof Object: 120.818ms
instanceof F: 6786.285ms
$ ~/.nvm/versions/node/v8.0.0-nightly2017042158066d16d5/bin/node instanceof.js 
instanceof RegExp: 7501.615ms
instanceof Object: 121.429ms
instanceof F: 7219.541ms
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Apr 21, 2017

Member

Should I open a new issue?

@bengl Yes, that seems like a good idea (and ping nodejs/v8 in it:))

Member

addaleax commented Apr 21, 2017

Should I open a new issue?

@bengl Yes, that seems like a good idea (and ping nodejs/v8 in it:))

@hashseed

This comment has been minimized.

Show comment
Hide comment
@hashseed

hashseed Apr 22, 2017

Member

This is https://bugs.chromium.org/p/v8/issues/detail?id=5902

I'll merge this on Monday to 5.8.

Member

hashseed commented Apr 22, 2017

This is https://bugs.chromium.org/p/v8/issues/detail?id=5902

I'll merge this on Monday to 5.8.

@hashseed

This comment has been minimized.

Show comment
Hide comment
@hashseed

This comment has been minimized.

Show comment
Hide comment
@mcollina

This comment has been minimized.

Show comment
Hide comment
@mcollina

mcollina May 2, 2017

Member

@hashseed fantastic!

Member

mcollina commented May 2, 2017

@hashseed fantastic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment