deps: update V8 to 6.1 #14730

targos · 2017-08-10T08:55:58Z

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

bnoordhuis

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 · 2017-08-10T16:02:52Z

@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 · 2017-08-10T16:45:05Z

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

refack · 2017-08-10T16:49:51Z

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

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

bnoordhuis · 2017-08-10T21:14:56Z

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 · 2017-08-11T06:37:57Z

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

jbajwa · 2017-08-15T16:00:13Z

@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 · 2017-08-15T16:17:04Z

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

targos · 2017-08-16T14:24:47Z

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 · 2017-08-16T14:39:10Z

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

rvagg · 2017-08-17T11:58:16Z

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 · 2017-08-17T11:58:51Z

@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 · 2017-08-17T12:20:43Z

@rvagg done

evanlucas · 2017-08-17T12:40:13Z

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 · 2017-08-17T13:07:28Z

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 · 2017-08-17T13:38:02Z

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

refack · 2017-08-17T15:07:08Z

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 · 2017-08-17T15:12:24Z

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.

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 · 2017-08-20T11:30:23Z

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 · 2017-09-13T15:05:16Z

@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 · 2017-09-13T15:18:21Z

@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 · 2017-09-13T15:54:16Z

Awesome work! 👍

refack · 2017-09-13T16:48:47Z

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.

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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 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

matthewloring mentioned this pull request Aug 10, 2017

test: test-cluster-send-handle-large-payload intermittent failures #14747

Closed

targos mentioned this pull request Aug 11, 2017

build: stop support building addons with VS 2013 #14764

Closed

2 tasks

targos force-pushed the v8-6.1 branch from 3197c37 to 8cbd785 Compare August 16, 2017 14:18

targos force-pushed the v8-6.1 branch from 8cbd785 to e9d63e8 Compare August 17, 2017 12:20

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

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

targos deleted the v8-6.1 branch October 17, 2017 06:30

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

ofrobots mentioned this pull request May 31, 2018

External memory leak on node v8.x #21021

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps: update V8 to 6.1 #14730

deps: update V8 to 6.1 #14730

targos commented Aug 10, 2017 •

edited by refack

Loading

bnoordhuis left a comment

misterdjules commented Aug 10, 2017

refack commented Aug 10, 2017

refack commented Aug 10, 2017

bnoordhuis commented Aug 10, 2017

targos commented Aug 11, 2017

jbajwa commented Aug 15, 2017

refack commented Aug 15, 2017

targos commented Aug 16, 2017

jbajwa commented Aug 16, 2017

rvagg commented Aug 17, 2017

rvagg commented Aug 17, 2017

targos commented Aug 17, 2017

evanlucas commented Aug 17, 2017

gibfahn commented Aug 17, 2017

bricss commented Aug 17, 2017 •

edited

Loading

refack commented Aug 17, 2017

addaleax commented Aug 17, 2017

rvagg commented Aug 20, 2017

targos commented Sep 13, 2017

misterdjules commented Sep 13, 2017

bmeurer commented Sep 13, 2017

refack commented Sep 13, 2017 •

edited

Loading

deps: update V8 to 6.1 #14730

deps: update V8 to 6.1 #14730

Conversation

targos commented Aug 10, 2017 • edited by refack Loading

Checklist

Affected core subsystem(s)

bnoordhuis left a comment

Choose a reason for hiding this comment

misterdjules commented Aug 10, 2017

refack commented Aug 10, 2017

refack commented Aug 10, 2017

bnoordhuis commented Aug 10, 2017

targos commented Aug 11, 2017

jbajwa commented Aug 15, 2017

refack commented Aug 15, 2017

targos commented Aug 16, 2017

jbajwa commented Aug 16, 2017

rvagg commented Aug 17, 2017

rvagg commented Aug 17, 2017

targos commented Aug 17, 2017

evanlucas commented Aug 17, 2017

gibfahn commented Aug 17, 2017

bricss commented Aug 17, 2017 • edited Loading

refack commented Aug 17, 2017

addaleax commented Aug 17, 2017

rvagg commented Aug 20, 2017

targos commented Sep 13, 2017

misterdjules commented Sep 13, 2017

bmeurer commented Sep 13, 2017

refack commented Sep 13, 2017 • edited Loading

targos commented Aug 10, 2017 •

edited by refack

Loading

bricss commented Aug 17, 2017 •

edited

Loading

refack commented Sep 13, 2017 •

edited

Loading