deps: update V8 to 5.9#13263

targos · 2017-05-28T10:42:54Z

I kept out v8: fix stack overflow in recursive method because Crankshaft is disabled in 5.9.

/cc @nodejs/v8

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

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

targos · 2017-05-28T11:39:08Z

There is one failing V8 test: https://ci.nodejs.org/job/node-test-commit-v8-linux/720/

jasnell · 2017-05-28T20:58:24Z

In order to make things easiest, please don't land this until the 8.0.0 release is out.

targos · 2017-05-29T10:46:45Z

I cannot reproduce the V8 test failure on Fedora 25.

targos · 2017-05-30T08:21:36Z

Added 22b3ad952d4c0e96610ae6604febf5be5d372790 to fix the Windows issue.
New CI: https://ci.nodejs.org/job/node-test-commit/10238/

targos · 2017-05-31T10:48:41Z

Another V8 CI run: https://ci.nodejs.org/job/node-test-commit-v8-linux/723/
The test seems to be flaky.

gibfahn

LGTM on the basis that node CI is green.

gibfahn · 2017-05-31T12:01:07Z

v8tests.mjsunit/es6/typedarray-construct-offset-not-smi certainly seems flaky, do you know if there's an open issue for that?

jbajwa · 2017-05-31T17:22:01Z

Looks like the issue is similar to #6678, where OOM killer kicks in and kills the process.

[7242863.995926] [34119] 1004 34119 671796 449914 1796 0 0 d8 [7242863.995927] Out of memory: Kill process 34119 (d8) score 224 or sacrifice child [7242863.995929] Killed process 34119 (d8) total-vm:2687184kB, anon-rss:1799632kB, file-rss:24kB

in typedarray-construct-offset-not-smi.js there is an allocation of 2G block of memory

var non_smi_byte_length = %_MaxSmi() + 11; // <<-- 2147483658 try{var buffer = new ArrayBuffer(non_smi_byte_length)} catch (e){

Following the same approach as #6678, following patch should fix this flaky behavior

diff --git a/deps/v8/test/mjsunit/mjsunit.status b/deps/v8/test/mjsunit/mjsunit.status index 66ecfcf..c8c3c4e 100644 --- a/deps/v8/test/mjsunit/mjsunit.status+++ b/deps/v8/test/mjsunit/mjsunit.status@@ -148,7 +148,7 @@ # Slow tests. 'copy-on-write-assert': [PASS, SLOW], 'es6/tail-call-megatest*': [PASS, SLOW, FAST_VARIANTS, ['tsan', SKIP]], - 'es6/typedarray-construct-offset-not-smi': [PASS, SLOW],+ 'es6/typedarray-construct-offset-not-smi': [PASS, SLOW, NO_VARIANTS], 'harmony/regexp-property-script-extensions': [PASS, SLOW], 'numops-fuzz-part*': [PASS, ['mode == debug', SLOW]], 'readonly': [PASS, SLOW],

bnoordhuis · 2017-05-31T21:24:28Z

I kept out v8: fix stack overflow in recursive method because Crankshaft is disabled in 5.9.

Disabled by default, not disabled altogether. Functions that don't pass --turbo_filter=... are still directed to crankshaft. If the patch is easy to forward-port, I'd keep it for now.

bnoordhuis

The VS 2013 support patch can be dropped and wasn't the plan to keep 5.9 ABI and API compatible with 5.8? In that case bumping NODE_MODULE_VERSION isn't necessary.

