build,src: fix Intel VTune profiling support #39374

fanchenkong1 · 2021-07-13T08:12:50Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test (Window; found some failures on Windows which also exist on main branch including parallel/test-xxx, test-wasi...) pass with this change?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression test (or a benchmark) included?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Affected core subsystem(s)

No effected core subsystem.

Description of change

This change fix the Intel Vtune profiling support for JITted
JavaScript on IA32 / X64 / X32 platform. The advantage of this profiling
is that the user / developer of NodeJS application can get the detailed
profiling information for every line of the JavaScript source code.
This information will be very useful for the owner to optimize their
applications.

This feature is a compile-time option. For windows platform, the user
needs to pass the following parameter to vcbuild.bat: "enable-vtune"

For other OS, the user needs to pass the following parameter to
./configure command: "--enable-vtune-profiling"

Fixes: #29060

richardlau · 2021-07-13T12:16:47Z

I'm fairly sure we had this before, but it was removed because it wasn't maintained and broke.

cc @nodejs/diagnostics @nodejs/v8-update

targos · 2021-07-13T12:36:54Z

There will have to be an update to https://github.com/nodejs/node-core-utils/blob/main/lib/update-v8/constants.js for deps/v8/third_party/ittapi.

configure.py

vcbuild.bat

Add ittapi to the list of V8 dependencies, which is used by VTune JIT profiling. Refs: nodejs/node#39374

targos · 2021-07-14T06:48:15Z

I wanted to try on my mac...
Well, at least I can confirm that the error works with arm64 architecture 😄

$ ./configure --ninja --enable-vtune-profiling
Node.js configure: Found Python 3.9.6...
Traceback (most recent call last):
  File "/Users/targos/git/nodejs/node/./configure", line 26, in <module>
    import configure
  File "/Users/targos/git/nodejs/node/configure.py", line 1921, in <module>
    configure_node(output)
  File "/Users/targos/git/nodejs/node/configure.py", line 1222, in configure_node
    raise Exception(
Exception: The VTune profiler for JavaScript is only supported on x32, x86, and x64 architectures.

fanchenkong1 · 2021-07-14T07:12:49Z

Thanks for looking at this change! This doc provides an example of profiling JIT-compiled JavaScript on x64/ia32 platform.

targos · 2021-07-14T07:18:44Z

So... I confirm that it compiles and runs fine on x64 architecture.

Only one test fails:

"zlib" should've been compiled **with** code cache
    at Object.<anonymous> (/home/mzasso/git/nodejs/node/test/parallel/test-code-cache.js:75:10)
    at Module._compile (node:internal/modules/cjs/loader:1095:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1124:10)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:816:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
    at node:internal/main/run_main_module:17:47 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: 153,
  expected: 0,
  operator: 'strictEqual'
}
Command: out/Release/node --expose-internals /home/mzasso/git/nodejs/node/test/parallel/test-code-cache.js

Is it expected that this feature disables code cache in V8?

targos · 2021-07-14T07:44:16Z

@fanchenkong1 did you fetch the version of ittapi referenced in V8 DEPS?

node/deps/v8/DEPS

Line 299 in cfb7c4f

    
           'url': Var('chromium_url') + '/external/github.com/intel/ittapi' + '@' + 'b4ae0122ba749163096058b4f1bb065bf4a7de94',

nodejs-github-bot · 2021-07-14T07:46:53Z

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

fanchenkong1 · 2021-07-14T07:55:32Z

@fanchenkong1 did you fetch the version of ittapi referenced in V8 DEPS?

node/deps/v8/DEPS

Line 299 in cfb7c4f

'url': Var('chromium_url') + '/external/github.com/intel/ittapi' + '@' + 'b4ae0122ba749163096058b4f1bb065bf4a7de94',

No, the version in V8 is earlier. Shall I update the ittapi here with the same version in V8?

targos · 2021-07-14T08:10:07Z

I'd say yes, because next time we update V8, it will downgrade it anyway.

fanchenkong1 · 2021-07-14T08:47:47Z

I'd say yes, because next time we update V8, it will downgrade it anyway.

Thanks for the suggestion! The version of ittapi is updated to align with V8 in the latest change.

BTW, I look into the test failure on code-cache a little bit. It seems that I can only reproduce this failure on Windows platform. And the failure seems not to disappear on the release build of main branch. Would you please help provide more information about your test?

targos · 2021-07-14T09:01:57Z

I just did the following on Linux with this branch:

./configure --ninja --enable-vtune-profiling
make test -j8 V=

fanchenkong1 · 2021-07-15T06:27:38Z

I just did the following on Linux with this branch:
./configure --ninja --enable-vtune-profiling
make test -j8 V=

Thanks for your help! Here are some updates for the failure in test-code-cache.js.

We have found that the reuse of code cache failed at the sanity check (https://source.chromium.org/chromium/chromium/src/+/main:v8/src/snapshot/code-serializer.cc;l=305?q=code-serializer.cc&ss=chromium) with the reason SanityCheckResult::FLAGS_MISMATCH. And this failure seems to be avoided by a following change in V8.

diff --git a/deps/v8/src/flags/flag-definitions.h b/deps/v8/src/flags/flag-definitions.h
index 1a40497555..85427085fa 100644
--- a/deps/v8/src/flags/flag-definitions.h
+++ b/deps/v8/src/flags/flag-definitions.h
@@ -1346,6 +1346,10 @@ DEFINE_STRING(expose_cputracemark_as, nullptr,
 #ifdef ENABLE_VTUNE_TRACEMARK
 DEFINE_BOOL(enable_vtune_domain_support, true, "enable vtune domain support")
 #endif  // ENABLE_VTUNE_TRACEMARK
+#ifdef ENABLE_VTUNE_JIT_INTERFACE
+DEFINE_BOOL(enable_vtune_jit, true, "enable vtune jit profiling support")
+DEFINE_NEG_IMPLICATION(enable_vtune_jit, compact_code_space)
+#endif

 // builtins.cc
 DEFINE_BOOL(allow_unsafe_function_constructor, false,
diff --git a/deps/v8/src/third_party/vtune/vtune-jit.cc b/deps/v8/src/third_party/vtune/vtune-jit.cc
index 08fbfbfe39..91bfadb456 100644
--- a/deps/v8/src/third_party/vtune/vtune-jit.cc
+++ b/deps/v8/src/third_party/vtune/vtune-jit.cc
@@ -289,7 +289,7 @@ void VTUNEJITInterface::event_handler(const v8::JitCodeEvent* event) {
 }  // namespace internal

 v8::JitCodeEventHandler GetVtuneCodeEventHandler() {
-  v8::V8::SetFlagsFromString("--no-compact-code-space");
+  // v8::V8::SetFlagsFromString("--no-compact-code-space");
   return vTune::internal::VTUNEJITInterface::event_handler;
 }

I'm currently not sure why setting the flag at GetVtuneCodeEventHandler may cause this flag mismatch in node. Further investigation on this issue is in progress. Any insight would be helpful.

fanchenkong1 · 2021-07-15T08:03:21Z

BTW, I also found the same issue on code cache by setting various v8 runtime flags from command line. For example,
with
out/Release/node --expose-internals --no-turboprop --trace-opt /home/fckong/node/test/parallel/test-code-cache.js
V8 options are the same as the default. Everything is good.

but with
out/Release/node --expose-internals --turboprop --trace-opt /home/fckong/node/test/parallel/test-code-cache.js
V8 options are different from default. Test failed.

I'm not sure. But it doesn't seem to be an ideal behaviour of code cache?

targos

Changes to src are minimal, LGTM

targos · 2021-07-16T12:02:19Z

BTW, I also found the same issue on code cache by setting various v8 runtime flags from command line. For example,
with
out/Release/node --expose-internals --no-turboprop --trace-opt /home/fckong/node/test/parallel/test-code-cache.js
V8 options are the same as the default. Everything is good.

but with
out/Release/node --expose-internals --turboprop --trace-opt /home/fckong/node/test/parallel/test-code-cache.js
V8 options are different from default. Test failed.

I'm not sure. But it doesn't seem to be an ideal behaviour of code cache?

Thanks for investigating this! In any case, it's a V8 issue and shouldn't block this PR.

nodejs-github-bot · 2021-07-16T12:05:47Z

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

mhdawson · 2021-07-16T18:20:15Z

deps/v8/third_party/ittapi/src/ittnotify/LICENSE.GPL

@@ -0,0 +1,65 @@
+The GNU General Public License (GPL)


Do we need to include this file ? Since there is the option to distribute under the BSD or GPL licence I think we'd be opting for BSD and could not want to include this one as it could be flagged by licence scanners?

mhdawson · 2021-07-16T18:21:26Z

deps/v8/third_party/ittapi/CMakeLists.txt

+#  Contact Information:
+#  http://software.intel.com/en-us/articles/intel-vtune-amplifier-xe/
+#
+#  BSD LICENSE


I think we need an addition to https://github.com/nodejs/node/blob/master/LICENSE which says that ittapi is licenced under BSD.

Do we need to add it to the main LICENSE even if we don't distribute any Node.js release with this dependency?

fanchenkong1 · 2021-07-19T05:25:21Z

There are some failures in the CI.

The node-test-commit-windows-fanned and node-test-linux-linked-openssl111 failed on

test.parallel/test-filehandle-readablestream

which is mentioned in this issue.

The node-test-linux-linked-openssl300 failed on

test.parallel/test-https-client-renegotiation-limit
test.parallel/test-tls-client-renegotiation-limit
test.parallel/test-tls-disable-renegotiation

As this change won't take effect without the build time configuration '--enable-vtune-profiling', shall I expect that there is no need to have a fix for those failures from this PR?

nodejs-github-bot · 2021-07-19T07:43:40Z

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

targos · 2021-07-19T07:44:04Z

@fanchenkong1 yeah, unfortunately our CI is a bit unreliable at the moment. I've triggered a rerun of the failing jobs.

fanchenkong1 · 2021-09-22T05:40:40Z

@targos This change is rebased and the ittapi is updated to version 3.18.13 to meet the latest change in V8. Is there any plan on merging this pr?

This change fix the Intel Vtune profiling support for JITted JavaScript on IA32 / X64 / X32 platform. The advantage of this profiling is that the user / developer of NodeJS application can get the detailed profiling information for every line of the JavaScript source code. This information will be very useful for the owner to optimize their applications. This feature is a compile-time option. For windows platform, the user needs to pass the following parameter to vcbuild.bat: "enable-vtune" For other OS, the user needs to pass the following parameter to ./configure command: "--enable-vtune-profiling" Fixes: nodejs#29060

mhdawson · 2021-09-22T14:58:17Z

@fanchenkong1 there are a few open questions about the licence above. Can you take a look/comment on those?

fanchenkong1 · 2021-09-23T02:28:18Z

@mhdawson, thanks for the reminder and sorry for the late response. The latest commit updated the license file with ittapi/LICENSES/BSD-3-Clause.txt. PATL, thanks!

mhdawson · 2021-09-28T14:40:55Z

Looks like we already had dual licenced references in deps/v8/src/third_party/vtune/LICENSE so this should not make it worse.

@brianwarner do you have any concerns on the licence front or how to best handle/include dual licenced code to minimize what those do who licence scans have to do?

lucshi · 2022-10-21T02:14:27Z

is this RP merged?

targos · 2022-11-08T15:28:49Z

Superseded by #45248

nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Jul 13, 2021

targos reviewed Jul 13, 2021

View reviewed changes

configure.py Outdated Show resolved Hide resolved

vcbuild.bat Outdated Show resolved Hide resolved

fanchenkong1 added a commit to fanchenkong1/node-core-utils that referenced this pull request Jul 14, 2021

v8: add ittapi as V8 dependency

9c25ca2

Add ittapi to the list of V8 dependencies, which is used by VTune JIT profiling. Refs: nodejs/node#39374

fanchenkong1 mentioned this pull request Jul 14, 2021

v8: add ittapi as V8 dependency nodejs/node-core-utils#552

Merged

2 tasks

fanchenkong1 added a commit to fanchenkong1/node-core-utils that referenced this pull request Jul 14, 2021

fix(v8): add ittapi as V8 dependency

bfb71f8

Add ittapi to the list of V8 dependencies, which is used by VTune JIT profiling. Refs: nodejs/node#39374

fanchenkong1 force-pushed the fix-vtune-profile-support branch from ead70c9 to 2872be9 Compare July 14, 2021 06:43

fanchenkong1 force-pushed the fix-vtune-profile-support branch from 2872be9 to 7bd7b39 Compare July 14, 2021 08:27

targos approved these changes Jul 16, 2021

View reviewed changes

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 16, 2021

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 16, 2021

mhdawson reviewed Jul 16, 2021

View reviewed changes

fanchenkong1 force-pushed the fix-vtune-profile-support branch from 7bd7b39 to 4a45d7a Compare September 22, 2021 05:14

fanchenkong1 added 2 commits September 22, 2021 17:56

update ittapi to version 3.18.13

4a45d7a

fanchenkong1 force-pushed the fix-vtune-profile-support branch from 0d4472e to 71f9905 Compare September 24, 2021 01:25

add license of ittapi

71f9905

lucshi mentioned this pull request Oct 31, 2022

Fix Intel VTune profiling support #45248

Merged

targos closed this Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build,src: fix Intel VTune profiling support #39374

build,src: fix Intel VTune profiling support #39374

fanchenkong1 commented Jul 13, 2021

richardlau commented Jul 13, 2021

targos commented Jul 13, 2021

targos commented Jul 14, 2021

fanchenkong1 commented Jul 14, 2021

targos commented Jul 14, 2021

targos commented Jul 14, 2021

nodejs-github-bot commented Jul 14, 2021 •

edited by targos

fanchenkong1 commented Jul 14, 2021

targos commented Jul 14, 2021

fanchenkong1 commented Jul 14, 2021

targos commented Jul 14, 2021

fanchenkong1 commented Jul 15, 2021

fanchenkong1 commented Jul 15, 2021 •

edited

targos left a comment

targos commented Jul 16, 2021

nodejs-github-bot commented Jul 16, 2021

mhdawson Jul 16, 2021

mhdawson Jul 16, 2021

targos Jul 16, 2021

fanchenkong1 commented Jul 19, 2021

nodejs-github-bot commented Jul 19, 2021

targos commented Jul 19, 2021

fanchenkong1 commented Sep 22, 2021

mhdawson commented Sep 22, 2021

fanchenkong1 commented Sep 23, 2021

mhdawson commented Sep 28, 2021

lucshi commented Oct 21, 2022

targos commented Nov 8, 2022

build,src: fix Intel VTune profiling support #39374

build,src: fix Intel VTune profiling support #39374

Conversation

fanchenkong1 commented Jul 13, 2021

Pull Request check-list

Affected core subsystem(s)

Description of change

richardlau commented Jul 13, 2021

targos commented Jul 13, 2021

targos commented Jul 14, 2021

fanchenkong1 commented Jul 14, 2021

targos commented Jul 14, 2021

targos commented Jul 14, 2021

nodejs-github-bot commented Jul 14, 2021 • edited by targos

fanchenkong1 commented Jul 14, 2021

targos commented Jul 14, 2021

fanchenkong1 commented Jul 14, 2021

targos commented Jul 14, 2021

fanchenkong1 commented Jul 15, 2021

fanchenkong1 commented Jul 15, 2021 • edited

targos left a comment

Choose a reason for hiding this comment

targos commented Jul 16, 2021

nodejs-github-bot commented Jul 16, 2021

mhdawson Jul 16, 2021

Choose a reason for hiding this comment

mhdawson Jul 16, 2021

Choose a reason for hiding this comment

targos Jul 16, 2021

Choose a reason for hiding this comment

fanchenkong1 commented Jul 19, 2021

nodejs-github-bot commented Jul 19, 2021

targos commented Jul 19, 2021

fanchenkong1 commented Sep 22, 2021

mhdawson commented Sep 22, 2021

fanchenkong1 commented Sep 23, 2021

mhdawson commented Sep 28, 2021

lucshi commented Oct 21, 2022

targos commented Nov 8, 2022

nodejs-github-bot commented Jul 14, 2021 •

edited by targos

fanchenkong1 commented Jul 15, 2021 •

edited