Node: Deprecating the process._startProfilerIdleNotifier & process._stopProfilerIdleNotifier methods

[JacksonTian]

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

[bnoordhuis]

I know I added them but I'll be damned if I remember why. I'm good with removing them.

[mcollina]

👍 to remove or runtime-deprecate them.

[benjamingr]

Anyone using these methods in userland?

[ofrobots]

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

1. 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.
2. 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?

[joyeecheung]

@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.

[ofrobots]

@joyeecheung thanks! if you have pointers to some of these discussions, those would be very useful for me. Perhaps you were thinking of requestIdleCallback (https://github.com/nodejs/node/issues/2543) – which is something very different?

[joyeecheung]

@ofrobots Did some archeology and I am pretty sure I was talking about https://github.com/nodejs/node/commit/d607d856afc744b1e6bc42cdb9b67b6c4e7e4876 although it seems specific to regulating GC

[Yogu]

@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. ❤️

[Trott]

Looks like these are now in src/node_process_methods.cc.

What's the status of this?

1. 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.
2. 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.

[bnoordhuis]

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) or kevent(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:

https://github.com/nodejs/node/blob/ea87809bb6696e2c1ec5d470031f137e18641183/src/node.cc#L759

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.

[Trott]

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) or kevent(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:

https://github.com/nodejs/node/blob/ea87809bb6696e2c1ec5d470031f137e18641183/src/node.cc#L759

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?

[bnoordhuis]

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. 6820054d2d...f649626c6f 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.

[addaleax]

If we decide to perform idle notifications by default, then I would just indefinitely leave these methods as no-op stubs, which renders deprecating unnecessary. I’ve opened https://github.com/nodejs/node/pull/33138 to implement that.