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

addaleax · 2016-11-22T04:41:08Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

deps/v8

Description of change

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
https://github.com/nodejs/node/issues/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: #9634

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@{nodejs#41059} Fixes: nodejs#9634

addaleax · 2016-11-22T04:47:05Z

CI: https://ci.nodejs.org/job/node-test-commit/6110/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/430/

I wouldn’t mind fast-tracking this if that means it gets into an earlier release.

/cc @fhinkel @mcollina

MylesBorins · 2016-11-22T05:19:27Z

/cc @nodejs/v8 This is being manually backported as it sounded like this wouldn't make it into the official 5.4 release

How do we want to handle these cases?

mcollina · 2016-11-22T09:20:42Z

IMHO we should backport it to v7. I'm all in for fast tracking, but I do not know what is the process.

fhinkel · 2016-11-22T09:24:04Z

Let's wait for @natorion or @ofrobots input.

@addaleax Thanks for opening the PR for me.

bnoordhuis · 2016-11-22T10:45:46Z

The 5.4 release branch is still maintained as far as I'm aware so that means we get a version number conflict if we land this and then upstream does a new 5.4 release. I see three options:

Lobby upstream harder to back-port.
Wait until V8 5.5 (which will probably land in v7.)
Append a suffix so our version number stays unique (e.g. 5.4.500.43 -> 5.4.500.43.1)

mcollina · 2016-11-22T11:36:22Z

When is due 5.5? How probable is another v5.4 release before 5.4? If it's v5.5 is close, it's probably easier to wait. If not, we'll have to backport and add our own version number.

We'll also need to commit the full patch somewhere, in order to reapply it when/if there is another v5.4 release.

natorion · 2016-11-22T12:40:41Z

This will not land in official 5.4. There are also no plans to land this in 5.5 because:

The TurboFan part of the backport would be quite involved
Node uses 5.4
-- The adoption of 5.5 is news to me, how likely is it?

Merging the CrankShaft part only to 5.5 would be an option though if done in the next few days.

bnoordhuis · 2016-11-22T13:36:27Z

@natorion #9618 is the very-much-in-progress work.

addaleax · 2016-11-22T20:57:02Z

Append a suffix so our version number stays unique (e.g. 5.4.500.43 -> 5.4.500.43.1)

Is there any precedent for this so I could add something based on that to the PR here?

bnoordhuis · 2016-11-22T21:16:40Z

No precedence that I'm aware of. You would have to patch (in deps/v8), include/v8-version.h, src/version.cc and src/log-utils.cc (which would be a version bump-worthy change in its own right.)

ofrobots · 2016-11-22T23:28:03Z

@natorion: I have requested a merge to 5.5 in the upstream bug.

Append a suffix so our version number stays unique (e.g. 5.4.500.43 -> 5.4.500.43.1)

Yeah, let's try this workflow out. Even if in this case there are alternatives, we may run into a similar situation in the future with a supported branch where we need a fix more urgently. It would be good to have this workflow in place and tested.

targos · 2016-11-23T09:55:15Z

Append a suffix so our version number stays unique (e.g. 5.4.500.43 -> 5.4.500.43.1)

I created #9754 so we can discuss it separately.

fhinkel · 2016-12-01T10:20:34Z

5.4 is not maintained anymore. So no risk of running into version conflicts. Can we go ahead an merge this?
@addaleax Can you update the patch number since you started the PR from your repo?

addaleax · 2016-12-01T13:42:54Z

@fhinkel I can do that later (a bit busy @ NINA), but if you want, feel free to use the “Allow edits from maintainers” bit and push to this PR yourself :)

fhinkel · 2016-12-04T14:15:36Z

Can I get an LGTM here?

mcollina · 2016-12-04T14:27:31Z

LGTM

fhinkel · 2016-12-04T14:42:12Z

Landed in b5453ee.

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>

addaleax · 2016-12-04T22:24:01Z

@fhinkel thanks for taking care of this!

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>

Notable changes: * buffer: - Reverted the runtime deprecation of calling `Buffer()` without `new`. (Anna Henningsen) #9529 - Fixed `buffer.transcode()` for single-byte character encodings to `UCS2`. (Anna Henningsen) #9838 * promise: `--trace-warnings` now produces useful stacktraces for Promise warnings. (Anna Henningsen) #9525 * repl: Fixed a bug preventing correct parsing of generator functions. (Teddy Katz) #9852 * V8: Fixed a significant `instanceof` performance regression. (Franziska Hinkelmann) #9730 PR-URL: #10127

Sometimes upstream V8 may not want to merge a fix to their stable branches, but we might. This adds a new version string that the embedder can set independently of the official V8 version to avoid running into conflicts. Refs: nodejs#9730

Notable changes: * buffer: - Reverted the runtime deprecation of calling `Buffer()` without `new`. (Anna Henningsen) nodejs/node#9529 - Fixed `buffer.transcode()` for single-byte character encodings to `UCS2`. (Anna Henningsen) nodejs/node#9838 * promise: `--trace-warnings` now produces useful stacktraces for Promise warnings. (Anna Henningsen) nodejs/node#9525 * repl: Fixed a bug preventing correct parsing of generator functions. (Teddy Katz) nodejs/node#9852 * V8: Fixed a significant `instanceof` performance regression. (Franziska Hinkelmann) nodejs/node#9730 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

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@{nodejs#41059} PR-URL: nodejs#9730 Fixes: nodejs#9634 Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

simov · 2016-12-16T08:46:52Z

Hey,

This might as well be an offtopic but this fix in V8 (v7.2.1) significantly improves a three years and half old code.

There is a project called benchmark-node-clone that tests various deep clone utilities in Node:

7.2.0

  14 tests completed.

  clone            x 13,138 ops/sec ±0.52% (89 runs sampled)
  clone-deep       x 12,453 ops/sec ±1.54% (88 runs sampled)
  clone-extend     x 10,224 ops/sec ±0.67% (93 runs sampled)
  cloneextend      x 20,309 ops/sec ±0.90% (92 runs sampled)
  component-clone  x 32,984 ops/sec ±0.65% (91 runs sampled)
-  deep-copy        x 20,239 ops/sec ±0.57% (93 runs sampled)
  deepclone        x 11,798 ops/sec ±0.81% (90 runs sampled)
  deepcopy         x 13,512 ops/sec ±0.70% (94 runs sampled)
  extend           x 30,427 ops/sec ±0.78% (94 runs sampled)
  fast-clone       x 15,082 ops/sec ±0.90% (91 runs sampled)
  lodash.cloneDeep x 16,431 ops/sec ±0.97% (90 runs sampled)
  stringify-clone  x 30,513 ops/sec ±0.71% (91 runs sampled)
  structured-clone x 18,772 ops/sec ±0.57% (93 runs sampled)
  utils-copy       x  8,468 ops/sec ±0.69% (94 runs sampled)

Fastest is component-clone
Slowest is utils-copy

7.2.1

14 tests completed.

  clone            x  12,612 ops/sec ±0.63% (87 runs sampled)
  clone-deep       x  24,516 ops/sec ±2.16% (84 runs sampled)
  clone-extend     x   9,766 ops/sec ±0.32% (89 runs sampled)
  cloneextend      x  20,939 ops/sec ±0.52% (91 runs sampled)
  component-clone  x  31,800 ops/sec ±0.85% (91 runs sampled)
+  deep-copy        x 113,871 ops/sec ±0.89% (91 runs sampled)
  deepclone        x  13,853 ops/sec ±1.54% (90 runs sampled)
  deepcopy         x  13,169 ops/sec ±0.88% (90 runs sampled)
  extend           x  31,117 ops/sec ±0.83% (93 runs sampled)
  fast-clone       x  19,035 ops/sec ±0.72% (90 runs sampled)
  lodash.cloneDeep x  16,213 ops/sec ±1.01% (90 runs sampled)
  stringify-clone  x  30,466 ops/sec ±0.92% (90 runs sampled)
  structured-clone x  19,776 ops/sec ±0.64% (93 runs sampled)
  utils-copy       x   7,890 ops/sec ±0.81% (91 runs sampled)

Fastest is deep-copy
Slowest is utils-copy

My module is called deep-copy and I haven't touched it since April 2013, only style changes since then.

6.9.2 - deep-copy        x 115,111 ops/sec ±0.75% (90 runs sampled)
6.9.1 - deep-copy        x 117,174 ops/sec ±0.76% (89 runs sampled)
6.3.1 - deep-copy        x 71,263 ops/sec ±0.67% (92 runs sampled)
6.3.0 - deep-copy        x 52,645 ops/sec ±1.59% (61 runs sampled)
4.7.0 - deep-copy        x 14,604 ops/sec ±1.81% (63 runs sampled)

As you can see the performance boost is quite dramatic and it is not entirely related to the performance regression in v7, so my thinking is that this fixes something that existed in js for a very long time.

fhinkel · 2016-12-16T11:38:49Z

Cool, thanks!

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Nov 22, 2016

addaleax added dont-land-on-v4.x labels Nov 22, 2016

targos mentioned this pull request Nov 23, 2016

deps: introduce embedder version number for V8 #9754

Closed

3 tasks

fhinkel closed this Dec 4, 2016

addaleax deleted the fix-9634 branch December 4, 2016 22:23

Fishrock123 mentioned this pull request Dec 5, 2016

v7.2.1 proposal #10127

Merged

2 tasks

addaleax mentioned this pull request Jan 17, 2017

Optimize event handling with large arguments #9643

Closed

mcollina mentioned this pull request Jan 27, 2017

v7.x: backport update to V8 5.5 #11029

Merged

2 tasks

shiftkey mentioned this pull request Mar 29, 2017

update tooling to node v7 and npm v4 desktop/desktop#1085

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

addaleax commented Nov 22, 2016

addaleax commented Nov 22, 2016

MylesBorins commented Nov 22, 2016

mcollina commented Nov 22, 2016

fhinkel commented Nov 22, 2016

bnoordhuis commented Nov 22, 2016

mcollina commented Nov 22, 2016

natorion commented Nov 22, 2016

bnoordhuis commented Nov 22, 2016

addaleax commented Nov 22, 2016

bnoordhuis commented Nov 22, 2016

ofrobots commented Nov 22, 2016

targos commented Nov 23, 2016

fhinkel commented Dec 1, 2016 •

edited

Loading

addaleax commented Dec 1, 2016

fhinkel commented Dec 4, 2016

mcollina commented Dec 4, 2016

fhinkel commented Dec 4, 2016 •

edited

Loading

addaleax commented Dec 4, 2016

simov commented Dec 16, 2016 •

edited

Loading

fhinkel commented Dec 16, 2016

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

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

Conversation

addaleax commented Nov 22, 2016

Checklist

Affected core subsystem(s)

Description of change

addaleax commented Nov 22, 2016

MylesBorins commented Nov 22, 2016

mcollina commented Nov 22, 2016

fhinkel commented Nov 22, 2016

bnoordhuis commented Nov 22, 2016

mcollina commented Nov 22, 2016

natorion commented Nov 22, 2016

bnoordhuis commented Nov 22, 2016

addaleax commented Nov 22, 2016

bnoordhuis commented Nov 22, 2016

ofrobots commented Nov 22, 2016

targos commented Nov 23, 2016

fhinkel commented Dec 1, 2016 • edited Loading

addaleax commented Dec 1, 2016

fhinkel commented Dec 4, 2016

mcollina commented Dec 4, 2016

fhinkel commented Dec 4, 2016 • edited Loading

addaleax commented Dec 4, 2016

simov commented Dec 16, 2016 • edited Loading

7.2.0

7.2.1

fhinkel commented Dec 16, 2016

fhinkel commented Dec 1, 2016 •

edited

Loading

fhinkel commented Dec 4, 2016 •

edited

Loading

simov commented Dec 16, 2016 •

edited

Loading