From 5c7867f435f9d94109ce11fcbbd153b016e02fe0 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Sat, 2 May 2015 13:31:56 -0400 Subject: [PATCH] Join worker tasks before destroying TileData Fixes #1309 --- src/mbgl/map/live_tile_data.cpp | 6 +++++- src/mbgl/map/raster_tile_data.cpp | 3 +++ src/mbgl/map/tile_data.cpp | 18 ++++------------ src/mbgl/map/tile_data.hpp | 6 ++++-- src/mbgl/map/vector_tile_data.cpp | 3 +++ src/mbgl/util/run_loop.hpp | 21 +++++-------------- src/mbgl/util/thread.hpp | 4 ++-- src/mbgl/util/work_request.cpp | 34 +++++++++++++++++++++++++++++++ src/mbgl/util/work_request.hpp | 31 ++++++++++++++++++++++++++++ src/mbgl/util/worker.cpp | 14 +++++++++---- src/mbgl/util/worker.hpp | 11 +++++++++- 11 files changed, 111 insertions(+), 40 deletions(-) create mode 100644 src/mbgl/util/work_request.cpp create mode 100644 src/mbgl/util/work_request.hpp diff --git a/src/mbgl/map/live_tile_data.cpp b/src/mbgl/map/live_tile_data.cpp index 1582b0190..096e899d8 100644 --- a/src/mbgl/map/live_tile_data.cpp +++ b/src/mbgl/map/live_tile_data.cpp @@ -24,7 +24,11 @@ LiveTileData::LiveTileData(const TileID& id_, state = State::loaded; } -LiveTileData::~LiveTileData() {} +LiveTileData::~LiveTileData() { + // Cancel in most derived class destructor so that worker tasks are joined before + // any member data goes away. + cancel(); +} void LiveTileData::parse() { if (state != State::loaded) { diff --git a/src/mbgl/map/raster_tile_data.cpp b/src/mbgl/map/raster_tile_data.cpp index 0b849ca7c..20c07b573 100644 --- a/src/mbgl/map/raster_tile_data.cpp +++ b/src/mbgl/map/raster_tile_data.cpp @@ -9,6 +9,9 @@ RasterTileData::RasterTileData(const TileID& id_, TexturePool &texturePool, } RasterTileData::~RasterTileData() { + // Cancel in most derived class destructor so that worker tasks are joined before + // any member data goes away. + cancel(); } void RasterTileData::parse() { diff --git a/src/mbgl/map/tile_data.cpp b/src/mbgl/map/tile_data.cpp index 80fd6346d..dd51cb841 100644 --- a/src/mbgl/map/tile_data.cpp +++ b/src/mbgl/map/tile_data.cpp @@ -55,21 +55,11 @@ void TileData::cancel() { env.cancelRequest(req); req = nullptr; } + if (workRequest) { + workRequest.join(); + } } void TileData::reparse(Worker& worker, std::function callback) { - util::ptr tile = shared_from_this(); - worker.send( - [this]() { - EnvironmentScope scope(env, ThreadType::TileWorker, "TileWorker_" + name); - parse(); - }, - [tile, callback]() { - // `tile` is bound in this lambda to ensure that if it's the last owning pointer, - // destruction happens on the map thread, not the worker thread. It is _not_ bound - // in the above lambda, because we do not want the possibility to arise that the - // after callback could execute and release the penultimate reference before the - // work callback has been destructed. - callback(); - }); + workRequest = worker.send([this] { parse(); }, callback); } diff --git a/src/mbgl/map/tile_data.hpp b/src/mbgl/map/tile_data.hpp index b85db7db7..14e937eec 100644 --- a/src/mbgl/map/tile_data.hpp +++ b/src/mbgl/map/tile_data.hpp @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -21,8 +22,7 @@ class StyleLayer; class Request; class Worker; -class TileData : public std::enable_shared_from_this, - private util::noncopyable { +class TileData : private util::noncopyable { public: enum class State { invalid, @@ -60,6 +60,8 @@ protected: Request *req = nullptr; std::string data; + WorkRequest workRequest; + // Contains the tile ID string for painting debug information. DebugFontBuffer debugFontBuffer; diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp index 4c6c1150b..e10e2f826 100644 --- a/src/mbgl/map/vector_tile_data.cpp +++ b/src/mbgl/map/vector_tile_data.cpp @@ -28,6 +28,9 @@ VectorTileData::VectorTileData(const TileID& id_, } VectorTileData::~VectorTileData() { + // Cancel in most derived class destructor so that worker tasks are joined before + // any member data goes away. + cancel(); glyphAtlas.removeGlyphs(reinterpret_cast(this)); } diff --git a/src/mbgl/util/run_loop.hpp b/src/mbgl/util/run_loop.hpp index 767a47b75..ed2b364ce 100644 --- a/src/mbgl/util/run_loop.hpp +++ b/src/mbgl/util/run_loop.hpp @@ -33,21 +33,10 @@ public: RunLoop* outer = current.get(); assert(outer); - invoke([fn, callback, outer] { - /* - With C++14, we could write: - - outer->invoke([cb = std::move(callback), result = std::move(fn())] () mutable { - cb(std::move(result)); - }); - - Instead we're using a workaround with std::bind - to obtain move-capturing semantics with C++11: - http://stackoverflow.com/a/12744730/52207 - */ - outer->invoke(std::bind([] (std::function& cb, R& result) { - cb(std::move(result)); - }, std::move(callback), std::move(fn()))); + invoke([fn = std::move(fn), callback = std::move(callback), outer] () mutable { + outer->invoke([callback = std::move(callback), result = std::move(fn())] () mutable { + callback(std::move(result)); + }); }); } @@ -57,7 +46,7 @@ public: RunLoop* outer = current.get(); assert(outer); - invoke([fn, callback, outer] { + invoke([fn = std::move(fn), callback = std::move(callback), outer] () mutable { fn(); outer->invoke(std::move(callback)); }); diff --git a/src/mbgl/util/thread.hpp b/src/mbgl/util/thread.hpp index e97872a50..a2ad95864 100644 --- a/src/mbgl/util/thread.hpp +++ b/src/mbgl/util/thread.hpp @@ -56,13 +56,13 @@ public: // Invoke object->fn(args...) in the runloop thread, then invoke callback(result) in the current thread. template void invokeWithResult(Fn fn, std::function&& callback, Args&&... args) { - loop->invokeWithResult(std::bind(fn, object, args...), std::move(callback)); + loop->invokeWithResult(std::bind(fn, object, std::move(args)...), std::move(callback)); } // Invoke object->fn(args...) in the runloop thread, then invoke callback() in the current thread. template void invokeWithResult(Fn fn, std::function&& callback, Args&&... args) { - loop->invokeWithResult(std::bind(fn, object, args...), std::move(callback)); + loop->invokeWithResult(std::bind(fn, object, std::move(args)...), std::move(callback)); } // Invoke object->fn(args...) in the runloop thread, and wait for the result. diff --git a/src/mbgl/util/work_request.cpp b/src/mbgl/util/work_request.cpp new file mode 100644 index 000000000..05fe53580 --- /dev/null +++ b/src/mbgl/util/work_request.cpp @@ -0,0 +1,34 @@ +#include + +namespace mbgl { + +WorkRequest::WorkRequest() = default; + +WorkRequest::WorkRequest(Future&& future) + : complete(std::move(future)) { +} + +WorkRequest::WorkRequest(WorkRequest&& o) + : complete(std::move(o.complete)) { +} + +WorkRequest::~WorkRequest() { + if (complete.valid()) { + complete.get(); + } +} + +WorkRequest& WorkRequest::operator=(WorkRequest&& o) { + complete = std::move(o.complete); + return *this; +} + +WorkRequest::operator bool() const { + return complete.valid(); +} + +void WorkRequest::join() { + complete.get(); +} + +} diff --git a/src/mbgl/util/work_request.hpp b/src/mbgl/util/work_request.hpp new file mode 100644 index 000000000..8c26abb60 --- /dev/null +++ b/src/mbgl/util/work_request.hpp @@ -0,0 +1,31 @@ +#ifndef MBGL_UTIL_WORK_REQUEST +#define MBGL_UTIL_WORK_REQUEST + +#include + +#include + +namespace mbgl { + +class WorkRequest : public util::noncopyable { +public: + using Future = std::future; + + WorkRequest(); + WorkRequest(Future&&); + WorkRequest(WorkRequest&&); + ~WorkRequest(); + + WorkRequest& operator=(WorkRequest&&); + operator bool() const; + + // Wait for the worker task to complete. + void join(); + +private: + Future complete; +}; + +} + +#endif diff --git a/src/mbgl/util/worker.cpp b/src/mbgl/util/worker.cpp index 9792f1a09..7f5620e1f 100644 --- a/src/mbgl/util/worker.cpp +++ b/src/mbgl/util/worker.cpp @@ -2,6 +2,7 @@ #include #include +#include namespace mbgl { @@ -9,8 +10,8 @@ class Worker::Impl { public: Impl(uv_loop_t*) {} - void doWork(Fn work) { - work(); + void doWork(std::packaged_task& task) { + task(); } }; @@ -22,9 +23,14 @@ Worker::Worker(std::size_t count) { Worker::~Worker() = default; -void Worker::send(Fn&& work, Fn&& after) { - threads[current]->invokeWithResult(&Worker::Impl::doWork, std::move(after), std::move(work)); +WorkRequest Worker::send(Fn work, Fn after) { + std::packaged_task task(work); + std::future future = task.get_future(); + + threads[current]->invokeWithResult(&Worker::Impl::doWork, std::move(after), task); + current = (current + 1) % threads.size(); + return WorkRequest(std::move(future)); } } diff --git a/src/mbgl/util/worker.hpp b/src/mbgl/util/worker.hpp index d8fbf6df7..c4a4c376e 100644 --- a/src/mbgl/util/worker.hpp +++ b/src/mbgl/util/worker.hpp @@ -2,6 +2,7 @@ #define MBGL_UTIL_WORKER #include +#include #include #include @@ -15,7 +16,15 @@ public: Worker(std::size_t count); ~Worker(); - void send(Fn&& work, Fn&& after); + // Request work be done on a thread pool. The optional after callback is + // executed on the invoking thread, which must have a run loop, after the + // work is complete. + // + // The return value represents the request to perform the work asynchronously. + // Its destructor guarantees that the work is either cancelled and will never + // execute, or has finished executing. In other words, the WorkRequest is + // guaranteed to outlive any references held by the work function. + WorkRequest send(Fn work, Fn after); private: class Impl;