inspector: implement V8Inspector timer #14346
Conversation
eugeneo
added
c++
nodejs-github-bot
added
c++
src/inspector_agent.cc
Outdated
| @@ -527,13 +555,25 @@ class NodeInspectorClient : public v8_inspector::V8InspectorClient { | |||
| return channel_.get(); | |||
| } | |||
|
|
|||
| void startRepeatingTimer(double interval, TimerCallback callback, void* data) | |||
| override { | |||
There was a problem hiding this comment.
nit: I’d align the arguments vertically instead, that seems a bit more readable
src/inspector_agent.cc
Outdated
| public: | ||
| RepeatingInspectorTimer(uv_loop_t* loop, double interval, | ||
| v8_inspector::V8InspectorClient::TimerCallback callback, | ||
| void* data) : timer_(uv_timer_t()), callback_(callback), |
There was a problem hiding this comment.
Doesn’t just timer_() work as well?
Also, I’d really rather align the arguments (and intializers) vertically for readability here
src/inspector_agent.cc
Outdated
| void* data) : timer_(uv_timer_t()), callback_(callback), | ||
| data_(data) { | ||
| uv_timer_init(loop, &timer_); | ||
| int64_t interval_ms = 1000 * interval; |
There was a problem hiding this comment.
This is just a question, not a request to change anything: Does V8 have any documention saying that this interval is supposed to be given in seconds? Where would one find that?
There was a problem hiding this comment.
I don't think so. I tried to find it myself... This code originally came from Chrome where it seem to be dated back to WebKit days.
src/inspector_agent.cc
Outdated
| RepeatingInspectorTimer(const RepeatingInspectorTimer&) = delete; | ||
|
|
||
| ~RepeatingInspectorTimer() { | ||
| uv_timer_stop(&timer_); |
There was a problem hiding this comment.
This should uv_close() the timer (and not free the memory until the close callback is called.)
There was a problem hiding this comment.
Oops. Thanks for pointing it out! I fixed it.
|
@bnoordhuis Please take a look. |
|
I'm waiting for arm-fanned CI bot to recover - not feeling confident enough to push this without as little as compiling on all platforms. |
|
Landed as 012206e |
This timer is currently only used by a profiler. Note that the timer invocation will not interrupt JavaScript code. Fixes: #11401 PR-URL: #14346 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
This timer is currently only used by a profiler. Note that the timer
invocation will not interrupt JavaScript code.
Fixes: #11401
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
inspector: implemented two client methods.