[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=[email protected]
Bug: chromium:836141
Change-Id: I67b76d0d7761c5aeb2cf1deda45b6842e494eed4
Reviewed-on: https://chromium-review.googlesource.com/1025774
Reviewed-by: Michael Starzinger [email protected]
Commit-Queue: Andreas Haas [email protected]
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

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. [email protected] Bug: chromium:836141 Change-Id: I67b76d0d7761c5aeb2cf1deda45b6842e494eed4 Reviewed-on: https://chromium-review.googlesource.com/1025774 Reviewed-by: Michael Starzinger <[email protected]> Commit-Queue: Andreas Haas <[email protected]> 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. [email protected] Bug: chromium:836141 Change-Id: I67b76d0d7761c5aeb2cf1deda45b6842e494eed4 Reviewed-on: https://chromium-review.googlesource.com/1025774 Reviewed-by: Michael Starzinger <[email protected]> Commit-Queue: Andreas Haas <[email protected]> Cr-Commit-Position: refs/heads/master@{#52755} PR-URL: #21334 Reviewed-By: Michaël Zasso <[email protected]>

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

ofrobots requested review from bnoordhuis and targos 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 upstreamJun 18, 2018

ofrobots force-pushed the backport/8/836141 branch from 2da030f to eeab6c1Compare 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 915ec4fCompare 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

Uh oh!

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

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

Uh oh!

Conversation

ofrobots commented Jun 14, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Jun 14, 2018

Uh oh!

BridgeARJun 18, 2018

Choose a reason for hiding this comment

Uh oh!

ofrobotsJun 18, 2018

Choose a reason for hiding this comment

Uh oh!

ofrobots commented Jun 18, 2018

Uh oh!

ofrobots commented Jun 18, 2018

Uh oh!

mmarchini commented Jun 18, 2018

Uh oh!

ofrobots commented Jun 19, 2018

Uh oh!

mmarchini commented Jun 19, 2018

Uh oh!

richardlau commented Jun 19, 2018

Uh oh!

mhdawson commented Jun 19, 2018

Uh oh!

mhdawson commented Jun 19, 2018

Uh oh!

mhdawson commented Jun 19, 2018

Uh oh!

mhdawson commented Jun 19, 2018

Uh oh!

ofrobots commented Jun 19, 2018

Uh oh!

mhdawson commented Jun 19, 2018

Uh oh!

ofrobots commented Jun 19, 2018

Uh oh!

mhdawson commented Jun 19, 2018

Uh oh!

richardlau commented Jun 19, 2018

Uh oh!

ofrobots commented Jun 19, 2018

Uh oh!

mhdawson commented Jun 19, 2018

Uh oh!

ofrobots commented Jun 19, 2018

Uh oh!

MylesBorins commented Jun 20, 2018

Uh oh!

mhdawson commented Jun 20, 2018

Uh oh!

mmarchini commented Jun 20, 2018

Uh oh!

mmarchini commented Jun 20, 2018

Uh oh!

mhdawson commented Jun 20, 2018

Uh oh!

mhdawson commented Jun 20, 2018

Uh oh!

ofrobots commented Jun 28, 2018

Uh oh!

ofrobots commented Jul 2, 2018

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

ofrobots commented Jul 3, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ofrobots commented Jul 9, 2018

Uh oh!

MylesBorins commented Jul 13, 2018

Uh oh!

Reviewers

ofrobots commented Jun 14, 2018•
edited
Loading

ofrobots commented Jul 3, 2018•
edited
Loading