From 3f2140eafe4ea864477b584a47b4d88db66d904d Mon Sep 17 00:00:00 2001 From: Sven Over Date: Tue, 29 Mar 2016 03:28:31 -0700 Subject: [PATCH] folly/futures: use folly::Function to store callback Summary:This diff makes it possible to pass callables (such as lambdas) to folly::Future::then that are not copy-constructible. As a consequence, move-only types (such as folly::Promise or std::unique_ptr) can now be captured in lambdas passed to folly::Future::then. Using C++14 notation, the following is now possible: Future().then([promise = std::move(promise)]() mutable { promise.setValue(123); }); See folly/futures/test/NonCopyableLambdaTest.cpp for more examples. folly::Future uses std::function to store callback functions. (More precisely, the callback function is stored in a std::function in folly::detail::Core.) std::function is a copy-constructible type and it requires that the callable that it stores is copy constructible as well. This diff changes the implementation of folly::detail::Core to use folly::Function instead of std::function. It also simplifies the code in folly::detail::Core a little bit: Core had a reserved space of size 8*sizeof(void*) to store callbacks in-place. Only larger callbacks (capturing more data) would be stored in the std::function, which puts those on the heap. folly::Function has a template parameter to set the size of the in-place callable storage. In Core, it is set to 8*sizeof(void*), so all callbacks that used to be stored in-place inside Core, are now stored in-place inside folly::Function. This even reduces the size of a Core object: the folly::Function object occupies 80 bytes, which is 16 bytes less than what was used before for a std::function (32 bytes) and the callback storage (64 bytes). The resulting size of a Core goes down from 192 to 176 bytes (on x86_64). Reviewed By: fugalh Differential Revision: D2884868 fb-gh-sync-id: 35548890c392f80e6c680676b0f98e711bc03ca3 fbshipit-source-id: 35548890c392f80e6c680676b0f98e711bc03ca3 --- folly/futures/detail/Core.h | 42 +++------ folly/futures/test/NonCopyableLambdaTest.cpp | 92 ++++++++++++++++++++ folly/test/Makefile.am | 1 + 3 files changed, 104 insertions(+), 31 deletions(-) create mode 100644 folly/futures/test/NonCopyableLambdaTest.cpp diff --git a/folly/futures/detail/Core.h b/folly/futures/detail/Core.h index 70f887d2..20bb78b1 100644 --- a/folly/futures/detail/Core.h +++ b/folly/futures/detail/Core.h @@ -21,13 +21,13 @@ #include #include -#include +#include +#include #include - -#include -#include +#include #include -#include +#include +#include #include #include @@ -147,34 +147,13 @@ class Core { } } - template - class LambdaBufHelper { - public: - template - explicit LambdaBufHelper(FF&& func) : func_(std::forward(func)) {} - void operator()(Try&& t) { - SCOPE_EXIT { this->~LambdaBufHelper(); }; - func_(std::move(t)); - } - private: - F func_; - }; - /// Call only from Future thread. template void setCallback(F func) { bool transitionToArmed = false; auto setCallback_ = [&]{ context_ = RequestContext::saveContext(); - - // Move the lambda into the Core if it fits - if (sizeof(LambdaBufHelper) <= lambdaBufSize) { - auto funcLoc = reinterpret_cast*>(&lambdaBuf_); - new (funcLoc) LambdaBufHelper(std::forward(func)); - callback_ = std::ref(*funcLoc); - } else { - callback_ = std::move(func); - } + callback_ = std::move(func); }; FSM_START(fsm_) @@ -395,13 +374,14 @@ class Core { // sizeof(Core) == size(Core). // See Core::convert for details. - // lambdaBuf occupies exactly one cache line - static constexpr size_t lambdaBufSize = 8 * sizeof(void*); - typename std::aligned_storage::type lambdaBuf_; + folly::Function< + void(Try&&), + folly::FunctionMoveCtor::MAY_THROW, + 8 * sizeof(void*)> + callback_; // place result_ next to increase the likelihood that the value will be // contained entirely in one cache line folly::Optional> result_; - std::function&&)> callback_ {nullptr}; FSM fsm_; std::atomic attached_; std::atomic active_ {true}; diff --git a/folly/futures/test/NonCopyableLambdaTest.cpp b/folly/futures/test/NonCopyableLambdaTest.cpp new file mode 100644 index 00000000..73bc6dc7 --- /dev/null +++ b/folly/futures/test/NonCopyableLambdaTest.cpp @@ -0,0 +1,92 @@ +/* + * Copyright 2016 Facebook, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include + +using namespace folly; + +TEST(NonCopyableLambda, basic) { + Promise promise; + Future future = promise.getFuture(); + + Future().then(std::bind( + [](Promise& promise) mutable { promise.setValue(123); }, + std::move(promise))); + + // The previous statement can be simplified in C++14: + // Future().then([promise = std::move(promise)]() mutable { + // promise.setValue(123); + // }); + + EXPECT_TRUE(future.isReady()); + EXPECT_EQ(future.get(), 123); +} + +TEST(NonCopyableLambda, unique_ptr) { + Promise promise; + auto int_ptr = folly::make_unique(1); + + EXPECT_EQ(*int_ptr, 1); + + auto future = promise.getFuture().then(std::bind( + [](std::unique_ptr& int_ptr) mutable { + ++*int_ptr; + return std::move(int_ptr); + }, + std::move(int_ptr))); + + // The previous statement can be simplified in C++14: + // auto future = + // promise.getFuture().then([int_ptr = std::move(int_ptr)]() mutable { + // ++*int_ptr; + // return std::move(int_ptr); + // }); + + EXPECT_FALSE(future.isReady()); + promise.setValue(); + EXPECT_TRUE(future.isReady()); + EXPECT_EQ(*future.get(), 2); +} + +TEST(NonCopyableLambda, Function) { + Promise promise; + + Function callback = [](int x) { return x + 1; }; + + auto future = promise.getFuture().then(std::move(callback)); + EXPECT_THROW(callback(0), std::bad_function_call); + + EXPECT_FALSE(future.isReady()); + promise.setValue(100); + EXPECT_TRUE(future.isReady()); + EXPECT_EQ(future.get(), 101); +} + +TEST(NonCopyableLambda, FunctionConst) { + Promise promise; + + Function callback = [](int x) { return x + 1; }; + + auto future = promise.getFuture().then(std::move(callback)); + EXPECT_THROW(callback(0), std::bad_function_call); + + EXPECT_FALSE(future.isReady()); + promise.setValue(100); + EXPECT_TRUE(future.isReady()); + EXPECT_EQ(future.get(), 101); +} diff --git a/folly/test/Makefile.am b/folly/test/Makefile.am index 7d037b96..71fe7a80 100644 --- a/folly/test/Makefile.am +++ b/folly/test/Makefile.am @@ -226,6 +226,7 @@ futures_test_SOURCES = \ ../futures/test/HeaderCompileTest.cpp \ ../futures/test/InterruptTest.cpp \ ../futures/test/MapTest.cpp \ + ../futures/test/NonCopyableLambdaTest.cpp \ ../futures/test/PollTest.cpp \ ../futures/test/PromiseTest.cpp \ ../futures/test/ReduceTest.cpp \ -- 2.34.1