Allow --perf-prof and --perf-basic-prof in NODE_OPTIONS by Leko · Pull Request #17600 · nodejs/node

Leko · 2017-12-11T04:20:31Z

This PR closes #17571

I added --perf-prof and --perf-basic-prof to whitelist.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

src

…E_OPTIONS

Leko · 2017-12-11T04:20:54Z

Please help me in solving error.
I got an error when run with --perf-prof in NODE_OPTIONS

$ NODE_OPTIONS=--perf-prof ./node


#
# Fatal error in , line 0
# unimplemented code
#
Illegal instruction: 4

When run with --perf-basic-prof, it works fine.

$ NODE_OPTIONS=--perf-basic-prof ./node
> // It is OK

What means of unimplemented code ?
What should I do ?

joyeecheung · 2017-12-11T09:11:13Z

I think you were running it on MacOS? PerfJitLogger is only implemented in Linux.

Leko · 2017-12-11T09:21:45Z

Thank you for your help @joyeecheung.

I think you were running it on MacOS?

Yes it is.
I'll use Linux in this PR.

I have a new question.
Should I avoid to test about perf-prof ?
It might be fail not Linux environment.

bnoordhuis · 2017-12-11T11:05:36Z

src/node.cc

    "--openssl-config",
    "--icu-data-dir",
+    "--perf-prof",
+    "--perf-basic-prof",


Move them below the comment and use underscores.

@bnoordhuis Thank you for review !
I moved them under v8 options.

perf-prof works in Linux only. For example, it not works in MacOS.

Leko · 2017-12-11T12:27:10Z

Should I avoid to test about perf-prof ?
It might be fail not Linux environment.

I use common.isLinux like:

if (common.isLinux) {
  expect('--perf-prof', 'B\n');
}

It is correct ?

cjihrig · 2017-12-11T13:32:57Z

test/parallel/test-cli-node-options.js

+
+if (common.isLinux) {
+  // PerfJitLogger is only implemented in Linux.
+  // https://github.com/nodejs/node/pull/17600#issuecomment-350664113


I think it's safe to drop the GitHub link.

@cjihrig Thank you for review.
I got it. I removed Github link.

@Leko FWIW this is written in the comments in deps/v8/src/perf-jit.h

richardlau

Please could you also update the documented list of allowed options for NODE_OPTIONS in doc/api/cli.md?

Leko · 2017-12-12T13:00:23Z

@richardlau Thank you for your review.
I added them to doc/api/cli.md.

richardlau

One minor nit re. documentation but otherwise LGTM.

richardlau · 2017-12-12T15:22:39Z

doc/api/cli.md


 V8 options that are allowed are:
+- `--perf-prof`
+- `--perf-basic-prof`


Thanks for updating, but please could you maintain the alphabetical sort?

I got it.
I sorted options in alphabetical order.

BridgeAR

LGTM

joyeecheung · 2017-12-17T08:54:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12155/

richardlau · 2017-12-22T17:03:00Z

Both LinuxONE and PPC Linux are failing:

not ok 229 parallel/test-cli-node-options
  ---
  duration_ms: 0.737
  severity: fail
  stack: |-
    assert.js:275
    assert.ifError = function ifError(err) { if (err) throw err; };
                                                      ^
    
    Error: Command failed: /data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/out/Release/node -e console.log("B")
    
    
    #
    # Fatal error in , line 0
    # unimplemented code
    #
    
        at ChildProcess.exithandler (child_process.js:276:12)
        at ChildProcess.emit (events.js:127:13)
        at maybeClose (internal/child_process.js:943:16)
        at Socket.stream.socket.on (internal/child_process.js:363:11)
        at Socket.emit (events.js:127:13)
        at Pipe._handle.close [as _onclose] (net.js:559:12)
  ...

@nodejs/platform-s390 @nodejs/platform-ppc can you confirm that --perf-prof is not expected to work on those platforms?

richardlau · 2017-12-22T17:13:34Z

Also Windows is failing, e.g. https://ci.nodejs.org/job/node-test-binary-windows/13699/COMPILED_BY=vs2017,RUNNER=win2008r2-vs2017,RUN_SUBSET=0/console

not ok 72 parallel/test-cli-node-options
  ---
  duration_ms: 0.682
  severity: fail
  stack: |-
    assert.js:275
    assert.ifError = function ifError(err) { if (err) throw err; };
                                                      ^
    
    Error: Command failed: c:\workspace\node-test-binary-windows\Release\node.exe -e console.log("B")
    
    
    #
    # Fatal error in log.cc, line 253
    # Check failed: (perf_output_handle_) != nullptr.
    #
    
        at ChildProcess.exithandler (child_process.js:276:12)
        at ChildProcess.emit (events.js:127:13)
        at maybeClose (internal/child_process.js:943:16)
        at Process.ChildProcess._handle.onexit (internal/child_process.js:220:5)
  ...

This must be with --perf-basic-prof (because --perf-prof is only tried on Linux).

cc @nodejs/platform-windows

bnoordhuis · 2017-12-23T11:58:37Z

@richardlau Can you try this patch?

diff --git a/deps/v8/src/perf-jit.h b/deps/v8/src/perf-jit.h
index 2b0b4831e0..45e4a4a212 100644
--- a/deps/v8/src/perf-jit.h
+++ b/deps/v8/src/perf-jit.h
@@ -73,6 +73,9 @@ class PerfJitLogger : public CodeEventLogger {
   static const uint32_t kElfMachARM = 40;
   static const uint32_t kElfMachMIPS = 10;
   static const uint32_t kElfMachARM64 = 183;
+  static const uint32_t kElfMachPPC32 = 20;
+  static const uint32_t kElfMachPPC64 = 21;
+  static const uint32_t kElfMachS390 = 22;
 
   uint32_t GetElfMach() {
 #if V8_TARGET_ARCH_IA32
@@ -85,6 +88,12 @@ class PerfJitLogger : public CodeEventLogger {
     return kElfMachMIPS;
 #elif V8_TARGET_ARCH_ARM64
     return kElfMachARM64;
+#elif V8_TARGET_ARCH_PPC
+    return kElfMachPPC32;
+#elif V8_TARGET_ARCH_PPC64
+    return kElfMachPPC64;
+#elif V8_TARGET_ARCH_S390
+    return kElfMachS390;
 #else
     UNIMPLEMENTED();
     return 0;

richardlau · 2017-12-23T17:13:29Z

@bnoordhuis Thanks, but not until I'm back in the office second week of January. Perhaps someone else in @nodejs/platform-ppc and/or @nodejs/platform-s390 could try in the meantime?

Leko · 2017-12-25T13:20:19Z

common.isLinux does not work correctly?

https://github.com/Leko/node/blob/b97987ad57f2794d3d0e355c47bd8fc3796a8ea2/test/parallel/test-cli-node-options.js#L29-L32

I think --perf-prof should not evaluate in Linux one, PPC Linux and Windows.

if (common.isLinux && !common.isLinuxPPCBE && !{TODO}) {

What is the variable corresponding to test/linux-one and test/windows-fanned?

joyeecheung · 2017-12-27T04:00:11Z

@Leko Maybe you can try whitelisting instead? I think it should be common.isLinux && ['arm', 'x64', 'ia32', 'mips'].includes(process.arch).

PR-URL: #17600 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) #18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) #18354 - ICU 60.2 bump (Steven R. Loomis) #17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) #16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) #15752 * http2: - add http fallback options to .createServer (Peter Marton) #15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) #16402 * inspector: - --inspect-brk for es modules (Guy Bedford) #18194 * lib: - allow process kill by signal number (Sam Roberts) #16944 * module: - enable dynamic import (Myles Borins) #18387 - dynamic import is now supported (Jan Krems) #15713 * napi: - add methods to open/close callback scope (Michael Dawson) #18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) #17600 * vm: - add support for es modules (Gus Caplan) #17560 PR-URL: #18902

PR-URL: nodejs#17600 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Notable changes: * async_hooks: - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 - rename PromiseWrap.parentId to PromiseWrap.isChainedPromise (Ali Ijaz Sheikh) nodejs#18633 * deps: - update node-inspect to 1.11.3 (Jan Krems) nodejs#18354 - ICU 60.2 bump (Steven R. Loomis) nodejs#17687 - Introduce ScriptOrModule and HostDefinedOptions to V8 (Jan Krems) nodejs#16889 * http: - add options to http.createServer() for `IncomingMessage` and `ServerReponse` (Peter Marton) nodejs#15752 * http2: - add http fallback options to .createServer (Peter Marton) nodejs#15752 * https: - Adds the remaining options from tls.createSecureContext() to the string generated by Agent#getName(). This allows https.request() to accept the options and generate unique sockets appropriately. (Jeff Principe) nodejs#16402 * inspector: - --inspect-brk for es modules (Guy Bedford) nodejs#18194 * lib: - allow process kill by signal number (Sam Roberts) nodejs#16944 * module: - enable dynamic import (Myles Borins) nodejs#18387 - dynamic import is now supported (Jan Krems) nodejs#15713 * napi: - add methods to open/close callback scope (Michael Dawson) nodejs#18089 * src: - allow --perf-(basic-)?prof in NODE_OPTIONS (Leko) nodejs#17600 * vm: - add support for es modules (Gus Caplan) nodejs#17560 PR-URL: nodejs#18902

PR-URL: #17600 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Leko added 2 commits December 11, 2017 13:05

src: allow --perf-prof and --perf-basic-prof in NODE_OPTIONS

b778ba1

test: add test about perf-prof and perf-basic-prof are allowed in NOD…

b35f846

…E_OPTIONS

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 11, 2017

bnoordhuis reviewed Dec 11, 2017

View reviewed changes

Leko added 2 commits December 11, 2017 20:52

src: redefine perf_prof and perf_basic_prof as option for v8

1b7a8d1

test: test --perf-prof only in Linux

702bd35

perf-prof works in Linux only. For example, it not works in MacOS.

cjihrig reviewed Dec 11, 2017

View reviewed changes

test: remove Github link

0899557

Leko changed the title ~~[WIP] Allow --perf-prof and --perf-basic-prof in NODE_OPTIONS~~ Allow --perf-prof and --perf-basic-prof in NODE_OPTIONS Dec 11, 2017

richardlau requested changes Dec 12, 2017

View reviewed changes

doc: add --perf-prof and --perf-basic-prof to V8 options

81a0684

richardlau approved these changes Dec 12, 2017

View reviewed changes

cjihrig approved these changes Dec 12, 2017

View reviewed changes

BridgeAR approved these changes Dec 12, 2017

View reviewed changes

jasnell approved these changes Dec 12, 2017

View reviewed changes

doc: sort V8 options in alphabetical order

b97987a

lpinca approved these changes Dec 13, 2017

View reviewed changes

benjamingr approved these changes Dec 26, 2017

View reviewed changes

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added the land-on-v8.x label Aug 7, 2018

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

Uh oh!

Conversation

Leko commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

Leko commented Dec 11, 2017

Uh oh!

joyeecheung commented Dec 11, 2017

Uh oh!

Leko commented Dec 11, 2017

Uh oh!

bnoordhuis Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

Leko Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

Leko commented Dec 11, 2017

Uh oh!

cjihrig Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

Leko Dec 11, 2017

Choose a reason for hiding this comment

Uh oh!

joyeecheung Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

Leko commented Dec 12, 2017

Uh oh!

richardlau left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

Leko Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

joyeecheung commented Dec 17, 2017

Uh oh!

richardlau commented Dec 22, 2017

Uh oh!

richardlau commented Dec 22, 2017

Uh oh!

bnoordhuis commented Dec 23, 2017

Uh oh!

richardlau commented Dec 23, 2017

Uh oh!

Leko commented Dec 25, 2017

Uh oh!

joyeecheung commented Dec 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

Leko commented Dec 11, 2017 •

edited

Loading