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

Closed
wants to merge 10 commits into from
Closed

deps: update V8 to 5.1.281.69 #7016

wants to merge 10 commits into from

Conversation

targos
Copy link
Member

@targos 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 targos added v8 engine Issues and PRs related to the V8 dependency. dont-land-on-v5.x labels May 27, 2016
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 27, 2016
@targos targos removed the lib / src Issues and PRs related to general changes in the lib or src directory. label May 27, 2016
@targos
Copy link
Member Author

targos commented May 27, 2016

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

@targos
Copy link
Member Author

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

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

@bnoordhuis
Copy link
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.

@mhdawson
Copy link
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.

@ofrobots
Copy link
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.

@targos
Copy link
Member Author

targos commented May 27, 2016

Nice, let's wait for it then.

@targos
Copy link
Member Author

targos commented May 27, 2016

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

targos commented May 27, 2016

OK. I will make the change.

@targos
Copy link
Member Author

targos commented May 27, 2016

@ofrobots done

@auguradmin
Copy link

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

Is this able to land in v6 or not?

@ofrobots
Copy link
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.

@targos targos changed the title deps: update V8 to 5.1.281.50 deps: update V8 to 5.1.281.56 May 31, 2016
@rvagg
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
Member Author

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

@ofrobots ofrobots mentioned this pull request Aug 10, 2016
2 tasks
@ofrobots
Copy link
Contributor

Backport to v6.x: #8054

ofrobots added a commit to ofrobots/node that referenced this pull request Aug 25, 2016
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 <[email protected]>
Reviewed-By: thealphanerd - Myles Borins <[email protected]>
Reviewed-By: mhdawson - Michael Dawson <[email protected]>
Reviewed-By: evanlucas - Evan Lucas <[email protected]>
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
ofrobots added a commit to ofrobots/node that referenced this pull request Aug 25, 2016
V8 improved the error message for illegal token in
v8/v8@879b617b. This breaks the recoverable
error handling in repl.

Ref: nodejs#6482

PR-URL: nodejs#7016
Reviewed-By: bnoordhuis - Ben Noordhuis <[email protected]>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
It is no longer necessary now that libplatform/libplatform.h uses proper
includes.

PR-URL: nodejs#7016
Reviewed-By: Ben Noordhuis <[email protected]>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
Ref: #23

PR-URL: nodejs#7016
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
ofrobots pushed a commit to ofrobots/node that referenced this pull request Aug 25, 2016
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 <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.