vm: migrate to new v8 api #147

gengjiawen · 2020-03-04T11:44:41Z

Related: #144.

2020-02-27T16:48:19.9302334Z Command: out/Release/node --no-concurrent-array-buffer-freeing /home/runner/work/node-v8/node-v8/test/parallel/test-memory-usage.js
2020-02-27T16:48:19.9302741Z === release test-vm-measure-memory ===
2020-02-27T16:48:19.9303896Z Path: parallel/test-vm-measure-memory
2020-02-27T16:48:19.9304508Z --- stderr ---
2020-02-27T16:48:19.9304791Z (node:94581) ExperimentalWarning: vm.measureMemory is an experimental feature. This feature could change at any time
2020-02-27T16:48:19.9305251Z /home/runner/work/node-v8/node-v8/test/common/index.js:619
2020-02-27T16:48:19.9305505Z const crashOnUnhandledRejection = (err) => { throw err; };
2020-02-27T16:48:19.9305734Z                                              ^
2020-02-27T16:48:19.9305872Z 
2020-02-27T16:48:19.9306068Z Error [ERR_CONTEXT_NOT_INITIALIZED]: context used is not initialized
2020-02-27T16:48:19.9306275Z     at Object.measureMemory (vm.js:386:26)
2020-02-27T16:48:19.9306720Z     at Object.<anonymous> (/home/runner/work/node-v8/node-v8/test/parallel/test-vm-measure-memory.js:29:6)
2020-02-27T16:48:19.9307022Z     at Module._compile (internal/modules/cjs/loader.js:1202:30)
2020-02-27T16:48:19.9307241Z     at Object.Module._extensions..js (internal/modules/cjs/loader.js:1222:10)
2020-02-27T16:48:19.9307465Z     at Module.load (internal/modules/cjs/loader.js:1051:32)
2020-02-27T16:48:19.9308062Z     at Function.Module._load (internal/modules/cjs/loader.js:947:14)
2020-02-27T16:48:19.9308929Z     at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
2020-02-27T16:48:19.9309325Z     at internal/main/run_main_module.js:17:47 {
2020-02-27T16:48:19.9310367Z   code: 'ERR_CONTEXT_NOT_INITIALIZED'
2020-02-27T16:48:19.9310723Z }

Original commit message: [testrunner] delete ancient junit compatible format support Testrunner has ancient support for JUnit compatible XML output. This CL removes this old feature. R=mstarzinger@chromium.org,jgruber@chromium.org,jkummerow@chromium.org CC=machenbach@chromium.org Bug: v8:8728 Change-Id: I7e1beb011dbaec3aa1a27398a5c52abdd778eaf0 Reviewed-on: https://chromium-review.googlesource.com/c/1430065 Reviewed-by: Jakob Gruber <jgruber@chromium.org> Reviewed-by: Michael Starzinger <mstarzinger@chromium.org> Commit-Queue: Tamer Tas <tmrts@chromium.org> Cr-Commit-Position: refs/heads/master@{#59045} Refs: v8/v8@bd019bd PR-URL: nodejs/node#26685 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: nodejs/node#26685 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Patch V8 (compiler/js-heap-broker.cc) to remove the use of an optional property, which is a fairly new C++ feature, since that requires a newer XCode version than the minimum requirement in BUILDING.md and thus breaks CI. PR-URL: nodejs/node#29694 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

This commit updates V8's postmortem metadata generation script to support V8 7.8. The following metadata has changed: - v8dbg_class_JSDate__value__Object - The postmortem metadata generation script needed to be updated. No action should be required by postmortem tools. - v8dbg_class_JSRegExp__source__Object - The postmortem metadata generation script needed to be updated. No action should be required by postmortem tools. PR-URL: nodejs/node#29694 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>

This commit updates V8's postmortem metadata generation script. This commit re-exposes the v8dbg_class_UncompiledData__inferred_name__String constant after it moved to Torque. PR-URL: nodejs/node#30020 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Fixes a compilation issue on some platforms PR-URL: nodejs/node#27375 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This should be semver-patch since actual invocation is version conditional. PR-URL: nodejs/node#27375 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

There is a bug in the most recent version of VS2015 that affects v8.h and therefore prevents compilation of addons. Refs: https://stackoverflow.com/q/38378693 PR-URL: nodejs/node#30020 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: nodejs#119

Co-authored-by: Ujjwal Sharma <usharma1998@gmail.com>

These methods will be removed in V8 8.1, hence we need to stop overriding them.

This reverts commit 5981fb7.

until a16b2c7eef289ddb1007193c1772b7c5f74190cc

until bed702fa07fae0d450b3907b29b4e21d1f41d0b8

This commit updates V8's postmortem metadata generation script to re-expose the v8dbg_class_Map__constructor_or_backpointer__Object constant.

The following metadata has changed in V8: - v8dbg_class_ConsString__first_offset__int - Use v8dbg_class_ConsString__first__String instead. - Refs: v8/v8@b38dfaf - v8dbg_class_ConsString__second_offset__int - Use v8dbg_class_ConsString__second__String instead. - Refs: v8/v8@b38dfaf - v8dbg_class_Map__constructor_or_backpointer__Object - This constant has not changed, but the postmortem metadata generation script required an update. - v8dbg_class_SlicedString__offset_offset__int - Use v8dbg_class_SlicedString__offset__SMI instead. - Refs: v8/v8@b38dfaf - v8dbg_class_ThinString__actual_offset__int - Use v8dbg_class_ThinString__actual__String instead. - Refs: v8/v8@b38dfaf

This commit updates v8abbr.h to use the updated metadata.

until 6bd85fc611dc66d08e6f0708c1b62bf4df3c1216

until 2f205f5a3343b37d3125af88efd925ab300a290e

until 221f4068d34da39726cfd0ac4d746a02e33edde5

until bc0c25b4a0cd29d12bb5acb800b85dbb265580cb

until 15ec4a09d35b31bad7c30db3b639a34e07646317

Fixes the following warning times a gazillion: <command-line>: warning: ISO C++11 requires whitespace after the macro name

It only uses adler32() and only in one place that's arguably not very performance critical so simplify the build by building just adler32.c and nothing else.

Bump minimum version of ICU needed to build node to 65. Refs: v8/v8@74bf96e

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

gengjiawen · 2020-03-04T11:45:47Z

Not sure this is the best way to fix it. The previous api won't work anymore.

cclauss · 2020-03-04T12:44:13Z

Interesting that the CI / lint-js tests are passing but the Python 3 testing / build (3.8) tests are failing on a JavaScript assertion error. ;-)

src/node_contextify.cc

gengjiawen · 2020-03-04T12:46:27Z

Interesting that the CI / lint-js are passing but the Python 3 testing / build (3.8) tests are failing on a JavaScript assertion error. ;-)

The left failed case beside ws connect (fixed in upstream) both related to gc. I suspect gc implementation changed in v8. But I have no clud why. @addaleax may have the answer.

addaleax · 2020-03-04T13:03:22Z

Yeah, I would assume that something recently changed with respect to some aspects of GC in V8…

richardlau · 2020-03-04T13:07:06Z

Interesting that the CI / lint-js tests are passing but the Python 3 testing / build (3.8) tests are failing on a JavaScript assertion error. ;-)

It just demonstrates that static analysis of the JavaScript source (CI / lint-js) alone doesn’t catch errors occurring in running tests.

gengjiawen · 2020-03-04T13:07:33Z

Yeah, I would assume that something recently changed with respect to some aspects of GC in V8…

Should we report it to https://github.com/v8/node or you can take a look ?
Those two should be the last issues for main repo upgrade to v8 8.2.

addaleax · 2020-03-04T13:08:38Z

@gengjiawen I think we should report these as issues on this repository? That’s the way we usually deal with it afaik

