From 9d0cef4fae5067125a51fb6f7190d6ad41240687 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 | 88 +++++++---------------------- src/async-wrap.h | 7 --- src/node.cc | 129 ++++++++++--------------------------------- 4 files changed, 53 insertions(+), 183 deletions(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index 38a5c7fd237a7f..7a298a1497bc4e 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 0bf52b0da30d25..4f742ce21eb1bf 100644 --- a/src/async-wrap.cc +++ b/src/async-wrap.cc @@ -37,30 +37,33 @@ using v8::Value; namespace node { -Handle AsyncWrap::MakeDomainCallback(const Handle cb, - int argc, - Handle* argv) { +Handle AsyncWrap::MakeCallback(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; + 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); - 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); + 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()); } @@ -73,10 +76,9 @@ Handle AsyncWrap::MakeDomainCallback(const Handle cb, } if (has_domain) { - Local exit = - domain->Get(env()->exit_string()).As(); - if (exit->IsFunction()) { - exit->Call(domain, 0, nullptr); + 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()); } @@ -111,54 +113,4 @@ Handle AsyncWrap::MakeDomainCallback(const Handle cb, 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(); - - TryCatch try_catch; - try_catch.SetVerbose(true); - - Local ret = cb->Call(context, argc, argv); - - 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; -} - } // namespace node diff --git a/src/async-wrap.h b/src/async-wrap.h index 9393c4c3457cfb..30e49946e7f845 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 c07a0d9c04cfd9..130d5ab9ea36ab 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[]) { +Handle MakeCallback(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()); + 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()) { - // domain has been disposed of. + 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 = callback->Call(recv, argc, argv); - - if (try_catch.HasCaught()) { - return Undefined(env->isolate()); - } + TryCatch try_catch; + try_catch.SetVerbose(true); if (has_domain) { - Local exit = domain->Get(env->exit_string()).As(); - if (exit->IsFunction()) { - exit->Call(domain, 0, nullptr); + 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()); } } - 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(); - } + Local ret = callback->Call(recv, argc, argv); - if (tick_info->length() == 0) { - tick_info->set_index(0); - return ret; + 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()); + } } - - 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(); - - TryCatch try_catch; - try_catch.SetVerbose(true); - - Local ret = callback->Call(recv, argc, argv); - 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) {