From a1da024cab797fd9318e7b754512887a71b1b242 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Tue, 9 Dec 2014 05:24:59 +0100 Subject: [PATCH] node, async-wrap: remove MakeDomainCallback C++ won't deoptimize like JS if specific conditional branches are sporadically met in the future. Combined with the amount of code duplication removal and simplified maintenance complexity, it makes more sense to merge MakeCallback and MakeDomainCallback. Additionally, type casting in V8 before verifying what that type is will cause V8 to abort in debug mode if that type isn't what was expected. Fix this by first checking the v8::Value before casting. PR-URL: https://github.com/joyent/node/pull/8110 Signed-off-by: Trevor Norris Reviewed-by: Fedor Indutny Reviewed-by: Alexis Campailla Reviewed-by: Julien Gilli --- src/async-wrap-inl.h | 12 ++-- src/async-wrap.cc | 108 +++++++++---------------------- src/async-wrap.h | 7 -- src/node.cc | 151 ++++++++++++------------------------------- 4 files changed, 74 insertions(+), 204 deletions(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index 38a5c7fd237..7a298a1497b 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -51,10 +51,8 @@ inline v8::Handle AsyncWrap::MakeCallback( int argc, v8::Handle* argv) { v8::Local cb_v = object()->Get(symbol); - v8::Local cb = cb_v.As(); - CHECK(cb->IsFunction()); - - return MakeCallback(cb, argc, argv); + CHECK(cb_v->IsFunction()); + return MakeCallback(cb_v.As(), argc, argv); } @@ -63,10 +61,8 @@ inline v8::Handle AsyncWrap::MakeCallback( int argc, v8::Handle* argv) { v8::Local cb_v = object()->Get(index); - v8::Local cb = cb_v.As(); - CHECK(cb->IsFunction()); - - return MakeCallback(cb, argc, argv); + CHECK(cb_v->IsFunction()); + return MakeCallback(cb_v.As(), argc, argv); } } // namespace node diff --git a/src/async-wrap.cc b/src/async-wrap.cc index 0bf52b0da30..4f742ce21eb 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -37,101 +37,53 @@ using v8::Value; namespace node { -Handle AsyncWrap::MakeDomainCallback(const Handle cb, - int argc, - Handle* argv) { - CHECK(env()->context() == env()->isolate()->GetCurrentContext()); - - Local context = object(); - Local process = env()->process_object(); - Local domain_v = context->Get(env()->domain_string()); - Local domain; - - TryCatch try_catch; - try_catch.SetVerbose(true); - - bool has_domain = domain_v->IsObject(); - if (has_domain) { - domain = domain_v.As(); - - if (domain->Get(env()->disposed_string())->IsTrue()) - return Undefined(env()->isolate()); - - Local enter = - domain->Get(env()->enter_string()).As(); - if (enter->IsFunction()) { - enter->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env()->isolate()); - } - } - - Local ret = cb->Call(context, argc, argv); - - if (try_catch.HasCaught()) { - return Undefined(env()->isolate()); - } - - if (has_domain) { - Local exit = - domain->Get(env()->exit_string()).As(); - if (exit->IsFunction()) { - exit->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env()->isolate()); - } - } - - Environment::TickInfo* tick_info = env()->tick_info(); - - if (tick_info->in_tick()) { - return ret; - } - - if (tick_info->length() == 0) { - env()->isolate()->RunMicrotasks(); - } - - if (tick_info->length() == 0) { - tick_info->set_index(0); - return ret; - } - - tick_info->set_in_tick(true); - - env()->tick_callback_function()->Call(process, 0, nullptr); - - tick_info->set_in_tick(false); - - if (try_catch.HasCaught()) { - tick_info->set_last_threw(true); - return Undefined(env()->isolate()); - } - - return ret; -} - - Handle AsyncWrap::MakeCallback(const Handle cb, int argc, Handle* argv) { - if (env()->using_domains()) - return MakeDomainCallback(cb, argc, argv); - CHECK(env()->context() == env()->isolate()->GetCurrentContext()); Local context = object(); Local process = env()->process_object(); + Local domain; + bool has_domain = false; + + if (env()->using_domains()) { + Local domain_v = context->Get(env()->domain_string()); + has_domain = domain_v->IsObject(); + if (has_domain) { + domain = domain_v.As(); + if (domain->Get(env()->disposed_string())->IsTrue()) + return Undefined(env()->isolate()); + } + } TryCatch try_catch; try_catch.SetVerbose(true); + if (has_domain) { + Local enter_v = domain->Get(env()->enter_string()); + if (enter_v->IsFunction()) { + enter_v.As()->Call(domain, 0, nullptr); + if (try_catch.HasCaught()) + return Undefined(env()->isolate()); + } + } + Local ret = cb->Call(context, argc, argv); if (try_catch.HasCaught()) { return Undefined(env()->isolate()); } + if (has_domain) { + Local exit_v = domain->Get(env()->exit_string()); + if (exit_v->IsFunction()) { + exit_v.As()->Call(domain, 0, nullptr); + if (try_catch.HasCaught()) + return Undefined(env()->isolate()); + } + } + Environment::TickInfo* tick_info = env()->tick_info(); if (tick_info->in_tick()) { diff --git a/src/async-wrap.h b/src/async-wrap.h index 9393c4c3457..30e49946e7f 100644 --- a/src/async-wrap.h +++ b/src/async-wrap.h @@ -74,13 +74,6 @@ class AsyncWrap : public BaseObject { private: inline AsyncWrap(); - // TODO(trevnorris): BURN IN FIRE! Remove this as soon as a suitable - // replacement is committed. - v8::Handle MakeDomainCallback( - const v8::Handle cb, - int argc, - v8::Handle* argv); - uint32_t provider_type_; }; diff --git a/src/node.cc b/src/node.cc index c07a0d9c04c..130d5ab9ea3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -986,111 +986,55 @@ void SetupNextTick(const FunctionCallbackInfo& args) { } -Handle MakeDomainCallback(Environment* env, - Handle recv, - const Handle callback, - int argc, - Handle argv[]) { - // If you hit this assertion, you forgot to enter the v8::Context first. - CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); - - Local process = env->process_object(); - Local object, domain; - Local domain_v; - - TryCatch try_catch; - try_catch.SetVerbose(true); - - bool has_domain = false; - - if (!object.IsEmpty()) { - domain_v = object->Get(env->domain_string()); - has_domain = domain_v->IsObject(); - if (has_domain) { - domain = domain_v.As(); - - if (domain->Get(env->disposed_string())->IsTrue()) { - // domain has been disposed of. - return Undefined(env->isolate()); - } - - Local enter = domain->Get(env->enter_string()).As(); - if (enter->IsFunction()) { - enter->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env->isolate()); - } - } - } - - Local ret = callback->Call(recv, argc, argv); - - if (try_catch.HasCaught()) { - return Undefined(env->isolate()); - } - - if (has_domain) { - Local exit = domain->Get(env->exit_string()).As(); - if (exit->IsFunction()) { - exit->Call(domain, 0, nullptr); - if (try_catch.HasCaught()) - return Undefined(env->isolate()); - } - } - - Environment::TickInfo* tick_info = env->tick_info(); - - if (tick_info->last_threw() == 1) { - tick_info->set_last_threw(0); - return ret; - } - - if (tick_info->in_tick()) { - return ret; - } - - if (tick_info->length() == 0) { - env->isolate()->RunMicrotasks(); - } - - if (tick_info->length() == 0) { - tick_info->set_index(0); - return ret; - } - - tick_info->set_in_tick(true); - - env->tick_callback_function()->Call(process, 0, nullptr); - - tick_info->set_in_tick(false); - - if (try_catch.HasCaught()) { - tick_info->set_last_threw(true); - return Undefined(env->isolate()); - } - - return ret; -} - - Handle MakeCallback(Environment* env, Handle recv, const Handle callback, int argc, Handle argv[]) { - if (env->using_domains()) - return MakeDomainCallback(env, recv, callback, argc, argv); - // If you hit this assertion, you forgot to enter the v8::Context first. CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); Local process = env->process_object(); + Local object, domain; + bool has_domain = false; + + if (env->using_domains()) { + CHECK(recv->IsObject()); + object = recv.As(); + Local domain_v = object->Get(env->domain_string()); + has_domain = domain_v->IsObject(); + if (has_domain) { + domain = domain_v.As(); + if (domain->Get(env->disposed_string())->IsTrue()) + return Undefined(env->isolate()); + } + } TryCatch try_catch; try_catch.SetVerbose(true); + if (has_domain) { + Local enter_v = domain->Get(env->enter_string()); + if (enter_v->IsFunction()) { + enter_v.As()->Call(domain, 0, nullptr); + if (try_catch.HasCaught()) + return Undefined(env->isolate()); + } + } + Local ret = callback->Call(recv, argc, argv); + if (has_domain) { + Local exit_v = domain->Get(env->exit_string()); + if (exit_v->IsFunction()) { + exit_v.As()->Call(domain, 0, nullptr); + if (try_catch.HasCaught()) + return Undefined(env->isolate()); + } + } + env->tick_callback_function()->Call(process, 0, nullptr); + CHECK_EQ(env->context(), env->isolate()->GetCurrentContext()); + if (try_catch.HasCaught()) { return Undefined(env->isolate()); } @@ -1132,10 +1076,9 @@ Handle MakeCallback(Environment* env, uint32_t index, int argc, Handle argv[]) { - Local callback = recv->Get(index).As(); - CHECK(callback->IsFunction()); - - return MakeCallback(env, recv.As(), callback, argc, argv); + Local cb_v = recv->Get(index); + CHECK(cb_v->IsFunction()); + return MakeCallback(env, recv.As(), cb_v.As(), argc, argv); } @@ -1144,9 +1087,9 @@ Handle MakeCallback(Environment* env, Handle symbol, int argc, Handle argv[]) { - Local callback = recv->Get(symbol).As(); - CHECK(callback->IsFunction()); - return MakeCallback(env, recv.As(), callback, argc, argv); + Local cb_v = recv->Get(symbol); + CHECK(cb_v->IsFunction()); + return MakeCallback(env, recv.As(), cb_v.As(), argc, argv); } @@ -1203,20 +1146,6 @@ Handle MakeCallback(Isolate* isolate, } -Handle MakeDomainCallback(Handle recv, - Handle callback, - int argc, - Handle argv[]) { - Local context = recv->CreationContext(); - Environment* env = Environment::GetCurrent(context); - Context::Scope context_scope(context); - EscapableHandleScope handle_scope(env->isolate()); - return handle_scope.Escape(Local::New( - env->isolate(), - MakeDomainCallback(env, recv, callback, argc, argv))); -} - - enum encoding ParseEncoding(Isolate* isolate, Handle encoding_v, enum encoding _default) {