gengjiawen · 2020-03-04T13:09:37Z

@gengjiawen I think we should report these as issues on this repository? That’s the way we usually deal with it afaik

Already have, see #144. (I also put it in PR description).

gengjiawen · 2020-03-08T09:51:32Z

Launched via 591b4d2.

mmarchini · 2020-03-08T10:11:07Z

IIRC this PR and 591b4d2 were not the same, but I'll try to double-check tomorrow

nodejs/node-v8#147 broke the `vm.measureMemory()` API. It only created a `MeasureMemoryDelegate` and without actually calling `v8::Isolate::MeasureMemory()` so the returned promise will never resolve. This was not caught by the tests because the promise resolvers were not wrapped with `common.mustCall()`. This patch migrates the API properly and also introduce the newly added execution option to the API. It also removes support for specifying contexts to measure - instead we'll just return the measurements for all contexts in the detailed mode, which is what the `performance.measureMemory()` prototype in V8 currently does. We can consider implementing our own `v8::MeasureMemoryDelegate` to select the target context in `ShouldMeasure()` in the future, but then we'll also need to implement `MeasurementComplete()` to assemble the result. For now it's probably too early to do that. Since this API is still experimental (and guarded with a warning), such breakage should be acceptable. Refs: nodejs/node-v8#147

nodejs/node-v8#147 broke the `vm.measureMemory()` API. It only created a `MeasureMemoryDelegate` and without actually calling `v8::Isolate::MeasureMemory()` so the returned promise will never resolve. This was not caught by the tests because the promise resolvers were not wrapped with `common.mustCall()`. This patch migrates the API properly and also introduce the newly added execution option to the API. It also removes support for specifying contexts to measure - instead we'll just return the measurements for all contexts in the detailed mode, which is what the `performance.measureMemory()` prototype in V8 currently does. We can consider implementing our own `v8::MeasureMemoryDelegate` to select the target context in `ShouldMeasure()` in the future, but then we'll also need to implement `MeasurementComplete()` to assemble the result. For now it's probably too early to do that. Since this API is still experimental (and guarded with a warning), such breakage should be acceptable. Refs: nodejs/node-v8#147 PR-URL: #32988 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-ci and others added 30 commits March 3, 2020 00:56

doc: update README for node-v8 repository

d91a42d

deps: update V8 to 8.2.237

a295ca6

build: reset embedder string to "-node.0"

6aae14a

deps: V8: silence irrelevant warnings

b16cfdd

PR-URL: nodejs/node#26685 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

build: enable backtrace when V8 is built for PPC and S390x

ecb8cea

Refs: nodejs#119

tools: update V8 gypfiles for V8 8.0

c9d3538

Co-authored-by: Ujjwal Sharma <usharma1998@gmail.com>

src: stop overriding deprecated V8 methods

b308552

These methods will be removed in V8 8.1, hence we need to stop overriding them.

Revert "assert: fix line number calculation after V8 upgrade"

ec140df

This reverts commit 5981fb7.

tools: update inspector.gypi

b5c1c8e

until a16b2c7eef289ddb1007193c1772b7c5f74190cc

tools: update v8.gyp

4867f9a

until bed702fa07fae0d450b3907b29b4e21d1f41d0b8

try to fix include in zlib.gyp

a0cc68a

deps: update V8's postmortem script

885a556

This commit updates V8's postmortem metadata generation script to re-expose the v8dbg_class_Map__constructor_or_backpointer__Object constant.

src: update v8abbr.h for V8 update

5152104

This commit updates v8abbr.h to use the updated metadata.

tools: try to fix zlib for cross-compilation

3c1f76a

tools: update v8.gyp

085ad05

until 6bd85fc611dc66d08e6f0708c1b62bf4df3c1216

tools: update gypfiles

91f574a

until 2f205f5a3343b37d3125af88efd925ab300a290e

tools: update features.gypi

db9ff5b

until 221f4068d34da39726cfd0ac4d746a02e33edde5

tools: update v8.gyp

b2cb8f2

until bc0c25b4a0cd29d12bb5acb800b85dbb265580cb

tools: update v8.gyp

1e21691

until 15ec4a09d35b31bad7c30db3b639a34e07646317

fixup! fix typo from commit 3078bcd

7b5322c

Fixes the following warning times a gazillion: <command-line>: warning: ISO C++11 requires whitespace after the macro name

build: simplify v8's vendored zlib build

fa2420b

It only uses adler32() and only in one place that's arguably not very performance critical so simplify the build by building just adler32.c and nothing else.

deps: bump minimum icu version to 65

3d7ad5d

Bump minimum version of ICU needed to build node to 65. Refs: v8/v8@74bf96e

syg and others added 2 commits March 3, 2020 00:57

Rename FinalizationGroup to FinalizationRegistry

2ab89a2

v8: include right headers in torque output

535dc9b

Co-authored-by: Ben Noordhuis <info@bnoordhuis.nl>

gengjiawen requested a review from joyeecheung March 4, 2020 11:44

gengjiawen mentioned this pull request Mar 4, 2020

Blockers for the update to V8 8.0 #127

Closed

11 tasks

addaleax approved these changes Mar 4, 2020

View reviewed changes

src/node_contextify.cc Outdated Show resolved Hide resolved

src/node_contextify.cc Show resolved Hide resolved

vm: migrate measureMemory to new v8 api

d1a1e20

gengjiawen force-pushed the bug/vm_measure branch from 38f472a to d1a1e20 Compare March 4, 2020 13:11

This was referenced Mar 4, 2020

src: refactor to more safe method nodejs/node#32087

Closed

Update canary-base to remove stale commits #148

Closed

joyeecheung approved these changes Mar 6, 2020

View reviewed changes

cjihrig approved these changes Mar 6, 2020

View reviewed changes

nodejs-ci force-pushed the canary branch from 535dc9b to 1e24bf6 Compare March 8, 2020 07:54

gengjiawen closed this Mar 8, 2020

gengjiawen deleted the bug/vm_measure branch March 8, 2020 09:51

joyeecheung mentioned this pull request Apr 22, 2020

vm: fix vm.measureMemory() and introduce execution option nodejs/node#32988

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: migrate to new v8 api #147

vm: migrate to new v8 api #147

gengjiawen commented Mar 4, 2020 •

edited

Loading

gengjiawen commented Mar 4, 2020

cclauss commented Mar 4, 2020 •

edited

Loading

gengjiawen commented Mar 4, 2020 •

edited

Loading

addaleax commented Mar 4, 2020

richardlau commented Mar 4, 2020 •

edited

Loading

gengjiawen commented Mar 4, 2020

addaleax commented Mar 4, 2020

gengjiawen commented Mar 4, 2020 •

edited

Loading

gengjiawen commented Mar 8, 2020

mmarchini commented Mar 8, 2020

vm: migrate to new v8 api #147

vm: migrate to new v8 api #147

Conversation

gengjiawen commented Mar 4, 2020 • edited Loading

gengjiawen commented Mar 4, 2020

cclauss commented Mar 4, 2020 • edited Loading

gengjiawen commented Mar 4, 2020 • edited Loading

addaleax commented Mar 4, 2020

richardlau commented Mar 4, 2020 • edited Loading

gengjiawen commented Mar 4, 2020

addaleax commented Mar 4, 2020

gengjiawen commented Mar 4, 2020 • edited Loading

gengjiawen commented Mar 8, 2020

mmarchini commented Mar 8, 2020

gengjiawen commented Mar 4, 2020 •

edited

Loading

cclauss commented Mar 4, 2020 •

edited

Loading

gengjiawen commented Mar 4, 2020 •

edited

Loading

richardlau commented Mar 4, 2020 •

edited

Loading

gengjiawen commented Mar 4, 2020 •

edited

Loading