-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
inspector: split --cpu-prof-path to --cpu-prof-dir and --cpu-prof-name #27306
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
cc @nodejs/workers @nodejs/performance @nodejs/process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Why |
To improve the integration of `--cpu-prof` with workers, this patch splits `--cpu-prof-path` into `--prof-dir` and `--cpu-prof-name`, so when a worker is launched from a thread that enables `--cpu-prof`, if the parent thread sets `--prof-dir`, then the profile of both thread would be generated to the specified directory. If they end up specifying the same `--cpu-prof-name` the behavior is undefined the last profile will overwritten the first one.
I prefer keeping the specific directory prefix in the flag names for clarity. If we support more profile types in the future it just gives more flexibility. |
How would that gives more flexibility? If the whole path is specified in the flag, how would BTW, this is also closer to how the reports are generated. I think it's better to have a consistent style to generate these files. |
This comment has been minimized.
This comment has been minimized.
I was referring to having individual profile type directory prefix flags like |
I see, that makes sense to me, I'll switch to that instead, thanks! |
This comment has been minimized.
This comment has been minimized.
…and --cpu-prof-name
This comment has been minimized.
This comment has been minimized.
If there are no more reviews, I'd like to land this later today. |
Landed in 49d3d11 |
To improve the integration of `--cpu-prof` with workers, this patch splits `--cpu-prof-path` into `--cpu-prof-dir` and `--cpu-prof-name`, so when a worker is launched from a thread that enables `--cpu-prof`, if the parent thread sets `--cpu-prof-dir`, then the profile of both thread would be generated to the specified directory. If they end up specifying the same `--cpu-prof-name` the behavior is undefined the last profile will overwritten the first one. PR-URL: #27306 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
To improve the integration of
--cpu-prof
with workers, this patchsplits
--cpu-prof-path
into--cpu-prof-dir
and--cpu-prof-name
,so when a worker is launched from a thread that enables
--cpu-prof
, if the parent thread sets--cpu-prof-dir
, then theprofile of both thread would be generated to the specified directory.
If they end up specifying the same
--cpu-prof-name
the behavioris undefined - the last profile will overwritten the first one.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes