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

process: delay and simplify the setup of async hooks trace events by joyeecheung · Pull Request #26062 · nodejs/node · GitHub

/ node Public

process: delay and simplify the setup of async hooks trace events - #26062

Closed
joyeecheung wants to merge 4 commits into
nodejs:masterfrom
joyeecheung:simplify-trace-async
ConversationCommits (4)ChecksFiles changed
Closed

process: delay and simplify the setup of async hooks trace events#26062
joyeecheung wants to merge 4 commits into
nodejs:masterfrom
joyeecheung:simplify-trace-async

Conversation

joyeecheung commented Feb 12, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

src: move async hooks trace events setup to pre_execution.js

Reasons:

  • Moves more environment-dependent setup out of bootstrap/node.js
  • No async operations should be done before the call to
    the setup functions in pre_execution.js so no async hooks should be
    triggered before that. Therefore it is safe to delay the setup
    until then.

process: simplify the setup of async hooks trace events

  • Remove trace_category_state from Environment - since this is
    only accessed in the bootstrap process and later in the
    trace category update handler, we could just pass the initial
    values into JS land via the trace_events binding, and pass
    the dynamic values directly to the handler later, instead of
    accessing them out-of-band via the AliasedBuffer.
  • Instead of creating the hooks directly in
    trace_events_async_hooks.js, export the hook factory and
    create the hooks in trace category state toggle.

test: add test for node.async_hooks tracing in workers

test: add test for dynamically enabling node.async_hooks tracing

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

nodejs-github-bot commented Feb 12, 2019

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 12, 2019
joyeecheung requested a review from jasnell February 12, 2019 23:18

joyeecheung commented Feb 12, 2019

Copy link
Copy Markdown
Member Author

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

jasnell commented Feb 18, 2019

Copy link
Copy Markdown
Member

Trace events are not available in worker threads.

I think I missed something. Why is this? They should be available.

addaleax commented Feb 18, 2019

Copy link
Copy Markdown
Member

@jasnell Right – it’s just the JS module that’s not accessible… Is there a test for this code? I think that should have failed in CI if this PR breaks trace_events + Workers, but it looks like everything passed here

joyeecheung commented Feb 19, 2019

Copy link
Copy Markdown
Member Author

@addaleax I don't think there is a test, it could be added but I don't think the dynamic switch can be tested in workers since dynamically enabling the category requires the trace_events module. I could add the preparation code to the worker bootstrap script though, at least that allows us to test the initial enabling of the events.

src: move async hooks trace events setup to pre_execution.js
b5f6b65 
Reasons:

- Moves more environment-dependent setup out of bootstrap/node.js
- No async operations should be done before the call to
  the setup functions in pre_execution.js so no async hooks should be
  triggered before that. Therefore it is safe to delay the setup
  until then.
process: simplify the setup of async hooks trace events
9e2ba4b 
- Remove `trace_category_state` from `Environment` - since this is
  only accessed in the bootstrap process and later in the
  trace category update handler, we could just pass the initial
  values into JS land via the trace_events binding, and pass
  the dynamic values directly to the handler later, instead of
  accessing them out-of-band via the AliasedBuffer.
- Instead of creating the hooks directly in
  `trace_events_async_hooks.js`, export the hook factory and
  create the hooks in trace category state toggle.

joyeecheung commented Feb 19, 2019

Copy link
Copy Markdown
Member Author

I added the setup to the worker bootstrap, fixed a bug in the C++ part (when the category is enabled the value is 0x7, not 1, so we should test for != 0 instead), and added two tests. @jasnell @addaleax PTAL.

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

joyeecheung commented Feb 20, 2019

Copy link
Copy Markdown
Member Author

Added hasTracing guards in tests. Not sure why the CI complained about missing dynamically linked deps and libc, let's see what happens in a new CI: https://ci.nodejs.org/job/node-test-pull-request/20907/

richardlau reviewed Feb 20, 2019
View reviewed changes
Comment thread test/parallel/test-trace-events-async-hooks-dynamic.js Outdated
Comment thread test/parallel/test-trace-events-async-hooks-worker.js Outdated

joyeecheung commented Feb 20, 2019

Copy link
Copy Markdown
Member Author

@richardlau Thanks! I've updated the env passed to spawnSync. CI: https://ci.nodejs.org/job/node-test-pull-request/20913/

joyeecheung added async_hooks Issues and PRs related to the async hooks subsystem. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Feb 21, 2019

joyeecheung commented Feb 21, 2019

Copy link
Copy Markdown
Member Author

Landed in dec3dad...85df2c4

joyeecheung added a commit that referenced this pull request Feb 21, 2019
src: move async hooks trace events setup to pre_execution.js
435eea6 
Reasons:

- Moves more environment-dependent setup out of bootstrap/node.js
- No async operations should be done before the call to
  the setup functions in pre_execution.js so no async hooks should be
  triggered before that. Therefore it is safe to delay the setup
  until then.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Feb 21, 2019
process: simplify the setup of async hooks trace events
c435f06 
- Remove `trace_category_state` from `Environment` - since this is
  only accessed in the bootstrap process and later in the
  trace category update handler, we could just pass the initial
  values into JS land via the trace_events binding, and pass
  the dynamic values directly to the handler later, instead of
  accessing them out-of-band via the AliasedBuffer.
- Instead of creating the hooks directly in
  `trace_events_async_hooks.js`, export the hook factory and
  create the hooks in trace category state toggle.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Feb 21, 2019
test: add test for node.async_hooks tracing in workers
908e114 
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this pull request Feb 21, 2019
test: add test for dynamically enabling node.async_hooks tracing
85df2c4 
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
src: move async hooks trace events setup to pre_execution.js
503a3fe 
Reasons:

- Moves more environment-dependent setup out of bootstrap/node.js
- No async operations should be done before the call to
  the setup functions in pre_execution.js so no async hooks should be
  triggered before that. Therefore it is safe to delay the setup
  until then.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
process: simplify the setup of async hooks trace events
0c7123b 
- Remove `trace_category_state` from `Environment` - since this is
  only accessed in the bootstrap process and later in the
  trace category update handler, we could just pass the initial
  values into JS land via the trace_events binding, and pass
  the dynamic values directly to the handler later, instead of
  accessing them out-of-band via the AliasedBuffer.
- Instead of creating the hooks directly in
  `trace_events_async_hooks.js`, export the hook factory and
  create the hooks in trace category state toggle.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
test: add test for node.async_hooks tracing in workers
317b86c 
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 21, 2019
test: add test for dynamically enabling node.async_hooks tracing
384ea31 
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
src: move async hooks trace events setup to pre_execution.js
5cc2574 
Reasons:

- Moves more environment-dependent setup out of bootstrap/node.js
- No async operations should be done before the call to
  the setup functions in pre_execution.js so no async hooks should be
  triggered before that. Therefore it is safe to delay the setup
  until then.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
process: simplify the setup of async hooks trace events
966546c 
- Remove `trace_category_state` from `Environment` - since this is
  only accessed in the bootstrap process and later in the
  trace category update handler, we could just pass the initial
  values into JS land via the trace_events binding, and pass
  the dynamic values directly to the handler later, instead of
  accessing them out-of-band via the AliasedBuffer.
- Instead of creating the hooks directly in
  `trace_events_async_hooks.js`, export the hook factory and
  create the hooks in trace category state toggle.

PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
test: add test for node.async_hooks tracing in workers
dcbd907 
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
test: add test for dynamically enabling node.async_hooks tracing
28758b8 
PR-URL: #26062
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung mentioned this pull request Mar 12, 2019
Closed
4 tasks
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

jasnell jasnell approved these changes
addaleax addaleax approved these changes
richardlau richardlau approved these changes

Assignees

No one assigned

Labels

async_hooks Issues and PRs related to the async hooks subsystem. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL