Repository navigation
Deprecating the process._startProfilerIdleNotifier & process._stopProfilerIdleNotifier methods #19009
Description
Activity
- changed the title
[-]remove[/-][+]remove the process._startProfilerIdleNotifier & process._stopProfilerIdleNotifier methods[/+]on Feb 26, 2018 - changed the title
[-]remove the process._startProfilerIdleNotifier & process._stopProfilerIdleNotifier methods[/-][+]Deprecate the process._startProfilerIdleNotifier & process._stopProfilerIdleNotifier methods[/+]on Feb 26, 2018 - changed the title
[-]Deprecate the process._startProfilerIdleNotifier & process._stopProfilerIdleNotifier methods[/-][+]Deprecating the process._startProfilerIdleNotifier & process._stopProfilerIdleNotifier methods[/+]on Feb 26, 2018 I know I added them but I'll be damned if I remember why. I'm good with removing them.
Reacted by Ron Korving, Benjamin Gruenbaum, Alex Yang and bl-ue👍 to remove or runtime-deprecate them.
Anyone using these methods in userland?
Just found this issue while trying to understand why Node doesn't notify the VM for idle phases by default.
I don't know the motivation that @bnoordhuis had originally. My motivation is from a profiler perspective. A profiler (e.g. Chrome DevTools) should be able to distinguish JS execution vs. native execute vs. idle time. Currently, when no JS is executing, the profiler has no idea if Node was spending time in C++ or was really idle.
Apart from the the diagnostic use-case, I think the VM might also be able to get a better idea of process load based on how busy the host (Node) seems to be, and might be able to take advantage of this in tweaking GC and background compilation heuristics. This is not profiling specific.
IOW, I think we should
- Perform idle notifications by default. I think the current mechanism (uv_prepare_t, uv_check_t) might do a few too many notifications (once per loop iteration). I think this can be improved.
- If we do that, we do not need to expose these functions to JavaScript. I'm +1 on deprecating and removing these, if we do the previous bullet (I'd be happy to go implement that).
WDYT? Any concerns/objections?
Reacted by Jan Melcher, Shawn Walton and Anna Henningsen@ofrobots (This may not be helpful but FWIW) I think I've seen somewhere in the comments that the idle notifications in core were removed because they "did not work well". But those notes were ancient and it's probably not a bad time to revisit this and integrate the core better with the VM.
@joyeecheung thanks! if you have pointers to some of these discussions, those would be very useful for me. Perhaps you were thinking of
requestIdleCallback(#2543) – which is something very different?@ofrobots I don't know any node internals, but I'm 100% with you on enabling these by default (if there are no significant performance problems). And thanks a lot for this PR where you fixed it in a project - I shamelessly copied it into my project to get idle times working. ❤️
Reacted by Baptiste MansonLooks like these are now in
src/node_process_methods.cc.What's the status of this?
- Perform idle notifications by default. I think the current mechanism (uv_prepare_t, uv_check_t) might do a few too many notifications (once per loop iteration). I think this can be improved.
- If we do that, we do not need to expose these functions to JavaScript. I'm +1 on deprecating and removing these, if we do the previous bullet (I'd be happy to go implement that).
Did the first bullet point ever happen? If not, is there someone motivated to implement it? I don't get the impression that either @JacksonTian or @ofrobots are terribly active on the project these days, but I'd be thrilled to see them or anyone else jump in on this to get it across the finish line.
I remember what my motivation for adding those methods was. It's to stop the V8 profiler from counting time spent sleeping in
epoll_wait(2)orkevent(2)as CPU (i.e., running) time.We handle that in libuv now so in that respect the start/stop methods aren't necessary anymore:
Line 759 in ea87809
There's probably still value in post-processing tools knowing when node is sleeping but prepare/check handles are too coarse for that: I/O callbacks run in between and, as a result, are counted as idle time.
I remember what my motivation for adding those methods was. It's to stop the V8 profiler from counting time spent sleeping in
epoll_wait(2)orkevent(2)as CPU (i.e., running) time.We handle that in libuv now so in that respect the start/stop methods aren't necessary anymore:
Line 759 in ea87809
There's probably still value in post-processing tools knowing when node is sleeping but prepare/check handles are too coarse for that: I/O callbacks run in between and, as a result, are counted as idle time.
I'm sorry, but I'm not sure I fully understand. Does this mean that this issue can be closed? Or does it mean that it can be closed after we do some work in core to rely on libuv's handling of this?
There's a bit of scope creep going on in this issue. Let's start with @JacksonTian's proposal to deprecate the methods.
I think that's a fine thing to do - they're not necessary and I shouldn't have added them. 6820054...f649626 is the relevant changeset; I added them because I could, not because I should.
This issue should stay open until that change is made.
Next is @ofrobots's sugggestion: perform idle notifications by default. I also think that's a fine thing to do but it's a separate issue. Let's open a new issue for that.
Reacted by Rich Trott- added a commit that references this issue
on Apr 29, 2020 - added a commit that references this issue
on Jun 22, 2020 - added 2 commits that reference this issue
on Jun 27, 2020 - added a commit that references this issue
on Jul 27, 2026
When I browse the source code of src/node.cc, I found _startProfilerIdleNotifier & _stopProfilerIdleNotifier methods. These methods never be documented and haven't any test cases in test/*.
I am proposing that we can mark the two methods as deprecated and remove later.
In user land, v8-profiler can do things better.
If no against for this proposal, I will start the work.
CC @nodejs/collaborators