Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trace_events: add internal binding for intrinsic trace #21899

Closed
wants to merge 2 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jul 20, 2018

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

@jasnell
Copy link
Member Author

jasnell commented Jul 20, 2018

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
Member Author

jasnell commented Jul 20, 2018

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

jasnell commented Jul 20, 2018

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
Member Author

jasnell commented Jul 20, 2018

@jasnell
Copy link
Member Author

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.

@jasnell jasnell 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
Member Author

jasnell commented Jul 20, 2018

CI is green!

Copy link
Contributor

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

@@ -11,7 +11,7 @@
#define V8_MAJOR_VERSION 6
#define V8_MINOR_VERSION 7
#define V8_BUILD_NUMBER 288
#define V8_PATCH_LEVEL 49
#define V8_PATCH_LEVEL 50
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 <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}
@jasnell
Copy link
Member Author

jasnell commented Jul 20, 2018

/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 <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
PR-URL: #21899
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@jasnell
Copy link
Member Author

jasnell commented Jul 22, 2018

Landed in a706456 and f7454a5

@jasnell jasnell 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 <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
PR-URL: #21899
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@targos targos 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
Member Author

jasnell commented Aug 7, 2018

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 <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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants