Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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
[email protected],[email protected]

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
[email protected],[email protected]

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

Fixes: nodejs#9634
@nodejs-github-bot nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Nov 22, 2016
@addaleax
Copy link
Member Author

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
Copy link
Contributor

/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
Copy link
Member

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

@fhinkel
Copy link
Member

fhinkel commented Nov 22, 2016

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

@addaleax Thanks for opening the PR for me.

@bnoordhuis
Copy link
Member

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:

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

@mcollina
Copy link
Member

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
Copy link

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
Copy link
Member

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

@addaleax
Copy link
Member Author

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
Copy link
Member

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
Copy link
Contributor

@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
Copy link
Member

targos commented Nov 23, 2016

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
Copy link
Member

fhinkel commented Dec 1, 2016

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
Copy link
Member Author

addaleax commented Dec 1, 2016

@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
Copy link
Member

fhinkel commented Dec 4, 2016

Can I get an LGTM here?

@mcollina
Copy link
Member

mcollina commented Dec 4, 2016

LGTM

@fhinkel
Copy link
Member

fhinkel commented Dec 4, 2016

Landed in b5453ee.

@fhinkel fhinkel closed this Dec 4, 2016
fhinkel added a commit that referenced this pull request Dec 4, 2016
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
[email protected],[email protected]

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

PR-URL: #9730
Fixes: #9634
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@addaleax addaleax deleted the fix-9634 branch December 4, 2016 22:23
@addaleax
Copy link
Member Author

addaleax commented Dec 4, 2016

@fhinkel thanks for taking care of this!

addaleax pushed a commit that referenced this pull request Dec 5, 2016
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
[email protected],[email protected]

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

PR-URL: #9730
Fixes: #9634
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Dec 5, 2016
2 tasks
Fishrock123 added a commit that referenced this pull request Dec 6, 2016
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
targos added a commit to targos/node that referenced this pull request Dec 7, 2016
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
imyller added a commit to imyller/meta-nodejs that referenced this pull request Dec 7, 2016
    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 <[email protected]>
jmdarling pushed a commit to jmdarling/node that referenced this pull request Dec 8, 2016
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
[email protected],[email protected]

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

PR-URL: nodejs#9730
Fixes: nodejs#9634
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@simov
Copy link
Contributor

simov commented Dec 16, 2016

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
Copy link
Member

fhinkel commented Dec 16, 2016

Cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants