From 33344a23da0a447833118c7456366a1aed7c3c03 Mon Sep 17 00:00:00 2001 From: Pavlo Kushnir Date: Wed, 27 Apr 2016 17:45:07 -0700 Subject: [PATCH] Faster onDestroy Summary: there is no need to call `std::function::invoke` for every DestructorGuard. Reviewed By: yfeldblum Differential Revision: D3229345 fb-gh-sync-id: c42f8cd05576d56b6a9b2f9d06878d9b01a36e94 fbshipit-source-id: c42f8cd05576d56b6a9b2f9d06878d9b01a36e94 --- folly/io/async/DelayedDestruction.h | 20 ++++++------- folly/io/async/DelayedDestructionBase.h | 15 +++++----- .../async/test/DelayedDestructionBaseTest.cpp | 29 ++++--------------- folly/io/async/test/UndelayedDestruction.h | 21 +++++++------- 4 files changed, 35 insertions(+), 50 deletions(-) diff --git a/folly/io/async/DelayedDestruction.h b/folly/io/async/DelayedDestruction.h index 71cd6915..5e179d53 100644 --- a/folly/io/async/DelayedDestruction.h +++ b/folly/io/async/DelayedDestruction.h @@ -54,7 +54,7 @@ class DelayedDestruction : public DelayedDestructionBase { if (getDestructorGuardCount() != 0) { destroyPending_ = true; } else { - onDestroy_(false); + onDelayedDestroy(false); } } @@ -98,15 +98,6 @@ class DelayedDestruction : public DelayedDestructionBase { DelayedDestruction() : destroyPending_(false) { - - onDestroy_ = [this] (bool delayed) { - // check if it is ok to destroy now - if (delayed && !destroyPending_) { - return; - } - destroyPending_ = false; - delete this; - }; } private: @@ -118,5 +109,14 @@ class DelayedDestruction : public DelayedDestructionBase { * guardCount_ drops to 0. */ bool destroyPending_; + + void onDelayedDestroy(bool delayed) override { + // check if it is ok to destroy now + if (delayed && !destroyPending_) { + return; + } + destroyPending_ = false; + delete this; + } }; } // folly diff --git a/folly/io/async/DelayedDestructionBase.h b/folly/io/async/DelayedDestructionBase.h index bd68c1cb..52505a06 100644 --- a/folly/io/async/DelayedDestructionBase.h +++ b/folly/io/async/DelayedDestructionBase.h @@ -37,7 +37,7 @@ namespace folly { * * Classes needing this functionality should: * - derive from DelayedDestructionBase directly - * - pass a callback to onDestroy_ which'll be called before the object is + * - implement onDelayedDestroy which'll be called before the object is * going to be destructed * - create a DestructorGuard object on the stack in each public method that * may invoke a callback @@ -93,7 +93,7 @@ class DelayedDestructionBase : private boost::noncopyable { assert(dd_->guardCount_ > 0); --dd_->guardCount_; if (dd_->guardCount_ == 0) { - dd_->onDestroy_(true); + dd_->onDelayedDestroy(true); } } } @@ -213,14 +213,15 @@ class DelayedDestructionBase : private boost::noncopyable { } /** - * Implement onDestroy_ in subclasses. - * onDestroy_() is invoked when the object is potentially being destroyed. + * Implement onDelayedDestroy in subclasses. + * onDelayedDestroy() is invoked when the object is potentially being + * destroyed. * * @param delayed This parameter is true if destruction was delayed because - * of a DestructorGuard object, or false if onDestroy_() is - * being called directly from the destructor. + * of a DestructorGuard object, or false if onDelayedDestroy() + * is being called directly from the destructor. */ - std::function onDestroy_; + virtual void onDelayedDestroy(bool delayed) = 0; private: /** diff --git a/folly/io/async/test/DelayedDestructionBaseTest.cpp b/folly/io/async/test/DelayedDestructionBaseTest.cpp index c9497de9..1d7ab3fc 100644 --- a/folly/io/async/test/DelayedDestructionBaseTest.cpp +++ b/folly/io/async/test/DelayedDestructionBaseTest.cpp @@ -43,11 +43,6 @@ using namespace folly; class DestructionOnCallback : public DelayedDestructionBase { public: DestructionOnCallback() : state_(0), deleted_(false) { - onDestroy_ = [this] (bool delayed) { - deleted_ = true; - delete this; - (void)delayed; // prevent unused variable warnings - }; } void onComplete(int n, int& state) { @@ -58,10 +53,6 @@ class DestructionOnCallback : public DelayedDestructionBase { state = state_; } - void setOnDestroy(std::function onDestroy) { - onDestroy_ = onDestroy; - } - int state() const { return state_; } bool deleted() const { return deleted_; } @@ -77,6 +68,12 @@ class DestructionOnCallback : public DelayedDestructionBase { private: int state_; bool deleted_; + + void onDelayedDestroy(bool delayed) override { + deleted_ = true; + delete this; + (void)delayed; // prevent unused variable warnings + } }; struct DelayedDestructionBaseTest : public ::testing::Test { @@ -89,17 +86,3 @@ TEST_F(DelayedDestructionBaseTest, basic) { d->onComplete(3, state); EXPECT_EQ(state, 10); // 10 = 6 + 3 + 1 } - -TEST_F(DelayedDestructionBaseTest, destructFromContainer) { - std::list l; - l.emplace_back(); - l.back().setOnDestroy([&] (bool delayed) { - l.erase(l.begin()); - (void)delayed; - }); - EXPECT_NE(l.size(), 0); - int32_t state; - l.back().onComplete(3, state); - EXPECT_EQ(state, 10); // 10 = 6 + 3 + 1 - EXPECT_EQ(l.size(), 0); -} diff --git a/folly/io/async/test/UndelayedDestruction.h b/folly/io/async/test/UndelayedDestruction.h index a6594e0e..47a3dba8 100644 --- a/folly/io/async/test/UndelayedDestruction.h +++ b/folly/io/async/test/UndelayedDestruction.h @@ -58,16 +58,6 @@ class UndelayedDestruction : public TDD { template explicit UndelayedDestruction(Args&& ...args) : TDD(std::forward(args)...) { - this->TDD::onDestroy_ = [&, this] (bool delayed) { - if (delayed && !this->TDD::getDestroyPending()) { - return; - } - // Do nothing. This will always be invoked from the call to destroy - // inside our destructor. - assert(!delayed); - // prevent unused variable warnings when asserts are compiled out. - (void)delayed; - }; } /** @@ -91,6 +81,17 @@ class UndelayedDestruction : public TDD { this->destroy(); } + void onDelayedDestroy(bool delayed) override { + if (delayed && !this->TDD::getDestroyPending()) { + return; + } + // Do nothing. This will always be invoked from the call to destroy + // inside our destructor. + assert(!delayed); + // prevent unused variable warnings when asserts are compiled out. + (void)delayed; + } + protected: /** * Override our parent's destroy() method to make it protected. -- 2.34.1