Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upsrc: remove TimerWrap #20894
Conversation
apapirovski
added
timers
semver-major
labels
May 22, 2018
apapirovski
requested review from
bnoordhuis,
addaleax and
Fishrock123
May 22, 2018
nodejs-github-bot
added
the
lib / src
label
May 22, 2018
This comment has been minimized.
This comment has been minimized.
apapirovski
force-pushed the
apapirovski:patch-timerwrap-refactor
branch
from
28a7570
to
30c54f9
May 22, 2018
addaleax
reviewed
May 22, 2018
| @@ -450,6 +459,59 @@ void Environment::RunAndClearNativeImmediates() { | |||
| } | |||
|
|
|||
|
|
|||
| void Environment::ScheduleTimer(int64_t duration) { | |||
This comment has been minimized.
This comment has been minimized.
addaleax
May 22, 2018
Member
Can we add the unit of measurement to duration? E.g. duration_ms if it’s milliseconds?
| do { | ||
| TryCatch try_catch(env->isolate()); | ||
| try_catch.SetVerbose(true); | ||
| ret = cb->Call(env->context(), process, 1, args); |
This comment has been minimized.
This comment has been minimized.
|
|
||
| if (expiry != 0) { | ||
| int64_t duration = | ||
| abs(expiry) - (uv_now(env->event_loop()) - env->timer_base()); |
This comment has been minimized.
This comment has been minimized.
addaleax
May 22, 2018
Member
ditto re: adding the unit to the name
(Also, why abs? Can you add a comment?)
| result->Set(env->context(), 1, timer_toggle_ref_function).FromJust(); | ||
| result->Set(env->context(), 2, | ||
| env->immediate_info()->fields().GetJSArray()).FromJust(); | ||
| result->Set(env->context(), 3, imm_toggle_ref_function).FromJust(); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Thanks @addaleax. I've cleaned up and added some detailed comments. Hope this is better :) |
addaleax
approved these changes
May 22, 2018
apapirovski
force-pushed the
apapirovski:patch-timerwrap-refactor
branch
2 times, most recently
from
a0b2f82
to
dc80758
May 22, 2018
jasnell
reviewed
May 23, 2018
| // DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR | ||
| // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE | ||
| // USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
|
|
This comment has been minimized.
This comment has been minimized.
jasnell
May 23, 2018
Member
Is there any possibility of userland native modules depending on this? Is it safe to remove without a deprecation cycle?
/cc @bnoordhuis
This comment has been minimized.
This comment has been minimized.
apapirovski
May 23, 2018
Author
Member
I've done an extensive search and there was not a single module I could find using it. I'm not really sure why someone would want to anyway given that Timer.now() is not that different from process.hrtime() but isn't officially supported.
But also process.binding shouldn't be part of our official deprecation cycle, should it? We broke Gulp in 10.0.0 with changes to contextify and that's a huge module.
(And the TimerWrap itself is impossible to use outside of Node.js.)
This comment has been minimized.
This comment has been minimized.
|
Does this need a proper deprecation cycle? /cc @nodejs/tsc |
This comment has been minimized.
This comment has been minimized.
|
Here's CitGM now that it's not broken anymore: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1431/ |
apapirovski
force-pushed the
apapirovski:patch-timerwrap-refactor
branch
from
dc80758
to
15b2ddd
May 23, 2018
apapirovski
reviewed
May 23, 2018
| @@ -52,7 +52,7 @@ async function test() { | |||
| 'node.perf.timerify', 'v8'], | |||
| categories); | |||
|
|
|||
| const traceConfig = { includedCategories: ['node'] }; | |||
| const traceConfig = { includedCategories: ['v8'] }; | |||
This comment has been minimized.
This comment has been minimized.
apapirovski
May 23, 2018
Author
Member
@eugeneo This test fails without this change. I suspect it has to do with the removal of the HandleWrap, right? Or is there something else at play? Would appreciate you having a look as the author of this test and someone with a lot of understanding of all this tracing stuff. Thanks!
This comment has been minimized.
This comment has been minimized.
apapirovski
May 23, 2018
Author
Member
@nodejs/diagnostics Perhaps some of you might have insight too? Thanks!
This test fails without this change. I suspect it has to do with the removal of the HandleWrap, right? Or is there something else at play? Would appreciate you having a look as the author of this test and someone with a lot of understanding of all this tracing stuff. Thanks!
This comment has been minimized.
This comment has been minimized.
bnoordhuis
May 23, 2018
Member
It's because timer_wrap.cc uses AsyncWrap::MakeCallback(), which calls AsyncWrap::EmitTraceEventAfter(). Since you no longer use that method, it's not traced any more.
I personally don't feel too strongly but I guess it would be nice to keep the current behavior.
This comment has been minimized.
This comment has been minimized.
apapirovski
May 23, 2018
•
Author
Member
I wonder if we should do something about Immediates too in that case? But I don't know how to best handle it since there's no longer an AsyncWrap (or obviously a matching provider). All of this trace event stuff is pretty outside of my expertise as far as Node goes so I'm all ears...
Since this is semver-major (and won't land in a release for a while), perhaps we could add a TODO: or start an issue so that more qualified people can chime in (including yourself)?
This comment has been minimized.
This comment has been minimized.
bnoordhuis
May 23, 2018
Member
I'd be okay with that. In my opinion, AsyncWrap currently conflates too much (mushes too much into a single class) and could use some decoupling.
This comment has been minimized.
This comment has been minimized.
apapirovski
force-pushed the
apapirovski:patch-timerwrap-refactor
branch
2 times, most recently
from
5ab8e99
to
2b7ae97
May 23, 2018
bnoordhuis
approved these changes
May 23, 2018
|
LGTM modulo comments. |
| @@ -53,8 +58,9 @@ const kCount = 0; | |||
| const kRefCount = 1; | |||
| const kHasOutstanding = 2; | |||
|
|
|||
| const [immediateInfo, toggleImmediateRef] = | |||
| setupTimers(processImmediate, processTimers); | |||
| // Call into C++ to assing callbacks that are responsible for processing | |||
This comment has been minimized.
This comment has been minimized.
| listOnTimeout(list, now); | ||
| ran = true; |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
apapirovski
May 23, 2018
Author
Member
Yeah, it's just cleaner this way since we don't assign each time.
| @@ -161,6 +163,9 @@ void Environment::Start(int argc, | |||
| HandleScope handle_scope(isolate()); | |||
| Context::Scope context_scope(context()); | |||
|
|
|||
| uv_timer_init(event_loop(), timer_handle()); | |||
This comment has been minimized.
This comment has been minimized.
bnoordhuis
May 23, 2018
Member
Can you CHECK_EQ(0, uv_timer_init(...));?
(Not that it can actually fail but that's why it should be CHECK'd.)
|
|
||
| Local<Function> cb = env->timers_callback_function(); | ||
| MaybeLocal<Value> ret; | ||
| Local<Value> args[] = { env->GetNow() }; |
This comment has been minimized.
This comment has been minimized.
bnoordhuis
May 23, 2018
Member
If you're just passing a single argument you can do Local<Value> arg = env->GetNow() and then call into JS with cb->Call(..., 1, &arg).
Not that this is wrong but it's a bit more obvious at the call site that you're just passing one arg.
| TryCatch try_catch(env->isolate()); | ||
| try_catch.SetVerbose(true); | ||
| ret = cb->Call(env->context(), process, arraysize(args), args); | ||
| } while (ret.IsEmpty()); |
This comment has been minimized.
This comment has been minimized.
bnoordhuis
May 23, 2018
Member
I realize you brought this over from timer_wrap.cc but is there a reason you can't handle exceptions in JS land?
This comment has been minimized.
This comment has been minimized.
apapirovski
May 23, 2018
Author
Member
We need to let it go to uncaughtException or domain first. I don't think there's a way to do so in JS?
|
|
||
| if (expiry_ms != 0) { | ||
| int64_t duration_ms = | ||
| abs(expiry_ms) - (uv_now(env->event_loop()) - env->timer_base()); |
This comment has been minimized.
This comment has been minimized.
bnoordhuis
May 23, 2018
Member
Either check that expiry_ms fits in an int or use llabs() (or manually turn it positive.)
This comment has been minimized.
This comment has been minimized.
| } | ||
| } else if (has_ref) { | ||
| uv_unref(h); | ||
| } |
This comment has been minimized.
This comment has been minimized.
bnoordhuis
May 23, 2018
Member
uv_ref() and uv_unref() are idempotent, you don't need to check if the timer is ref'd or not.
| #include <stdint.h> | ||
|
|
||
| namespace node { | ||
| namespace timers { |
This comment has been minimized.
This comment has been minimized.
| @@ -52,7 +52,7 @@ async function test() { | |||
| 'node.perf.timerify', 'v8'], | |||
| categories); | |||
|
|
|||
| const traceConfig = { includedCategories: ['node'] }; | |||
| const traceConfig = { includedCategories: ['v8'] }; | |||
This comment has been minimized.
This comment has been minimized.
bnoordhuis
May 23, 2018
Member
It's because timer_wrap.cc uses AsyncWrap::MakeCallback(), which calls AsyncWrap::EmitTraceEventAfter(). Since you no longer use that method, it's not traced any more.
I personally don't feel too strongly but I guess it would be nice to keep the current behavior.
| const N = 30; | ||
|
|
||
| let last_i = 0; | ||
| let last_ts = 0; | ||
| const [seconds, nanoseconds] = process.hrtime(); | ||
| let last_ts = (seconds * 1e3 + nanoseconds * 1e-6) | 0; |
This comment has been minimized.
This comment has been minimized.
bnoordhuis
May 23, 2018
Member
Not sound. process.hrtime() returns the number of nanoseconds since an unspecified point in the past. If that point is the UNIX epoch then this stops working sometime next year because seconds * 1e3 >= 2**32.
This comment has been minimized.
This comment has been minimized.
apapirovski
May 23, 2018
Author
Member
Ok, sounds like it's best to expose getLibuvNow() from internal/timers and just use that then.
This comment has been minimized.
This comment has been minimized.
|
CI after changes: https://ci.nodejs.org/job/node-test-pull-request/15053/ |
cjihrig
approved these changes
May 23, 2018
jasnell
approved these changes
May 23, 2018
|
LGTM! Thank you for the investigation on the semveriness :-) |
bnoordhuis
approved these changes
May 23, 2018
BridgeAR
approved these changes
May 24, 2018
BridgeAR
added
the
author ready
label
May 24, 2018
This comment has been minimized.
This comment has been minimized.
|
Just an FYI, while this is marked as "author ready" and could land, I'm hoping to have @Fishrock123 review it. Please don't land until they either review or say they won't have time to do so. |
This comment has been minimized.
This comment has been minimized.
|
I’m on Vacation. It’l have to wait until during or after JSConf.EU. |
apapirovski commentedMay 22, 2018
Remove TimerWrap in favour of
uv_timer_thandle stored on the Environment, more similar to howImmediatescurrently function. Since there's now only a single TimerWrap, it doesn't really make much sense for it to exist at all (hence this PR). In addition, the async stack from the TimerWrap is actually not desirable and every single public module that works with async hooks seems to filter it out anyway.I've also done a quick scan of npm modules and I could not find a single use case of
TimerWrap.now(), which would be the only reason to keep it publicly available. It's also currently impossible for user land to use theTimerWrapgiven howOnTimeoutis implemented (it can only process our own internal lists).This will make it possible to continue further improving our Timers architecture and C++ code.
Fixes: #10154
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes