Performance regression of instanceof in v7 and master

[mcollina]

• 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]

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

[mcollina]

@targos done, thanks.

[targos]

I cannot reproduce this regression in Chrome 54.

[mcollina]

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]

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]

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]

Huh… yeah, I did not see that coming.

[addaleax]

(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]

@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]

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]

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