From 030234c7a6c7f6c5a901dab68591c9cfb2d4828f Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 3 Sep 2016 13:28:55 -0700 Subject: [core] Rework invokeWithCallback so that the callback is last --- include/mbgl/util/run_loop.hpp | 6 +++--- include/mbgl/util/work_task.hpp | 6 +++--- include/mbgl/util/work_task_impl.hpp | 24 +++++++++++++++++++----- platform/android/src/asset_file_source.cpp | 2 +- platform/default/asset_file_source.cpp | 2 +- platform/default/default_file_source.cpp | 2 +- src/mbgl/util/thread.hpp | 15 +++++++++------ src/mbgl/util/worker.cpp | 18 +++++++++--------- test/util/thread.cpp | 28 ++++++++++++++-------------- 9 files changed, 60 insertions(+), 43 deletions(-) diff --git a/include/mbgl/util/run_loop.hpp b/include/mbgl/util/run_loop.hpp index af1053dfdc..6559b72ef8 100644 --- a/include/mbgl/util/run_loop.hpp +++ b/include/mbgl/util/run_loop.hpp @@ -61,10 +61,10 @@ public: } // Invoke fn(args...) on this RunLoop, then invoke callback(results...) on the current RunLoop. - template + template std::unique_ptr - invokeWithCallback(Fn&& fn, Cb&& callback, Args&&... args) { - std::shared_ptr task = WorkTask::makeWithCallback(std::forward(fn), std::forward(callback), std::forward(args)...); + invokeWithCallback(Fn&& fn, Args&&... args) { + std::shared_ptr task = WorkTask::makeWithCallback(std::forward(fn), std::forward(args)...); push(task); return std::make_unique(task); } diff --git a/include/mbgl/util/work_task.hpp b/include/mbgl/util/work_task.hpp index e9c2062d9c..dda8e5d00f 100644 --- a/include/mbgl/util/work_task.hpp +++ b/include/mbgl/util/work_task.hpp @@ -17,10 +17,10 @@ public: virtual void cancel() = 0; template - static std::shared_ptr make(Fn&& fn, Args&&... args); + static std::shared_ptr make(Fn&&, Args&&...); - template - static std::shared_ptr makeWithCallback(Fn&& fn, Cb&& callback, Args&&... args); + template + static std::shared_ptr makeWithCallback(Fn&&, Args&&...); }; } // namespace mbgl diff --git a/include/mbgl/util/work_task_impl.hpp b/include/mbgl/util/work_task_impl.hpp index 2c016601bb..a2f55e00c5 100644 --- a/include/mbgl/util/work_task_impl.hpp +++ b/include/mbgl/util/work_task_impl.hpp @@ -62,10 +62,12 @@ std::shared_ptr WorkTask::make(Fn&& fn, Args&&... args) { flag); } -template -std::shared_ptr WorkTask::makeWithCallback(Fn&& fn, Cb&& callback, Args&&... args) { - auto flag = std::make_shared>(); - *flag = false; +namespace detail { +template +auto packageArgumentsAndCallback(std::shared_ptr> flag, + Tuple&& args, + std::index_sequence) { + auto callback = std::get(args); // Create a lambda L1 that invokes another lambda L2 on the current RunLoop R, that calls // the callback C. Both lambdas check the flag before proceeding. L1 needs to check the flag @@ -82,7 +84,19 @@ std::shared_ptr WorkTask::makeWithCallback(Fn&& fn, Cb&& callback, Arg } }; - auto tuple = std::make_tuple(std::move(args)..., after); + return std::make_tuple(std::move(std::get(args))..., after); +} +} // namespace detail + +template +std::shared_ptr WorkTask::makeWithCallback(Fn&& fn, Args&&... args) { + auto flag = std::make_shared>(); + *flag = false; + + auto tuple = detail::packageArgumentsAndCallback(flag, + std::forward_as_tuple(std::forward(args)...), + std::make_index_sequence()); + return std::make_shared>( std::move(fn), std::move(tuple), diff --git a/platform/android/src/asset_file_source.cpp b/platform/android/src/asset_file_source.cpp index ec4925a02e..c72b86af5c 100644 --- a/platform/android/src/asset_file_source.cpp +++ b/platform/android/src/asset_file_source.cpp @@ -107,7 +107,7 @@ AssetFileSource::AssetFileSource(const std::string& root) AssetFileSource::~AssetFileSource() = default; std::unique_ptr AssetFileSource::request(const Resource& resource, Callback callback) { - return thread->invokeWithCallback(&Impl::request, callback, resource.url); + return thread->invokeWithCallback(&Impl::request, resource.url, callback); } } diff --git a/platform/default/asset_file_source.cpp b/platform/default/asset_file_source.cpp index 2872c6bb3a..1832818378 100644 --- a/platform/default/asset_file_source.cpp +++ b/platform/default/asset_file_source.cpp @@ -64,7 +64,7 @@ AssetFileSource::AssetFileSource(const std::string& root) AssetFileSource::~AssetFileSource() = default; std::unique_ptr AssetFileSource::request(const Resource& resource, Callback callback) { - return thread->invokeWithCallback(&Impl::request, callback, resource.url); + return thread->invokeWithCallback(&Impl::request, resource.url, callback); } } // namespace mbgl diff --git a/platform/default/default_file_source.cpp b/platform/default/default_file_source.cpp index 6a94fa3866..9465728509 100644 --- a/platform/default/default_file_source.cpp +++ b/platform/default/default_file_source.cpp @@ -164,7 +164,7 @@ std::unique_ptr DefaultFileSource::request(const Resource& resourc public: DefaultFileRequest(Resource resource_, FileSource::Callback callback_, util::Thread& thread_) : thread(thread_), - workRequest(thread.invokeWithCallback(&DefaultFileSource::Impl::request, callback_, this, resource_)) { + workRequest(thread.invokeWithCallback(&DefaultFileSource::Impl::request, this, resource_, callback_)) { } ~DefaultFileRequest() override { diff --git a/src/mbgl/util/thread.hpp b/src/mbgl/util/thread.hpp index 31a56a4834..91eef745fa 100644 --- a/src/mbgl/util/thread.hpp +++ b/src/mbgl/util/thread.hpp @@ -30,20 +30,23 @@ public: Thread(const ThreadContext&, Args&&... args); ~Thread(); - // Invoke object->fn(args...) in the runloop thread. + // Invoke object->fn(args...) asynchronously. template void invoke(Fn fn, Args&&... args) { loop->invoke(bind(fn), std::forward(args)...); } - // Invoke object->fn(args...) in the runloop thread, then invoke callback(result) in the current thread. - template + // Invoke object->fn(args...) asynchronously. The final argument to fn must be a callback. + // The provided callback is wrapped such that it is invoked, in the current thread (which + // must have a RunLoop), once for each time the invocation of fn invokes the wrapper, each + // time forwarding the passed arguments, until such time as the AsyncRequest is cancelled. + template std::unique_ptr - invokeWithCallback(Fn fn, Cb&& callback, Args&&... args) { - return loop->invokeWithCallback(bind(fn), callback, std::forward(args)...); + invokeWithCallback(Fn fn, Args&&... args) { + return loop->invokeWithCallback(bind(fn), std::forward(args)...); } - // Invoke object->fn(args...) in the runloop thread, and wait for the result. + // Invoke object->fn(args...) asynchronously, but wait for the result. template auto invokeSync(Fn fn, Args&&... args) { using R = std::result_of_t; diff --git a/src/mbgl/util/worker.cpp b/src/mbgl/util/worker.cpp index e05a5cf837..f28e6e17b8 100644 --- a/src/mbgl/util/worker.cpp +++ b/src/mbgl/util/worker.cpp @@ -84,8 +84,8 @@ Worker::parseRasterTile(std::unique_ptr bucket, const std::shared_ptr data, std::function callback) { current = (current + 1) % threads.size(); - return threads[current]->invokeWithCallback(&Worker::Impl::parseRasterTile, callback, bucket, - data); + return threads[current]->invokeWithCallback(&Worker::Impl::parseRasterTile, bucket, + data, callback); } std::unique_ptr @@ -95,8 +95,8 @@ Worker::parseGeometryTile(TileWorker& worker, PlacementConfig config, std::function callback) { current = (current + 1) % threads.size(); - return threads[current]->invokeWithCallback(&Worker::Impl::parseGeometryTile, callback, &worker, - std::move(layers), std::move(tileData), config); + return threads[current]->invokeWithCallback(&Worker::Impl::parseGeometryTile, &worker, + std::move(layers), std::move(tileData), config, callback); } std::unique_ptr @@ -105,7 +105,7 @@ Worker::parsePendingGeometryTileLayers(TileWorker& worker, std::function callback) { current = (current + 1) % threads.size(); return threads[current]->invokeWithCallback(&Worker::Impl::parsePendingGeometryTileLayers, - callback, &worker, config); + &worker, config, callback); } std::unique_ptr @@ -114,8 +114,8 @@ Worker::redoLayout(TileWorker& worker, PlacementConfig config, std::function callback) { current = (current + 1) % threads.size(); - return threads[current]->invokeWithCallback(&Worker::Impl::redoLayout, callback, &worker, - std::move(layers), config); + return threads[current]->invokeWithCallback(&Worker::Impl::redoLayout, &worker, + std::move(layers), config, callback); } std::unique_ptr @@ -124,8 +124,8 @@ Worker::redoPlacement(TileWorker& worker, PlacementConfig config, std::function)> callback) { current = (current + 1) % threads.size(); - return threads[current]->invokeWithCallback(&Worker::Impl::redoPlacement, callback, &worker, - &buckets, config); + return threads[current]->invokeWithCallback(&Worker::Impl::redoPlacement, &worker, + &buckets, config, callback); } } // end namespace mbgl diff --git a/test/util/thread.cpp b/test/util/thread.cpp index bf76ec4aef..fc41fd4b78 100644 --- a/test/util/thread.cpp +++ b/test/util/thread.cpp @@ -83,10 +83,10 @@ TEST(Thread, invoke) { EXPECT_EQ(*result, 1); })); - requests.push_back(thread.invokeWithCallback(&TestObject::transferInOut, [&] (std::unique_ptr result) { + requests.push_back(thread.invokeWithCallback(&TestObject::transferInOut, std::make_unique(1), [&] (std::unique_ptr result) { EXPECT_EQ(tid, std::this_thread::get_id()); EXPECT_EQ(*result, 1); - }, std::make_unique(1))); + })); thread.invoke(&TestObject::transferInShared, std::make_shared(1)); requests.push_back(thread.invokeWithCallback(&TestObject::transferOutShared, [&] (std::shared_ptr result) { @@ -100,11 +100,11 @@ TEST(Thread, invoke) { }); std::string test("test"); - requests.push_back(thread.invokeWithCallback(&TestObject::transferString, [&] (std::string result){ + requests.push_back(thread.invokeWithCallback(&TestObject::transferString, test, [&] (std::string result){ EXPECT_EQ(tid, std::this_thread::get_id()); EXPECT_EQ(result, "test"); loop.stop(); - }, test)); + })); test.clear(); }); @@ -147,10 +147,10 @@ TEST(Thread, ExecutesAfter) { bool didAfter = false; auto request = thread.invokeWithCallback(&TestWorker::send, [&] { + didWork = true; + }, [&] { didAfter = true; loop.stop(); - }, [&] { - didWork = true; }); loop.run(); @@ -167,11 +167,11 @@ TEST(Thread, WorkRequestDeletionWaitsForWorkToComplete) { std::promise started; bool didWork = false; - auto request = thread.invokeWithCallback(&TestWorker::send, [&] {}, [&] { + auto request = thread.invokeWithCallback(&TestWorker::send, [&] { started.set_value(); usleep(10000); didWork = true; - }); + }, [&] {}); started.get_future().get(); request.reset(); @@ -186,9 +186,9 @@ TEST(Thread, WorkRequestDeletionCancelsAfter) { bool didAfter = false; auto request = thread.invokeWithCallback(&TestWorker::send, [&] { - didAfter = true; - }, [&] { started.set_value(); + }, [&] { + didAfter = true; }); started.get_future().get(); @@ -203,14 +203,14 @@ TEST(Thread, WorkRequestDeletionCancelsImmediately) { std::promise started; - auto request1 = thread.invokeWithCallback(&TestWorker::send, [&] {}, [&] { + auto request1 = thread.invokeWithCallback(&TestWorker::send, [&] { usleep(10000); started.set_value(); - }); + }, [&] {}); - auto request2 = thread.invokeWithCallback(&TestWorker::send, [&] {}, [&] { + auto request2 = thread.invokeWithCallback(&TestWorker::send, [&] { ADD_FAILURE() << "Second work item should not be invoked"; - }); + }, [&] {}); request2.reset(); started.get_future().get(); -- cgit v1.2.1