FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: update V8 to 6.0 by targos · Pull Request #14004 · nodejs/node · GitHub

/ node Public

deps: update V8 to 6.0 - #14004

Closed
targos wants to merge 8 commits into
nodejs:masterfrom
targos:v8-6.0
ConversationCommits (8)ChecksFiles changed
Closed

deps: update V8 to 6.0#14004
targos wants to merge 8 commits into
nodejs:masterfrom
targos:v8-6.0

Conversation

targos commented Jun 30, 2017 •
edited by refack
Loading

Copy link
Copy Markdown
Member

As discussed in nodejs/CTC#146, I am opening this PR early so we can have test builds of Node with V8 6.0 as soon as possible.
The update is ready but blocked on five issues:

/cc @nodejs/v8 @nodejs/ctc

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

V8

targos added blocked PRs that are blocked by other issues or PRs. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency. labels Jun 30, 2017
nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jun 30, 2017
targos mentioned this pull request Jun 30, 2017
Closed

targos commented Jun 30, 2017

Copy link
Copy Markdown
Member Author

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

targos mentioned this pull request Jun 30, 2017
Closed

targos commented Jun 30, 2017

Copy link
Copy Markdown
Member Author

I forgot there are two other issues (initially reported here: nodejs/node-v8#7):

And finally a V8 cctest fails: https://ci.nodejs.org/job/node-test-commit-v8-linux/763/

misterdjules commented Jun 30, 2017

Copy link
Copy Markdown

@targos When do you need those SmartOS build errors fixed? In other words, is it urgent?

joaocgreis commented Jun 30, 2017

Copy link
Copy Markdown
Member

@targos https://chromium-review.googlesource.com/c/556599/ landed upstream, it would be better to replace my floating patch (deps,win: increase msvs_shard in gyp for V8) with a backport of that commit.

targos commented Jul 3, 2017

Copy link
Copy Markdown
Member Author

@joaocgreis Thank you. I cherry-picked that commit.

targos commented Jul 3, 2017

Copy link
Copy Markdown
Member Author

@misterdjules No, this is not urgent, and we have other issues to fix in the meantime

bmeurer commented Jul 6, 2017

Copy link
Copy Markdown
Member

cc @natorion

natorion commented Jul 11, 2017

Copy link
Copy Markdown

Some comments/questions:

Node crashes when running jest. See: #13804, https://bugs.chromium.org/p/v8/issues/detail?id=6490 and https://github.com/targos/jest-crash.
==> scheduled to be merged upstream to 6.0

async-hooks/test-zlib.zlib-binding.deflate can crash (seems to happen only with the 32-bit build): https://ci.nodejs.org/job/node-test-commit-linux/10874/
==> Is there already a V8 issue? That sounds like something that might be fixed upstream. Sadly, I can't access ci.nodejs.org

V8 cctest failure: https://ci.nodejs.org/job/node-test-commit-v8-linux/763/
==> Same as above, it is hard to judge what needs to be done.

Compilation errors on SmartOS and FreeBSD =>
Are you going to float those patches? Are you expecting that they are going to be upstreamed? Please keep in mind that we will promote 6.0 to V8 stable next week. That means, that merges will have a much higher burden to overcome. Depending on the actual patch it might not be an issue though because of SmartOS and FreeBSD.

Trott commented Jul 11, 2017

Copy link
Copy Markdown
Member

Sadly, I can't access ci.nodejs.org

@natorion It should be available again in a few hours, if all goes well.

Access is extremely restricted right now because it's being used for the scheduled security release of Node.js. Any changes to that schedule are likely to be posted to #14153. If necessary to know more, @MylesBorins can fill you in.

gibfahn commented Jul 11, 2017

Copy link
Copy Markdown
Member

@natorion It should be available again in a few hours, if all goes well.

It's back btw.

targos commented Jul 11, 2017

Copy link
Copy Markdown
Member Author

New CI run: https://ci.nodejs.org/job/node-test-pull-request/9075/

natorion commented Jul 12, 2017

Copy link
Copy Markdown

Thanks, seems like https://ci.nodejs.org/job/node-test-pull-request/9075/ was aborted and the other links produce 404s.

MylesBorins commented Jul 12, 2017

Copy link
Copy Markdown
Contributor

@natorion it would appear that the test runner was hanging on centos 5

https://ci.nodejs.org/job/node-test-commit-linux/11105/nodes=centos5-64/console

targos commented Jul 12, 2017

Copy link
Copy Markdown
Member Author

I updated the branch to V8 6.0.286.34 to include the fix for https://bugs.chromium.org/p/v8/issues/detail?id=6490.

New CI: https://ci.nodejs.org/job/node-test-commit/11071/
V8 tests: https://ci.nodejs.org/job/node-test-commit-v8-linux/776/

MylesBorins commented Jul 12, 2017

Copy link
Copy Markdown
Contributor

@targos @addaleax any interest in floating the bsd / smart os packages upstream?

addaleax commented Jul 12, 2017

Copy link
Copy Markdown
Member

Sorry, I didn’t quite follow here – which are the patches you are referring to?

MylesBorins commented Jul 12, 2017

Copy link
Copy Markdown
Contributor

@addaleax patches that fix the compilation errors, Im not sure we have them yet...

was trying to summarize @natorion's questions in #14004 (comment)

targos commented Jul 12, 2017

Copy link
Copy Markdown
Member Author

We don't have those patches yet and I don't know if anyone is working on it.

addaleax commented Jul 12, 2017

Copy link
Copy Markdown
Member

async-hooks/test-zlib.zlib-binding.deflate can crash (seems to happen only with the 32-bit build): https://ci.nodejs.org/job/node-test-commit-linux/10874/

Do you have a CI log that contains that failure? Could it be the same as #14161?

targos commented Jul 12, 2017

Copy link
Copy Markdown
Member Author

Do you have a CI log that contains that failure?

No, the logs have been deleted. It does not seem to be reproducible anymore. I'll mark this point as done for now.

MylesBorins closed this Aug 1, 2017
MylesBorins added a commit that referenced this pull request Aug 1, 2017
deps: update V8 to 6.0.286.52
0a66b22 
PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins added a commit that referenced this pull request Aug 1, 2017
src: update NODE_MODULE_VERSION to 57
db476fc 
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 6.0.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
deps: limit regress/regress-crbug-514081 v8 test
ebd13c3 
regress/regress-crbug-514081 allocates a 2G block of memory
and if there  are multiple variants running at the
same time this can lead to crashes, OOM kills or
the OS failing to allocate memory.  This patch
limits us to running a single variant of the test

Fixes: #6340
Refs: #6678

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
v8: fix stack overflow in recursive method
28aa7d0 
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Refs: #12460

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
deps: fix addons compilation with VS2013
ed30842 
VS2013 does not support defaulting move constructor and assignment
operator. This adds explicit definitions of those methods for two
classes.
This fix is required because we still support building addons with
VS2013 and the incompatibility is in v8.h.

Fixes: nodejs/node-v8#4
Refs: #13263

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
deps: backport c0f1ff2 from upstream V8
5651a6f 
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

    Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9
    Reviewed-on: https://chromium-review.googlesource.com/530227
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#46045}

Refs: #13517
Fixes: #10388
Refs: #12392

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
src: fix new V8 compiler warnings
a3c5ccd 
PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 1, 2017
deps: backport rehash strings after deserialization
98ddab4 
Original commit messages:
v8/v8@a2ab135
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46732}

v8/v8@182caaf
  Do not track transitions for built-in objects.

  Objects created during bootstrapping do not need
  a transition tree except for elements kind transitions.

  Bug: v8:6596
  Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
  Reviewed-on: https://chromium-review.googlesource.com/571750
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46693}

Fixes: #14171
Refs: #14345

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Aug 1, 2017
Closed
Closed
addaleax pushed a commit that referenced this pull request Aug 2, 2017
deps: limit regress/regress-crbug-514081 v8 test
160e2f0 
regress/regress-crbug-514081 allocates a 2G block of memory
and if there  are multiple variants running at the
same time this can lead to crashes, OOM kills or
the OS failing to allocate memory.  This patch
limits us to running a single variant of the test

Fixes: #6340
Refs: #6678

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 2, 2017
v8: fix stack overflow in recursive method
357873d 
HGlobalValueNumberingPhase::CollectSideEffectsOnPathsToDominatedBlock()
used to self-recurse before this commit, causing stack overflows on
systems with small stack sizes.  Make it non-recursive by storing
intermediate results in a heap-allocated list.

Fixes: #11991
Refs: #12460

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 2, 2017
deps: fix addons compilation with VS2013
efd297a 
VS2013 does not support defaulting move constructor and assignment
operator. This adds explicit definitions of those methods for two
classes.
This fix is required because we still support building addons with
VS2013 and the incompatibility is in v8.h.

