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.

Sign up for GitHub

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

timers: cross JS/C++ border less frequently for setImmediate() #17064

Closed
wants to merge 1 commit into from
Closed

timers: cross JS/C++ border less frequently for setImmediate() #17064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 24 additions & 22 deletions lib/timers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ const { kInit, kDestroy, kAsyncIdCounter } = async_wrap.constants;
const async_id_symbol = Symbol('asyncId');
const trigger_async_id_symbol = Symbol('triggerAsyncId');

/* This is an Uint32Array for easier sharing with C++ land. */
const scheduledImmediateCount = process._scheduledImmediateCount;
delete process._scheduledImmediateCount;
/* Kick off setImmediate processing */
const activateImmediateCheck = process._activateImmediateCheck;
delete process._activateImmediateCheck;

// Timeout values > TIMEOUT_MAX are set to 1.
const TIMEOUT_MAX = 2147483647; // 2^31-1

Expand Down Expand Up @@ -742,15 +749,9 @@ function processImmediate() {
else
immediate = next;
}

// Only round-trip to C++ land if we have to. Calling clearImmediate() on an
// immediate that's in |queue| is okay. Worst case is we make a superfluous
// call to NeedImmediateCallbackSetter().
if (!immediateQueue.head) {
process._needImmediateCallback = false;
}
}

process._immediateCallback = processImmediate;