(I realize the pull request in its current shape breaks API/ABI but I'm wondering what the plan is.)

addaleax · 2017-05-31T21:32:55Z

@bnoordhuis I think the plan is to have clean V8 versions on master and apply the compatibility fixes only on v8.x(-staging), so this should be fine

targos · 2017-06-01T06:40:49Z

If the patch is easy to forward-port, I'd keep it for now.

OK, I'll add it back.

The VS 2013 support patch can be dropped

Do you mean we don't support VS 2013 anymore? Then the CI jobs must change.

...wasn't the plan to keep 5.9 ABI and API compatible with 5.8?

What @addaleax said.

bnoordhuis · 2017-06-01T08:35:24Z

Do you mean we don't support VS 2013 anymore? Then the CI jobs must change.

The vs2013 buildbot is only used for the v4.x branch at this point, I think, and possibly addon tests in v6.x.

fhinkel

Thanks!

targos · 2017-06-01T12:00:04Z

@bnoordhuis see nodejs/node-v8#4 (comment)

It looks like addon tests are compiled with vs2013 for all branches.

/cc @joaocgreis

joaocgreis · 2017-06-01T12:28:36Z

We still support building native addons with VS2013. The CI machines with Windows 2008 all have VS2013 to make sure the addons test suite is run with VS2013.

psmarshall · 2017-06-01T14:38:25Z

That test allocating 2GB of memory is a bit extreme, we'll look into it on the v8 side for the future: (v8 bug)

bnoordhuis · 2017-06-01T15:29:13Z

It looks like addon tests are compiled with vs2013 for all branches.

Okay, but that means the V8 patch can be dropped. It's many things but an add-on it is not. LGTM apart from that.

targos · 2017-06-01T15:35:25Z

But without the patch, win2008r2 tests are red: https://ci.nodejs.org/job/node-test-binary-windows/8831/

bnoordhuis · 2017-06-01T15:50:52Z

Ah, I somehow had the idea that the patch only touched files in deps/v8/src. Okay, objection withdrawn.

bnoordhuis · 2017-06-01T15:58:21Z

Raised #13372 to discuss dropping support for VS 2013 in node 9.

MylesBorins

We should only land this if it has the ABI compat patches for 6.0

jasnell · 2017-06-05T22:10:24Z

Agreed. Landing this without the ABI compat patches would be a regression

addaleax · 2017-06-05T22:10:42Z

@MylesBorins Even on master? We didn’t do that for V8 5.8

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: #4 PR-URL: nodejs/node#13263 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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 PR-URL: nodejs#13263 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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: #4 PR-URL: nodejs/node#13263 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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 PR-URL: #13263 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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 PR-URL: nodejs#13263 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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: #4 PR-URL: nodejs/node#13263 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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 PR-URL: nodejs#13263 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Myles Borins <[email protected]>

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 Backport-PR-URL: #15393 PR-URL: #13263 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Myles Borins <[email protected]>

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label May 28, 2017

targos force-pushed the v8-5.9 branch from 1f65015 to 782f735Compare May 30, 2017 08:15

gibfahn approved these changes May 31, 2017
View reviewed changes

bnoordhuis requested changes May 31, 2017
View reviewed changes

targos force-pushed the v8-5.9 branch from 782f735 to 592ae8bCompare June 1, 2017 07:32

fhinkel approved these changes Jun 1, 2017
View reviewed changes

bnoordhuis approved these changes Jun 1, 2017
View reviewed changes

bnoordhuis mentioned this pull request Jun 1, 2017
build: drop support for building addons with VS 2013 in v9 #13372
Closed

refack mentioned this pull request Jun 5, 2017
net: use rest parameters instead of arguments #13472
Closed
2 tasks

MylesBorins previously requested changes Jun 5, 2017
View reviewed changes

MylesBorins mentioned this pull request Oct 3, 2017
v8.7.0 proposal #15762
Merged

jasnell mentioned this pull request Oct 30, 2017
Proposal: 2017-10-31, Version 9.0.0 (Current) #15135
Merged

Uh oh!

deps: update V8 to 5.9#13263

deps: update V8 to 5.9 #13263

Uh oh!

Conversation

targos commented May 28, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

targos commented May 28, 2017

Uh oh!

jasnell commented May 28, 2017

Uh oh!

targos commented May 29, 2017

Uh oh!

targos commented May 30, 2017

Uh oh!

targos commented May 31, 2017

Uh oh!

gibfahn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibfahn commented May 31, 2017

Uh oh!

jbajwa commented May 31, 2017• edited by gibfahn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented May 31, 2017

Uh oh!

bnoordhuis left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented May 31, 2017

Uh oh!

targos commented Jun 1, 2017

Uh oh!

bnoordhuis commented Jun 1, 2017

Uh oh!

fhinkel left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Jun 1, 2017

Uh oh!

joaocgreis commented Jun 1, 2017

Uh oh!

psmarshall commented Jun 1, 2017

Uh oh!

bnoordhuis commented Jun 1, 2017

Uh oh!

targos commented Jun 1, 2017

Uh oh!

bnoordhuis commented Jun 1, 2017

Uh oh!

bnoordhuis commented Jun 1, 2017

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 5, 2017

Uh oh!

addaleax commented Jun 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

targos commented May 28, 2017•
edited
Loading

gibfahn left a comment •
edited
Loading

jbajwa commented May 31, 2017•
edited by gibfahn
Loading

bnoordhuis left a comment •
edited
Loading