FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

trace_events: add internal binding for intrinsic trace by jasnell · Pull Request #21899 · nodejs/node · GitHub

/ node Public

trace_events: add internal binding for intrinsic trace - #21899

Closed
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:trace-intrinsic
ConversationCommits (2)ChecksFiles changed
Closed

trace_events: add internal binding for intrinsic trace#21899
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:trace-intrinsic

Conversation

jasnell commented Jul 20, 2018

Copy link
Copy Markdown
Member

Continuation (and completion) of the work started in #20618 ...

Backports the intrinsic trace/isTraceCategoryEnabled functions that have landed upstream in V8 (v8/v8@0dd3390). These will need to be floated until we get a version of V8 that includes the commit.

This currently does not add code that uses the internal functions. That will come as separate PRs.

Note: There is currently one place in the javascript where we emit trace events through the old binding (in lib/internal/trace_events_async_hooks.js) but switching would require a change in the actual output format, which will break some tooling. We'll want to take it a bit slow on that. Will make those changes in a separate PR.

/cc @nodejs/v8 @nodejs/v8-update @nodejs/trace-events

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

nodejs-github-bot commented Jul 20, 2018

Copy link
Copy Markdown
Collaborator

@jasnell build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/319/pipeline

jasnell commented Jul 20, 2018

Copy link
Copy Markdown
Member Author

Note: the benchmark added by this can occasionally Abort due to known issues in the NodeTraceWriter implementation (/cc @addaleax @eugeneo). This segfault is caused by the benchmark emitting trace events too quickly. The segfault is caused by any flaws in this particular PR.

jasnell commented Jul 20, 2018

Copy link
Copy Markdown
Member Author

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

jasnell mentioned this pull request Jul 20, 2018
Closed
4 tasks
jasnell added v8 engine Issues and PRs related to the V8 dependency. lib / src Issues and PRs related to general changes in the lib or src directory. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Jul 20, 2018

jasnell commented Jul 20, 2018

Copy link
Copy Markdown
Member Author

Benchmark comparison with the existing legacy emit and category checks:

(trace and isTraceCategoryEnabled are the new versions)

misc/trace.js method="trace" n=100000: 509,507.6314919208
misc/trace.js method="emit" n=100000: 477,461.6085892631
misc/trace.js method="isTraceCategoryEnabled" n=100000: 4,949,474.281690222
misc/trace.js method="categoryGroupEnabled" n=100000: 1,904,456.030985652

jasnell commented Jul 20, 2018

Copy link
Copy Markdown
Member Author

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

jasnell requested review from addaleax, ofrobots and targos July 20, 2018 02:49

jasnell commented Jul 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

@nodejs/build ... the OSX build bots are failing unfortunately... see https://ci.nodejs.org/job/node-test-commit-osx/19929/nodes=osx1012/console

Trott commented Jul 20, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

@nodejs/build ... the OSX build bots are failing unfortunately... see https://ci.nodejs.org/job/node-test-commit-osx/19929/nodes=osx1012/console

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/15957/

@jasnell: It seems to have gotten farther on in osx1012 than last two times. Since the console you link to shows git timing out, I'd chalk it up to network problems between the host and GitHub and hope it doesn't recur.

jasnell added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 20, 2018

jasnell commented Jul 20, 2018

Copy link
Copy Markdown
Member Author

CI is green!

ofrobots approved these changes Jul 20, 2018
View reviewed changes

ofrobots left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM w/ nit.

Comment thread deps/v8/include/v8-version.h Outdated

ofrobots Jul 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This doest not need to bumped given the bump in common.gypi above.

ofrobots commented Jul 20, 2018

Copy link
Copy Markdown
Contributor

/cc @nodejs/v8-update If this lands before the V8 6.8 upgrade, this patch will need to be re-floated.

jasnell added 2 commits July 20, 2018 10:21
deps: V8: Backport of 0dd3390 from upstream
cc39d16 
Original commit message:

  Reland "[builtins] Add %IsTraceCategoryEnabled and %Trace builtins"

  This is a reland of 8d4572a

  Original change's description:
  > [builtins] Add %IsTraceCategoryEnabled and %Trace builtins
  >
  > Adds the builtin Trace and IsTraceCategoryEnabled functions
  > exposed via extra bindings. These are intended to use by
  > embedders to allow basic trace event support from JavaScript.
  >
  > ```js
  > isTraceCategoryEnabled('v8.some-category')
  >
  > trace('e'.charCodeAt(0), 'v8.some-category',
  >       'Foo', 0, { abc: 'xyz'})
  > ```
  >
  > Bug: v8:7851
  > Change-Id: I7bfb9bb059efdf87d92a56a0aae326650730c250
  > Reviewed-on: chromium-review.googlesource.com/1103294
  > Commit-Queue: Yang Guo <yangguo@chromium.org>
  > Reviewed-by: Yang Guo <yangguo@chromium.org>
  > Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
  > Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  > Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
  > Cr-Commit-Position: refs/heads/master@{nodejs#54121}

  TBR=cbruni@chromium.org

  Bug: v8:7851
  Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8
  Reviewed-on: chromium-review.googlesource.com/1137071
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Reviewed-by: Yang Guo <yangguo@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
  Cr-Commit-Position: refs/heads/master@{nodejs#54532}
trace_events: add support for builtin trace
4a40c18

jasnell commented Jul 20, 2018

Copy link
Copy Markdown
Member Author

/cc @nodejs/v8-update If this lands before the V8 6.8 upgrade, this patch will need to be re-floated.

Also note, there are some minor variances between this and the commit that landed in v8 master due to some minor internal API changes that were made. It's nothing that impacts Node.js' use of the API but does have some variances inside builtins_trace.cc and v8's bootstrapper.cc. I'm not sure exactly which version of V8 those API changes landed in so when relanding this there may need to be some fixups.

jasnell added a commit that referenced this pull request Jul 22, 2018
deps: V8: Backport of 0dd3390 from upstream
a706456 
Original commit message:

  Reland "[builtins] Add %IsTraceCategoryEnabled and %Trace builtins"

  This is a reland of 8d4572a

  Original change's description:
  > [builtins] Add %IsTraceCategoryEnabled and %Trace builtins
  >
  > Adds the builtin Trace and IsTraceCategoryEnabled functions
  > exposed via extra bindings. These are intended to use by
  > embedders to allow basic trace event support from JavaScript.
  >
  > ```js
  > isTraceCategoryEnabled('v8.some-category')
  >
  > trace('e'.charCodeAt(0), 'v8.some-category',
  >       'Foo', 0, { abc: 'xyz'})
  > ```
  >
  > Bug: v8:7851
  > Change-Id: I7bfb9bb059efdf87d92a56a0aae326650730c250
  > Reviewed-on: chromium-review.googlesource.com/1103294
  > Commit-Queue: Yang Guo <yangguo@chromium.org>
  > Reviewed-by: Yang Guo <yangguo@chromium.org>
  > Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
  > Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  > Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
  > Cr-Commit-Position: refs/heads/master@{#54121}

  TBR=cbruni@chromium.org

  Bug: v8:7851
  Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8
  Reviewed-on: chromium-review.googlesource.com/1137071
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Reviewed-by: Yang Guo <yangguo@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54532}

PR-URL: #21899
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
jasnell added a commit that referenced this pull request Jul 22, 2018
trace_events: add support for builtin trace
f7454a5 
PR-URL: #21899
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

jasnell commented Jul 22, 2018

Copy link
Copy Markdown
Member Author

Landed in a706456 and f7454a5

jasnell closed this Jul 22, 2018
targos pushed a commit that referenced this pull request Jul 24, 2018
deps: V8: Backport of 0dd3390 from upstream
b56c8ad 
Original commit message:

  Reland "[builtins] Add %IsTraceCategoryEnabled and %Trace builtins"

  This is a reland of 8d4572a

  Original change's description:
  > [builtins] Add %IsTraceCategoryEnabled and %Trace builtins
  >
  > Adds the builtin Trace and IsTraceCategoryEnabled functions
  > exposed via extra bindings. These are intended to use by
  > embedders to allow basic trace event support from JavaScript.
  >
  > ```js
  > isTraceCategoryEnabled('v8.some-category')
  >
  > trace('e'.charCodeAt(0), 'v8.some-category',
  >       'Foo', 0, { abc: 'xyz'})
  > ```
  >
  > Bug: v8:7851
  > Change-Id: I7bfb9bb059efdf87d92a56a0aae326650730c250
  > Reviewed-on: chromium-review.googlesource.com/1103294
  > Commit-Queue: Yang Guo <yangguo@chromium.org>
  > Reviewed-by: Yang Guo <yangguo@chromium.org>
  > Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
  > Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  > Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
  > Cr-Commit-Position: refs/heads/master@{#54121}

  TBR=cbruni@chromium.org

  Bug: v8:7851
  Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8
  Reviewed-on: chromium-review.googlesource.com/1137071
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Reviewed-by: Yang Guo <yangguo@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54532}

PR-URL: #21899
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Jul 24, 2018
trace_events: add support for builtin trace
cfeed2b 
PR-URL: #21899
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jul 24, 2018
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed

MylesBorins commented Aug 7, 2018

Copy link
Copy Markdown
Contributor

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

jasnell commented Aug 7, 2018

Copy link
Copy Markdown
Member Author

Putting the don't land labels for v8 and v6. This can go into v10 but let's hold off on bringing it back to v8 for the time being.

MylesBorins pushed a commit that referenced this pull request Aug 10, 2018
deps: V8: Backport of 0dd3390 from upstream
eb9850a 
Original commit message:

  Reland "[builtins] Add %IsTraceCategoryEnabled and %Trace builtins"

  This is a reland of 8d4572a

  Original change's description:
  > [builtins] Add %IsTraceCategoryEnabled and %Trace builtins
  >
  > Adds the builtin Trace and IsTraceCategoryEnabled functions
  > exposed via extra bindings. These are intended to use by
  > embedders to allow basic trace event support from JavaScript.
  >
  > ```js
  > isTraceCategoryEnabled('v8.some-category')
  >
  > trace('e'.charCodeAt(0), 'v8.some-category',
  >       'Foo', 0, { abc: 'xyz'})
  > ```
  >
  > Bug: v8:7851
  > Change-Id: I7bfb9bb059efdf87d92a56a0aae326650730c250
  > Reviewed-on: chromium-review.googlesource.com/1103294
  > Commit-Queue: Yang Guo <yangguo@chromium.org>
  > Reviewed-by: Yang Guo <yangguo@chromium.org>
  > Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
  > Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  > Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
  > Cr-Commit-Position: refs/heads/master@{#54121}

  TBR=cbruni@chromium.org

  Bug: v8:7851
  Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8
  Reviewed-on: chromium-review.googlesource.com/1137071
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Reviewed-by: Yang Guo <yangguo@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54532}

Backport-PR-URL: #21668
PR-URL: #21899
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
rvagg mentioned this pull request Aug 13, 2018
Merged
This was referenced Aug 16, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Jun 18, 2021
Open
Open
Open
Open
Open
firass111 pushed a commit to firass111/Project_node1 that referenced this pull request Apr 16, 2025
deps: V8: Backport of 0dd3390 from upstream
7eccaf8 
Original commit message:

  Reland "[builtins] Add %IsTraceCategoryEnabled and %Trace builtins"

  This is a reland of 8d4572a

  Original change's description:
  > [builtins] Add %IsTraceCategoryEnabled and %Trace builtins
  >
  > Adds the builtin Trace and IsTraceCategoryEnabled functions
  > exposed via extra bindings. These are intended to use by
  > embedders to allow basic trace event support from JavaScript.
  >
  > ```js
  > isTraceCategoryEnabled('v8.some-category')
  >
  > trace('e'.charCodeAt(0), 'v8.some-category',
  >       'Foo', 0, { abc: 'xyz'})
  > ```
  >
  > Bug: v8:7851
  > Change-Id: I7bfb9bb059efdf87d92a56a0aae326650730c250
  > Reviewed-on: chromium-review.googlesource.com/1103294
  > Commit-Queue: Yang Guo <yangguo@chromium.org>
  > Reviewed-by: Yang Guo <yangguo@chromium.org>
  > Reviewed-by: Fadi Meawad <fmeawad@chromium.org>
  > Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  > Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
  > Cr-Commit-Position: refs/heads/master@{#54121}

  TBR=cbruni@chromium.org

  Bug: v8:7851
  Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8
  Reviewed-on: chromium-review.googlesource.com/1137071
  Commit-Queue: Yang Guo <yangguo@chromium.org>
  Reviewed-by: Yang Guo <yangguo@chromium.org>
  Reviewed-by: Camillo Bruni <cbruni@chromium.org>
  Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
  Cr-Commit-Position: refs/heads/master@{#54532}

Backport-PR-URL: nodejs/node#21668
PR-URL: nodejs/node#21899
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

targos Awaiting requested review from targos
addaleax Awaiting requested review from addaleax
1 more reviewer
ofrobots ofrobots approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL