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: update V8 to 5.1.281.69 #7016

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
@targos
Member

targos commented May 27, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

v8

Description of change

V8 5.1 is out with stable Chrome so here is the PR to update our copy!
I picked up the latest commit from the 5.1-lkgr branch, applied all (I hope) relevant floating patches and the commits from our vee-eight-5.1 branch.

cc @nodejs/v8, @nodejs/post-mortem

CI: https://ci.nodejs.org/job/node-test-pull-request/2817/

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos May 27, 2016

Member

@mhdawson would you like me to try a revert of 0899ea7 and 2d524bc ?

Member

targos commented May 27, 2016

@mhdawson would you like me to try a revert of 0899ea7 and 2d524bc ?

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos May 27, 2016

Member

FYI here is the list of V8 API changes in 5.1:

  • Add memory pressure notification API: CL
  • Introduce v8::MicrotasksScope: CL
  • Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage: CL
  • MicrotasksCompletedCallback: CL
  • Clarify the limits of ResourceConstraints: CL

Source: https://docs.google.com/document/d/1g8JFi8T_oAE_7uAri7Njtig7fKaPDfotU6huOa1alds

Edit: I guess these make the update semver-major ?

Member

targos commented May 27, 2016

FYI here is the list of V8 API changes in 5.1:

  • Add memory pressure notification API: CL
  • Introduce v8::MicrotasksScope: CL
  • Use a different GCCallbackFlag for GCs triggered by CollectAllAvailableGarbage: CL
  • MicrotasksCompletedCallback: CL
  • Clarify the limits of ResourceConstraints: CL

Source: https://docs.google.com/document/d/1g8JFi8T_oAE_7uAri7Njtig7fKaPDfotU6huOa1alds

Edit: I guess these make the update semver-major ?

@ronkorving

This comment has been minimized.

Show comment
Hide comment
@ronkorving

ronkorving May 27, 2016

Contributor

