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: trace threadpool event #44458

Merged

Conversation

theanarkh
Copy link
Contributor

trace threadpool events, such as APIs of Blob, zlib, crypto and node_api.

  • 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 nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 30, 2022
@theanarkh theanarkh force-pushed the trace_threadpoolwork_event branch from 6f35dbf to 245ef10 Compare August 30, 2022 16:35
@theanarkh theanarkh force-pushed the trace_threadpoolwork_event branch 2 times, most recently from b87e591 to 39bd5b5 Compare August 30, 2022 17:44
src/node_internals.h Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the trace_threadpoolwork_event branch from 39bd5b5 to 39f400b Compare August 31, 2022 02:26
src/node_internals.h Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the trace_threadpoolwork_event branch from 39f400b to ef94baa Compare August 31, 2022 02:40
src/threadpoolwork-inl.h Show resolved Hide resolved
doc/api/tracing.md Outdated Show resolved Hide resolved
doc/api/tracing.md Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the trace_threadpoolwork_event branch 2 times, most recently from 40c49ab to 672d9ad Compare August 31, 2022 13:24
src/node_internals.h Outdated Show resolved Hide resolved
@theanarkh theanarkh force-pushed the trace_threadpoolwork_event branch 2 times, most recently from e077266 to 24d32ce Compare September 1, 2022 06:28
src/threadpoolwork-inl.h Outdated Show resolved Hide resolved
src/threadpoolwork-inl.h Show resolved Hide resolved
src/threadpoolwork-inl.h Outdated Show resolved Hide resolved
src/threadpoolwork-inl.h Show resolved Hide resolved
@theanarkh
Copy link
Contributor Author

@legendecas @RaisinTen Hi, can we just trace the total time of work. the code is as follows.

void ThreadPoolWork::ScheduleWork() {
  env_->IncreaseWaitingRequestCounter();

  TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(...);

  int status = uv_queue_work(
      env_->event_loop(),
      &work_req_,
      [](uv_work_t* req) {
        ThreadPoolWork* self = ContainerOf(&ThreadPoolWork::work_req_, req);
        self->DoThreadPoolWork();
      },
      [](uv_work_t* req, int status) {
        ThreadPoolWork* self = ContainerOf(&ThreadPoolWork::work_req_, req);
        self->env_->DecreaseWaitingRequestCounter();

        TRACE_EVENT_NESTABLE_ASYNC_END1(...);

        self->AfterThreadPoolWork(status);
      });
  CHECK_EQ(status, 0);
}

Or trace all the time by two different event type.

void ThreadPoolWork::ScheduleWork() {
  env_->IncreaseWaitingRequestCounter();

  TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(...);

  int status = uv_queue_work(
      env_->event_loop(),
      &work_req_,
      [](uv_work_t* req) {
        ThreadPoolWork* self = ContainerOf(&ThreadPoolWork::work_req_, req);
        TRACE_EVENT_BEGIN(...);
        self->DoThreadPoolWork();
        TRACE_EVENT_END(...);
      },
      [](uv_work_t* req, int status) {
        ThreadPoolWork* self = ContainerOf(&ThreadPoolWork::work_req_, req);
        self->env_->DecreaseWaitingRequestCounter();

        TRACE_EVENT_NESTABLE_ASYNC_END1(...);

        self->AfterThreadPoolWork(status);
      });
  CHECK_EQ(status, 0);
}

@legendecas
Copy link
Member

I'd still find that tracing the synchronous event is important. So the latter alternative is LGTM.

@theanarkh theanarkh force-pushed the trace_threadpoolwork_event branch 2 times, most recently from 1703fd4 to 05cbb9c Compare September 14, 2022 16:26
@theanarkh
Copy link
Contributor Author

It seems we can not use TRACE_EVENT_BEGIN0 and TRACE_EVENT_END0 in another thread, i will take a look later.
https://github.com/nodejs/node/actions/runs/3054446637/jobs/4926355622.

@theanarkh theanarkh force-pushed the trace_threadpoolwork_event branch 3 times, most recently from a8b5f8f to b1128a1 Compare October 26, 2022 00:38
@theanarkh
Copy link
Contributor Author

@legendecas Hi, it seems the TRACE_EVENT_xx is not thread-safe, so i record the time of synchronous API call in args field of async event. what do you think ?

@theanarkh theanarkh force-pushed the trace_threadpoolwork_event branch from b1128a1 to 1f402fc Compare October 26, 2022 01:30
@legendecas
Copy link
Member

@theanarkh The macros are declared to be thread-safe. If there is any problem related to thread safety, it should be fixed.

I'd find the problem here is related to the incomplete shutdown -- e.g. the global state of the tracing controller is not reset after the platform is shut down https://github.com/nodejs/node/blob/main/src/tracing/trace_event.cc#L8. This global state is used in the macro expansion: https://github.com/nodejs/node/blob/main/src/tracing/trace_event.h#L474. We should properly shut down the platform with the tracing global state.

@theanarkh
Copy link
Contributor Author

theanarkh commented Oct 26, 2022

Do you mean this test case https://github.com/nodejs/node/actions/runs/3054446637/jobs/4926355622 ? I remember when i change common.platformTimeout(10) to common.platformTimeout(100), the test exits normally🤔.

@legendecas
Copy link
Member

@theanarkh yeah, IIUC that test case is exactly testing a premature exit while the async work is still in progress.

@nodejs-github-bot
Copy link
Collaborator

@theanarkh theanarkh added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 1, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 1, 2022
@nodejs-github-bot nodejs-github-bot merged commit 8fb282e into nodejs:main Nov 1, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 8fb282e

RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 1, 2022
PR-URL: #44458
Reviewed-By: Chengzhong Wu <[email protected]>
@theanarkh theanarkh mentioned this pull request Nov 1, 2022
4 tasks
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2022
lucshi pushed a commit to lucshi/node that referenced this pull request Nov 9, 2022
PR-URL: nodejs#44458
Reviewed-By: Chengzhong Wu <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Nov 10, 2022
PR-URL: #44458
Reviewed-By: Chengzhong Wu <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Nov 19, 2022
PR-URL: #45266
Refs: #45092
Refs: #44458
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45266
Refs: #45092
Refs: #44458
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
PR-URL: nodejs#45266
Refs: nodejs#45092
Refs: nodejs#44458
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #44458
Reviewed-By: Chengzhong Wu <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45266
Refs: #45092
Refs: #44458
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #44458
Reviewed-By: Chengzhong Wu <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45266
Refs: #45092
Refs: #44458
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
When the process exits, there may be tasks in the thread pool
that need to access data in the platform, such as trace agent.
So make sure the thread pool exits first.
see #44458

PR-URL: #45226
Refs: #44458
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #44458
Reviewed-By: Chengzhong Wu <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45266
Refs: #45092
Refs: #44458
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45266
Refs: #45092
Refs: #44458
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
PR-URL: #45266
Refs: #45092
Refs: #44458
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants