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

deps: update V8 to 6.1 by targos · Pull Request #14730 · nodejs/node · GitHub

/ node Public

deps: update V8 to 6.1 - #14730

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

deps: update V8 to 6.1#14730
targos wants to merge 13 commits into
nodejs:masterfrom
targos:v8-6.1

Conversation

targos commented Aug 10, 2017 •
edited by refack
Loading

Copy link
Copy Markdown
Member

In progress because:

  • SmartOS must be fixed properly upstream (ref)
  • Waiting for the merge of v8/v8@d8e37c3 in 6.1 (ref)
  • 6.1 is still in beta

/cc @nodejs/platform-smartos @nodejs/v8

CI: https://ci.nodejs.org/job/node-test-commit/11673/

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

V8

targos added wip Issues and PRs that are still a work in progress. 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 Aug 10, 2017
bnoordhuis approved these changes Aug 10, 2017
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, LGTM.

parallel/test-cluster-send-handle-large-payload is flaky for me too (locally, with master) so probably not related.

parallel/test-windows-failed-heap-allocation looks like it might though?

misterdjules commented Aug 10, 2017

Copy link
Copy Markdown

@targos

SmartOS must be fixed properly upstream

Is your target date for merging this still early September as previously mentioned in nodejs/node-v8#8?

refack commented Aug 10, 2017

Copy link
Copy Markdown
Contributor

WOW we're down to two floating patches. Great work upstreaming 🥇

refack commented Aug 10, 2017

Copy link
Copy Markdown
Contributor

If this is semver-major, maybe we can drop VS2013 support? /cc @nodejs/node-gyp @nodejs/release

  1. For the general public, both VS2015 & VS2017 have free C++ build tools
  2. Assumption is, if someone is locked into VS2013 they are unlikely to upgrade a node major version.

matthewloring mentioned this pull request Aug 10, 2017
Closed

bnoordhuis commented Aug 10, 2017

Copy link
Copy Markdown
Member

I'm +1 on dropping support for VS 2013 but it can't happen just yet if we plan on upgrading to v6.1 in node 8 (which is on the table, see #14220.)

targos commented Aug 11, 2017

Copy link
Copy Markdown
Member Author

@misterdjules yes. Chrome 61 will be stable around Sep 5th.

targos mentioned this pull request Aug 11, 2017
Closed
2 tasks

jbajwa commented Aug 15, 2017

Copy link
Copy Markdown

@targos
Hi, could you kick off the V8 CI job https://ci.nodejs.org/job/node-test-commit-v8-linux/ with this PR? Thank you.

refack commented Aug 15, 2017

Copy link
Copy Markdown
Contributor

@jbajwa https://ci.nodejs.org/job/node-test-commit-v8-linux/855/

targos commented Aug 16, 2017

Copy link
Copy Markdown
Member Author

Rebased.
New CI: https://ci.nodejs.org/job/node-test-pull-request/9687/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/858/

jbajwa commented Aug 16, 2017

Copy link
Copy Markdown

@targos there are a couple of failures on ppc/s390. I'm working on fixing them.

rvagg commented Aug 17, 2017

Copy link
Copy Markdown
Member

So we did have a not-explicitly-stated policy of not changing V8 2 months prior to going LTS, there's some discussion of it here: https://github.com/nodejs/LTS/pull/121/files#diff-04c6e90faac2675aa89e2176d2eec7d8R129 (PR never merged, not because it was rejected but because it was too much in one go and I lost steam, but discussion still stands).

It could be argued that TF+I makes this a special case because it is still stabilising and we're presumably going to benefit from it. However the original case also applies—we're locking ourselves in to a version and can't afford a major misstep going in to LTS or we could be tied to something that we regret but only realise too late; plus there's the expectation factor in that users should be able to predict what LTS is going to be like before it lands, no surprises!

Worthy of discussion though eh @nodejs/lts? I'd like to know what 6.1 gives us, particularly as it relates to TF+I. Anyone know?

rvagg commented Aug 17, 2017

Copy link
Copy Markdown
Member

@targos can you update & rebase this PR so we can run it through CI again? I'm interested in the compiler compatibility story given the drama's we've had with 6.0 and the old glibc bug.

targos commented Aug 17, 2017

Copy link
Copy Markdown
Member Author

@rvagg done

evanlucas commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

I'd like to know what 6.1 gives us, particularly as it relates to TF+I. Anyone know?

@rvagg My understanding is that 6.1 offers lots of performance improvements over 6.0, but I could be mistaken. /cc @nodejs/v8

gibfahn commented Aug 17, 2017

Copy link
Copy Markdown
Member

However the original case also applies—we're locking ourselves in to a version and can't afford a major misstep going in to LTS or we could be tied to something that we regret but only realise too late

If it's ABI compatible with 6.0 (or shimmed to be), then we could always revert back if there were issues right?

bricss commented Aug 17, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

V8 6.1 have some significant performance gain-> https://v8project.blogspot.com/2017/08/v8-release-61.html

refack commented Aug 17, 2017

Copy link
Copy Markdown
Contributor

Does anyone know a node/V8 performance expert 👀 @bmeurer ...

If it's ABI compatible with 6.0 (or shimmed to be), then we could always revert back if there were issues right?

Theoretically true, realistically I don't see that happening.

I (can't believe I'm writing this) don't think performance is a strong enough incentive 🤷‍♂️ . There is another interesting notable-change asm.js is now validated and compiled to WebAssembly (And better WebAssembly debugging) which IMHO a stronger motivator.

addaleax commented Aug 17, 2017

Copy link
Copy Markdown
Member

If it's ABI compatible with 6.0 (or shimmed to be), then we could always revert back if there were issues right?

So far we always shimmed for backwards compatibility only; if we want to have the option of reverting back, we’d technically need to disable new features from V8 6.1 as well.

targos added a commit to targos/node that referenced this pull request Aug 18, 2017
build: stop support building addons with VS 2013
2062a69 
PR-URL: nodejs#14764
Refs: nodejs#14730 (comment)
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

rvagg commented Aug 20, 2017

Copy link
Copy Markdown
Member

https://ci.nodejs.org/job/node-test-commit/11919/console still got git conflicts unfortunately

Target date for stable is Sept 5th so CTC is going to have to make a call on this I reckon. Either that or CTC could delegate the decision to the new Release WG?

Binary size reduction

This one's interesting, the removal of Crankshaft gives us a bit of an improvement in our binary size. Comparing the 6.2 v8-canary builds to nightly (https://nodejs.org/download/v8-canary/v9.0.0-v8-canary20170819c29776ec09/ vs https://nodejs.org/download/nightly/v9.0.0-nightly20170819467385a49b/) shows the V8 6.2 tarballs are ~95% the size of the current 6.0 tarballs and the node binary itself is ~93%, for Linux x64 at least. So that's neat.

targos force-pushed the v8-6.1 branch 3 times, most recently from 0f1121c to f4b3ac9 Compare August 25, 2017 08:04

targos commented Sep 13, 2017

Copy link
Copy Markdown
Member Author

@misterdjules

I'm sorry, there was indeed a misunderstanding. I had a discussion about that on IRC with @cjihrig. He said that it was OK to merge this change as-is and that the correct one could be submitted later.

misterdjules commented Sep 13, 2017

Copy link
Copy Markdown

@targos

He said that it was OK to merge this change as-is and that the correct one could be submitted later.

Sounds good then!

bmeurer commented Sep 13, 2017

Copy link
Copy Markdown
Member

Awesome work! 👍

refack commented Sep 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Woops completely missed the pings (nowadays I'm always on-line in IRC I case I'm needed in a rush)
For posterity: the AssertionError [ERR_ASSERTION]: 1 === 134 means the subprocess exited with code 1 (general error) and not the expected 134 (OOM).
This is probably because the JS layer throws RangeError: Invalid array length, because the nn iterator grows to 2**32 too faster, and the heap does not get exhausted.

targos added a commit to targos/node that referenced this pull request Sep 14, 2017
deps: cherry-pick f19b889 from upstream V8
83bbf8d 
Original commit message:

    [inspector] support for cases when embedder doesn't call contextDestroyed

    Node.js doesn't have good place to call contextDestroyed.
    We need to cleanup everything on our side to allow clients to not call
    contextDestroyed method.

    R=dgozman@chromium.org,eostroukhov@chromium.com

    Bug: none
    Change-Id: Ibe3f01fd18afbfa579e5db66ab6f174d5fad7c82
    Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
    Reviewed-on: https://chromium-review.googlesource.com/575519
    Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
    Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
    Cr-Original-Commit-Position: refs/heads/master@{nodejs#46849}
    Reviewed-on: https://chromium-review.googlesource.com/596549
    Cr-Commit-Position: refs/heads/master@{nodejs#47060}

PR-URL: nodejs#14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit to targos/node that referenced this pull request Sep 14, 2017
deps: add postmortem metadata for V8 TurboFan
491fd20 
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
PR-URL: nodejs#14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit to targos/node that referenced this pull request Sep 14, 2017
src: fix SmartOS compilation
8368603 
Refs: nodejs/node-v8#8
PR-URL: nodejs#14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit to targos/node that referenced this pull request Sep 21, 2017
deps: cherry-pick f19b889 from upstream V8
d501fa2 
Original commit message:

    [inspector] support for cases when embedder doesn't call contextDestroyed

    Node.js doesn't have good place to call contextDestroyed.
    We need to cleanup everything on our side to allow clients to not call
    contextDestroyed method.

    R=dgozman@chromium.org,eostroukhov@chromium.com

    Bug: none
    Change-Id: Ibe3f01fd18afbfa579e5db66ab6f174d5fad7c82
    Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
    Reviewed-on: https://chromium-review.googlesource.com/575519
    Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
    Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
    Cr-Original-Commit-Position: refs/heads/master@{nodejs#46849}
    Reviewed-on: https://chromium-review.googlesource.com/596549
    Cr-Commit-Position: refs/heads/master@{nodejs#47060}

PR-URL: nodejs#14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit to targos/node that referenced this pull request Sep 21, 2017
deps: add postmortem metadata for V8 TurboFan
8c0f800 
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
PR-URL: nodejs#14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos added a commit to targos/node that referenced this pull request Sep 21, 2017
src: fix SmartOS compilation
398d496 
Refs: nodejs/node-v8#8
PR-URL: nodejs#14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2017
deps: cherry-pick f19b889 from upstream V8
c183134 
Original commit message:

    [inspector] support for cases when embedder doesn't call contextDestroyed

    Node.js doesn't have good place to call contextDestroyed.
    We need to cleanup everything on our side to allow clients to not call
    contextDestroyed method.

    R=dgozman@chromium.org,eostroukhov@chromium.com

    Bug: none
    Change-Id: Ibe3f01fd18afbfa579e5db66ab6f174d5fad7c82
    Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
    Reviewed-on: https://chromium-review.googlesource.com/575519
    Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
    Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
    Cr-Original-Commit-Position: refs/heads/master@{#46849}
    Reviewed-on: https://chromium-review.googlesource.com/596549
    Cr-Commit-Position: refs/heads/master@{#47060}

Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2017
deps: add postmortem metadata for V8 TurboFan
4042c0c 
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 28, 2017
src: fix SmartOS compilation
1976234 
Refs: nodejs/node-v8#8
Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
deps: cherry-pick f19b889 from upstream V8
7cad8dd 
Original commit message:

    [inspector] support for cases when embedder doesn't call contextDestroyed

    Node.js doesn't have good place to call contextDestroyed.
    We need to cleanup everything on our side to allow clients to not call
    contextDestroyed method.

    R=dgozman@chromium.org,eostroukhov@chromium.com

    Bug: none
    Change-Id: Ibe3f01fd18afbfa579e5db66ab6f174d5fad7c82
    Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
    Reviewed-on: https://chromium-review.googlesource.com/575519
    Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
    Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
    Cr-Original-Commit-Position: refs/heads/master@{#46849}
    Reviewed-on: https://chromium-review.googlesource.com/596549
    Cr-Commit-Position: refs/heads/master@{#47060}

Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
deps: add postmortem metadata for V8 TurboFan
8032f8c 
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 29, 2017
src: fix SmartOS compilation
aa22c86 
Refs: nodejs/node-v8#8
Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins mentioned this pull request Oct 3, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
deps: cherry-pick f19b889 from upstream V8
ede9d2e 
Original commit message:

    [inspector] support for cases when embedder doesn't call contextDestroyed

    Node.js doesn't have good place to call contextDestroyed.
    We need to cleanup everything on our side to allow clients to not call
    contextDestroyed method.

    R=dgozman@chromium.org,eostroukhov@chromium.com

    Bug: none
    Change-Id: Ibe3f01fd18afbfa579e5db66ab6f174d5fad7c82
    Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
    Reviewed-on: https://chromium-review.googlesource.com/575519
    Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
    Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
    Cr-Original-Commit-Position: refs/heads/master@{#46849}
    Reviewed-on: https://chromium-review.googlesource.com/596549
    Cr-Commit-Position: refs/heads/master@{#47060}

Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
deps: add postmortem metadata for V8 TurboFan
c0b5b09 
Refs: nodejs/llnode#130
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/650746
Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 3, 2017
src: fix SmartOS compilation
2e8652e 
Refs: nodejs/node-v8#8
Backport-PR-URL: #15393
PR-URL: #14730
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos deleted the v8-6.1 branch October 17, 2017 06:30
jasnell mentioned this pull request Oct 30, 2017
Merged
ofrobots mentioned this pull request May 31, 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
bnoordhuis bnoordhuis approved these changes
cjihrig cjihrig approved these changes
+1 more reviewer
ofrobots ofrobots 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

9.0.0

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL