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.

Sign up for GitHub

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

Jump to bottom

[v8.x] deps: update V8 to 5.9 #13515

Closed
wants to merge 24 commits into from
Closed

[v8.x] deps: update V8 to 5.9 #13515

wants to merge 24 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Jun 7, 2017 •
edited
Loading

This is a fresh update of V8 applied to v8.x-staging with:

/cc @nodejs/v8 @jasnell @MylesBorins @addaleax

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

V8

@targos targos added dont-land-on-v4.x 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. labels Jun 7, 2017
@nodejs-github-bot nodejs-github-bot added v8 engine Issues and PRs related to the V8 dependency. v8.x labels Jun 7, 2017
@targos targos mentioned this pull request Jun 7, 2017
Closed
2 tasks
@targos
Copy link
Member Author

targos commented Jun 7, 2017

@addaleax thank you for doing the forward compatibility in multiple commits. That helped a lot to prepare this :)

addaleax
addaleax approved these changes Jun 7, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked through the include/ diffs to Node 8.0.0 and V8 6.0, this should be good to go 👍

@gibfahn
Copy link
Member

gibfahn commented Jun 7, 2017

cc/ @nodejs/lts, note that targos@ec7cbaf updates the NODE_MODULE_VERSION to 57 so it's compatible with Node 8.x (and it also has the 6.0 ABI changes).

@gibfahn
Copy link
Member

gibfahn commented Jun 7, 2017

Here's a CI: https://ci.nodejs.org/job/node-test-commit/10418/

I guess this also needs CitGM and V8 tests.

@targos
Copy link
Member Author

targos commented Jun 7, 2017

There seems to be an issue with #10388 on master (and therefore here too). None of the existing fixes apply cleanly to V8 5.9 and I don't have gcc 7 to investigate.

@targos
Copy link
Member Author

targos commented Jun 7, 2017

Updated to add the fix from #10388 (comment): 99d154e

@targos
Copy link
Member Author

targos commented Jun 7, 2017

V8 tests: https://ci.nodejs.org/job/node-test-commit-v8-linux/733/

@MylesBorins
Copy link
Contributor

I'll try and get the ABI smoker up and running today so we can test

@addaleax
Copy link
Member

addaleax commented Jun 7, 2017 •
edited
Loading

Btw, you might also want to include v8/v8@a16c3c910548e – it’s the only breaking change in V8 6.0 for a feature that was just introduced in V8 5.9. I doubt anybody gets hurt when we don’t include it, but I’d guess it applies cleanly.

@targos
Copy link
Member Author

targos commented Jun 12, 2017

Btw, you might also want to include v8/v8@a16c3c9

Done

@targos
Copy link
Member Author

targos commented Jun 13, 2017

Just added a0f8faa

CI: https://ci.nodejs.org/job/node-test-pull-request/8626/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/739/

@winksaville
Copy link

I found a performance issue which might be related to node using v8 5.8. What I see is that my application is 6x slower targeting es6 vs es5. For details see this post on the v8 email list.

When this is merged I'll test again and see if v8 5.9 resolves the issue as is speculated.

@winksaville winksaville mentioned this pull request Jun 13, 2017
Open
@addaleax
Copy link
Member

@targos Could you rebase this against v8.x-staging? Github doesn’t see a problem but I can’t apply it cleanly.

@addaleax
Copy link
Member

Okay, I’ve rebased this. The previous head was at 532be47fc690d300b5b097d3e529bc5a9b60394d, in case it’s necessary to roll back.

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

@addaleax
Copy link
Member

Landed in 369b4e53e8806d38cbe1d8f57abde9fb465427a8...36bab2190a75c4c615e91b0a924fd84f4110ec0f

@addaleax addaleax closed this Jun 17, 2017
addaleax and others added 9 commits July 21, 2017 08:32
@addaleax @targos
v8: do not test v8 with -Werror
81895a5 
PR-URL: nodejs#12875
Reviewed-By: James M Snell <[email protected]>
@mi-ac @targos
v8: backport bd59e7452be from upstream v8
6cd148c 
Original commit message:
    [PATCH] Merged: Make Object::GetOwnPropertyDescriptor() take a Name, not a String.

    Revision: b5e610c19208ef854755eec67011ca7aff008bf4

    NOTRY=true
    NOPRESUBMIT=true
    NOTREECHECKS=true
    [email protected]

    Bug:
    Change-Id: I396b559b28aab6afa138db747711e50cd0da3da7
    Reviewed-on: https://chromium-review.googlesource.com/513927
    Reviewed-by: Michael Achenbach <[email protected]>
    Cr-Commit-Position: refs/branch-heads/6.0@{#5}
    Cr-Branched-From: 97dbf624a5eeffb3a8df36d24cdb2a883137385f-refs/heads/6.0.286@{#1}
    Cr-Branched-From: 12e6f1cb5cd9616da7b9d4a7655c088778a6d415-refs/heads/master@{nodejs#45439}

PR-URL: nodejs#13217
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@psmarshall @targos
v8: backport a9e56f4f36d from upstream v8
876031b 
Because 5.8 still had other uses of the removed flag, there are some extra
changes in Heap::ConfigureHeap and api.cc:SetResourceConstraints.

Original commit message:
    [heap] Remove max_executable_size resource constraint.

    BUG=chromium:716032

    Review-Url: https://codereview.chromium.org/2890603007
    Cr-Commit-Position: refs/heads/master@{nodejs#45400}

PR-URL: nodejs#13217
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@hannespayer @targos
v8: backport 4f82f1d948c from upstream v8
f01016c 
Original commit message:
    [Api] Add an idle time garbage collection callback flag to GCCallbackFlags.

    BUG=chromium:718484

    Review-Url: https://codereview.chromium.org/2867073002
    Cr-Commit-Position: refs/heads/master@{nodejs#45167}

PR-URL: nodejs#13217
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@hannespayer @targos
v8: backport 4f82f1d948c from upstream v8
aa2dfc4 
Original commit message:
    [PATCH] Rename idle garbage collection callback flag.

    [email protected]

    Review-Url: https://codereview.chromium.org/2867863002
    Cr-Commit-Position: refs/heads/master@{nodejs#45173}

PR-URL: nodejs#13217
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@psmarshall @targos
v8: backport pieces from 18a26cfe174 from upstream v8
2a1b8bf 
Backport new virtual methods from 18a26cfe174
("Add memory protection API to ArrayBuffer::Allocator")

PR-URL: nodejs#13217
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jeremyroman @targos
deps: cherry-pick a16c3c9 from upstream V8
fb8aba0 
Original commit message:

    Expose the ValueSerializer data format version as a compile-time constant.

    BUG=chromium:704293

    Review-Url: https://codereview.chromium.org/2804643006
    Cr-Commit-Position: refs/heads/master@{nodejs#44945}
@targos
deps: cherry-pick 866ee63 from upstream V8
5d5eb0b 
Original commit message:

    [string] Re-enable result caching for String.p.split

    Runtime::kStringSplit's result caching is only enabled when limit equals
    kMaxUInt32.

    BUG=v8:6463

    Review-Url: https://codereview.chromium.org/2923183002
    Cr-Commit-Position: refs/heads/master@{nodejs#45724}

Fixes: nodejs#13445
@bzoz @targos
v8: fix debug builds on Windows
31142a7 
Adds missing return which fixes debug builds on Windows

Fixes: nodejs#13392
Ref: https://codereview.chromium.org/2929993003/
PR-URL: nodejs#13634
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@targos
Copy link
Member Author

targos commented Jul 21, 2017

@addaleax rebased ;)

targos and others added 3 commits July 21, 2017 12:52
@targos @addaleax
deps: update V8 to 5.9.211.37
147d8f1 
PR-URL: nodejs#13790
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@oliverchang @addaleax
deps: cherry-pick 3f4536894ac from V8 upstream
356af65 
Original commit message:

    d8: Make in process stack dumping optional

    Adds a flag (--disable-in-process-stack-traces) to not install
    signal handlers so that e.g. ASan signal handlers will work.

    This flag mirrors chromium's one.

    [email protected]
    BUG=chromium:716235

    Review-Url: https://codereview.chromium.org/2854173002
    Cr-Commit-Position: refs/heads/master@{nodejs#45142}

PR-URL: nodejs#13985
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
src: fix process.abort() interaction with V8
e2ae710 
Since V8 5.9 V8 installs a default signal handler for some signals
when creating a default platform instance that prints a stack trace.

However, Node already does the same thing, so it would seem like the
two different stack traces would be printed; also, the V8 handler
would lead to a `SIGSEGV` under some circumstances, rather than
letting the abort continue normally.

Resolve this by disabling V8’s signal handler by default.

PR-URL: nodejs#13985
Fixes: nodejs#13865
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member

addaleax commented Jul 21, 2017 •
edited
Loading

@targos I cherry-picked 3 more commits that should be part of the upgrade if I understand correctly (and previously were on v8.x-staging), please remove if I’m mistaken. :)

@bnoordhuis bnoordhuis mentioned this pull request Jul 21, 2017
Closed
addaleax pushed a commit that referenced this pull request Jul 24, 2017
@targos @addaleax
deps: update V8 to 5.9.211.35
272f494 
PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 24, 2017
@targos @addaleax
v8: fix gcc 7 build errors
0be4d17 
Porting #12392 to V8 5.9

Ref: #12392
Fixes: #10388
PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 24, 2017
@jeremyroman @addaleax
deps: cherry-pick a16c3c9 from upstream V8
8e96729 
Original commit message:

    Expose the ValueSerializer data format version as a compile-time constant.

    BUG=chromium:704293

    Review-Url: https://codereview.chromium.org/2804643006
    Cr-Commit-Position: refs/heads/master@{#44945}

PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 24, 2017
@targos @addaleax
deps: cherry-pick 866ee63 from upstream V8
06d419f 
Original commit message:

    [string] Re-enable result caching for String.p.split

    Runtime::kStringSplit's result caching is only enabled when limit equals
    kMaxUInt32.

    BUG=v8:6463

    Review-Url: https://codereview.chromium.org/2923183002
    Cr-Commit-Position: refs/heads/master@{#45724}

Fixes: #13445
PR-URL: #13515
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax addaleax mentioned this pull request Jul 24, 2017
Closed
addaleax added a commit that referenced this pull request Jul 24, 2017
@addaleax
2017-07-??, Version 8.3.0 (Current)
d4c2406 
Notable changes

* **V8**
    * The V8 engine has been upgraded to version 5.9, which has a significantly
    changed performance profile.
    [#13515](#13515)
@isheludko @addaleax
deps: cherry-pick 6cb999b97b from V8 upstream
646eff0 
Original commit message:

    Properly handle loads from global interceptor via prototype chain.

    ... when receiver is in dictionary mode.

    Bug: v8:6490
    Change-Id: Ic5a8d214adcc4efd4cb163cbc6b351c4e6b596af
    Reviewed-on: https://chromium-review.googlesource.com/559548
    Reviewed-by: Camillo Bruni <[email protected]>
    Commit-Queue: Igor Sheludko <[email protected]>
    Cr-Commit-Position: refs/heads/master@{nodejs#46428}

Ref: https://chromium.googlesource.com/v8/v8.git/+/6cb999b97b7953ebfd4aabf2e1f62bf405f21c69
Fixes: nodejs#13804
PR-URL: nodejs#14188
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax
Copy link
Member

I guess this is just not going to happen. Let’s wait for 6.0!

@addaleax addaleax closed this Jul 27, 2017
@MylesBorins MylesBorins mentioned this pull request Aug 1, 2017
Closed
@targos targos deleted the v8-59-v8.x branch August 10, 2017 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
notable-change PRs with changes that should be highlighted in changelogs. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.