Skip to content

Conversation

@jasnell
Copy link
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
Copy link
Collaborator

@jasnell
Copy link
MemberAuthor

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
Copy link
MemberAuthor

@jasnelljasnell 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
Copy link
MemberAuthor

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
Copy link
MemberAuthor

@jasnell
Copy link
MemberAuthor

jasnell commented Jul 20, 2018

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

@Trott
Copy link
Member

Trott commented Jul 20, 2018

@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.

@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 20, 2018
@jasnell
Copy link
MemberAuthor

CI is green!

Copy link
Contributor

@ofrobotsofrobots left a comment

Choose a reason for hiding this comment

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

LGTM w/ nit.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@ofrobots
Copy link
Contributor

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

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 <[email protected]> > Reviewed-by: Yang Guo <[email protected]> > Reviewed-by: Fadi Meawad <[email protected]> > Reviewed-by: Camillo Bruni <[email protected]> > Reviewed-by: Benedikt Meurer <[email protected]> > Cr-Commit-Position: refs/heads/master@{nodejs#54121} [email protected] Bug: v8:7851 Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8 Reviewed-on: chromium-review.googlesource.com/1137071 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{nodejs#54532}
@jasnell
Copy link
MemberAuthor

/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
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 <[email protected]> > Reviewed-by: Yang Guo <[email protected]> > Reviewed-by: Fadi Meawad <[email protected]> > Reviewed-by: Camillo Bruni <[email protected]> > Reviewed-by: Benedikt Meurer <[email protected]> > Cr-Commit-Position: refs/heads/master@{#54121} [email protected] Bug: v8:7851 Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8 Reviewed-on: chromium-review.googlesource.com/1137071 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{#54532} PR-URL: #21899 Reviewed-By: Ali Ijaz Sheikh <[email protected]>
jasnell added a commit that referenced this pull request Jul 22, 2018
@jasnell
Copy link
MemberAuthor

Landed in a706456 and f7454a5

@jasnelljasnell closed this Jul 22, 2018
targos pushed a commit that referenced this pull request Jul 24, 2018
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 <[email protected]> > Reviewed-by: Yang Guo <[email protected]> > Reviewed-by: Fadi Meawad <[email protected]> > Reviewed-by: Camillo Bruni <[email protected]> > Reviewed-by: Benedikt Meurer <[email protected]> > Cr-Commit-Position: refs/heads/master@{#54121} [email protected] Bug: v8:7851 Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8 Reviewed-on: chromium-review.googlesource.com/1137071 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{#54532} PR-URL: #21899 Reviewed-By: Ali Ijaz Sheikh <[email protected]>
targos pushed a commit that referenced this pull request Jul 24, 2018
@targostargos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 24, 2018
@MylesBorins
Copy link
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
Copy link
MemberAuthor

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
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 <[email protected]> > Reviewed-by: Yang Guo <[email protected]> > Reviewed-by: Fadi Meawad <[email protected]> > Reviewed-by: Camillo Bruni <[email protected]> > Reviewed-by: Benedikt Meurer <[email protected]> > Cr-Commit-Position: refs/heads/master@{#54121} [email protected] Bug: v8:7851 Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8 Reviewed-on: chromium-review.googlesource.com/1137071 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{#54532} Backport-PR-URL: #21668 PR-URL: #21899 Reviewed-By: Ali Ijaz Sheikh <[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: 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 <[email protected]> > Reviewed-by: Yang Guo <[email protected]> > Reviewed-by: Fadi Meawad <[email protected]> > Reviewed-by: Camillo Bruni <[email protected]> > Reviewed-by: Benedikt Meurer <[email protected]> > Cr-Commit-Position: refs/heads/master@{#54121} [email protected] Bug: v8:7851 Change-Id: Id063754b2834b3b6a2b2654e76e8637bcd6aa5f8 Reviewed-on: chromium-review.googlesource.com/1137071 Commit-Queue: Yang Guo <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Benedikt Meurer <[email protected]> Cr-Commit-Position: refs/heads/master@{#54532} Backport-PR-URL: nodejs/node#21668 PR-URL: nodejs/node#21899 Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@jasnell@nodejs-github-bot@Trott@ofrobots@MylesBorins@targos