From 42df679c458b3edc2d4754baaf3b16ba015a1dd6 Mon Sep 17 00:00:00 2001 From: Trevor Norris Date: Wed, 12 Nov 2014 16:08:12 -0800 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 | 14 ++--- src/async-wrap.cc | 88 +++++++--------------------- src/async-wrap.h | 7 --- src/node.cc | 133 ++++++++++--------------------------------- 4 files changed, 55 insertions(+), 187 deletions(-) diff --git a/src/async-wrap-inl.h b/src/async-wrap-inl.h index f3270c9837b..9bd5744b164 100644 --- a/src/async-wrap-inl.h +++ b/src/async-wrap-inl.h @@ -31,7 +31,6 @@ #include "util-inl.h" #include "v8.h" -#include namespace node { @@ -46,6 +45,7 @@ inline AsyncWrap::AsyncWrap(Environment* env, inline AsyncWrap::~AsyncWrap() { } + inline uint32_t AsyncWrap::provider_type() const { return provider_type_; } @@ -56,10 +56,8 @@ inline v8::Handle AsyncWrap::MakeCallback( int argc, v8::Handle* argv) { v8::Local cb_v = object()->Get(symbol); - v8::Local cb = cb_v.As(); - assert(cb->IsFunction()); - - return MakeCallback(cb, argc, argv); + ASSERT(cb_v->IsFunction()); + return MakeCallback(cb_v.As(), argc, argv); } @@ -68,10 +66,8 @@ inline v8::Handle AsyncWrap::MakeCallback( int argc, v8::Handle* argv) { v8::Local cb_v = object()->Get(index); - v8::Local cb = cb_v.As(); - assert(cb->IsFunction()); - - return MakeCallback(cb, argc, argv); + ASSERT(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 6f3f1e92123..66455a3046e 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, NULL); + Local enter_v = domain->Get(env()->enter_string()); + if (enter_v->IsFunction()) { + enter_v.As()->Call(domain, 0, NULL); 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, NULL); + Local exit_v = domain->Get(env()->exit_string()); + if (exit_v->IsFunction()) { + exit_v.As()->Call(domain, 0, NULL); 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, NULL); - - 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 1d30a3b1db4..b1f1fef3bde 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 df1c8eb1bf3..f4809a5147a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -978,111 +978,53 @@ 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. - assert(env->context() == env->isolate()->GetCurrentContext()); + CHECK(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, NULL); - 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, NULL); + Local enter_v = domain->Get(env->enter_string()); + if (enter_v->IsFunction()) { + enter_v.As()->Call(domain, 0, NULL); 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, NULL); - - tick_info->set_in_tick(false); + Local ret = callback->Call(recv, argc, argv); - if (try_catch.HasCaught()) { - tick_info->set_last_threw(true); - 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, NULL); + if (try_catch.HasCaught()) + 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. - assert(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()); } @@ -1124,10 +1066,9 @@ Handle MakeCallback(Environment* env, uint32_t index, int argc, Handle argv[]) { - Local callback = recv->Get(index).As(); - assert(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); } @@ -1136,9 +1077,9 @@ Handle MakeCallback(Environment* env, Handle symbol, int argc, Handle argv[]) { - Local callback = recv->Get(symbol).As(); - assert(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); } @@ -1195,20 +1136,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) {