trace_events: add internal binding for intrinsic trace #21899

jasnell · 2018-07-20T00:36:55Z

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 · 2018-07-20T00:36:57Z

@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 · 2018-07-20T00:40:09Z

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 · 2018-07-20T00:40:52Z

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

jasnell · 2018-07-20T00:43:07Z

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 · 2018-07-20T02:48:16Z

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

jasnell · 2018-07-20T03:19:34Z

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

Trott · 2018-07-20T04:26:03Z

@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 · 2018-07-20T14:01:32Z

CI is green!

ofrobots

LGTM w/ nit.

ofrobots · 2018-07-20T17:10:55Z

deps/v8/include/v8-version.h

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


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

ofrobots · 2018-07-20T17:13:54Z

/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@{#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}

jasnell · 2018-07-20T17:23:44Z

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

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>

PR-URL: #21899 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

jasnell · 2018-07-22T14:37:53Z

Landed in a706456 and f7454a5

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>

PR-URL: #21899 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

MylesBorins · 2018-08-07T15:49:58Z

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 · 2018-08-07T15:52:32Z

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.

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>

jasnell mentioned this pull request Jul 20, 2018

[WIP] trace_events: V8 Intrinsic (do not land) #20618

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 force-pushed the trace-intrinsic branch from 8f7e9d5 to abdb53e Compare July 20, 2018 02:47

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

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

ofrobots approved these changes Jul 20, 2018

View reviewed changes

jasnell added 2 commits July 20, 2018 10:21

trace_events: add support for builtin trace

4a40c18

jasnell force-pushed the trace-intrinsic branch from abdb53e to 4a40c18 Compare July 20, 2018 17:38

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 closed this Jul 22, 2018

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 pending requests for changes, and a CI started. label Jul 24, 2018

MylesBorins added the backport-requested-v8.x label Aug 7, 2018

jasnell added dont-land-on-v6.x and removed dont-land-on-v10.x labels Aug 7, 2018

jasnell removed the backport-requested-v8.x label Aug 7, 2018

rvagg mentioned this pull request Aug 13, 2018

Release proposal: v10.9.0 #22295

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trace_events: add internal binding for intrinsic trace #21899

trace_events: add internal binding for intrinsic trace #21899

jasnell commented Jul 20, 2018

nodejs-github-bot commented Jul 20, 2018

jasnell commented Jul 20, 2018

jasnell commented Jul 20, 2018

jasnell commented Jul 20, 2018

jasnell commented Jul 20, 2018

jasnell commented Jul 20, 2018 •

edited

Trott commented Jul 20, 2018 •

edited

jasnell commented Jul 20, 2018

ofrobots left a comment

ofrobots Jul 20, 2018

ofrobots commented Jul 20, 2018

jasnell commented Jul 20, 2018

jasnell commented Jul 22, 2018

MylesBorins commented Aug 7, 2018

jasnell commented Aug 7, 2018

trace_events: add internal binding for intrinsic trace #21899

trace_events: add internal binding for intrinsic trace #21899

Conversation

jasnell commented Jul 20, 2018

Checklist

nodejs-github-bot commented Jul 20, 2018

jasnell commented Jul 20, 2018

jasnell commented Jul 20, 2018

jasnell commented Jul 20, 2018

jasnell commented Jul 20, 2018

jasnell commented Jul 20, 2018 • edited

Trott commented Jul 20, 2018 • edited

jasnell commented Jul 20, 2018

ofrobots left a comment

Choose a reason for hiding this comment

ofrobots Jul 20, 2018

Choose a reason for hiding this comment

ofrobots commented Jul 20, 2018

jasnell commented Jul 20, 2018

jasnell commented Jul 22, 2018

MylesBorins commented Aug 7, 2018

jasnell commented Aug 7, 2018

jasnell commented Jul 20, 2018 •

edited

Trott commented Jul 20, 2018 •

edited