[8.x] deps: V8: backport 49712d8a from upstream #21334

ofrobots · 2018-06-14T21:33:59Z

Original commit message:

[wasm] Call AsyncInstantiate directly when instantiating a module object

WebAssembly.instantiate is polymorphic, it can either take a module
object as parameter, or a buffer source which should be compiled first.
To share code between the two implementations, the module object was
first passed to a promise (i.e. which is the result of compilation).
However, passing the module object to a promise has a side effect if
the module object has a then function. To avoid this side effect I
remove this code sharing and call AsyncInstantiate directly in case
the parameter is a module object.

R=mstarzinger@chromium.org

Bug: chromium:836141
Change-Id: I67b76d0d7761c5aeb2cf1deda45b6842e494eed4
Reviewed-on: https://chromium-review.googlesource.com/1025774
Reviewed-by: Michael Starzinger mstarzinger@chromium.org
Commit-Queue: Andreas Haas ahaas@chromium.org
Cr-Commit-Position: refs/heads/master@{#52755}

Checklist

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

This effects 9.x as well, but I expect it is not worthwhile fixing there at this point.

/cc @nodejs/v8-update

V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1425/

nodejs-github-bot · 2018-06-14T21:34:00Z

@ofrobots build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/67/pipeline

BridgeAR · 2018-06-18T10:22:37Z

deps/v8/src/wasm/wasm-js.cc

+  Local<Value> ffi = args[1];
+  i::Handle<i::Object> first_arg = Utils::OpenHandle(*first_arg_value);
+  if (!first_arg->IsJSObject()) {
+>>>>>>>


Seems like some merge conflicts sneaked in.

Yikes. Fixed.

ofrobots · 2018-06-18T19:06:52Z

Fixed the merge conflict. This didn't land cleanly on 8.x and tweaks were necessary. Changed title from cherry-pick to a backport.

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

ofrobots · 2018-06-18T19:07:56Z

@nodejs/build @mhdawson the V8-CI seems to be hitting infra issues AFAICT. Any ideas?

15:03:01 In file included from ../src/setup-isolate-full.cc:7:
15:03:01 .././src/base/logging.h:8:10: fatal error: 'cstring' file not found
15:03:01 #include <cstring>
15:03:01          ^~~~~~~~~

mmarchini · 2018-06-18T22:42:28Z

make test-v8 works on my machine, confirming this is an infra issue. But let's do another run, just in case:

https://ci.nodejs.org/job/node-test-commit-v8-linux/1442/

ofrobots · 2018-06-19T14:39:27Z

@mhdawson @nodejs/build ping. Several V8 back-port are blocked because of this.

mmarchini · 2018-06-19T16:57:51Z

It's not happening on all V8 backports though. Maybe it's only happening to v8.x backports?

richardlau · 2018-06-19T18:13:07Z

It's not happening on all V8 backports though. Maybe it's only happening to v8.x backports?

The compiler level used on LinuxOne (s390x), ppcle Linux and AIX is based on the version of Node.js (i.e., >9): https://github.com/nodejs/build/blob/master/jenkins/scripts/select-compiler.sh

mhdawson · 2018-06-19T20:47:42Z

There seem to be a few issues. I fixed IBM platform specific ones related to changes made to build with GN. But those were not what was reported here.

opened nodejs/build#1370 as it seems to fail for other reasons on x86 for 10.x

mhdawson · 2018-06-19T20:51:09Z

It seems to fail on V8.x without any other changes, @mmarchini confirmed it fails the same for V8.x-staging.

mhdawson · 2018-06-19T20:57:03Z

Cleaned the workspace, if that still fails then my next guess is that something landed in v8.x v8.x-staging which broke it. Run on v8.x after cleaning workspace: https://ci.nodejs.org/job/node-test-commit-v8-linux/1456/

mhdawson · 2018-06-19T21:00:51Z

@richardlau I don't know what you are trying to point out? It fails across the board for v8.x including x86.

ofrobots · 2018-06-19T21:07:24Z

Relaunched V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1457/

mhdawson · 2018-06-19T21:10:21Z

Build to see if it passed on 6.x or not https://ci.nodejs.org/job/node-test-commit-v8-linux/1458/

It still failed on 8.x. If it passes on 6.x then I think its something in the 8.x line. .

ofrobots · 2018-06-19T21:15:31Z

It failed on my PR branch.

Here's a build of the v8.x-staging branch: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1459/.

mhdawson · 2018-06-19T21:24:33Z

Workspace clean then build on 6.x - https://ci.nodejs.org/job/node-test-commit-v8-linux/1460/

richardlau · 2018-06-19T21:26:15Z

@richardlau I don't know what you are trying to point out? It fails across the board for v8.x including x86.

My apologies, I didn't spot that nodes=benchmark was x64.

ofrobots · 2018-06-19T21:29:41Z

All failed.

mhdawson · 2018-06-19T21:34:20Z

Failed for a different reason on 6.x. I'm wondering if it has to do with the google tooling required to build.

@MylesBorins when was the last time these worked on 6.x and 8.x?
@ofrobots can you run 8.x on a linux machine outside the ci to see if its related to the build machine or something else?

ofrobots · 2018-06-19T23:35:38Z

make test-v8 runs fine on my linux machine, and it seems for @mmarchini as per #21334 (comment). This seems like an infra issue.

MylesBorins · 2018-06-20T15:07:41Z

we had V8 ci pass on 8.x-staging recently https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1426/

mhdawson · 2018-06-20T19:16:58Z

@mmarchini mentioned his test run was on OSX not linux, so not necessary confirmation one way or the other.

mmarchini · 2018-06-20T20:54:55Z

Same failures while running tests on another CI machine:
https://ci.nodejs.org/job/node-test-commit-v8-linux/1472/nodes=ubuntu1604-intel-64,v8test=v8test/console

I'm working on it, but might take some time to track down and fix this one.

mmarchini · 2018-06-20T22:01:21Z

I was able to reproduce in an Ubuntu 16.04 x64 VM on my computer, so this is definitely not a problem with our infrastructure. Let me see if I can find when this started to happen.

mhdawson · 2018-06-20T22:12:47Z

The other thing I noticed was it looks like the passing builds (10.x and later) use g++ while the failing ones use clang (8.x). Not sure why what would be the case.

Also the failures on ppc and s390 look different, have asked @mmallick-ca to take a look at what might be going on there. Sounds like gclient sync is not pulling in something that is required.

mhdawson · 2018-06-20T22:17:05Z

At this point I'll wait to see what @mmarchini figures out before spending any more time on this.

ofrobots · 2018-06-28T20:36:01Z

New V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1496/

@nodejs/lts @nodejs/v8-update this backport needs an LGTM before it can land.

Original commit message: [wasm] Call AsyncInstantiate directly when instantiating a module object WebAssembly.instantiate is polymorphic, it can either take a module object as parameter, or a buffer source which should be compiled first. To share code between the two implementations, the module object was first passed to a promise (i.e. which is the result of compilation). However, passing the module object to a promise has a side effect if the module object has a then function. To avoid this side effect I remove this code sharing and call AsyncInstantiate directly in case the parameter is a module object. R=mstarzinger@chromium.org Bug: chromium:836141 Change-Id: I67b76d0d7761c5aeb2cf1deda45b6842e494eed4 Reviewed-on: https://chromium-review.googlesource.com/1025774 Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Commit-Queue: Andreas Haas <ahaas@chromium.org> Cr-Commit-Position: refs/heads/master@{nodejs#52755}

ofrobots · 2018-07-02T22:23:26Z

@nodejs/lts @nodejs/v8-update another ping. This needs an LGTM. A rubber stamp would do.

targos

rubber stamp

ofrobots · 2018-07-03T18:39:45Z

~~Actually, I was mistaken, these fixes not in 10 just yet. They are in V8 6.8 (rather than 6.7). Let's hold off on landing this until a 10.x release picks them up.~~

ofrobots · 2018-07-09T20:53:01Z

Ooops, I intended to add the above to #21558.

MylesBorins · 2018-07-13T20:21:48Z

landed in 2afc05e

Original commit message: [wasm] Call AsyncInstantiate directly when instantiating a module object WebAssembly.instantiate is polymorphic, it can either take a module object as parameter, or a buffer source which should be compiled first. To share code between the two implementations, the module object was first passed to a promise (i.e. which is the result of compilation). However, passing the module object to a promise has a side effect if the module object has a then function. To avoid this side effect I remove this code sharing and call AsyncInstantiate directly in case the parameter is a module object. R=mstarzinger@chromium.org Bug: chromium:836141 Change-Id: I67b76d0d7761c5aeb2cf1deda45b6842e494eed4 Reviewed-on: https://chromium-review.googlesource.com/1025774 Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Commit-Queue: Andreas Haas <ahaas@chromium.org> Cr-Commit-Position: refs/heads/master@{#52755} PR-URL: #21334 Reviewed-By: Michaël Zasso <targos@protonmail.com>

nodejs-github-bot added v8 engine Issues and PRs related to the V8 dependency. v8.x labels Jun 14, 2018

ofrobots requested review from targos and bnoordhuis June 14, 2018 21:41

BridgeAR reviewed Jun 18, 2018

View reviewed changes

ofrobots changed the title ~~[8.x] deps: V8: cherrypick 49712d8a from upstream~~ [8.x] deps: V8: backport 49712d8a from upstream Jun 18, 2018

ofrobots force-pushed the backport/8/836141 branch from 2da030f to eeab6c1 Compare June 18, 2018 18:58

This was referenced Jun 21, 2018

make test-v8 is broken on Node.js v8.x #21433

Closed

node-test-commit-v8-linux: use test-nearform_intel-ubuntu1604-x64-1 instead of infra-softlayer-ubuntu1404-x64-2 nodejs/build#1381

Closed

ofrobots added the blocked PRs that are blocked by other issues or PRs. label Jun 28, 2018

ofrobots removed the blocked PRs that are blocked by other issues or PRs. label Jun 28, 2018

ofrobots force-pushed the backport/8/836141 branch from eeab6c1 to 915ec4f Compare July 2, 2018 22:22

targos approved these changes Jul 3, 2018

View reviewed changes

ofrobots mentioned this pull request Jul 9, 2018

[8.x] deps: V8: backport 20 CPU profiler commits from upstream #21558

Closed

2 tasks

MylesBorins closed this Jul 13, 2018

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[8.x] deps: V8: backport 49712d8a from upstream #21334

[8.x] deps: V8: backport 49712d8a from upstream #21334

ofrobots commented Jun 14, 2018 •

edited

Loading

nodejs-github-bot commented Jun 14, 2018

BridgeAR Jun 18, 2018

ofrobots Jun 18, 2018

ofrobots commented Jun 18, 2018

ofrobots commented Jun 18, 2018

mmarchini commented Jun 18, 2018

ofrobots commented Jun 19, 2018

mmarchini commented Jun 19, 2018

richardlau commented Jun 19, 2018

mhdawson commented Jun 19, 2018

mhdawson commented Jun 19, 2018

mhdawson commented Jun 19, 2018

mhdawson commented Jun 19, 2018

ofrobots commented Jun 19, 2018

mhdawson commented Jun 19, 2018

ofrobots commented Jun 19, 2018

mhdawson commented Jun 19, 2018

richardlau commented Jun 19, 2018

ofrobots commented Jun 19, 2018

mhdawson commented Jun 19, 2018

ofrobots commented Jun 19, 2018

MylesBorins commented Jun 20, 2018

mhdawson commented Jun 20, 2018

mmarchini commented Jun 20, 2018

mmarchini commented Jun 20, 2018

mhdawson commented Jun 20, 2018

mhdawson commented Jun 20, 2018

ofrobots commented Jun 28, 2018

ofrobots commented Jul 2, 2018

targos left a comment

ofrobots commented Jul 3, 2018 •

edited

Loading

ofrobots commented Jul 9, 2018

MylesBorins commented Jul 13, 2018

[8.x] deps: V8: backport 49712d8a from upstream #21334

[8.x] deps: V8: backport 49712d8a from upstream #21334

Conversation

ofrobots commented Jun 14, 2018 • edited Loading

Checklist

nodejs-github-bot commented Jun 14, 2018

BridgeAR Jun 18, 2018

Choose a reason for hiding this comment

ofrobots Jun 18, 2018

Choose a reason for hiding this comment

ofrobots commented Jun 18, 2018

ofrobots commented Jun 18, 2018

mmarchini commented Jun 18, 2018

ofrobots commented Jun 19, 2018

mmarchini commented Jun 19, 2018

richardlau commented Jun 19, 2018

mhdawson commented Jun 19, 2018

mhdawson commented Jun 19, 2018

mhdawson commented Jun 19, 2018

mhdawson commented Jun 19, 2018

ofrobots commented Jun 19, 2018

mhdawson commented Jun 19, 2018

ofrobots commented Jun 19, 2018

mhdawson commented Jun 19, 2018

richardlau commented Jun 19, 2018

ofrobots commented Jun 19, 2018

mhdawson commented Jun 19, 2018

ofrobots commented Jun 19, 2018

MylesBorins commented Jun 20, 2018

mhdawson commented Jun 20, 2018

mmarchini commented Jun 20, 2018

mmarchini commented Jun 20, 2018

mhdawson commented Jun 20, 2018

mhdawson commented Jun 20, 2018

ofrobots commented Jun 28, 2018

ofrobots commented Jul 2, 2018

targos left a comment

Choose a reason for hiding this comment

ofrobots commented Jul 3, 2018 • edited Loading

ofrobots commented Jul 9, 2018

MylesBorins commented Jul 13, 2018

ofrobots commented Jun 14, 2018 •

edited

Loading

ofrobots commented Jul 3, 2018 •

edited

Loading