Skip to content

Performance regression of instanceof in v7 and master #9634

Closed
Closed
Performance regression of instanceof in v7 and master#9634
Labels
performanceIssues and PRs related to the performance of Node.js.v8 engineIssues and PRs related to the V8 dependency.

Description

@mcollina
mcollina
opened on 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

Activity

  1. mcollina
    added
    v7.x
    v8 engineIssues and PRs related to the V8 dependency.
    on Nov 16, 2016

  2. targos commented on Nov 16, 2016

    @targos
    targos
    on Nov 16, 2016
    Member

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

  3. mcollina commented on Nov 16, 2016

    @mcollina
    mcollina
    on Nov 16, 2016
    SponsorMemberAuthor

    @targos done, thanks.

  4. targos commented on Nov 16, 2016

    @targos
    targos
    on Nov 16, 2016
    Member

    I cannot reproduce this regression in Chrome 54.

  5. mcollina commented on Nov 16, 2016

    @mcollina
    mcollina
    on Nov 16, 2016
    SponsorMemberAuthor

    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.

  6. bnoordhuis commented on Nov 16, 2016

    @bnoordhuis
    bnoordhuis
    on 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.

  7. mcollina commented on Nov 16, 2016

    @mcollina
    mcollina
    on Nov 16, 2016
    SponsorMemberAuthor

    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?

  8. addaleax commented on Nov 16, 2016

    @addaleax
    addaleax
    on Nov 16, 2016
    Member

    Huh… yeah, I did not see that coming.

  9. addaleax commented on Nov 16, 2016

    @addaleax
    addaleax
    on 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?

  10. mcollina commented on Nov 16, 2016

    @mcollina
    mcollina
    on Nov 16, 2016
    SponsorMemberAuthor

    @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!!!).

  11. bnoordhuis commented on Nov 16, 2016

    @bnoordhuis
    bnoordhuis
    on 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. :-)

  12. bmeurer commented on Nov 17, 2016

    @bmeurer
    bmeurer
    on Nov 17, 2016
    Member

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

  13. kisg
    added a commit that references this issue on Nov 17, 2016

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

    08377af
  14. mcollina
    changed the title [-]Performance regression in instanceof in v7 and master[/-] [+]Performance regression of instanceof in v7 and master[/+] on Nov 17, 2016

  15. 14 remaining items

  16. fhinkel commented on Nov 21, 2016

    @fhinkel
    fhinkel
    on Nov 21, 2016
    Contributor

    Should we wait until #9697 has landed?

  17. mcollina commented on Nov 21, 2016

    @mcollina
    mcollina
    on Nov 21, 2016
    SponsorMemberAuthor

    We need to get this into node v7.

  18. bnoordhuis commented on Nov 21, 2016

    @bnoordhuis
    bnoordhuis
    on 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.

  19. addaleax
    added a commit that references this issue on Nov 22, 2016

    deps: cherry-pick 08377af from v8 upstream

    9e403cf
  20. fhinkel
    added a commit that references this issue on Dec 4, 2016

    deps: cherry-pick 08377af from v8 upstream

    b5453ee
  21. fhinkel
    closed this as completedin b5453eeon Dec 4, 2016
  22. addaleax
    added a commit that references this issue on Dec 5, 2016

    deps: cherry-pick 08377af from v8 upstream

    03b1c31
  23. jmdarling
    added a commit that references this issue on Dec 8, 2016

    deps: cherry-pick 08377af from v8 upstream

    445dc8e
  24. cscott
    added a commit that references this issue on Jan 26, 2017

    Upgrade `should` to avoid performance problems in node 7.x.

    b4ce50c

  25. bengl commented on Apr 21, 2017

    @bengl
    bengl
    on 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

  26. addaleax commented on Apr 21, 2017

    @addaleax
    addaleax
    on Apr 21, 2017
    Member

    Should I open a new issue?

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

  27. hashseed commented on Apr 22, 2017

    @hashseed
    hashseed
    on 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.

  28. hashseed commented on Apr 24, 2017

    @hashseed
    hashseed
    on Apr 24, 2017
    Member

    Merged to V8 version 5.8: https://chromium.googlesource.com/v8/v8.git/+/e62656ffe685408e06d175ff5f371c8fd2556b58

  29. hashseed commented on Apr 24, 2017

    @hashseed
    hashseed
    on Apr 24, 2017
    Member

    Also see https://bugs.chromium.org/p/v8/issues/detail?id=6295

  30. mcollina commented on May 2, 2017

    @mcollina
    mcollina
    on May 2, 2017
    SponsorMemberAuthor

    @hashseed fantastic!

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    performanceIssues and PRs related to the performance of Node.js.v8 engineIssues and PRs related to the V8 dependency.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions