Fix Intel VTune profiling support by lucshi · Pull Request #45248 · nodejs/node

lucshi · 2022-10-31T04:24:29Z

We received upcoming requests to fix the broken building of Intel
Vtune on Node.js to profile JS and JITted code.

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 and
JITted code.

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"

This fix was originally address in PR and author is @fanchenkong1
fanchen.kong@intel.com in
#39374 last year. The PR has been
broken in current Node.js repo and I fixed the broken, fully tested and
submit this new PR on behalf of @fanchenkong1.

nodejs-github-bot · 2022-10-31T04:24:33Z

Review requested:

@nodejs/tsc
@nodejs/v8-update

mscdex · 2022-10-31T04:43:37Z

There seems to be unrelated (eslint-related) commits added here?

Also, with vtune support being removed back in 2019 due to bitrot, is there any kind of guarantee we won't see a repeat of this?

gengjiawen · 2022-10-31T04:51:44Z

You also need to upstream v8 patches in v8 repo

lucshi · 2022-10-31T06:03:59Z

You also need to upstream v8 patches in v8 repo

No need for now. The ittapi code has been in V8 repo. At present Node.js node-core-util lack of the auto sync-up support for V8 ittapi componenent and this was addressed by PR nodejs/node-core-utils#552. The reviewing decision of PR 552 is that it will be landed after vtune patch landed. So for now, the solution is I commit the V8 depedency ittapi to Node.js for the first time, and then land the node-core-util PR. In future ittapi could be sync up automatically.

lucshi · 2022-10-31T06:09:22Z

There seems to be unrelated (eslint-related) commits added here?
Also, with vtune support being removed back in 2019 due to bitrot, is there any kind of guarantee we won't see a repeat of this?

The root cause of 2019 bitrot is that we had code changes in V8 repo but not synced to Node.js repo. To avoid such issue there are options like

Add maintainer for vtune support. Maintainer is responsible for tracking code changes.
Add vtune building flag into CI.

Please advice more options. Thanks!

targos · 2022-10-31T07:14:35Z

Changes to config/build files LGTM. /cc @nodejs/cpp-reviewers

lucshi · 2022-10-31T08:14:50Z

There are 2 lint related patch submitted. Is there a way to remove them without resubmitting PR again?

legendecas · 2022-10-31T08:48:23Z

There are 2 lint related patch submitted. Is there a way to remove them without resubmitting PR again?

You can try squashing the commits and force-push the same branch to GitHub.

BridgeAR · 2022-10-31T08:59:17Z

@lucshi

git rebase -i HEAD~3
# Remove the unreleated commits
git push --force-with-lease

bnoordhuis · 2022-10-31T09:15:33Z

Add vtune building flag into CI.

Node's CI matrix is already unwieldy and realistically no one cares (reports bugs) about vtune except Intel personnel.

If Intel wants to be more proactive w.r.t. fixing issues, that's of course welcome, just send pull requests.

lucshi · 2022-11-01T01:35:57Z

git rebase -i HEAD~3
# Remove the unreleated commits
git push --force-with-lease

Done. Thank you!

lucshi · 2022-11-01T02:34:39Z

Add vtune building flag into CI.
Node's CI matrix is already unwieldy and realistically no one cares (reports bugs) about vtune except Intel personnel.
If Intel wants to be more proactive w.r.t. fixing issues, that's of course welcome, just send pull requests.

Sure. Will submit a new PR to enable the checks in Windows build and Linux buid CI checks.

lucshi · 2022-11-01T07:11:17Z

All review comments are resolved. Please kindly have a review and submit.
After this landed, I will submit a new PR to add vtune build into windows and linux build CI.

Trott · 2022-11-01T18:32:43Z

@nodejs/build

mcollina

lgtm

BridgeAR · 2022-11-09T12:14:42Z

@lucshi seems like something went wrong while pushing something new to the branch :-)
You could rebase with main to remove those again and then push force with lease.

nodejs-github-bot · 2022-11-12T10:19:43Z

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

nodejs-github-bot · 2022-11-12T10:20:04Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu1804-intel-64,v8test=v8test/4977/

nodejs-github-bot · 2022-11-12T10:21:08Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/4977/

nodejs-github-bot · 2022-11-12T10:22:08Z

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/4977/

nodejs-github-bot · 2022-11-17T07:03:49Z

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

lucshi · 2022-11-22T05:01:42Z

Can this PR be merged?

nodejs-github-bot · 2022-11-22T08:28:21Z

Landed in d783a1c

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

PR-URL: #45248 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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

nodejs-github-bot added build Issues and PRs related to build files or the CI. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Oct 31, 2022

build,deps,src: fix Intel VTune profiling support
f1374b6

lucshi force-pushed the vtune branch from 89430b6 to f1374b6Compare November 1, 2022 01:33

mcollina approved these changes Nov 2, 2022
View reviewed changes

targos reviewed Nov 8, 2022
View reviewed changes

Comment threadLICENSE

targos mentioned this pull request Nov 8, 2022
build,src: fix Intel VTune profiling support #39374
Closed
4 tasks

update license builder to add ittapi license section
5f8eb01

lucshi force-pushed the vtune branch from ed72bf4 to 5f8eb01Compare November 9, 2022 12:21

lucshi requested a review from targos November 9, 2022 12:22

targos reviewed Nov 9, 2022
View reviewed changes

Comment threaddeps/v8/third_party/ittapi/LICENSE Outdated

targos reviewed Nov 9, 2022
View reviewed changes

Comment threadtools/license-builder.sh Outdated

change ittapi license content
c864ed2

targos added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022

github-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022

targos added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 22, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 22, 2022

nodejs-github-bot merged commit d783a1c into nodejs:mainNov 22, 2022

targos pushed a commit to nodejs/node-core-utils that referenced this pull request Nov 22, 2022
fix(v8): add ittapi as V8 dependency (#552)
20713c0
Add ittapi to the list of V8 dependencies, which is used by VTune JIT profiling. Refs: nodejs/node#45248

ruyadorno pushed a commit that referenced this pull request Nov 24, 2022
build,deps,src: fix Intel VTune profiling support
9ffe3c0
PR-URL: #45248 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

ruyadorno mentioned this pull request Nov 24, 2022
v19.2.0 proposal #45615
Merged

danielleadams mentioned this pull request Dec 30, 2022
v18.13.0 release proposal #46025
Merged

danielleadams pushed a commit that referenced this pull request Dec 30, 2022
build,deps,src: fix Intel VTune profiling support
995a286
PR-URL: #45248 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

danielleadams pushed a commit that referenced this pull request Jan 3, 2023
build,deps,src: fix Intel VTune profiling support
fb008a2
PR-URL: #45248 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

danielleadams pushed a commit that referenced this pull request Jan 5, 2023
build,deps,src: fix Intel VTune profiling support
5502c3a
PR-URL: #45248 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>

lucshi mentioned this pull request Jan 6, 2023
build: add vtune profiling enabling flag in default Linux buiding CI #46116
Closed

patrickm68 added a commit to patrickm68/NodeJS-core-utils that referenced this pull request Sep 14, 2023
fix(v8): add ittapi as V8 dependency (#552)
cf4a315
Add ittapi to the list of V8 dependencies, which is used by VTune JIT profiling. Refs: nodejs/node#45248

lucshi deleted the vtune branch December 4, 2023 00:52

Uh oh!

Uh oh!

Conversation

lucshi commented Oct 31, 2022

Uh oh!

nodejs-github-bot commented Oct 31, 2022

Uh oh!

mscdex commented Oct 31, 2022

Uh oh!

gengjiawen commented Oct 31, 2022

Uh oh!

lucshi commented Oct 31, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucshi commented Oct 31, 2022

Uh oh!

targos commented Oct 31, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucshi commented Oct 31, 2022

Uh oh!

legendecas commented Oct 31, 2022

Uh oh!

BridgeAR commented Oct 31, 2022

Uh oh!

bnoordhuis commented Oct 31, 2022

Uh oh!

lucshi commented Nov 1, 2022

Uh oh!

lucshi commented Nov 1, 2022

Uh oh!

lucshi commented Nov 1, 2022

Uh oh!

Trott commented Nov 1, 2022

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BridgeAR commented Nov 9, 2022

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 12, 2022

Uh oh!

nodejs-github-bot commented Nov 12, 2022

Uh oh!

nodejs-github-bot commented Nov 12, 2022

Uh oh!

nodejs-github-bot commented Nov 12, 2022

Uh oh!

nodejs-github-bot commented Nov 17, 2022

Uh oh!

lucshi commented Nov 22, 2022

Uh oh!

nodejs-github-bot commented Nov 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lucshi commented Oct 31, 2022•
edited
Loading

targos commented Oct 31, 2022•
edited
Loading