Fixes: nodejs/node-v8#4
Refs: #13263

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 2, 2017
deps: backport c0f1ff2 from upstream V8
2dbf95d 
Original commit message:

    Fix GCC 7 build errors

    BUG=chromium:691681
    R=franzih@chromium.org

    Change-Id: Id7e5698487f16dc217a804f6d3f24da7213c72b9
    Reviewed-on: https://chromium-review.googlesource.com/530227
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#46045}

Refs: #13517
Fixes: #10388
Refs: #12392

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 2, 2017
src: fix new V8 compiler warnings
997204a 
Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 2, 2017
deps: backport rehash strings after deserialization
e2b306c 
Original commit messages:
v8/v8@a2ab135
  [snapshot] Rehash strings after deserialization.

  See https://goo.gl/6aN8xA

  Bug: v8:6593
  Change-Id: Ic8b0b57195d01d41591397d5d45de3f0f3ebc3d9
  Reviewed-on: https://chromium-review.googlesource.com/574527
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Jakob Gruber <jgruber@chromium.org>
  Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46732}

v8/v8@182caaf
  Do not track transitions for built-in objects.

  Objects created during bootstrapping do not need
  a transition tree except for elements kind transitions.

  Bug: v8:6596
  Change-Id: I237b8b2792f201336e1c9731c815095dd06bc182
  Reviewed-on: https://chromium-review.googlesource.com/571750
  Reviewed-by: Igor Sheludko <ishell@chromium.org>
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#46693}

Fixes: #14171
Refs: #14345

Backport-PR-URL: #14574
Backport-Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14004
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax mentioned this pull request Aug 2, 2017
Merged
targos deleted the v8-6.0 branch August 10, 2017 09:24
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Oct 2, 2017
build: run test-hash-seed at the end of test-v8
4cb743d 
The v8 and test-hash-seed targets cannot be run in parallel because they
need different copies of the deps/v8 directory.

Ref: nodejs#14004 (comment)
PR-URL: nodejs#14219
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 18, 2017
build: run test-hash-seed at the end of test-v8
2ae2a96 
The v8 and test-hash-seed targets cannot be run in parallel because they
need different copies of the deps/v8 directory.

Ref: #14004 (comment)
Backport-PR-URL: #15562
PR-URL: #14219
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
build: run test-hash-seed at the end of test-v8
a7f7a87 
The v8 and test-hash-seed targets cannot be run in parallel because they
need different copies of the deps/v8 directory.

Ref: #14004 (comment)
Backport-PR-URL: #15562
PR-URL: #14219
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell mentioned this pull request Oct 30, 2017
Merged
remojansen mentioned this pull request Dec 15, 2017
Closed
nfalco79 mentioned this pull request Feb 14, 2018
Closed
refack mentioned this pull request Nov 23, 2018
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
+2 more reviewers
refack refack approved these changes
fhinkel fhinkel approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major 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.

Back | FazBrowse Home | New Git URL