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

src: refactor tracing code #21867

Closed
wants to merge 13 commits into from
Closed

src: refactor tracing code #21867

wants to merge 13 commits into from

Conversation

addaleax
Copy link
Member

This addresses a number with issues with our tracing code related to code style, readability, race conditions, memory leaks, file descriptor leaks, and other bugs.

I hope that this might help a bit with the flaky test situation around trace_events.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@addaleax addaleax requested review from ofrobots, eugeneo and jasnell July 18, 2018 10:20
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 18, 2018
@addaleax addaleax added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jul 18, 2018
@addaleax
Copy link
Member Author

Btw, this might not completely resolve the issues with this code – but I think this is an improvement over the current situation.

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

@eugeneo
Copy link
Contributor

eugeneo commented Jul 18, 2018

Sorry, I do not have a time to do a proper review today. I had been working on an alternative implementation for this (80% done), just want to share design..

  1. File writer and inspector writers live entirely on a "tracing thread". I.e. no asyncs, no mutexes, etc.
  2. node_trace_buffer is fully thread safe (all calls are synchronized).
  3. tracing/agent.cc is the one that passes buffers from "any" thread (i.e. the thread that trace controller chose to call the buffer) to the writers on the tracing thread.
  4. inspector protocol writer uses MainThreadInterface to pass the message from the tracing thread to the inspector dispatcher on the main thread.

@addaleax
Copy link
Member Author

@eugeneo If you’d like to share code, I’m happy to take a look!

@eugeneo
Copy link
Contributor

eugeneo commented Jul 19, 2018

@addaleax https://github.com/eugeneo/node/tree/single-threaded-tracing - this is my wip branch, but it does not even compile.

@eugeneo
Copy link
Contributor

eugeneo commented Jul 23, 2018

I would like to suggest adding a test case, e.g. - https://github.com/eugeneo/node/blob/19379fef2767dcb691b75b432a76bc0715a3ffe1/test/parallel/test-tracing-no-crash.js

@ofrobots
Copy link
Contributor

@addaleax
Copy link
Member Author

addaleax commented Jul 27, 2018

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

(edit: Windows failures are real)

addaleax added 11 commits August 1, 2018 15:11
A forward declaration suffices and means that the tracing
agent header and its dependencies don’t need to be included
in nearly every Node.js file.
Avoid unnecessary copies/extra operations & align with our style guide,
and add protection against throwing getters.
Avoid unnecessary operations, add a bit of documentation,
and turn the `ClientHandle` smart pointer alias into a real
class.

This should allow de-coupling the unnecessary combination of
a single specific `file_writer` from `Agent`.
The agent code is supposed to manage multiple writers/clients.
Adding the default writer into the mix breaks that encapsulation.
Instead, manage default options through a separate "virtual"
default client handle, and keep the file writer management
all to the actual users.
Otherwise this would have crashed the process.
In particular, checking the return value of an libuv call against `-1`
was invalid to begin with, as libuv uses it to propagate the
error code.
Clean up resources when tearing down the tracing agent.
Run the initialization for the file trace writer’s `uv_async_t`s
on the same thread as `uv_run()` for their loop to avoid race
conditions.
Close existing file descriptors before overriding
the field with new ones.

Also, use `nullptr` as the loop for these synchronous
operations, since they do not run on the same thread
as the `uv_run()` call for the previously used loop.
Concurrent writes to the same fd are generally not ideal,
since it’s not generally guaranteed that data from those
writes will end up on disk in the right order.
addaleax added a commit that referenced this pull request Aug 1, 2018
PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
addaleax added a commit that referenced this pull request Aug 1, 2018
Concurrent writes to the same fd are generally not ideal,
since it’s not generally guaranteed that data from those
writes will end up on disk in the right order.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
addaleax added a commit that referenced this pull request Aug 1, 2018
Fixes: #22042

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
addaleax pushed a commit that referenced this pull request Aug 1, 2018
PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
@addaleax addaleax deleted the tracing branch August 1, 2018 15:28
targos pushed a commit that referenced this pull request Aug 1, 2018
A forward declaration suffices and means that the tracing
agent header and its dependencies don’t need to be included
in nearly every Node.js file.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
Avoid unnecessary copies/extra operations & align with our style guide,
and add protection against throwing getters.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
Avoid unnecessary operations, add a bit of documentation,
and turn the `ClientHandle` smart pointer alias into a real
class.

This should allow de-coupling the unnecessary combination of
a single specific `file_writer` from `Agent`.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
The agent code is supposed to manage multiple writers/clients.
Adding the default writer into the mix breaks that encapsulation.
Instead, manage default options through a separate "virtual"
default client handle, and keep the file writer management
all to the actual users.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
Otherwise this would have crashed the process.
In particular, checking the return value of an libuv call against `-1`
was invalid to begin with, as libuv uses it to propagate the
error code.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
Clean up resources when tearing down the tracing agent.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
Run the initialization for the file trace writer’s `uv_async_t`s
on the same thread as `uv_run()` for their loop to avoid race
conditions.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
Close existing file descriptors before overriding
the field with new ones.

Also, use `nullptr` as the loop for these synchronous
operations, since they do not run on the same thread
as the `uv_run()` call for the previously used loop.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
Concurrent writes to the same fd are generally not ideal,
since it’s not generally guaranteed that data from those
writes will end up on disk in the right order.

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
Fixes: #22042

PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
targos pushed a commit that referenced this pull request Aug 1, 2018
PR-URL: #21867
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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
c++ Issues and PRs that require attention from people who are familiar with C++. 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
Development

Successfully merging this pull request may close these issues.

5 participants