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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@ deps/npm/node_modules/.bin/
/SHASUMS*.txt*

# test artifacts
tools/faketime
tools/remark-cli/node_modules
tools/remark-preset-lint-node/node_modules
icu_config.gypi
Expand Down
7 changes: 0 additions & 7 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -537,13 +537,6 @@ test-addons-clean:
$(RM) test/addons/.buildstamp test/addons/.docbuildstamp
$(MAKE) test-addons-napi-clean

test-timers:
$(MAKE) --directory=tools faketime
$(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) timers

test-timers-clean:
$(MAKE) --directory=tools clean

test-async-hooks:
$(PYTHON) tools/test.py $(PARALLEL_ARGS) --mode=$(BUILDTYPE_LOWER) async-hooks

Expand Down
2 changes: 1 addition & 1 deletion doc/api/async_hooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ resource's constructor.
```text
FSEVENTWRAP, FSREQWRAP, GETADDRINFOREQWRAP, GETNAMEINFOREQWRAP, HTTPPARSER,
JSSTREAM, PIPECONNECTWRAP, PIPEWRAP, PROCESSWRAP, QUERYWRAP, SHUTDOWNWRAP,
SIGNALWRAP, STATWATCHER, TCPCONNECTWRAP, TCPSERVER, TCPWRAP, TIMERWRAP, TTYWRAP,
SIGNALWRAP, STATWATCHER, TCPCONNECTWRAP, TCPSERVER, TCPWRAP, TTYWRAP,
UDPSENDWRAP, UDPWRAP, WRITEWRAP, ZLIB, SSLCONNECTION, PBKDF2REQUEST,
RANDOMBYTESREQUEST, TLSWRAP, Timeout, Immediate, TickObject
```
Expand Down
4 changes: 3 additions & 1 deletion lib/internal/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const {
initHooksExist,
emitInit
} = require('internal/async_hooks');
const { internalBinding } = require('internal/bootstrap/loaders');
// Symbols for storing async id state.
const async_id_symbol = Symbol('asyncId');
const trigger_async_id_symbol = Symbol('triggerId');
Expand All @@ -30,7 +31,8 @@ module.exports = {
kRefed,
initAsyncResource,
setUnrefTimeout,
validateTimerDuration
validateTimerDuration,
getLibuvNow: internalBinding('timers').getLibuvNow,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getMonotonicNow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if you still want to rename. I prefer this for now, at least until we decide to change this to use a more precise (unrounded) timer to compare. I think it's somewhat important to communicate that this relates to the internal libuv loop time.

};

var timers;
Expand Down
64 changes: 25 additions & 39 deletions lib/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,15 @@

'use strict';

const { internalBinding } = require('internal/bootstrap/loaders');
const {
Timer: TimerWrap,
getLibuvNow,
setupTimers,
} = process.binding('timer_wrap');
scheduleTimer,
toggleTimerRef,
immediateInfo,
toggleImmediateRef
} = internalBinding('timers');
const L = require('internal/linkedlist');
const PriorityQueue = require('internal/priority_queue');
const {
Expand Down Expand Up @@ -53,8 +58,9 @@ const kCount = 0;
const kRefCount = 1;
const kHasOutstanding = 2;

const [immediateInfo, toggleImmediateRef] =
setupTimers(processImmediate, processTimers);
// Call into C++ to assign callbacks that are responsible for processing
// Immediates and TimerLists.
setupTimers(processImmediate, processTimers);

// HOW and WHY the timers implementation works the way it does.
//
Expand Down Expand Up @@ -156,47 +162,38 @@ function setPosition(node, pos) {
node.priorityQueuePosition = pos;
}

let handle = null;
let nextExpiry = Infinity;

let timerListId = Number.MIN_SAFE_INTEGER;
let refCount = 0;

function incRefCount() {
if (refCount++ === 0)
handle.ref();
toggleTimerRef(true);
}

function decRefCount() {
if (--refCount === 0)
handle.unref();
}

function createHandle(refed) {
debug('initial run, creating TimerWrap handle');
handle = new TimerWrap();
if (!refed)
handle.unref();
toggleTimerRef(false);
}

// Schedule or re-schedule a timer.
// The item must have been enroll()'d first.
const active = exports.active = function(item) {
insert(item, true, TimerWrap.now());
insert(item, true, getLibuvNow());
};

// Internal APIs that need timeouts should use `_unrefActive()` instead of
// `active()` so that they do not unnecessarily keep the process open.
exports._unrefActive = function(item) {
insert(item, false, TimerWrap.now());
insert(item, false, getLibuvNow());
};


// The underlying logic for scheduling or re-scheduling a timer.
//
// Appends a timer onto the end of an existing timers list, or creates a new
// TimerWrap backed list if one does not already exist for the specified timeout
// duration.
// list if one does not already exist for the specified timeout duration.
function insert(item, refed, start) {
const msecs = item._idleTimeout;
if (msecs < 0 || msecs === undefined)
Expand All @@ -213,9 +210,7 @@ function insert(item, refed, start) {
queue.insert(list);

if (nextExpiry > expiry) {
if (handle === null)
createHandle(refed);
handle.start(msecs);
scheduleTimer(msecs);
nextExpiry = expiry;
}
}
Expand Down Expand Up @@ -252,32 +247,23 @@ function processTimers(now) {

let list, ran;
while (list = queue.peek()) {
if (list.expiry > now)
break;
if (list.expiry > now) {
nextExpiry = list.expiry;
return refCount > 0 ? nextExpiry : -nextExpiry;
}
if (ran)
runNextTicks();
else
ran = true;
listOnTimeout(list, now);
ran = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a functionally null change, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if (refCount > 0)
handle.ref();
else
handle.unref();

if (list !== undefined) {
nextExpiry = list.expiry;
handle.start(Math.max(nextExpiry - TimerWrap.now(), 1));
}

return true;
return 0;
}

function listOnTimeout(list, now) {
const msecs = list.msecs;

debug('timeout callback %d', msecs);
debug('now: %d', now);

var diff, timer;
while (timer = L.peek(list)) {
Expand Down Expand Up @@ -336,7 +322,7 @@ function listOnTimeout(list, now) {
// 4.7) what is in this smaller function.
function tryOnTimeout(timer, start) {
if (start === undefined && timer._repeat)
start = TimerWrap.now();
start = getLibuvNow();
try {
ontimeout(timer);
} finally {
Expand Down Expand Up @@ -474,7 +460,7 @@ function ontimeout(timer) {
}

function rearm(timer, start) {
// // Do not re-arm unenroll'd or closed timers.
// Do not re-arm unenroll'd or closed timers.
if (timer._idleTimeout === -1)
return;

Expand Down
2 changes: 1 addition & 1 deletion node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@
'src/stream_pipe.cc',
'src/stream_wrap.cc',
'src/tcp_wrap.cc',
'src/timer_wrap.cc',
'src/timers.cc',
'src/tracing/agent.cc',
'src/tracing/node_trace_buffer.cc',
'src/tracing/node_trace_writer.cc',
Expand Down
1 change: 0 additions & 1 deletion src/async_wrap.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ namespace node {
V(TCPCONNECTWRAP) \
V(TCPSERVERWRAP) \
V(TCPWRAP) \
V(TIMERWRAP) \
V(TTYWRAP) \
V(UDPSENDWRAP) \
V(UDPWRAP) \
Expand Down
5 changes: 0 additions & 5 deletions src/callback_scope.cc
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,6 @@ InternalCallbackScope::InternalCallbackScope(Environment* env,
AsyncWrap::EmitBefore(env, asyncContext.async_id);
}

if (!IsInnerMakeCallback()) {
env->tick_info()->set_has_thrown(false);
}

env->async_hooks()->push_async_ids(async_context_.async_id,
async_context_.trigger_async_id);
pushed_ids_ = true;
Expand Down Expand Up @@ -120,7 +116,6 @@ void InternalCallbackScope::Close() {
if (!env_->can_call_into_js()) return;

if (env_->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
env_->tick_info()->set_has_thrown(true);
failed_ = true;
}
}
Expand Down
16 changes: 8 additions & 8 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -269,10 +269,6 @@ inline bool Environment::TickInfo::has_scheduled() const {
return fields_[kHasScheduled] == 1;
}

inline bool Environment::TickInfo::has_thrown() const {
return fields_[kHasThrown] == 1;
}

inline bool Environment::TickInfo::has_promise_rejections() const {
return fields_[kHasPromiseRejections] == 1;
}
Expand All @@ -281,10 +277,6 @@ inline void Environment::TickInfo::promise_rejections_toggle_on() {
fields_[kHasPromiseRejections] = 1;
}

inline void Environment::TickInfo::set_has_thrown(bool state) {
fields_[kHasThrown] = state ? 1 : 0;
}

inline void Environment::AssignToContext(v8::Local<v8::Context> context,
const ContextInfo& info) {
context->SetAlignedPointerInEmbedderData(
Expand Down Expand Up @@ -334,6 +326,14 @@ inline tracing::Agent* Environment::tracing_agent() const {
return tracing_agent_;
}

inline Environment* Environment::from_timer_handle(uv_timer_t* handle) {
return ContainerOf(&Environment::timer_handle_, handle);
}

inline uv_timer_t* Environment::timer_handle() {
return &timer_handle_;
}

inline Environment* Environment::from_immediate_check_handle(
uv_check_t* handle) {
return ContainerOf(&Environment::immediate_check_handle_, handle);
Expand Down
81 changes: 81 additions & 0 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
namespace node {

using v8::Context;
using v8::Function;
using v8::FunctionTemplate;
using v8::HandleScope;
using v8::Integer;
Expand All @@ -25,6 +26,7 @@ using v8::StackFrame;
using v8::StackTrace;
using v8::String;
using v8::Symbol;
using v8::TryCatch;
using v8::Value;
using worker::Worker;

Expand Down Expand Up @@ -172,6 +174,9 @@ void Environment::Start(int argc,
HandleScope handle_scope(isolate());
Context::Scope context_scope(context());

CHECK_EQ(0, uv_timer_init(event_loop(), timer_handle()));
uv_unref(reinterpret_cast<uv_handle_t*>(timer_handle()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we shouldn't run a bit more of seemingly unrelated benchmarks, as we saw yesterday (at the collab summit impromptu core debug session), uv does run it's get clocktime MONOTONIC (or whatever exactly it is) quite a lot when a a timer handle exists.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully addressed in our chat but let me know if there's still something actionable?


uv_check_init(event_loop(), immediate_check_handle());
uv_unref(reinterpret_cast<uv_handle_t*>(immediate_check_handle()));

Expand Down Expand Up @@ -226,6 +231,10 @@ void Environment::RegisterHandleCleanups() {
env->CloseHandle(handle, [](uv_handle_t* handle) {});
};

RegisterHandleCleanup(
reinterpret_cast<uv_handle_t*>(timer_handle()),
close_and_finish,
nullptr);
RegisterHandleCleanup(
reinterpret_cast<uv_handle_t*>(immediate_check_handle()),
close_and_finish,
Expand Down Expand Up @@ -469,6 +478,78 @@ void Environment::RunAndClearNativeImmediates() {
}


void Environment::ScheduleTimer(int64_t duration_ms) {
uv_timer_start(timer_handle(), RunTimers, duration_ms, 0);
}

void Environment::ToggleTimerRef(bool ref) {
if (ref) {
uv_ref(reinterpret_cast<uv_handle_t*>(timer_handle()));
} else {
uv_unref(reinterpret_cast<uv_handle_t*>(timer_handle()));
}
}

void Environment::RunTimers(uv_timer_t* handle) {
Environment* env = Environment::from_timer_handle(handle);

if (!env->can_call_into_js())
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't we need to re-schedule the uv_timer if this happens? On like, isn't this handled from callback scope or something else?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timers won't ever run again if this happens. This happens if we're shutting down the process or a worker.


HandleScope handle_scope(env->isolate());
Context::Scope context_scope(env->context());

Local<Object> process = env->process_object();
InternalCallbackScope scope(env, process, {0, 0});

Local<Function> cb = env->timers_callback_function();
MaybeLocal<Value> ret;
Local<Value> arg = env->GetNow();
// This code will loop until all currently due timers will process. It is
// impossible for us to end up in an infinite loop due to how the JS-side
// is structured.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is, but I'll need to double check if there is a test for this

Copy link
Member Author

@apapirovski apapirovski Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any newly scheduled timer will run on next tick of the event loop (or later) so eventually once all currently due timers clear, it'll just continue on its journey. There are only so many user callbacks that can be called during this run of the event loop and once they all clear, even if every single one of them fails, we'll break out of the loop.

(FWIW this looping part is almost exactly the same as the current code on 9.x and 10.x so I think we're pretty safe.)

do {
TryCatch try_catch(env->isolate());
try_catch.SetVerbose(true);
ret = cb->Call(env->context(), process, 1, &arg);
} while (ret.IsEmpty() && env->can_call_into_js());

// NOTE(apapirovski): If it ever becomes possibble that `call_into_js` above
// is reset back to `true` after being previously set to `false` then this
// code becomes invalid and needs to be rewritten. Otherwise catastrophic
// timers corruption will occurr and all timers behaviour will become
// entirely unpredictable.
if (ret.IsEmpty())
return;

// To allow for less JS-C++ boundary crossing, the value returned from JS
// serves a few purposes:
// 1. If it's 0, no more timers exist and the handle should be unrefed
// 2. If it's > 0, the value represents the next timer's expiry and there
// is at least one timer remaining that is refed.
// 3. If it's < 0, the absolute value represents the next timer's expiry
// and there are no timers that are refed.
int64_t expiry_ms =
ret.ToLocalChecked()->IntegerValue(env->context()).FromJust();

uv_handle_t* h = reinterpret_cast<uv_handle_t*>(handle);

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

env->ScheduleTimer(duration_ms > 0 ? duration_ms : 1);

if (expiry_ms > 0)
uv_ref(h);
else
uv_unref(h);
} else {
uv_unref(h);
}
}


void Environment::CheckImmediate(uv_check_t* handle) {
Environment* env = Environment::from_immediate_check_handle(handle);

Expand Down
Loading