lib,src: remove cpu profiler idle notifier #34010
Closed
Conversation
This file contains 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
I added it in commit 57231d5 ("src: notify V8 profiler when we're idle") from October 2013 as a stop-gap measure to measure CPU time rather than wall clock time, otherwise processes that spend a lot of time sleeping in system calls give a false impression of being very busy. That fix is not without drawbacks because the idle flag is set before libuv makes I/O callbacks and cleared again after. I/O callbacks can result into calls into JS code and executing JS code is as non-idle as you can get. In commit 96ffcb9 ("src: reduce cpu profiler overhead") from January 2015, I made Node.js block off the SIGPROF signal that V8's CPU profiler uses before Node.js goes to sleep. The goal of that commit is to reduce the overhead from EINTR system call wakeups but it also has the pleasant side effect of fixing what the idle notifier tried to fix. This commit removes the idle notifier and turns the JS process object methods into no-ops. Fixes: nodejs#19009 Refs: nodejs#33138
nodejs-github-bot
added
c++
bnoordhuis
commented
Jun 22, 2020
| process._startProfilerIdleNotifier = rawMethods._startProfilerIdleNotifier; | ||
| process._stopProfilerIdleNotifier = rawMethods._stopProfilerIdleNotifier; | ||
| process._startProfilerIdleNotifier = () => {}; | ||
| process._stopProfilerIdleNotifier = () => {}; |
There was a problem hiding this comment.
2 tasks
jasnell
reviewed
Jun 22, 2020
addaleax
approved these changes
Jun 22, 2020
29 tasks
This was referenced Jun 24, 2020
addaleax
added
the
author ready
jasnell
pushed a commit
that referenced
this pull request
Jun 25, 2020
I added it in commit 57231d5 ("src: notify V8 profiler when we're idle") from October 2013 as a stop-gap measure to measure CPU time rather than wall clock time, otherwise processes that spend a lot of time sleeping in system calls give a false impression of being very busy. That fix is not without drawbacks because the idle flag is set before libuv makes I/O callbacks and cleared again after. I/O callbacks can result into calls into JS code and executing JS code is as non-idle as you can get. In commit 96ffcb9 ("src: reduce cpu profiler overhead") from January 2015, I made Node.js block off the SIGPROF signal that V8's CPU profiler uses before Node.js goes to sleep. The goal of that commit is to reduce the overhead from EINTR system call wakeups but it also has the pleasant side effect of fixing what the idle notifier tried to fix. This commit removes the idle notifier and turns the JS process object methods into no-ops. Fixes: #19009 Refs: #33138 PR-URL: #34010 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
Landed in 40bc309 |
This was referenced Jun 26, 2020
codebytere
pushed a commit
that referenced
this pull request
Jun 27, 2020
I added it in commit 57231d5 ("src: notify V8 profiler when we're idle") from October 2013 as a stop-gap measure to measure CPU time rather than wall clock time, otherwise processes that spend a lot of time sleeping in system calls give a false impression of being very busy. That fix is not without drawbacks because the idle flag is set before libuv makes I/O callbacks and cleared again after. I/O callbacks can result into calls into JS code and executing JS code is as non-idle as you can get. In commit 96ffcb9 ("src: reduce cpu profiler overhead") from January 2015, I made Node.js block off the SIGPROF signal that V8's CPU profiler uses before Node.js goes to sleep. The goal of that commit is to reduce the overhead from EINTR system call wakeups but it also has the pleasant side effect of fixing what the idle notifier tried to fix. This commit removes the idle notifier and turns the JS process object methods into no-ops. Fixes: #19009 Refs: #33138 PR-URL: #34010 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
15 tasks
Merged
This was referenced Jun 29, 2020
codebytere
pushed a commit
that referenced
this pull request
Jun 30, 2020
I added it in commit 57231d5 ("src: notify V8 profiler when we're idle") from October 2013 as a stop-gap measure to measure CPU time rather than wall clock time, otherwise processes that spend a lot of time sleeping in system calls give a false impression of being very busy. That fix is not without drawbacks because the idle flag is set before libuv makes I/O callbacks and cleared again after. I/O callbacks can result into calls into JS code and executing JS code is as non-idle as you can get. In commit 96ffcb9 ("src: reduce cpu profiler overhead") from January 2015, I made Node.js block off the SIGPROF signal that V8's CPU profiler uses before Node.js goes to sleep. The goal of that commit is to reduce the overhead from EINTR system call wakeups but it also has the pleasant side effect of fixing what the idle notifier tried to fix. This commit removes the idle notifier and turns the JS process object methods into no-ops. Fixes: #19009 Refs: #33138 PR-URL: #34010 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
@bnoordhuis Now that this has landed, how is it possible to know when the process is idling? (without replicating the same logic with a native add-on hopefully). |
|
@rochdev Depends on your definition of "idle" and what you want to do in that case. Can you open an issue over at nodejs/help? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I added it in commit 57231d5 ("src: notify V8 profiler when we're
idle") from October 2013 as a stop-gap measure to measure CPU time
rather than wall clock time, otherwise processes that spend a lot
of time sleeping in system calls give a false impression of being
very busy.
That fix is not without drawbacks because the idle flag is set before
libuv makes I/O callbacks and cleared again after. I/O callbacks can
result into calls into JS code and executing JS code is as non-idle
as you can get.
In commit 96ffcb9 ("src: reduce cpu profiler overhead") from January
2015, I made Node.js block off the SIGPROF signal that V8's CPU profiler
uses before Node.js goes to sleep. The goal of that commit is to reduce
the overhead from EINTR system call wakeups but it also has the pleasant
side effect of fixing what the idle notifier tried to fix.
This commit removes the idle notifier and turns the JS process object
methods into no-ops.
Fixes: #19009
Refs: #33138