Skip to content

Conversation

@mmarchini
Copy link
Contributor

Original commit message:

[log][api] introduce public CodeEventListener API Introduce a new public API called CodeEventListener to allow embedders to better support external profilers and other diagnostic tools without relying on unsupported methods like --perf-basic-prof. Bug: v8:7694 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I063cc965394d59401358757634c9ea84c11517e9 Co-authored-by: Daniel Beckert <[email protected]> Reviewed-on: chromium-review.googlesource.com/1028770 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Hannes Payer <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Andreas Haas <[email protected]> Cr-Commit-Position: refs/heads/master@{#53382} 

Refs: v8/v8@aa6ce3e

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Jun 4, 2018
@mmarchini
Copy link
ContributorAuthor

@mmarchini
Copy link
ContributorAuthor

cc @nodejs/diagnostics

@targos
Copy link
Member

The fixup isn't upstream?

@mmarchini
Copy link
ContributorAuthor

The fixup isn't upstream?

The patch didn't build because the Address typedef is different between V8 6.7 and V8 6.8 (see https://chromium-review.googlesource.com/c/v8/v8/+/988657). I can try to float https://chromium-review.googlesource.com/c/v8/v8/+/988657 as well instead of changing the Patch to work on V8 6.7.

@targos
Copy link
Member

I can try to float chromium-review.googlesource.com/c/v8/v8/+/988657 as well instead of changing the Patch to work on V8 6.7.

It's a big patch. I prefer we stick with your fix.

@mmarchini
Copy link
ContributorAuthor

Floating https://chromium-review.googlesource.com/c/v8/v8/+/988657 is non-trivial :(

Error: Command failed: git apply -3 --directory=deps/v8 error: patch failed: deps/v8/src/compiler/wasm-compiler.cc:2619 error: repository lacks the necessary blob to fall back on 3-way merge. error: deps/v8/src/compiler/wasm-compiler.cc: patch does not apply error: patch failed: deps/v8/src/log.cc:309 Falling back to three-way merge... Applied patch to 'deps/v8/src/log.cc' cleanly. error: patch failed: deps/v8/src/profiler/profile-generator.h:47 error: repository lacks the necessary blob to fall back on 3-way merge. error: deps/v8/src/profiler/profile-generator.h: patch does not apply error: patch failed: deps/v8/src/profiler/profiler-listener.h:59 error: repository lacks the necessary blob to fall back on 3-way merge. error: deps/v8/src/profiler/profiler-listener.h: patch does not apply error: patch failed: deps/v8/test/cctest/test-code-stubs-arm.cc:53 error: repository lacks the necessary blob to fall back on 3-way merge. error: deps/v8/test/cctest/test-code-stubs-arm.cc: patch does not apply error: patch failed: deps/v8/test/cctest/test-code-stubs-arm64.cc:54 error: repository lacks the necessary blob to fall back on 3-way merge. error: deps/v8/test/cctest/test-code-stubs-arm64.cc: patch does not apply error: patch failed: deps/v8/test/cctest/test-code-stubs-ia32.cc:54 error: repository lacks the necessary blob to fall back on 3-way merge. error: deps/v8/test/cctest/test-code-stubs-ia32.cc: patch does not apply error: patch failed: deps/v8/test/cctest/test-code-stubs-mips.cc:55 error: repository lacks the necessary blob to fall back on 3-way merge. error: deps/v8/test/cctest/test-code-stubs-mips.cc: patch does not apply error: patch failed: deps/v8/test/cctest/test-code-stubs-mips64.cc:55 error: repository lacks the necessary blob to fall back on 3-way merge. error: deps/v8/test/cctest/test-code-stubs-mips64.cc: patch does not apply error: patch failed: deps/v8/test/cctest/test-code-stubs-x64.cc:54 error: repository lacks the necessary blob to fall back on 3-way merge. error: deps/v8/test/cctest/test-code-stubs-x64.cc: patch does not apply 

For context: this API will allow users to generate the map file required by Linux perf during runtime. Today the file is generated by --perf-basic-prof, which will keep writing to the file during the entire application lifetime, leading to file growth problems. Sice V8 6.7 fixed Linux perf for Interpreted Functions, it would be really nice to also have this API when V8 6.7 lands on Node.js v10.x.

@hashseed
Copy link
Member

Yeah let's not float the Address patch.

@mmarchini
Copy link
ContributorAuthor

mmarchini commented Jun 11, 2018

@mmarchini
Copy link
ContributorAuthor

Cherry-picking one more commit here (v8/v8@b20faff) to fix a bug when using this API with --interpreted-frames-native-stack on. If there are no objections I'd like to land this tomorrow.

/cc @nodejs/diagnostics @nodejs/v8-update

@targos
Copy link
Member

There is an issue with the v8 ci. Can you try to run it again?

@mmarchini
Copy link
ContributorAuthor

Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mmarchinimmarchini added the blocked PRs that are blocked by other issues or PRs. label Jun 12, 2018
@mmarchini
Copy link
ContributorAuthor

mmarchini commented Jun 12, 2018

Blocking due to another bug found in the API.

Waiting for upstream fix to land: https://chromium-review.googlesource.com/c/v8/v8/+/1097578

@mmarchini
Copy link
ContributorAuthor

@mmarchinimmarchiniforce-pushed the v8-float-code-event-listener-api branch from 20dc9b8 to 348a2beCompareJune 21, 2018 15:51
@mmarchini
Copy link
ContributorAuthor

@targos should I squash the fixup when landing, or sould I keep it as a separate commit?

@targos
Copy link
Member

Squash please and write "backport" instead of cherry-pick in the commit message

@mmarchinimmarchiniforce-pushed the v8-float-code-event-listener-api branch from 348a2be to f43a71bCompareJune 21, 2018 22:39
@mmarchini
Copy link
ContributorAuthor

@mmarchinimmarchini added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Jun 22, 2018
Matheus Marchini added 2 commits June 22, 2018 09:42
Original commit message: [log][api] introduce public CodeEventListener API Introduce a new public API called CodeEventListener to allow embedders to better support external profilers and other diagnostic tools without relying on unsupported methods like --perf-basic-prof. Bug: v8:7694 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I063cc965394d59401358757634c9ea84c11517e9 Co-authored-by: Daniel Beckert <[email protected]> Reviewed-on: https://chromium-review.googlesource.com/1028770 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Hannes Payer <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Andreas Haas <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#53382} Refs: v8/v8@aa6ce3e PR-URL: nodejs#21126 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
Original commit message: [log] fix ExistingCodeLogger behavior on edge case ExistingCodeLogger was behaving incorrectly when the CodeEventHandler API was used in combination with --interpreted-frames-native-stack. Instead of collecting copied trampolines as InterpretedFunction:functionName, they were being collected as Builtin:IntepreterEntryTrampolines. This patch adds special handling for copied trampolines when using ExistingCodeLogger. [email protected] Change-Id: I3ee4be03800122d28d53b51b20c60dcf6263e4c1 Reviewed-on: https://chromium-review.googlesource.com/1087813 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#53624} Refs: v8/v8@b20faff PR-URL: nodejs#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 25, 2018
Original commit message: [log] fix ExistingCodeLogger behavior on edge case ExistingCodeLogger was behaving incorrectly when the CodeEventHandler API was used in combination with --interpreted-frames-native-stack. Instead of collecting copied trampolines as InterpretedFunction:functionName, they were being collected as Builtin:IntepreterEntryTrampolines. This patch adds special handling for copied trampolines when using ExistingCodeLogger. [email protected] Change-Id: I3ee4be03800122d28d53b51b20c60dcf6263e4c1 Reviewed-on: https://chromium-review.googlesource.com/1087813 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#53624} Refs: v8/v8@b20faff PR-URL: nodejs#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 25, 2018
Original commit message: [log] fix boolean logic on LogCodeObject [email protected] Change-Id: Icb4825344991e5b2d15050e037064c60eeb9617e Reviewed-on: https://chromium-review.googlesource.com/1097578 Reviewed-by: Benedikt Meurer <[email protected]> Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#53777} Refs: v8/v8@acc336c PR-URL: nodejs#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 25, 2018
Original commit message: [log][api] Fix GCC 4.9 build failure GCC 4.9 used on some Node.js CI machines complains when the control reaches the end of a non-void function and no return is encountered. [email protected], [email protected], [email protected] Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I5af0192cb187eccbf34dbb60ff3ac2e4774af803 Reviewed-on: https://chromium-review.googlesource.com/1105619 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#53861} Refs: v8/v8@70c4340 PR-URL: nodejs#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message: [log] fix ExistingCodeLogger behavior on edge case ExistingCodeLogger was behaving incorrectly when the CodeEventHandler API was used in combination with --interpreted-frames-native-stack. Instead of collecting copied trampolines as InterpretedFunction:functionName, they were being collected as Builtin:IntepreterEntryTrampolines. This patch adds special handling for copied trampolines when using ExistingCodeLogger. [email protected] Change-Id: I3ee4be03800122d28d53b51b20c60dcf6263e4c1 Reviewed-on: https://chromium-review.googlesource.com/1087813 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#53624} Refs: v8/v8@b20faff PR-URL: nodejs#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message: [log] fix boolean logic on LogCodeObject [email protected] Change-Id: Icb4825344991e5b2d15050e037064c60eeb9617e Reviewed-on: https://chromium-review.googlesource.com/1097578 Reviewed-by: Benedikt Meurer <[email protected]> Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#53777} Refs: v8/v8@acc336c PR-URL: nodejs#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message: [log][api] Fix GCC 4.9 build failure GCC 4.9 used on some Node.js CI machines complains when the control reaches the end of a non-void function and no return is encountered. [email protected], [email protected], [email protected] Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I5af0192cb187eccbf34dbb60ff3ac2e4774af803 Reviewed-on: https://chromium-review.googlesource.com/1105619 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#53861} Refs: v8/v8@70c4340 PR-URL: nodejs#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message: [log] fix ExistingCodeLogger behavior on edge case ExistingCodeLogger was behaving incorrectly when the CodeEventHandler API was used in combination with --interpreted-frames-native-stack. Instead of collecting copied trampolines as InterpretedFunction:functionName, they were being collected as Builtin:IntepreterEntryTrampolines. This patch adds special handling for copied trampolines when using ExistingCodeLogger. [email protected] Change-Id: I3ee4be03800122d28d53b51b20c60dcf6263e4c1 Reviewed-on: https://chromium-review.googlesource.com/1087813 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#53624} Refs: v8/v8@b20faff PR-URL: nodejs#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message: [log] fix boolean logic on LogCodeObject [email protected] Change-Id: Icb4825344991e5b2d15050e037064c60eeb9617e Reviewed-on: https://chromium-review.googlesource.com/1097578 Reviewed-by: Benedikt Meurer <[email protected]> Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#53777} Refs: v8/v8@acc336c PR-URL: nodejs#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Jul 26, 2018
Original commit message: [log][api] Fix GCC 4.9 build failure GCC 4.9 used on some Node.js CI machines complains when the control reaches the end of a non-void function and no return is encountered. [email protected], [email protected], [email protected] Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I5af0192cb187eccbf34dbb60ff3ac2e4774af803 Reviewed-on: https://chromium-review.googlesource.com/1105619 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#53861} Refs: v8/v8@70c4340 PR-URL: nodejs#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 10, 2018
Original commit message: [log] fix ExistingCodeLogger behavior on edge case ExistingCodeLogger was behaving incorrectly when the CodeEventHandler API was used in combination with --interpreted-frames-native-stack. Instead of collecting copied trampolines as InterpretedFunction:functionName, they were being collected as Builtin:IntepreterEntryTrampolines. This patch adds special handling for copied trampolines when using ExistingCodeLogger. [email protected] Change-Id: I3ee4be03800122d28d53b51b20c60dcf6263e4c1 Reviewed-on: https://chromium-review.googlesource.com/1087813 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#53624} Refs: v8/v8@b20faff Backport-PR-URL: #21668 PR-URL: #21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 10, 2018
Original commit message: [log] fix boolean logic on LogCodeObject [email protected] Change-Id: Icb4825344991e5b2d15050e037064c60eeb9617e Reviewed-on: https://chromium-review.googlesource.com/1097578 Reviewed-by: Benedikt Meurer <[email protected]> Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{#53777} Refs: v8/v8@acc336c Backport-PR-URL: #21668 PR-URL: #21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 10, 2018
Original commit message: [log][api] Fix GCC 4.9 build failure GCC 4.9 used on some Node.js CI machines complains when the control reaches the end of a non-void function and no return is encountered. [email protected], [email protected], [email protected] Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I5af0192cb187eccbf34dbb60ff3ac2e4774af803 Reviewed-on: https://chromium-review.googlesource.com/1105619 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#53861} Refs: v8/v8@70c4340 Backport-PR-URL: #21668 PR-URL: #21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
@rvaggrvagg mentioned this pull request Aug 13, 2018
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
Original commit message: [log] fix ExistingCodeLogger behavior on edge case ExistingCodeLogger was behaving incorrectly when the CodeEventHandler API was used in combination with --interpreted-frames-native-stack. Instead of collecting copied trampolines as InterpretedFunction:functionName, they were being collected as Builtin:IntepreterEntryTrampolines. This patch adds special handling for copied trampolines when using ExistingCodeLogger. [email protected] Change-Id: I3ee4be03800122d28d53b51b20c60dcf6263e4c1 Reviewed-on: https://chromium-review.googlesource.com/1087813 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#53624} Refs: v8/v8@b20faff Backport-PR-URL: nodejs/node#21668 PR-URL: nodejs/node#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
Original commit message: [log] fix boolean logic on LogCodeObject [email protected] Change-Id: Icb4825344991e5b2d15050e037064c60eeb9617e Reviewed-on: https://chromium-review.googlesource.com/1097578 Reviewed-by: Benedikt Meurer <[email protected]> Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{#53777} Refs: v8/v8@acc336c Backport-PR-URL: nodejs/node#21668 PR-URL: nodejs/node#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
Original commit message: [log][api] Fix GCC 4.9 build failure GCC 4.9 used on some Node.js CI machines complains when the control reaches the end of a non-void function and no return is encountered. [email protected], [email protected], [email protected] Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I5af0192cb187eccbf34dbb60ff3ac2e4774af803 Reviewed-on: https://chromium-review.googlesource.com/1105619 Reviewed-by: Yang Guo <[email protected]> Commit-Queue: Yang Guo <[email protected]> Cr-Commit-Position: refs/heads/master@{#53861} Refs: v8/v8@70c4340 Backport-PR-URL: nodejs/node#21668 PR-URL: nodejs/node#21126 Refs: v8/v8@aa6ce3e Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.buildIssues and PRs related to build files or the CI.v8 engineIssues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@mmarchini@targos@hashseed@jasnell@mhdawson@nodejs-github-bot