Skip to content

deps: V8: backport --perf-prof for macOS - #58010

Closed
tmm1 wants to merge 1 commit into
nodejs:mainfrom
tmm1:tmm1/v8-macos-perf
Closed

deps: V8: backport --perf-prof for macOS#58010
tmm1 wants to merge 1 commit into
nodejs:mainfrom
tmm1:tmm1/v8-macos-perf

Conversation

@tmm1

@tmm1tmm1 commented Apr 24, 2025•
edited by RaisinTen
Loading

Copy link
Copy Markdown

@YoungWenMing@tmm1
deps: V8: backport e5dbbbadcbff
f528a3e
Original commit message:
Enable --perf-prof flag on MacOS
MacOS actually can share the implementation of {PerfJitLogger} with
Linux. From the issue 40112126, only Fuzzer tests on Windows ran
into UNREACHABLE/FATAL because of the unimplemented {PerfJitLogger}.
This CL enables the flag --perf-prof on MacOS.
Bug: 403765219
Change-Id: I97871fbcc0cb9890c51ca14fd7a6e65bd0e3c0d2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6385655
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Reviewed-by: Matthias Liedtke <mliedtke@chromium.org>
Auto-Submit: 杨文明 <yangwenming@bytedance.com>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#99885}
Refs: v8/v8@e5dbbba
Co-authored-by: Aman Karmani <aman@tmm1.net>
@nodejs-github-bot

nodejs-github-bot commented Apr 24, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-botnodejs-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 Apr 24, 2025
@anonrig

anonrig commented Apr 25, 2025

Copy link
Copy Markdown
Member

Which commit does this backport? Can you share it?

@tmm1

tmm1 commented Apr 25, 2025

Copy link
Copy Markdown
Author

v8/v8@e5dbbba, via https://chromium-review.googlesource.com/c/v8/v8/+/6385655 to fix https://issues.chromium.org/issues/403765219

@tmm1

tmm1 commented Apr 25, 2025

Copy link
Copy Markdown
Author

With this change, it becomes possible to profile nodejs apps on macOS and see a full-stack profile which shows all this information in one place:

  • libSystem and other OS-level calls
  • v8 c++ internals, including garbage collection and worker threads
  • nodejs c++ internals
  • nodejs builtin javascript internals
  • c/c++/rust running in any node bindings
  • application-level javascript (both jit compiled and interpreted)

As noted in nodejs/diagnostics#652, this kind of combined profile is invaluable for investigating and fixing performance issues. Since many developers use macOS, this is especially critical to improve the performance of nodejs itself and its surrounding development tooling.

An example of how to use this patched nodejs with samply:

$ cargo install samply
$ export NODE_OPTIONS="--perf-basic-prof --perf-prof-unwinding-info --interpreted-frames-native-stack"
$ samply record node ...

For reference, other competing runtimes already offer this level of integration. For example, bun is based on JavaScriptCore which enables this feature by default. The equivalent command for them is:

$ export BUN_JSC_logJITCodeForPerf=1
$ samply record bun ...

juanarbol
juanarbol approved these changes Apr 28, 2025
View reviewed changes

@juanarboljuanarbol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@tmm1

tmm1 commented Apr 30, 2025

Copy link
Copy Markdown
Author

👋 could someone add the labels needed to move this forward? thx

@RafaelGSSRafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tmm1

tmm1 commented May 1, 2025

Copy link
Copy Markdown
Author

CI appears to be done. I can't tell if there are real failures I need to address.

RaisinTen
RaisinTen approved these changes May 2, 2025
View reviewed changes

@RaisinTenRaisinTen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@targos could you please take a look when you get some time?

@nodejs-github-bot

nodejs-github-bot commented May 2, 2025

Copy link
Copy Markdown
Collaborator

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

@targos

targos commented May 2, 2025

Copy link
Copy Markdown
Member

This commit is included in #58070.
Please don't land this on main, or it will block v24.0.0 more.

@RaisinTen

RaisinTen commented May 2, 2025

Copy link
Copy Markdown
Member

Okay, will close this in favor of your V8 upgrade PR.

@targos would it be fine if we create a PR to backport just this cherrypick to the v22 LTS branch?

@targos

targos commented May 2, 2025

Copy link
Copy Markdown
Member

Sure, as long as the V8 version in v22 is ready for it.

@RaisinTenRaisinTen mentioned this pull request May 2, 2025
Merged
@RaisinTen

RaisinTen commented May 2, 2025

Copy link
Copy Markdown
Member

Backport for LTS submitted in #58120

@RaisinTenRaisinTen mentioned this pull request Nov 1, 2025
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QardQardQard approved these changes
@anonriganonriganonrig approved these changes
@juanarboljuanarboljuanarbol approved these changes
@RaisinTenRaisinTenRaisinTen approved these changes

Assignees

No one assigned

Labels

buildIssues and PRs related to build files or the CI.needs-ciPRs that need a full CI run.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

consider reintroduce perf-prof support for macOS

9 participants

@tmm1@nodejs-github-bot@anonrig@targos@RaisinTen@Qard@juanarbol@RafaelGSS@YoungWenMing