// An optimization so that the try/finally only de-optimizes (since at least v8
// 4.7) what is in this smaller function.
Expand All @@ -762,13 +763,17 @@ function tryOnImmediate(immediate, oldTail) {
runCallback(immediate);
threw = false;
} finally {
// clearImmediate checks _onImmediate === null for kDestroy hooks.
immediate._onImmediate = null;
if (!threw)
emitAfter(immediate[async_id_symbol]);
if (async_hook_fields[kDestroy] > 0 && !immediate._destroyed) {
emitDestroy(immediate[async_id_symbol]);

if (!immediate._destroyed) {
immediate._destroyed = true;
scheduledImmediateCount[0]--;

if (async_hook_fields[kDestroy] > 0) {
emitDestroy(immediate[async_id_symbol]);
}
}

if (threw && immediate._idleNext) {
Expand Down Expand Up @@ -870,10 +875,9 @@ function createImmediate(args, callback) {
immediate._argv = args;
immediate._onImmediate = callback;

if (!process._needImmediateCallback) {
process._needImmediateCallback = true;
process._immediateCallback = processImmediate;
}
if (scheduledImmediateCount[0] === 0)
activateImmediateCheck();
scheduledImmediateCount[0]++;

immediateQueue.append(immediate);

Expand All @@ -884,18 +888,16 @@ function createImmediate(args, callback) {
exports.clearImmediate = function(immediate) {
if (!immediate) return;

if (async_hook_fields[kDestroy] > 0 &&
immediate._onImmediate !== null &&
!immediate._destroyed) {
emitDestroy(immediate[async_id_symbol]);
if (!immediate._destroyed) {
scheduledImmediateCount[0]--;
immediate._destroyed = true;

if (async_hook_fields[kDestroy] > 0) {
emitDestroy(immediate[async_id_symbol]);
}
}

immediate._onImmediate = null;

immediateQueue.remove(immediate);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to move this into the if (or even flip the if to if (immediate._destroyed) return;)

Copy link
Member Author

Choose a reason for hiding this comment

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

You can try it but all of this code is so fragile I’d rather not touch more than necessary


if (!immediateQueue.head) {
process._needImmediateCallback = false;
}
};
6 changes: 6 additions & 0 deletions src/env-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ inline Environment::Environment(IsolateData* isolate_data,
abort_on_uncaught_exception_(false),
emit_napi_warning_(true),
makecallback_cntr_(0),
scheduled_immediate_count_(isolate_, 1),
#if HAVE_INSPECTOR
inspector_agent_(new inspector::Agent(this)),
#endif
Expand Down Expand Up @@ -512,6 +513,11 @@ inline void Environment::set_fs_stats_field_array(double* fields) {
fs_stats_field_array_ = fields;
}

inline AliasedBuffer<uint32_t, v8::Uint32Array>&
Environment::scheduled_immediate_count() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really a fan of returning a mutable reference. I realize other AliasedBuffer things do that too but IMO they shouldn't, it obscures whether call sites deal with a copy or a reference.

(I'll allow that it looks like a little nicer coupled with array syntax.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I know. :) I don’t like it when one can’t tell the difference either, but in these cases it should be pretty clear.

return scheduled_immediate_count_;
}

inline performance::performance_state* Environment::performance_state() {
return performance_state_;
}
Expand Down
4 changes: 4 additions & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,8 @@ class Environment {
inline double* fs_stats_field_array() const;
inline void set_fs_stats_field_array(double* fields);

inline AliasedBuffer<uint32_t, v8::Uint32Array>& scheduled_immediate_count();

inline performance::performance_state* performance_state();
inline std::map<std::string, uint64_t>* performance_marks();

Expand Down Expand Up @@ -731,6 +733,8 @@ class Environment {
size_t makecallback_cntr_;
std::vector<double> destroy_async_id_list_;

AliasedBuffer<uint32_t, v8::Uint32Array> scheduled_immediate_count_;

performance::performance_state* performance_state_ = nullptr;
std::map<std::string, uint64_t> performance_marks_;

Expand Down
86 changes: 35 additions & 51 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,25 +400,6 @@ static void PrintErrorString(const char* format, ...) {
va_end(ap);
}


static void CheckImmediate(uv_check_t* handle) {
Environment* env = Environment::from_immediate_check_handle(handle);
HandleScope scope(env->isolate());
Context::Scope context_scope(env->context());
MakeCallback(env->isolate(),
env->process_object(),
env->immediate_callback_string(),
0,
nullptr,
{0, 0}).ToLocalChecked();
}


static void IdleImmediateDummy(uv_idle_t* handle) {
// Do nothing. Only for maintaining event loop.
// TODO(bnoordhuis) Maybe make libuv accept nullptr idle callbacks.
}

const char *signo_string(int signo) {
#define SIGNO_CASE(e) case e: return #e;
switch (signo) {
Expand Down Expand Up @@ -3019,39 +3000,40 @@ static void DebugEnd(const FunctionCallbackInfo<Value>& args);

namespace {

void NeedImmediateCallbackGetter(Local<Name> property,
const PropertyCallbackInfo<Value>& info) {
Environment* env = Environment::GetCurrent(info);
const uv_check_t* immediate_check_handle = env->immediate_check_handle();
bool active = uv_is_active(
reinterpret_cast<const uv_handle_t*>(immediate_check_handle));
info.GetReturnValue().Set(active);
bool MaybeStopImmediate(Environment* env) {
if (env->scheduled_immediate_count()[0] == 0) {
uv_check_stop(env->immediate_check_handle());
uv_idle_stop(env->immediate_idle_handle());
return true;
}
return false;
}

void CheckImmediate(uv_check_t* handle) {
Environment* env = Environment::from_immediate_check_handle(handle);
HandleScope scope(env->isolate());
Context::Scope context_scope(env->context());

void NeedImmediateCallbackSetter(
Local<Name> property,
Local<Value> value,
const PropertyCallbackInfo<void>& info) {
Environment* env = Environment::GetCurrent(info);
if (MaybeStopImmediate(env))
Copy link
Contributor

@refack refack Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What's the case here? A callback from uv after the Immediate was cleared on the JS side?
If so maybe add a comment?

Copy link
Member Author

@addaleax addaleax Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, that is one way to trigger this, but the check here isn’t semantically tied to that – the general idea is that if there’s nothing to run, don’t bother calling into JS, no matter why there is nothing to run.

(In particular, it’s not quite worked out yet how this is going to work with other native code also using this mechanism, as hinted at in the commit message, and it’s quite possible that that may not be the only way to trigger it.)

return;

uv_check_t* immediate_check_handle = env->immediate_check_handle();
bool active = uv_is_active(
reinterpret_cast<const uv_handle_t*>(immediate_check_handle));
MakeCallback(env->isolate(),
env->process_object(),
env->immediate_callback_string(),
0,
nullptr,
{0, 0}).ToLocalChecked();

if (active == value->BooleanValue())
return;
MaybeStopImmediate(env);
}

uv_idle_t* immediate_idle_handle = env->immediate_idle_handle();

if (active) {
uv_check_stop(immediate_check_handle);
uv_idle_stop(immediate_idle_handle);
} else {
uv_check_start(immediate_check_handle, CheckImmediate);
// Idle handle is needed only to stop the event loop from blocking in poll.
uv_idle_start(immediate_idle_handle, IdleImmediateDummy);
}
void ActivateImmediateCheck(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
uv_check_start(env->immediate_check_handle(), CheckImmediate);
// Idle handle is needed only to stop the event loop from blocking in poll.
uv_idle_start(env->immediate_idle_handle(),
[](uv_idle_t*){ /* do nothing, just keep the loop running */ });
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the comment came from the previous impl, but it seems redundant with the comment above the line...

Copy link
Member Author

@addaleax addaleax Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe. One comment is describing the what, one comment is describing the why, so I’m okay with having two.

Edit: On second thought, yes, I guess this can be removed if you like

}


Expand Down Expand Up @@ -3277,12 +3259,11 @@ void SetupProcessObject(Environment* env,
FIXED_ONE_BYTE_STRING(env->isolate(), "ppid"),
GetParentProcessId).FromJust());

auto need_immediate_callback_string =
FIXED_ONE_BYTE_STRING(env->isolate(), "_needImmediateCallback");
CHECK(process->SetAccessor(env->context(), need_immediate_callback_string,
NeedImmediateCallbackGetter,
NeedImmediateCallbackSetter,
env->as_external()).FromJust());
auto scheduled_immediate_count =
FIXED_ONE_BYTE_STRING(env->isolate(), "_scheduledImmediateCount");
CHECK(process->Set(env->context(),
scheduled_immediate_count,
env->scheduled_immediate_count().GetJSArray()).FromJust());

// -e, --eval
if (eval_string) {
Expand Down Expand Up @@ -3408,6 +3389,9 @@ void SetupProcessObject(Environment* env,
env->as_external()).FromJust());

// define various internal methods
env->SetMethod(process,
"_activateImmediateCheck",
ActivateImmediateCheck);
env->SetMethod(process,
"_startProfilerIdleNotifier",
StartProfilerIdleNotifier);
Expand Down