src: always enable idle notifier #33138
Conversation
This removes the functionality behind `process._startProfilerIdleNotifier()` and `process._stopProfilerIdleNotifier()` and instead always informs V8 over the current event loop state. The methods themselves are left as noop stubs as per our deprecation policy. Fixes: nodejs#19009
nodejs-github-bot
added
c++
|
Running it unconditionally is wrong. Not just "unconditionally" - the profiler idle notifier hack hasn't aged well, I'm afraid. Libuv runs things in this order: prepare handles -> I/O handles -> check handles. I/O handles result in calls into JS land. Not idle, in other words. |
|
Sorry, one of my kids slammed on my keyboard. :-) |
|
Forgot to mention, calling |
|
I guess I would have to ask what purpose these even serve? If it's just so we can know when the event loop is idle, I don't think it actually serves the purpose adequately and I would have to question the premise anyway. Trevor Norris' idle time proposal would seem a much more logical and better fit for what we'd need to know. I'd suggest that we could just remove this entirely. Make the functions noops and deprecate them. |
|
It's so that Trevor's proposal won't work for that - that data doesn't end up in the log file - but the hooks I suggested in libuv/libuv#2725 (comment) could fix it. That said, I hacked around it for Removing the |
|
|
||
| // Stubs for legacy methods. | ||
| process._startProfilerIdleNotifier = () => {}; | ||
| process._stopProfilerIdleNotifier = () => {}; |
There was a problem hiding this comment.
Just turning them into stubs does not seem an ideal way to handle this.
It would be very surprising for me to see that a function would just silently not behave as it originally did (if I would use the function).
I suggest to at least add a note with --pending-deprecation and make this function non-enumerable. Ideally, we could just run CITGM and https://github.com/nodejs/Gzemnid to see how many libraries actually use these. Those should ideally be fixed by us and in that case I would also add a runtime deprecation.
There was a problem hiding this comment.
@BridgeAR … this PR turns the functionality always-on rather than using these methods to enable or disable it. This isn’t even a breaking change, and in fact adding a runtime deprecation (--pending-deprecation or not) would be far more invasive, so I can’t really agree here at all.
(That being said, I’ll probably not update the PR until I have looked into Ben’s concerns anyway.)
There was a problem hiding this comment.
If a user wanted to disable it, it would be a changed behavior?
If we want to prevent usage of an API we should definitely let the user know about it. Just silently doing nothing seems pretty bad to me.
A deprecation notice is IMO also not invasive. If a user does not want to get notified, it's possible to opt-out of those.
There was a problem hiding this comment.
If a user wanted to disable it, it would be a changed behavior?
Only in terms of diagnostic output, not program behavior. Practically speaking, this is not something that’s used in end-user code but in diagnostic platforms.
If we want to prevent usage of an API we should definitely let the user know about it. Just silently doing nothing seems pretty bad to me.
I don’t want to prevent usage of this API, hence the lack of a deprecation.
A deprecation notice is IMO also not be invasive. If a user does not want to get notified, it's possible to opt-out of those.
I mean, you know I disagree strongly here.
BridgeAR
force-pushed
the
master
branch
2 times, most recently
from
8ae28ff to
2935f72
Compare
|
Not sure where this one left off at... @bnoordhuis ... what is the correct thing to do with this one? |
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
I've opened #34010 that does ^ |
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
This removes the functionality behind
process._startProfilerIdleNotifier()andprocess._stopProfilerIdleNotifier()and instead always informs V8over the current event loop state.
The methods themselves are left as noop stubs as per our deprecation
policy.
Fixes: #19009
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes