Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: remove TimerWrap #20894

Closed
Closed

Conversation

@apapirovski
Copy link
Member

apapirovski commented May 22, 2018

Remove TimerWrap in favour of uv_timer_t handle stored on the Environment, more similar to how Immediates currently 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 the TimerWrap given how OnTimeout is 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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@apapirovski apapirovski force-pushed the apapirovski:patch-timerwrap-refactor branch from 28a7570 to 30c54f9 May 22, 2018
Copy link
Member

addaleax left a comment

Heads up, this is going to have a minor conflict with #20884 :)

src/env.cc Outdated
@@ -450,6 +459,59 @@ void Environment::RunAndClearNativeImmediates() {
}


void Environment::ScheduleTimer(int64_t duration) {

This comment has been minimized.

Copy link
@addaleax

addaleax May 22, 2018

Member

Can we add the unit of measurement to duration? E.g. duration_ms if it’s milliseconds?

src/env.cc Outdated
do {
TryCatch try_catch(env->isolate());
try_catch.SetVerbose(true);
ret = cb->Call(env->context(), process, 1, args);

This comment has been minimized.

Copy link
@addaleax

addaleax May 22, 2018

Member

tiny style nit: arraysize(args)

src/env.cc Outdated

if (expiry != 0) {
int64_t duration =
abs(expiry) - (uv_now(env->event_loop()) - env->timer_base());

This comment has been minimized.

Copy link
@addaleax

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.

Copy link
@addaleax

addaleax May 22, 2018

Member

Is there a reason for not providing these directly on the binding?

This comment has been minimized.

Copy link
@apapirovski

apapirovski May 22, 2018

Author Member

I wondered about that. I don't think so?

@apapirovski

This comment has been minimized.

Copy link
Member Author

apapirovski commented May 22, 2018

Thanks @addaleax. I've cleaned up and added some detailed comments. Hope this is better :)

@apapirovski apapirovski force-pushed the apapirovski:patch-timerwrap-refactor branch 2 times, most recently from a0b2f82 to dc80758 May 22, 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.

Copy link
@jasnell

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.

Copy link
@apapirovski

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

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented May 23, 2018

Does this need a proper deprecation cycle? /cc @nodejs/tsc

@apapirovski

This comment has been minimized.

Copy link
Member Author

apapirovski commented May 23, 2018

@apapirovski apapirovski force-pushed the apapirovski:patch-timerwrap-refactor branch from dc80758 to 15b2ddd 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.

Copy link
@apapirovski

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.

Copy link
@apapirovski

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.

Copy link
@bnoordhuis

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.

Copy link
@apapirovski

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.

Copy link
@bnoordhuis

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.

Copy link
@ofrobots

ofrobots May 24, 2018

Contributor

LGTM on the test change. I haven't reviewed the rest of the PR.

@apapirovski apapirovski force-pushed the apapirovski:patch-timerwrap-refactor branch 2 times, most recently from 5ab8e99 to 2b7ae97 May 23, 2018
Copy link
Member

bnoordhuis left a comment

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.

Copy link
@bnoordhuis

bnoordhuis May 23, 2018

Member

I suspect you intended to write 'assign.' :-)

listOnTimeout(list, now);
ran = true;

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis May 23, 2018

Member

This is a functionally null change, right?

This comment has been minimized.

Copy link
@apapirovski

apapirovski May 23, 2018

Author Member

Yeah, it's just cleaner this way since we don't assign each time.

src/env.cc Outdated
@@ -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.

Copy link
@bnoordhuis

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

src/env.cc Outdated

Local<Function> cb = env->timers_callback_function();
MaybeLocal<Value> ret;
Local<Value> args[] = { env->GetNow() };

This comment has been minimized.

Copy link
@bnoordhuis

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.

src/env.cc Outdated
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.

Copy link
@bnoordhuis

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.

Copy link
@apapirovski

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?

src/env.cc Outdated

if (expiry_ms != 0) {
int64_t duration_ms =
abs(expiry_ms) - (uv_now(env->event_loop()) - env->timer_base());

This comment has been minimized.

Copy link
@bnoordhuis

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.

Copy link
@apapirovski

apapirovski May 23, 2018

Author Member

Ah, yeah... didn't even realize!

src/env.cc Outdated
}
} else if (has_ref) {
uv_unref(h);
}

This comment has been minimized.

Copy link
@bnoordhuis

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.

Copy link
@bnoordhuis

bnoordhuis May 23, 2018

Member

You could also use an anonymous namespace here.

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

Copy link
@bnoordhuis

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.

Copy link
@bnoordhuis

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.

Copy link
@apapirovski

apapirovski May 23, 2018

Author Member

Ok, sounds like it's best to expose getLibuvNow() from internal/timers and just use that then.

@apapirovski

This comment has been minimized.

Copy link
Member Author

apapirovski commented May 23, 2018

Copy link
Member

jasnell left a comment

LGTM! Thank you for the investigation on the semveriness :-)

@apapirovski

This comment has been minimized.

Copy link
Member Author

apapirovski commented May 26, 2018

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.

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented May 26, 2018

I’m on Vacation. It’l have to wait until during or after JSConf.EU.

Copy link
Member

mcollina left a comment

LGTM

maclover7 added a commit to maclover7/node that referenced this pull request Jul 14, 2018
Unused since the excellent refactoring in 2930bd1,
which also removed `src/timer_wrap.cc` from `node.gyp`. If you try and
get at the binding on a v11.0-pre build, you'll get an error, since the
file is no longer in the GYP build.

```
Jonathans-MBP:node jon$ ./node -v
v11.0.0-pre
Jonathans-MBP:node jon$ ./node -e "process.binding('timer_wrap')"
internal/bootstrap/loaders.js:81
        mod = bindingObj[module] = getBinding(module);
                                   ^

Error: No such module: timer_wrap
    at process.binding (internal/bootstrap/loaders.js:81:36)
    at [eval]:1:9
    at Script.runInThisContext (vm.js:89:20)
    at Object.runInThisContext (vm.js:286:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at evalScript (internal/bootstrap/node.js:562:27)
    at startup (internal/bootstrap/node.js:248:9)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:595:3)
```

Refs: nodejs#20894
maclover7 added a commit that referenced this pull request Jul 15, 2018
Unused since the excellent refactoring in 2930bd,
which also removed `src/timer_wrap.cc` from `node.gyp`. If you try and
get at the binding on a v11.0-pre build, you'll get an error, since the
file is no longer in the GYP build.

```
Jonathans-MBP:node jon$ ./node -v
v11.0.0-pre
Jonathans-MBP:node jon$ ./node -e "process.binding('timer_wrap')"
internal/bootstrap/loaders.js:81
        mod = bindingObj[module] = getBinding(module);
                                   ^

Error: No such module: timer_wrap
    at process.binding (internal/bootstrap/loaders.js:81:36)
    at [eval]:1:9
    at Script.runInThisContext (vm.js:89:20)
    at Object.runInThisContext (vm.js:286:38)
    at Object.<anonymous> ([eval]-wrapper:6:22)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at evalScript (internal/bootstrap/node.js:562:27)
    at startup (internal/bootstrap/node.js:248:9)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:595:3)
```

PR-URL: #21777
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Refs: #20894
targos added a commit to targos/node that referenced this pull request Jul 31, 2018
The timers directory test, utilizing FakeTime, has not worked in
quite a while and is not truly testing Node.js behaviour. If a
similar test is necessary it would be better suited to libuv
on which Node.js relies for timers functionality.

PR-URL: nodejs#20894
Fixes: nodejs#10154
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
targos added a commit that referenced this pull request Aug 2, 2018
The timers directory test, utilizing FakeTime, has not worked in
quite a while and is not truly testing Node.js behaviour. If a
similar test is necessary it would be better suited to libuv
on which Node.js relies for timers functionality.

PR-URL: #20894
Fixes: #10154
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>

Backport-PR-URL: #22039
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
rvagg added a commit to rvagg/io.js that referenced this pull request Aug 13, 2018
nodejs#20894 / 2930bd1 was introduced on
master which removed an offending line in this doc before linting was
applied to test/ in nodejs#22221 /
56103ab. Since 20894 is semver-major, the full changes were not
backported.
rvagg added a commit that referenced this pull request Aug 13, 2018
#20894 / 2930bd1 was introduced on
master which removed an offending line in this doc before linting was
applied to test/ in #22221 /
56103ab. Since 20894 is semver-major, the full changes were not
backported.

PR-URL: #22296
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
rvagg added a commit that referenced this pull request Aug 15, 2018
#20894 / 2930bd1 was introduced on
master which removed an offending line in this doc before linting was
applied to test/ in #22221 /
56103ab. Since 20894 is semver-major, the full changes were not
backported.

PR-URL: #22296
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Flarna added a commit to Flarna/node that referenced this pull request Nov 2, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in nodejs#20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

Ref: nodejs#20894
@Flarna Flarna mentioned this pull request Nov 2, 2018
2 of 2 tasks complete
vsemozhetbyt added a commit that referenced this pull request Nov 11, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in #20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

PR-URL: #24050
Refs: #20894
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added a commit that referenced this pull request Nov 14, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in #20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

PR-URL: #24050
Refs: #20894
Reviewed-By: James M Snell <jasnell@gmail.com>
kiyomizumia added a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in nodejs#20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

PR-URL: nodejs#24050
Refs: nodejs#20894
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere added a commit that referenced this pull request Dec 14, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in #20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

PR-URL: #24050
Refs: #20894
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Dec 26, 2018
Correct the output of async_hooks samples
* `TIMERWRAP` has been removed in #20894
* `console.log()` doesn't issue `TTYWRAP` nor `SIGNALWRAP`

I don't know which PR caused that `console.log()` is no longer using
`TTYWRAP` and `SIGNALWRAP`; I think it was between 8.4.0 and 8.5.0.

PR-URL: #24050
Refs: #20894
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.