diff options
author | John Firebaugh <john.firebaugh@gmail.com> | 2015-05-02 13:31:56 -0400 |
---|---|---|
committer | John Firebaugh <john.firebaugh@gmail.com> | 2015-05-04 15:09:19 -0700 |
commit | 5c7867f435f9d94109ce11fcbbd153b016e02fe0 (patch) | |
tree | 5879bdf22b3f8d757c730378b0a2dc49a81fa15d /src | |
parent | e476fcbf3be861b4b080d647fd7d7fa71570c86a (diff) | |
download | qtlocation-mapboxgl-5c7867f435f9d94109ce11fcbbd153b016e02fe0.tar.gz |
Join worker tasks before destroying TileData
Fixes #1309
Diffstat (limited to 'src')
-rw-r--r-- | src/mbgl/map/live_tile_data.cpp | 6 | ||||
-rw-r--r-- | src/mbgl/map/raster_tile_data.cpp | 3 | ||||
-rw-r--r-- | src/mbgl/map/tile_data.cpp | 18 | ||||
-rw-r--r-- | src/mbgl/map/tile_data.hpp | 6 | ||||
-rw-r--r-- | src/mbgl/map/vector_tile_data.cpp | 3 | ||||
-rw-r--r-- | src/mbgl/util/run_loop.hpp | 21 | ||||
-rw-r--r-- | src/mbgl/util/thread.hpp | 4 | ||||
-rw-r--r-- | src/mbgl/util/work_request.cpp | 34 | ||||
-rw-r--r-- | src/mbgl/util/work_request.hpp | 31 | ||||
-rw-r--r-- | src/mbgl/util/worker.cpp | 14 | ||||
-rw-r--r-- | src/mbgl/util/worker.hpp | 11 |
11 files changed, 111 insertions, 40 deletions
diff --git a/src/mbgl/map/live_tile_data.cpp b/src/mbgl/map/live_tile_data.cpp index 1582b0190e..096e899d8f 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 0b849ca7cb..20c07b573d 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 80fd6346d3..dd51cb841f 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<void()> callback) { - util::ptr<TileData> 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 b85db7db78..14e937eec1 100644 --- a/src/mbgl/map/tile_data.hpp +++ b/src/mbgl/map/tile_data.hpp @@ -7,6 +7,7 @@ #include <mbgl/util/noncopyable.hpp> #include <mbgl/util/ptr.hpp> +#include <mbgl/util/work_request.hpp> #include <atomic> #include <string> @@ -21,8 +22,7 @@ class StyleLayer; class Request; class Worker; -class TileData : public std::enable_shared_from_this<TileData>, - 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 4c6c1150bd..e10e2f826a 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<uintptr_t>(this)); } diff --git a/src/mbgl/util/run_loop.hpp b/src/mbgl/util/run_loop.hpp index 767a47b754..ed2b364cef 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<void (R)>& 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 e97872a502..a2ad958645 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 <typename Fn, class R, class... Args> void invokeWithResult(Fn fn, std::function<void (R)>&& 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 <typename Fn, class... Args> void invokeWithResult(Fn fn, std::function<void ()>&& 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 0000000000..05fe535802 --- /dev/null +++ b/src/mbgl/util/work_request.cpp @@ -0,0 +1,34 @@ +#include <mbgl/util/work_request.hpp> + +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 0000000000..8c26abb60f --- /dev/null +++ b/src/mbgl/util/work_request.hpp @@ -0,0 +1,31 @@ +#ifndef MBGL_UTIL_WORK_REQUEST +#define MBGL_UTIL_WORK_REQUEST + +#include <mbgl/util/noncopyable.hpp> + +#include <future> + +namespace mbgl { + +class WorkRequest : public util::noncopyable { +public: + using Future = std::future<void>; + + 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 9792f1a099..7f5620e1fa 100644 --- a/src/mbgl/util/worker.cpp +++ b/src/mbgl/util/worker.cpp @@ -2,6 +2,7 @@ #include <mbgl/platform/platform.hpp> #include <cassert> +#include <future> namespace mbgl { @@ -9,8 +10,8 @@ class Worker::Impl { public: Impl(uv_loop_t*) {} - void doWork(Fn work) { - work(); + void doWork(std::packaged_task<void ()>& 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<void ()> task(work); + std::future<void> 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 d8fbf6df7d..c4a4c376e2 100644 --- a/src/mbgl/util/worker.hpp +++ b/src/mbgl/util/worker.hpp @@ -2,6 +2,7 @@ #define MBGL_UTIL_WORKER #include <mbgl/util/noncopyable.hpp> +#include <mbgl/util/work_request.hpp> #include <mbgl/util/thread.hpp> #include <functional> @@ -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; |