Did any of those APIs actually change or are they just new APIs? (seems like at least mostly they're just new..)

Contributor

ronkorving commented May 27, 2016

Did any of those APIs actually change or are they just new APIs? (seems like at least mostly they're just new..)

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis May 27, 2016

Member

@targos Did you check whether it's possible to drop the deps/v8 include from node.gyp now? I mean this one:

diff --git a/node.gyp b/node.gyp
index 0d32905..4187ce6 100644
--- a/node.gyp
+++ b/node.gyp
@@ -117,7 +117,6 @@
         'tools/msvs/genfiles',
         'deps/uv/src/ares',
         '<(SHARED_INTERMEDIATE_DIR)', # for node_natives.h
-        'deps/v8' # include/v8_platform.h
       ],

       'sources': [

@ronkorving Yes, they're mostly additive. It's not a drop-in replacement, though.

Member

bnoordhuis commented May 27, 2016

@targos Did you check whether it's possible to drop the deps/v8 include from node.gyp now? I mean this one:

diff --git a/node.gyp b/node.gyp
index 0d32905..4187ce6 100644
--- a/node.gyp
+++ b/node.gyp
@@ -117,7 +117,6 @@
         'tools/msvs/genfiles',
         'deps/uv/src/ares',
         '<(SHARED_INTERMEDIATE_DIR)', # for node_natives.h
-        'deps/v8' # include/v8_platform.h
       ],

       'sources': [

@ronkorving Yes, they're mostly additive. It's not a drop-in replacement, though.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson May 27, 2016

Member

@targos if its easy for you that would be useful, but otherwise I'm happy to test after the upgrade islanded and remove if appropriate.

Member

mhdawson commented May 27, 2016

@targos if its easy for you that would be useful, but otherwise I'm happy to test after the upgrade islanded and remove if appropriate.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots May 27, 2016

Contributor

I think we should add ofrobots#23 (once we have thumbs up from the V8 team).

There are no breaking API changes between 5.0 and 5.1, there were some ABI differences. @matthewloring has been working on bridging that ABI gap. This would make V8 5.1 a semver-minor as opposed to a semver-major, and enable us to get this landed into Node 6.x.

I think we should wait for ofrobots#23 is reviewed, and add it here before landing this PR.

Contributor

ofrobots commented May 27, 2016

I think we should add ofrobots#23 (once we have thumbs up from the V8 team).

There are no breaking API changes between 5.0 and 5.1, there were some ABI differences. @matthewloring has been working on bridging that ABI gap. This would make V8 5.1 a semver-minor as opposed to a semver-major, and enable us to get this landed into Node 6.x.

I think we should wait for ofrobots#23 is reviewed, and add it here before landing this PR.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos May 27, 2016

Member

Nice, let's wait for it then.

Member

targos commented May 27, 2016

Nice, let's wait for it then.

@targos

This comment has been minimized.

Show comment
Hide comment
@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots May 27, 2016

Contributor

For the first commit (4f0c3be317b0b5ad1ea12306409671d9be53b762) I would prefer if the floating backports were refloated rather than squashing. That makes it easier to audit what changes from upstream we are floating on Node.js side.
EDIT: typo.

Contributor

ofrobots commented May 27, 2016

For the first commit (4f0c3be317b0b5ad1ea12306409671d9be53b762) I would prefer if the floating backports were refloated rather than squashing. That makes it easier to audit what changes from upstream we are floating on Node.js side.
EDIT: typo.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos May 27, 2016

Member

OK. I will make the change.

Member

targos commented May 27, 2016

OK. I will make the change.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos
Member

targos commented May 27, 2016

@ofrobots done

@ofrobots ofrobots referenced this pull request May 27, 2016

Closed

deps: upgrade V8 from 5.1.281.49 to 5.1.281.51 #7020

2 of 2 tasks complete
@auguradmin

This comment has been minimized.

Show comment
Hide comment
@auguradmin

auguradmin May 27, 2016

@targos What's the plan on merging v8 5.1.x with master? As I'm sure you're aware, it has a new garbage collector ( Orinoco ) that should speed node up dramatically and reduce memory. Can you share anything?

auguradmin commented May 27, 2016

@targos What's the plan on merging v8 5.1.x with master? As I'm sure you're aware, it has a new garbage collector ( Orinoco ) that should speed node up dramatically and reduce memory. Can you share anything?

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 May 29, 2016

Member

Is this able to land in v6 or not?

Member

Fishrock123 commented May 29, 2016

Is this able to land in v6 or not?

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots May 29, 2016

Contributor

@Fishrock123 That's the objective behind ofrobots#23. Once / If we are able to get sign-off from the V8 team and @nodejs/v8, we could add that commit to this PR. That would make this PR a semver-minor, and hence landable on v6.x.

Contributor

ofrobots commented May 29, 2016

@Fishrock123 That's the objective behind ofrobots#23. Once / If we are able to get sign-off from the V8 team and @nodejs/v8, we could add that commit to this PR. That would make this PR a semver-minor, and hence landable on v6.x.

@targos targos changed the title from deps: update V8 to 5.1.281.50 to deps: update V8 to 5.1.281.56 May 31, 2016

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg May 31, 2016

Member

@ofrobots it looks like there might be some challenges over at the ABI compatibility changes being proposed. In the event that we're unable to make enough compromise we should start a discussion about a possible floating patch, although I think it would need to be pretty minimal to be palatable here. Ideal is to do this all upstream of course, just keep us in the loop so there's enough brew-time for decision making on this because it's such a new way of thinking about our major release lines.

Member

rvagg commented May 31, 2016

@ofrobots it looks like there might be some challenges over at the ABI compatibility changes being proposed. In the event that we're unable to make enough compromise we should start a discussion about a possible floating patch, although I think it would need to be pretty minimal to be palatable here. Ideal is to do this all upstream of course, just keep us in the loop so there's enough brew-time for decision making on this because it's such a new way of thinking about our major release lines.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Jun 1, 2016

Contributor

@rvagg This indeed a new way of thinking about major releases, and I am definitely interested in careful vetting of the ABI compatibility patch. The patch (ofrobots#23) is looking pretty small at the moment.

Note that I don't expect the patch to land upstream in V8 5.1 – this is not a bugfix, and will have no coverage from Chrome canaries to qualify it to land upstream in V8 5.1 branch.

The only reason @matthewloring has been putting energy into this for the benefit of Node.js. We're happy to do the work to make it possible for V8 5.1 to be landable in Node.js w/ ABI compatibility with V8 5.0. It is independent decision on whether the patch looks reasonable enough to land that should probably be made by the @nodejs/v8 or the CTC after careful evaluation/brew time.

The benefit of course is that V8 5.1 brings-in almost all of ES6 along with performance improvements.

Contributor

ofrobots commented Jun 1, 2016

@rvagg This indeed a new way of thinking about major releases, and I am definitely interested in careful vetting of the ABI compatibility patch. The patch (ofrobots#23) is looking pretty small at the moment.

Note that I don't expect the patch to land upstream in V8 5.1 – this is not a bugfix, and will have no coverage from Chrome canaries to qualify it to land upstream in V8 5.1 branch.

The only reason @matthewloring has been putting energy into this for the benefit of Node.js. We're happy to do the work to make it possible for V8 5.1 to be landable in Node.js w/ ABI compatibility with V8 5.0. It is independent decision on whether the patch looks reasonable enough to land that should probably be made by the @nodejs/v8 or the CTC after careful evaluation/brew time.

The benefit of course is that V8 5.1 brings-in almost all of ES6 along with performance improvements.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Jun 1, 2016

Contributor

@targos For 2d2d6ca, I normally squash that commit with the previous two, but it is okay to keep it unsquashed too. If you chose the latter option, can you truncate the commit abstract to fit the 50 char limit?

Contributor

ofrobots commented Jun 1, 2016

@targos For 2d2d6ca, I normally squash that commit with the previous two, but it is okay to keep it unsquashed too. If you chose the latter option, can you truncate the commit abstract to fit the 50 char limit?

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Jun 1, 2016

Member

👍 thanks for the hardwork @ofrobots, @matthewloring and others. This shift is something we've not communicated to the public very well and will be a surprise for a lot of people. I'm planning on doing some writing ASAP to start the messaging about the change in policy.

Member

rvagg commented Jun 1, 2016

👍 thanks for the hardwork @ofrobots, @matthewloring and others. This shift is something we've not communicated to the public very well and will be a surprise for a lot of people. I'm planning on doing some writing ASAP to start the messaging about the change in policy.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Jun 1, 2016

Member

I normally squash that commit with the previous two, but it is okay to keep it unsquashed too.

I will squash them when I land the PR

Member

targos commented Jun 1, 2016

I normally squash that commit with the previous two, but it is okay to keep it unsquashed too.

I will squash them when I land the PR

@targos targos changed the title from deps: update V8 to 5.1.281.56 to deps: update V8 to 5.1.281.59 Jun 6, 2016

@ofrobots ofrobots referenced this pull request Aug 10, 2016

Merged

Backport V8 5.1 to v6.x #8054

2 of 2 tasks complete
@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Aug 10, 2016

Contributor

Backport to v6.x: #8054

Contributor

ofrobots commented Aug 10, 2016

Backport to v6.x: #8054

ofrobots added a commit to ofrobots/node that referenced this pull request Aug 25, 2016

deps: update V8 to 5.1.281.75
Pick up the latest branch-head for V8 5.1. This branch brings in
improved language support and performance improvements. For full
details: http://v8project.blogspot.com/2016/04/v8-release-51.html

* Picks up the latest branch head for 5.1 [1]
* Edit v8 gitignore to allow trace_event copy
* Update V8 DEP trace_event as per deps/v8/DEPS [2]

[1] https://chromium.googlesource.com/v8/v8.git/+/5.1.281.75
[2] https://chromium.googlesource.com/chromium/src/base/trace_event/common/+/c8c8665

Introduces a semver-minor overload of v8::Function::New() for use
by v8_inspector.

PR-URL: nodejs#8054
Refs: nodejs#7016
Refs: nodejs#7586
Refs: nodejs#7615
Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net>
Reviewed-By: thealphanerd - Myles Borins <myles.borins@gmail.com>
Reviewed-By: mhdawson - Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: evanlucas - Evan Lucas <evanlucas@me.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

ofrobots added a commit to ofrobots/node that referenced this pull request Aug 25, 2016

repl: fix repl after V8 upgrade
V8 improved the error message for illegal token in
v8/v8@879b617. This breaks the recoverable
error handling in repl.

Ref: nodejs#6482

PR-URL: nodejs#7016
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

ofrobots added a commit to ofrobots/node that referenced this pull request Aug 25, 2016

build: don't include V8 from node.gyp
It is no longer necessary now that libplatform/libplatform.h uses proper
includes.

PR-URL: nodejs#7016
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

ofrobots added a commit to ofrobots/node that referenced this pull request Aug 25, 2016

deps: revert removal of V8::PromiseEvent
The removal of the promise debug event is an API/ABI breaking change.

Ref: https://codereview.chromium.org/1833563002
Ref: #23

PR-URL: nodejs#7016
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

ofrobots added a commit to ofrobots/node that referenced this pull request Aug 25, 2016

deps: bring in V8 5.1 - 5.0 ABI compatibility
Ref: #23

PR-URL: nodejs#7016
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

ofrobots added a commit to ofrobots/node that referenced this pull request Aug 25, 2016

deps: remove extra field from v8::HeapStatistics
Remove the `_malloced_memory` field from the `HeapStatistics`
class to achieve full ABI compatibility with V8 5.0.

Ref: nodejs#7016
PR-URL: nodejs#7526
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment