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: TraceEventScope should mark sync duration events #42977

Closed
wants to merge 1 commit into from

Conversation

legendecas
Copy link
Member

According to the chrome trace event format document, works that are performed on one single thread should be traced with sync duration events. In this way, these events can be grouped under one thread and the trace event viewer can estimate the CPU usage of that thread.

click to see screen shots

Before:
Screen Shot 2022-05-05 at 11 54 19 PM

Note that there are no events recorded in the JavaScriptMainThread nor WorkerThread 1.

After:
Screen Shot 2022-05-05 at 11 54 13 PM

Environment events like cleanup are correctly grouped under their thread.

@legendecas legendecas marked this pull request as draft May 5, 2022 15:59
@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. needs-ci PRs that need a full CI run. labels May 5, 2022
@legendecas legendecas marked this pull request as ready for review May 5, 2022 17:06
@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 6, 2022
@nodejs-github-bot

This comment was marked as outdated.

src/node_internals.h Outdated Show resolved Hide resolved
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM apart from one tiny optional nitpick.

src/node_contextify.cc Show resolved Hide resolved
@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 8, 2022
@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

jasnell commented May 8, 2022

Not sure if this is the correct fix. They were implemented the way they were so that we can trace how long these various methods take to complete so we can better determine if changes introduce performance regressions. That said, they likely aren't broadly used and everything under trace events is still considered experimental and therefore breaking changes are ok.

@legendecas
Copy link
Member Author

legendecas commented May 9, 2022

The duration events do contain the timestamps on which we can trace how long these methods take to complete, as same as async events. However, synchronous duration events also suggest that these methods are performed in one single thread for a period of time, while async events are not and can be performed intermittently.

@jasnell I'm not sure what you mean by "correct fix". Do you suggest that the methods changed in this PR can be performed in different threads for a given unique id or intermittently?

@legendecas
Copy link
Member Author

@jasnell would you mind elaborating your suggestion here? This PR has opened for 7 days but I'd still like to know your opinions before landing. Thank you.

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
Member Author

Landed in 7035186

@legendecas legendecas closed this May 16, 2022
legendecas added a commit that referenced this pull request May 16, 2022
According to the chrome trace event format document, works that
are performed on one single thread should be traced with sync
duration events. In this way, these events can be grouped under
one thread and the trace event viewer can estimate the CPU usage
of that thread.

PR-URL: #42977
Reviewed-By: Darshan Sen <[email protected]>
@legendecas legendecas deleted the trace-scope branch May 16, 2022 14:29
bengl pushed a commit that referenced this pull request May 30, 2022
According to the chrome trace event format document, works that
are performed on one single thread should be traced with sync
duration events. In this way, these events can be grouped under
one thread and the trace event viewer can estimate the CPU usage
of that thread.

PR-URL: #42977
Reviewed-By: Darshan Sen <[email protected]>
@bengl bengl mentioned this pull request May 31, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
According to the chrome trace event format document, works that
are performed on one single thread should be traced with sync
duration events. In this way, these events can be grouped under
one thread and the trace event viewer can estimate the CPU usage
of that thread.

PR-URL: #42977
Reviewed-By: Darshan Sen <[email protected]>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
According to the chrome trace event format document, works that
are performed on one single thread should be traced with sync
duration events. In this way, these events can be grouped under
one thread and the trace event viewer can estimate the CPU usage
of that thread.

PR-URL: #42977
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
According to the chrome trace event format document, works that
are performed on one single thread should be traced with sync
duration events. In this way, these events can be grouped under
one thread and the trace event viewer can estimate the CPU usage
of that thread.

PR-URL: #42977
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
According to the chrome trace event format document, works that
are performed on one single thread should be traced with sync
duration events. In this way, these events can be grouped under
one thread and the trace event viewer can estimate the CPU usage
of that thread.

PR-URL: #42977
Reviewed-By: Darshan Sen <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
According to the chrome trace event format document, works that
are performed on one single thread should be traced with sync
duration events. In this way, these events can be grouped under
one thread and the trace event viewer can estimate the CPU usage
of that thread.

PR-URL: nodejs/node#42977
Reviewed-By: Darshan Sen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants