From 4fff8a5d33852c7bc4c5d8c989092eb42769a71d Mon Sep 17 00:00:00 2001 From: mwilsnd <53413200+mwilsnd@users.noreply.github.com> Date: Wed, 28 Feb 2024 17:18:08 -0500 Subject: [PATCH] Asynchronous GeometryTile deletion (#2051) Co-authored-by: Tim Sylvester --- CMakeLists.txt | 7 +- bazel/core.bzl | 16 +-- include/mbgl/actor/actor.hpp | 2 +- include/mbgl/actor/mailbox.hpp | 12 +- include/mbgl/actor/scheduler.hpp | 18 ++- include/mbgl/gfx/context.hpp | 6 +- include/mbgl/util/run_loop.hpp | 4 +- include/mbgl/util/scoped.hpp | 21 ++++ include/mbgl/util/string.hpp | 10 +- .../src/cpp/map_renderer.cpp | 19 ++- .../src/cpp/map_renderer.hpp | 6 +- .../android/maps/renderer/MapRenderer.java | 5 + .../maps/renderer/MapRendererScheduler.java | 5 + .../GLSurfaceViewMapRenderer.java | 7 ++ .../glsurfaceview/MapLibreGLSurfaceView.java | 43 +++++++ .../textureview/TextureViewMapRenderer.java | 9 ++ .../textureview/TextureViewRenderThread.java | 33 ++++++ .../android/maps/NativeMapViewTest.kt | 5 + platform/android/src/run_loop.cpp | 39 +++++- platform/android/src/run_loop_impl.hpp | 3 + platform/darwin/src/run_loop.cpp | 25 +++- platform/default/src/mbgl/util/run_loop.cpp | 24 +++- .../default/src/mbgl/util/thread_local.cpp | 4 +- platform/qt/src/mbgl/run_loop.cpp | 20 ++++ platform/qt/src/mbgl/thread_local.cpp | 4 +- platform/qt/src/utils/scheduler.cpp | 26 +++- platform/qt/src/utils/scheduler.hpp | 8 +- platform/windows/src/thread_local.cpp | 5 +- src/mbgl/actor/mailbox.cpp | 39 ++++++ .../annotation/render_annotation_source.cpp | 5 +- .../annotation/render_annotation_source.hpp | 2 +- src/mbgl/gl/context.cpp | 5 + src/mbgl/mtl/context.cpp | 7 +- src/mbgl/renderer/image_manager.cpp | 38 +++++- src/mbgl/renderer/image_manager.hpp | 10 +- src/mbgl/renderer/render_orchestrator.cpp | 16 ++- src/mbgl/renderer/render_orchestrator.hpp | 6 +- src/mbgl/renderer/render_source.cpp | 23 ++-- src/mbgl/renderer/render_source.hpp | 19 +-- .../sources/render_custom_geometry_source.cpp | 5 +- .../sources/render_custom_geometry_source.hpp | 2 +- .../sources/render_geojson_source.cpp | 5 +- .../sources/render_geojson_source.hpp | 2 +- .../sources/render_raster_dem_source.cpp | 5 +- .../sources/render_raster_dem_source.hpp | 2 +- .../renderer/sources/render_raster_source.cpp | 5 +- .../renderer/sources/render_raster_source.hpp | 2 +- .../renderer/sources/render_tile_source.cpp | 7 +- .../renderer/sources/render_tile_source.hpp | 4 +- .../renderer/sources/render_vector_source.cpp | 5 +- .../renderer/sources/render_vector_source.hpp | 2 +- src/mbgl/renderer/tile_parameters.hpp | 4 +- src/mbgl/renderer/tile_pyramid.cpp | 26 ++-- src/mbgl/renderer/tile_pyramid.hpp | 2 +- src/mbgl/text/glyph_manager.cpp | 105 +++++++++-------- src/mbgl/text/glyph_manager.hpp | 3 + src/mbgl/tile/geometry_tile.cpp | 26 +++- src/mbgl/tile/geometry_tile.hpp | 10 +- src/mbgl/tile/geometry_tile_worker.cpp | 5 +- src/mbgl/tile/tile.hpp | 2 +- src/mbgl/tile/tile_cache.cpp | 66 +++++++++-- src/mbgl/tile/tile_cache.hpp | 22 +++- src/mbgl/tile/tile_loader.hpp | 17 +++ src/mbgl/tile/tile_loader_impl.hpp | 77 ++++++++---- src/mbgl/util/thread_local.hpp | 4 +- src/mbgl/util/thread_pool.cpp | 92 +++++++++++++-- src/mbgl/util/thread_pool.hpp | 74 ++++++++++-- test/BUILD.bazel | 4 +- test/actor/actor.test.cpp | 7 +- test/include/mbgl/test/vector_tile_test.hpp | 49 ++++++++ test/map/map.test.cpp | 3 + test/renderer/image_manager.test.cpp | 41 ++++--- test/style/source.test.cpp | 55 +++++---- test/tile/custom_geometry_tile.test.cpp | 4 +- test/tile/geojson_tile.test.cpp | 4 +- test/tile/raster_dem_tile.test.cpp | 4 +- test/tile/raster_tile.test.cpp | 4 +- test/tile/tile_cache.test.cpp | 67 ++++++----- test/tile/vector_tile.test.cpp | 24 +--- test/util/thread.test.cpp | 111 ++++++++++++++++++ 80 files changed, 1180 insertions(+), 334 deletions(-) create mode 100644 include/mbgl/util/scoped.hpp create mode 100644 test/include/mbgl/test/vector_tile_test.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 6fa5197b0da..8faf667cd32 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -158,7 +158,6 @@ if(MLN_DRAWABLE_RENDERER) ${PROJECT_SOURCE_DIR}/include/mbgl/renderer/layer_tweaker.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/renderer/render_target.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/shaders/shader_program_base.hpp - ${PROJECT_SOURCE_DIR}/include/mbgl/util/identity.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/suppress_copies.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/shaders/gl/shader_info.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/shaders/gl/shader_program_gl.hpp @@ -216,7 +215,6 @@ if(MLN_DRAWABLE_RENDERER) ${PROJECT_SOURCE_DIR}/src/mbgl/renderer/layers/collision_layer_tweaker.cpp ${PROJECT_SOURCE_DIR}/src/mbgl/renderer/layers/collision_layer_tweaker.hpp ${PROJECT_SOURCE_DIR}/src/mbgl/shaders/shader_program_base.cpp - ${PROJECT_SOURCE_DIR}/src/mbgl/util/identity.cpp ${PROJECT_SOURCE_DIR}/src/mbgl/shaders/gl/shader_program_gl.cpp ${PROJECT_SOURCE_DIR}/src/mbgl/gl/buffer_allocator.cpp ${PROJECT_SOURCE_DIR}/src/mbgl/gl/drawable_gl.cpp @@ -403,6 +401,7 @@ list(APPEND INCLUDE_FILES ${PROJECT_SOURCE_DIR}/include/mbgl/util/geo.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/geojson.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/geometry.hpp + ${PROJECT_SOURCE_DIR}/include/mbgl/util/identity.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/ignore.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/image.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/immutable.hpp @@ -415,6 +414,7 @@ list(APPEND INCLUDE_FILES ${PROJECT_SOURCE_DIR}/include/mbgl/util/projection.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/range.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/run_loop.hpp + ${PROJECT_SOURCE_DIR}/include/mbgl/util/scoped.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/size.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/string.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/string_indexer.hpp @@ -939,6 +939,7 @@ list(APPEND SRC_FILES ${PROJECT_SOURCE_DIR}/src/mbgl/util/http_timeout.hpp ${PROJECT_SOURCE_DIR}/src/mbgl/util/i18n.cpp ${PROJECT_SOURCE_DIR}/src/mbgl/util/i18n.hpp + ${PROJECT_SOURCE_DIR}/src/mbgl/util/identity.cpp ${PROJECT_SOURCE_DIR}/src/mbgl/util/interpolate.cpp ${PROJECT_SOURCE_DIR}/src/mbgl/util/intersection_tests.cpp ${PROJECT_SOURCE_DIR}/src/mbgl/util/intersection_tests.hpp @@ -1444,4 +1445,4 @@ endif() add_subdirectory(${PROJECT_SOURCE_DIR}/test) add_subdirectory(${PROJECT_SOURCE_DIR}/benchmark) -add_subdirectory(${PROJECT_SOURCE_DIR}/render-test) \ No newline at end of file +add_subdirectory(${PROJECT_SOURCE_DIR}/render-test) diff --git a/bazel/core.bzl b/bazel/core.bzl index 7fe2e6fcc77..06cd361eae3 100644 --- a/bazel/core.bzl +++ b/bazel/core.bzl @@ -141,7 +141,6 @@ MLN_CORE_SOURCE = [ "src/mbgl/geometry/line_atlas.hpp", "src/mbgl/gfx/attribute.cpp", "src/mbgl/gfx/attribute.hpp", - "include/mbgl/gfx/backend.hpp", "src/mbgl/gfx/color_mode.hpp", "src/mbgl/gfx/command_encoder.hpp", "src/mbgl/gfx/cull_face_mode.hpp", @@ -149,9 +148,11 @@ MLN_CORE_SOURCE = [ "src/mbgl/gfx/depth_mode.hpp", "src/mbgl/gfx/draw_mode.hpp", "src/mbgl/gfx/draw_scope.hpp", + "src/mbgl/gfx/fill_generator.cpp", "src/mbgl/gfx/index_buffer.hpp", "src/mbgl/gfx/index_vector.hpp", "src/mbgl/gfx/offscreen_texture.hpp", + "src/mbgl/gfx/polyline_generator.cpp", "src/mbgl/gfx/program.hpp", "src/mbgl/gfx/render_pass.hpp", "src/mbgl/gfx/renderbuffer.hpp", @@ -598,6 +599,7 @@ MLN_CORE_SOURCE = [ "src/mbgl/util/http_timeout.hpp", "src/mbgl/util/i18n.cpp", "src/mbgl/util/i18n.hpp", + "src/mbgl/util/identity.cpp", "src/mbgl/util/interpolate.cpp", "src/mbgl/util/intersection_tests.cpp", "src/mbgl/util/intersection_tests.hpp", @@ -618,8 +620,6 @@ MLN_CORE_SOURCE = [ "src/mbgl/util/premultiply.cpp", "src/mbgl/util/quaternion.cpp", "src/mbgl/util/quaternion.hpp", - "src/mbgl/gfx/polyline_generator.cpp", - "src/mbgl/gfx/fill_generator.cpp", "src/mbgl/util/rapidjson.cpp", "src/mbgl/util/rapidjson.hpp", "src/mbgl/util/rect.hpp", @@ -661,8 +661,11 @@ MLN_CORE_HEADERS = [ "include/mbgl/actor/message.hpp", "include/mbgl/actor/scheduler.hpp", "include/mbgl/annotation/annotation.hpp", + "include/mbgl/gfx/backend.hpp", "include/mbgl/gfx/backend_scope.hpp", + "include/mbgl/gfx/fill_generator.hpp", "include/mbgl/gfx/gfx_types.hpp", + "include/mbgl/gfx/polyline_generator.hpp", "include/mbgl/gfx/renderable.hpp", "include/mbgl/gfx/renderer_backend.hpp", "include/mbgl/gfx/rendering_stats.hpp", @@ -820,6 +823,7 @@ MLN_CORE_HEADERS = [ "include/mbgl/util/geo.hpp", "include/mbgl/util/geojson.hpp", "include/mbgl/util/geometry.hpp", + "include/mbgl/util/identity.hpp", "include/mbgl/util/ignore.hpp", "include/mbgl/util/image.hpp", "include/mbgl/util/immutable.hpp", @@ -829,12 +833,11 @@ MLN_CORE_HEADERS = [ "include/mbgl/util/monotonic_timer.hpp", "include/mbgl/util/noncopyable.hpp", "include/mbgl/util/platform.hpp", - "include/mbgl/gfx/polyline_generator.hpp", - "include/mbgl/gfx/fill_generator.hpp", "include/mbgl/util/premultiply.hpp", "include/mbgl/util/projection.hpp", "include/mbgl/util/range.hpp", "include/mbgl/util/run_loop.hpp", + "include/mbgl/util/scoped.hpp", "include/mbgl/util/size.hpp", "include/mbgl/util/string.hpp", "include/mbgl/util/string_indexer.hpp", @@ -905,7 +908,6 @@ MLN_OPENGL_SOURCE = [ ] MLN_OPENGL_HEADERS = [ - "include/mbgl/gfx/backend.hpp", "include/mbgl/gl/renderable_resource.hpp", "include/mbgl/gl/renderer_backend.hpp", "include/mbgl/layermanager/location_indicator_layer_factory.hpp", @@ -957,7 +959,6 @@ MLN_DRAWABLES_SOURCE = [ "src/mbgl/renderer/layers/collision_layer_tweaker.cpp", "src/mbgl/renderer/layers/collision_layer_tweaker.hpp", "src/mbgl/shaders/shader_program_base.cpp", - "src/mbgl/util/identity.cpp", "src/mbgl/style/layers/custom_drawable_layer.cpp", "src/mbgl/layermanager/custom_drawable_layer_factory.cpp", "src/mbgl/style/layers/custom_drawable_layer_impl.cpp", @@ -999,7 +1000,6 @@ MLN_DRAWABLES_HEADERS = [ "include/mbgl/shaders/shader_defines.hpp", "include/mbgl/shaders/shader_program_base.hpp", "include/mbgl/shaders/symbol_layer_ubo.hpp", - "include/mbgl/util/identity.hpp", "include/mbgl/util/suppress_copies.hpp", "include/mbgl/style/layers/custom_drawable_layer.hpp", "include/mbgl/layermanager/custom_drawable_layer_factory.hpp", diff --git a/include/mbgl/actor/actor.hpp b/include/mbgl/actor/actor.hpp index 7ecc559d57e..af42bec7b97 100644 --- a/include/mbgl/actor/actor.hpp +++ b/include/mbgl/actor/actor.hpp @@ -71,7 +71,7 @@ class Actor { ActorRef> self() { return parent.self(); } private: - std::shared_ptr retainer; + const std::shared_ptr retainer; AspiringActor parent; EstablishedActor target; }; diff --git a/include/mbgl/actor/mailbox.hpp b/include/mbgl/actor/mailbox.hpp index 782af736608..b558062aeb3 100644 --- a/include/mbgl/actor/mailbox.hpp +++ b/include/mbgl/actor/mailbox.hpp @@ -1,5 +1,5 @@ #pragma once - +#include #include #include #include @@ -28,6 +28,9 @@ class Mailbox : public std::enable_shared_from_this { void open(Scheduler& scheduler_); void close(); + // Indicate this mailbox will no longer be checked for messages + void abandon(); + bool isOpen() const; void push(std::unique_ptr); @@ -37,11 +40,18 @@ class Mailbox : public std::enable_shared_from_this { static std::function makeClosure(std::weak_ptr); private: + enum class State : uint32_t { + Idle = 0, + Processing, + Abandoned + }; + mapbox::base::WeakPtr weakScheduler; std::recursive_mutex receivingMutex; std::mutex pushingMutex; + std::atomic state{State::Idle}; bool closed{false}; std::mutex queueMutex; diff --git a/include/mbgl/actor/scheduler.hpp b/include/mbgl/actor/scheduler.hpp index 8d4fa187d75..3f99e11c4ea 100644 --- a/include/mbgl/actor/scheduler.hpp +++ b/include/mbgl/actor/scheduler.hpp @@ -1,5 +1,7 @@ #pragma once +#include + #include #include @@ -36,9 +38,12 @@ class Scheduler { virtual ~Scheduler() = default; /// Enqueues a function for execution. - virtual void schedule(std::function) = 0; + virtual void schedule(std::function&&) = 0; /// Makes a weak pointer to this Scheduler. virtual mapbox::base::WeakPtr makeWeakPtr() = 0; + /// Enqueues a function for execution on the render thread. + virtual void runOnRenderThread(std::function&&){}; + virtual void runRenderJobs() {} /// Returns a closure wrapping the given one. /// @@ -62,6 +67,11 @@ class Scheduler { scheduleAndReplyValue(task, reply, GetCurrent()->makeWeakPtr()); } + /// Wait until there's nothing pending or in process + /// Must not be called from a task provided to this scheduler. + /// @param timeout Time to wait, or zero to wait forever. + virtual std::size_t waitForEmpty(Milliseconds timeout = Milliseconds{0}) = 0; + /// Set/Get the current Scheduler for this thread static Scheduler* GetCurrent(); static void SetCurrent(Scheduler*); @@ -84,6 +94,9 @@ class Scheduler { /// on the same thread-unsafe object. [[nodiscard]] static std::shared_ptr GetSequenced(); + /// Set a function to be called when an exception occurs on a thread controlled by the scheduler + void setExceptionHandler(std::function handler_) { handler = std::move(handler_); } + protected: template void scheduleAndReplyValue(const TaskFn& task, @@ -97,9 +110,10 @@ class Scheduler { }; replyScheduler->schedule(std::move(scheduledReply)); }; - schedule(std::move(scheduled)); } + + std::function handler; }; } // namespace mbgl diff --git a/include/mbgl/gfx/context.hpp b/include/mbgl/gfx/context.hpp index 3ec22e7b766..b6f95fe14f5 100644 --- a/include/mbgl/gfx/context.hpp +++ b/include/mbgl/gfx/context.hpp @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -53,7 +54,8 @@ using VertexAttributeArrayPtr = std::shared_ptr; class Context { protected: Context(uint32_t maximumVertexBindingCount_) - : maximumVertexBindingCount(maximumVertexBindingCount_) {} + : maximumVertexBindingCount(maximumVertexBindingCount_), + backgroundScheduler(Scheduler::GetBackground()) {} public: static constexpr const uint32_t minimumRequiredVertexBindingCount = 8; @@ -161,6 +163,8 @@ class Context { virtual std::unique_ptr createDrawScopeResource() = 0; gfx::RenderingStats stats; + + std::shared_ptr backgroundScheduler; }; } // namespace gfx diff --git a/include/mbgl/util/run_loop.hpp b/include/mbgl/util/run_loop.hpp index 7e1d94332a9..841814638ab 100644 --- a/include/mbgl/util/run_loop.hpp +++ b/include/mbgl/util/run_loop.hpp @@ -78,9 +78,11 @@ class RunLoop : public Scheduler, private util::noncopyable { return std::make_unique(task); } - void schedule(std::function fn) override { invoke(std::move(fn)); } + void schedule(std::function&& fn) override { invoke(std::move(fn)); } ::mapbox::base::WeakPtr makeWeakPtr() override { return weakFactory.makeWeakPtr(); } + std::size_t waitForEmpty(Milliseconds timeout) override; + class Impl; private: diff --git a/include/mbgl/util/scoped.hpp b/include/mbgl/util/scoped.hpp new file mode 100644 index 00000000000..40766371986 --- /dev/null +++ b/include/mbgl/util/scoped.hpp @@ -0,0 +1,21 @@ +#pragma once + +namespace mbgl { + +/// Run a lambda on scope exit +template +struct Scoped { + Scoped(Func&& fn) + : cb(std::move(fn)){}; + ~Scoped() { cb(); } + + Scoped(const Scoped&) = delete; + Scoped(Scoped&&) noexcept = delete; + Scoped& operator=(const Scoped&) = delete; + Scoped& operator=(Scoped&&) noexcept = delete; + +private: + Func cb; +}; + +} // namespace mbgl diff --git a/include/mbgl/util/string.hpp b/include/mbgl/util/string.hpp index 70e1212f949..a6d4d2e4c5f 100644 --- a/include/mbgl/util/string.hpp +++ b/include/mbgl/util/string.hpp @@ -1,10 +1,12 @@ #pragma once -#include #include #include -#include #include +#include +#include +#include +#include // Polyfill needed by Qt when building for Android with GCC #if defined(__ANDROID__) && defined(__GLIBCXX__) @@ -76,6 +78,10 @@ inline std::string toString(long double t, bool decimal = false) { return toString(static_cast(t), decimal); } +inline std::string toString(std::thread::id threadId) { + return (std::ostringstream() << threadId).str(); +} + std::string toString(const std::exception_ptr &); template diff --git a/platform/android/MapboxGLAndroidSDK/src/cpp/map_renderer.cpp b/platform/android/MapboxGLAndroidSDK/src/cpp/map_renderer.cpp index 3ea039d1fda..472b96d14c2 100644 --- a/platform/android/MapboxGLAndroidSDK/src/cpp/map_renderer.cpp +++ b/platform/android/MapboxGLAndroidSDK/src/cpp/map_renderer.cpp @@ -60,7 +60,7 @@ ActorRef MapRenderer::actor() const { return *rendererRef; } -void MapRenderer::schedule(std::function scheduled) { +void MapRenderer::schedule(std::function&& scheduled) { try { // Create a runnable android::UniqueEnv _env = android::AttachEnv(); @@ -84,6 +84,23 @@ void MapRenderer::schedule(std::function scheduled) { } } +std::size_t MapRenderer::waitForEmpty(Milliseconds timeout) { + try { + android::UniqueEnv _env = android::AttachEnv(); + static auto& javaClass = jni::Class::Singleton(*_env); + static auto waitForEmpty = javaClass.GetMethod(*_env, "waitForEmpty"); + if (auto weakReference = javaPeer.get(*_env)) { + return weakReference.Call(*_env, waitForEmpty, static_cast(timeout.count())); + } + // If the peer is already cleaned up, there's nothing to wait for + return 0; + } catch (...) { + Log::Error(Event::Android, "MapRenderer::waitForEmpty failed"); + jni::ThrowJavaError(*android::AttachEnv(), std::current_exception()); + return 0; + } +} + void MapRenderer::requestRender() { try { android::UniqueEnv _env = android::AttachEnv(); diff --git a/platform/android/MapboxGLAndroidSDK/src/cpp/map_renderer.hpp b/platform/android/MapboxGLAndroidSDK/src/cpp/map_renderer.hpp index 25ffc60695d..4ecb8f5d94b 100644 --- a/platform/android/MapboxGLAndroidSDK/src/cpp/map_renderer.hpp +++ b/platform/android/MapboxGLAndroidSDK/src/cpp/map_renderer.hpp @@ -66,9 +66,13 @@ class MapRenderer : public Scheduler { // From Scheduler. Schedules by using callbacks to the // JVM to process the mailbox on the right thread. - void schedule(std::function scheduled) override; + void schedule(std::function&& scheduled) override; mapbox::base::WeakPtr makeWeakPtr() override { return weakFactory.makeWeakPtr(); } + // Wait for the queue to be empty + // A timeout of zero results in an unbounded wait + std::size_t waitForEmpty(Milliseconds timeout) override; + void requestRender(); // Snapshot - requires a RunLoop on the calling thread diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/MapRenderer.java b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/MapRenderer.java index 7be40232479..72c539040de 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/MapRenderer.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/MapRenderer.java @@ -120,6 +120,11 @@ void queueEvent(MapRendererRunnable runnable) { this.queueEvent((Runnable) runnable); } + /// Wait indefinitely for the queue to become empty + public void waitForEmpty() { + waitForEmpty(0); + } + private native void nativeInitialize(MapRenderer self, float pixelRatio, String localIdeographFontFamily); diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/MapRendererScheduler.java b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/MapRendererScheduler.java index 145ae6397fc..f1e17e658f7 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/MapRendererScheduler.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/MapRendererScheduler.java @@ -14,4 +14,9 @@ public interface MapRendererScheduler { @Keep void queueEvent(Runnable runnable); + @Keep + void waitForEmpty(); + + @Keep + long waitForEmpty(long timeoutMillis); } diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/glsurfaceview/GLSurfaceViewMapRenderer.java b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/glsurfaceview/GLSurfaceViewMapRenderer.java index ac803068388..a8a3c3fb41c 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/glsurfaceview/GLSurfaceViewMapRenderer.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/glsurfaceview/GLSurfaceViewMapRenderer.java @@ -116,4 +116,11 @@ public void queueEvent(Runnable runnable) { glSurfaceView.queueEvent(runnable); } + /** + * {@inheritDoc} + */ + @Override + public long waitForEmpty(long timeoutMillis) { + return glSurfaceView.waitForEmpty(timeoutMillis); + } } \ No newline at end of file diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/glsurfaceview/MapLibreGLSurfaceView.java b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/glsurfaceview/MapLibreGLSurfaceView.java index 111cbc9e5a6..a78a8a907a4 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/glsurfaceview/MapLibreGLSurfaceView.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/glsurfaceview/MapLibreGLSurfaceView.java @@ -317,6 +317,16 @@ public void queueEvent(Runnable r) { glThread.queueEvent(r); } + /** + * Wait for the queue to become empty + * @param timeoutMillis Timeout in milliseconds + * @return Number of queue items remaining + */ + public long waitForEmpty(long timeoutMillis) { + return glThread.waitForEmpty(timeoutMillis); + } + + /** * This method is used as part of the View class and is not normally * called or subclassed by clients of GLSurfaceView. @@ -1023,6 +1033,39 @@ public void queueEvent(@NonNull Runnable r) { } } + /** + * Wait for the queue to become empty + * @param timeoutMillis Timeout in milliseconds, zero for indefinite wait + * @return Number of queue items remaining + */ + public int waitForEmpty(long timeoutMillis) { + final long startTime = System.nanoTime(); + synchronized (glThreadManager) { + // Wait for the queue to be empty + while (!this.eventQueue.isEmpty()) { + if (timeoutMillis > 0) { + final long elapsedMillis = (System.nanoTime() - startTime) / 1000 / 1000; + if (elapsedMillis < timeoutMillis) { + try { + glThreadManager.wait(timeoutMillis - elapsedMillis); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + } + } else { + break; + } + } else { + try { + glThreadManager.wait(); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + } + } + } + return this.eventQueue.size(); + } + } + // Once the thread is started, all accesses to the following member // variables are protected by the sGLThreadManager monitor private boolean shouldExit; diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/textureview/TextureViewMapRenderer.java b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/textureview/TextureViewMapRenderer.java index 420b23d10a4..ee20aae6089 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/textureview/TextureViewMapRenderer.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/textureview/TextureViewMapRenderer.java @@ -35,6 +35,7 @@ public TextureViewMapRenderer(@NonNull Context context, super(context, localIdeographFontFamily); this.translucentSurface = translucentSurface; renderThread = new TextureViewRenderThread(textureView, this); + renderThread.setName("TextureViewRenderer"); renderThread.start(); } @@ -86,6 +87,14 @@ public void queueEvent(Runnable runnable) { renderThread.queueEvent(runnable); } + /** + * {@inheritDoc} + */ + @Override + public long waitForEmpty(long timeoutMillis) { + return renderThread.waitForEmpty(timeoutMillis); + } + /** * {@inheritDoc} */ diff --git a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/textureview/TextureViewRenderThread.java b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/textureview/TextureViewRenderThread.java index 5f5772af479..51598b967ee 100644 --- a/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/textureview/TextureViewRenderThread.java +++ b/platform/android/MapboxGLAndroidSDK/src/main/java/org/maplibre/android/maps/renderer/textureview/TextureViewRenderThread.java @@ -135,6 +135,39 @@ void queueEvent(@NonNull Runnable runnable) { } } + /** + * Wait for the queue to be empty. + * @param timeoutMillis Maximum time to wait, in milliseconds + * @return The number of items remaining in the queue + */ + @UiThread + int waitForEmpty(long timeoutMillis) { + final long startTime = System.nanoTime(); + synchronized (lock) { + // Wait for the queue to be empty + while (!this.eventQueue.isEmpty()) { + if (timeoutMillis > 0) { + final long elapsedMillis = (System.nanoTime() - startTime) / 1000 / 1000; + if (elapsedMillis < timeoutMillis) { + try { + lock.wait(timeoutMillis - elapsedMillis); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + } + } else { + break; + } + } else { + try { + lock.wait(0); + } catch (InterruptedException ex) { + Thread.currentThread().interrupt(); + } + } + } + return this.eventQueue.size(); + } + } @UiThread void onPause() { diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/org/maplibre/android/maps/NativeMapViewTest.kt b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/org/maplibre/android/maps/NativeMapViewTest.kt index fafcac7d1a3..307bd4a8611 100644 --- a/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/org/maplibre/android/maps/NativeMapViewTest.kt +++ b/platform/android/MapboxGLAndroidSDKTestApp/src/androidTest/java/org/maplibre/android/maps/NativeMapViewTest.kt @@ -442,5 +442,10 @@ class NativeMapViewTest : AppCenter() { override fun queueEvent(runnable: Runnable?) { // no-op } + + override fun waitForEmpty(timeoutMillis: Long): Long { + // no-op + return 0 + } } } diff --git a/platform/android/src/run_loop.cpp b/platform/android/src/run_loop.cpp index 8fcf2fb30e8..a26c91ead0d 100644 --- a/platform/android/src/run_loop.cpp +++ b/platform/android/src/run_loop.cpp @@ -1,11 +1,13 @@ #include "run_loop_impl.hpp" +#include +#include +#include +#include #include -#include #include +#include #include -#include -#include #include @@ -18,7 +20,6 @@ #include #include -#include #define PIPE_OUT 0 #define PIPE_IN 1 @@ -167,6 +168,9 @@ void RunLoop::Impl::addRunnable(Runnable* runnable) { void RunLoop::Impl::removeRunnable(Runnable* runnable) { std::lock_guard lock(mutex); runnables.remove(runnable); + if (runnables.empty()) { + cvEmpty.notify_all(); + } } Milliseconds RunLoop::Impl::processRunnables() { @@ -196,6 +200,10 @@ Milliseconds RunLoop::Impl::processRunnables() { runnable->runTask(); } + if (runnables.empty()) { + cvEmpty.notify_all(); + } + if (runnables.empty() || nextDue == TimePoint::max()) { return Milliseconds(-1); } @@ -208,6 +216,25 @@ Milliseconds RunLoop::Impl::processRunnables() { return timeout; } +std::size_t RunLoop::Impl::waitForEmpty(Milliseconds timeout) { + const auto startTime = mbgl::util::MonotonicTimer::now(); + while (true) { + std::size_t remaining; + { + std::lock_guard lock(mutex); + remaining = runnables.size(); + } + + const auto elapsed = mbgl::util::MonotonicTimer::now() - startTime; + const auto elapsedMillis = std::chrono::duration_cast(elapsed); + if (remaining == 0 || (Milliseconds::zero() < timeout && timeout <= elapsedMillis)) { + return remaining; + } + + runLoop->runOnce(); + } +} + RunLoop* RunLoop::Get() { assert(static_cast(Scheduler::GetCurrent())); return static_cast(Scheduler::GetCurrent()); @@ -230,6 +257,10 @@ void RunLoop::wake() { impl->wake(); } +std::size_t RunLoop::waitForEmpty(std::chrono::milliseconds timeout) { + return impl->waitForEmpty(timeout); +} + void RunLoop::run() { MBGL_VERIFY_THREAD(tid); diff --git a/platform/android/src/run_loop_impl.hpp b/platform/android/src/run_loop_impl.hpp index 8bbf6dfc2e3..ff2420cd02e 100644 --- a/platform/android/src/run_loop_impl.hpp +++ b/platform/android/src/run_loop_impl.hpp @@ -42,6 +42,8 @@ class RunLoop::Impl { Milliseconds processRunnables(); + std::size_t waitForEmpty(Milliseconds timeout); + ALooper* loop = nullptr; RunLoop* runLoop = nullptr; std::atomic running; @@ -57,6 +59,7 @@ class RunLoop::Impl { std::unique_ptr> alarm; std::mutex mutex; + std::condition_variable cvEmpty; std::list runnables; }; diff --git a/platform/darwin/src/run_loop.cpp b/platform/darwin/src/run_loop.cpp index 0e22dc9e337..bf5bf71d0bb 100644 --- a/platform/darwin/src/run_loop.cpp +++ b/platform/darwin/src/run_loop.cpp @@ -1,6 +1,8 @@ -#include -#include #include +#include +#include +#include +#include #include @@ -45,5 +47,24 @@ void RunLoop::stop() { invoke([&] { CFRunLoopStop(CFRunLoopGetCurrent()); }); } +std::size_t RunLoop::waitForEmpty(Milliseconds timeout) { + const auto startTime = mbgl::util::MonotonicTimer::now(); + while (true) { + std::size_t remaining; + { + std::lock_guard lock(mutex); + remaining = defaultQueue.size() + highPriorityQueue.size(); + } + + const auto elapsed = mbgl::util::MonotonicTimer::now() - startTime; + const auto elapsedMillis = std::chrono::duration_cast(elapsed); + if (remaining == 0 || (Milliseconds::zero() < timeout && timeout <= elapsedMillis)) { + return remaining; + } + + runOnce(); + } +} + } // namespace util } // namespace mbgl diff --git a/platform/default/src/mbgl/util/run_loop.cpp b/platform/default/src/mbgl/util/run_loop.cpp index 1a5939ec14b..39d31d87b74 100644 --- a/platform/default/src/mbgl/util/run_loop.cpp +++ b/platform/default/src/mbgl/util/run_loop.cpp @@ -1,7 +1,8 @@ -#include +#include #include +#include +#include #include -#include #include @@ -148,6 +149,25 @@ void RunLoop::stop() { invoke([&] { uv_unref(impl->holderHandle()); }); } +std::size_t RunLoop::waitForEmpty(Milliseconds timeout) { + const auto startTime = mbgl::util::MonotonicTimer::now(); + while (true) { + std::size_t remaining; + { + std::lock_guard lock(mutex); + remaining = defaultQueue.size() + highPriorityQueue.size(); + } + + const auto elapsed = mbgl::util::MonotonicTimer::now() - startTime; + const auto elapsedMillis = std::chrono::duration_cast(elapsed); + if (remaining == 0 || (Milliseconds::zero() < timeout && timeout <= elapsedMillis)) { + return remaining; + } + + runOnce(); + } +} + void RunLoop::addWatch(int fd, Event event, std::function&& callback) { MBGL_VERIFY_THREAD(tid); diff --git a/platform/default/src/mbgl/util/thread_local.cpp b/platform/default/src/mbgl/util/thread_local.cpp index e8d9d93715a..3ab51d92349 100644 --- a/platform/default/src/mbgl/util/thread_local.cpp +++ b/platform/default/src/mbgl/util/thread_local.cpp @@ -30,8 +30,8 @@ ThreadLocalBase::~ThreadLocalBase() { } } -void* ThreadLocalBase::get() { - return pthread_getspecific(reinterpret_cast(storage)); +void* ThreadLocalBase::get() const { + return pthread_getspecific(reinterpret_cast(storage)); } void ThreadLocalBase::set(void* ptr) { diff --git a/platform/qt/src/mbgl/run_loop.cpp b/platform/qt/src/mbgl/run_loop.cpp index 6bd9cb035c5..c978bd81ea5 100644 --- a/platform/qt/src/mbgl/run_loop.cpp +++ b/platform/qt/src/mbgl/run_loop.cpp @@ -1,6 +1,7 @@ #include "run_loop_impl.hpp" #include +#include #include @@ -90,6 +91,25 @@ void RunLoop::runOnce() { } } +std::size_t RunLoop::waitForEmpty(std::chrono::milliseconds timeout) { + const auto startTime = mbgl::util::MonotonicTimer::now(); + while (true) { + std::size_t remaining; + { + std::lock_guard lock(mutex); + remaining = defaultQueue.size() + highPriorityQueue.size(); + } + + const auto elapsed = mbgl::util::MonotonicTimer::now() - startTime; + const auto elapsedMillis = std::chrono::duration_cast(elapsed); + if (remaining == 0 || timeout <= elapsedMillis) { + return remaining; + } + + runOnce(); + } +} + void RunLoop::addWatch(int fd, Event event, std::function&& cb) { MBGL_VERIFY_THREAD(tid); diff --git a/platform/qt/src/mbgl/thread_local.cpp b/platform/qt/src/mbgl/thread_local.cpp index 1f69a8aac36..88da18079df 100644 --- a/platform/qt/src/mbgl/thread_local.cpp +++ b/platform/qt/src/mbgl/thread_local.cpp @@ -27,8 +27,8 @@ ThreadLocalBase::~ThreadLocalBase() { reinterpret_cast(storage).~QThreadStorage(); } -void* ThreadLocalBase::get() { - return reinterpret_cast(storage).localData()[0]; +void* ThreadLocalBase::get() const { + return reinterpret_cast(storage).localData()[0]; } void ThreadLocalBase::set(void* ptr) { diff --git a/platform/qt/src/utils/scheduler.cpp b/platform/qt/src/utils/scheduler.cpp index 0c450a1bc63..45e83926151 100644 --- a/platform/qt/src/utils/scheduler.cpp +++ b/platform/qt/src/utils/scheduler.cpp @@ -1,5 +1,6 @@ #include "scheduler.hpp" +#include #include #include @@ -12,7 +13,7 @@ Scheduler::~Scheduler() { MBGL_VERIFY_THREAD(tid); } -void Scheduler::schedule(std::function function) { +void Scheduler::schedule(std::function&& function) { const std::lock_guard lock(m_taskQueueMutex); m_taskQueue.push(std::move(function)); @@ -26,6 +27,7 @@ void Scheduler::processEvents() { { const std::unique_lock lock(m_taskQueueMutex); std::swap(taskQueue, m_taskQueue); + pendingItems += taskQueue.size(); } while (!taskQueue.empty()) { @@ -34,7 +36,29 @@ void Scheduler::processEvents() { function(); } taskQueue.pop(); + pendingItems--; } + + cvEmpty.notify_all(); +} + +std::size_t Scheduler::waitForEmpty(std::chrono::milliseconds timeout) { + MBGL_VERIFY_THREAD(tid); + + const auto startTime = mbgl::util::MonotonicTimer::now(); + std::unique_lock lock(m_taskQueueMutex); + const auto isDone = [&] { + return m_taskQueue.empty() && pendingItems == 0; + }; + while (!isDone()) { + const auto elapsed = mbgl::util::MonotonicTimer::now() - startTime; + if (timeout <= elapsed || !cvEmpty.wait_for(lock, timeout - elapsed, isDone)) { + assert(isDone()); + break; + } + } + + return m_taskQueue.size() + pendingItems; } } // namespace QMapLibre diff --git a/platform/qt/src/utils/scheduler.hpp b/platform/qt/src/utils/scheduler.hpp index f5dc1802846..56f9fd954d7 100644 --- a/platform/qt/src/utils/scheduler.hpp +++ b/platform/qt/src/utils/scheduler.hpp @@ -5,6 +5,7 @@ #include +#include #include #include #include @@ -19,7 +20,10 @@ class Scheduler : public QObject, public mbgl::Scheduler { ~Scheduler() override; // mbgl::Scheduler implementation. - void schedule(std::function function) final; + void schedule(std::function&& function) final; + + std::size_t waitForEmpty(std::chrono::milliseconds timeout) override; + mapbox::base::WeakPtr makeWeakPtr() override { return weakFactory.makeWeakPtr(); } void processEvents(); @@ -31,6 +35,8 @@ class Scheduler : public QObject, public mbgl::Scheduler { MBGL_STORE_THREAD(tid); std::mutex m_taskQueueMutex; + std::condition_variable cvEmpty; + std::atomic pendingItems; std::queue> m_taskQueue; mapbox::base::WeakPtrFactory weakFactory{this}; }; diff --git a/platform/windows/src/thread_local.cpp b/platform/windows/src/thread_local.cpp index 7e0b8b9c4bf..66c9f1efb6d 100644 --- a/platform/windows/src/thread_local.cpp +++ b/platform/windows/src/thread_local.cpp @@ -10,6 +10,7 @@ #include "thread.h" #define StorageToThreadInfo reinterpret_cast(storage) +#define StorageToConstThreadInfo reinterpret_cast(storage) namespace mbgl { namespace util { @@ -47,8 +48,8 @@ ThreadLocalBase::~ThreadLocalBase() { } } -void* ThreadLocalBase::get() { - return TlsGetValue(StorageToThreadInfo->key); +void* ThreadLocalBase::get() const { + return TlsGetValue(StorageToConstThreadInfo->key); } void ThreadLocalBase::set(void* ptr) { diff --git a/src/mbgl/actor/mailbox.cpp b/src/mbgl/actor/mailbox.cpp index 4c50f74719a..ad63f4bc5d3 100644 --- a/src/mbgl/actor/mailbox.cpp +++ b/src/mbgl/actor/mailbox.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #include @@ -32,6 +33,8 @@ void Mailbox::open(Scheduler& scheduler_) { } void Mailbox::close() { + abandon(); + // Block until neither receive() nor push() are in progress. Two mutexes are // used because receive() must not block send(). Of the two, the receiving // mutex must be acquired first, because that is the order that an actor @@ -44,14 +47,37 @@ void Mailbox::close() { closed = true; } +void Mailbox::abandon() { + auto idleValue = State::Idle; + while (!state.compare_exchange_strong(idleValue, State::Abandoned)) { + if (state == State::Abandoned) { + break; + } + } +} + bool Mailbox::isOpen() const { return bool(weakScheduler); } void Mailbox::push(std::unique_ptr message) { + auto idleState = State::Idle; + while (!state.compare_exchange_strong(idleState, State::Processing)) { + if (state == State::Abandoned) { + return; + } + } + + Scoped activityFlag{[this]() { + if (state == State::Processing) { + state = State::Idle; + } + }}; + std::lock_guard pushingLock(pushingMutex); if (closed) { + state = State::Abandoned; return; } @@ -65,12 +91,25 @@ void Mailbox::push(std::unique_ptr message) { } void Mailbox::receive() { + auto idleState = State::Idle; + while (!state.compare_exchange_strong(idleState, State::Processing)) { + if (state == State::Abandoned) { + return; + } + } + + Scoped activityFlag{[this]() { + if (state == State::Processing) { + state = State::Idle; + } + }}; std::lock_guard receivingLock(receivingMutex); auto guard = weakScheduler.lock(); assert(weakScheduler); if (closed) { + state = State::Abandoned; return; } diff --git a/src/mbgl/annotation/render_annotation_source.cpp b/src/mbgl/annotation/render_annotation_source.cpp index 1333c39a11e..ec60eecbe68 100644 --- a/src/mbgl/annotation/render_annotation_source.cpp +++ b/src/mbgl/annotation/render_annotation_source.cpp @@ -9,8 +9,9 @@ namespace mbgl { using namespace style; -RenderAnnotationSource::RenderAnnotationSource(Immutable impl_) - : RenderTileSource(std::move(impl_)) { +RenderAnnotationSource::RenderAnnotationSource(Immutable impl_, + std::shared_ptr threadPool_) + : RenderTileSource(std::move(impl_), std::move(threadPool_)) { assert(LayerManager::annotationsEnabled); tilePyramid.setObserver(this); } diff --git a/src/mbgl/annotation/render_annotation_source.hpp b/src/mbgl/annotation/render_annotation_source.hpp index 23187b6c31e..d195ea9cb27 100644 --- a/src/mbgl/annotation/render_annotation_source.hpp +++ b/src/mbgl/annotation/render_annotation_source.hpp @@ -7,7 +7,7 @@ namespace mbgl { class RenderAnnotationSource final : public RenderTileSource { public: - explicit RenderAnnotationSource(Immutable); + explicit RenderAnnotationSource(Immutable, std::shared_ptr); void update(Immutable, const std::vector>&, diff --git a/src/mbgl/gl/context.cpp b/src/mbgl/gl/context.cpp index 3f7b36c526b..87d0dad5e55 100644 --- a/src/mbgl/gl/context.cpp +++ b/src/mbgl/gl/context.cpp @@ -15,6 +15,7 @@ #include #include #include +#include #if MLN_DRAWABLE_RENDERER #include @@ -81,6 +82,8 @@ Context::Context(RendererBackend& backend_) Context::~Context() noexcept { if (cleanupOnDestruction) { + Scheduler::GetBackground()->runRenderJobs(); + reset(); #if !defined(NDEBUG) Log::Debug(Event::General, "Rendering Stats:\n" + stats.toString("\n")); @@ -90,6 +93,8 @@ Context::~Context() noexcept { } void Context::beginFrame() { + Scheduler::GetBackground()->runRenderJobs(); + #if MLN_DRAWABLE_RENDERER frameInFlightFence = std::make_shared(); diff --git a/src/mbgl/mtl/context.cpp b/src/mbgl/mtl/context.cpp index edd8e60e1ff..2b5014debb2 100644 --- a/src/mbgl/mtl/context.cpp +++ b/src/mbgl/mtl/context.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -42,6 +43,7 @@ Context::Context(RendererBackend& backend_) Context::~Context() noexcept { if (cleanupOnDestruction) { + Scheduler::GetBackground()->runRenderJobs(); performCleanup(); emptyVertexBuffer.reset(); @@ -60,7 +62,10 @@ Context::~Context() noexcept { } } -void Context::beginFrame() {} +void Context::beginFrame() { + Scheduler::GetBackground()->runRenderJobs(); +} + void Context::endFrame() {} std::unique_ptr Context::createCommandEncoder() { diff --git a/src/mbgl/renderer/image_manager.cpp b/src/mbgl/renderer/image_manager.cpp index 74d109f0107..af08124b238 100644 --- a/src/mbgl/renderer/image_manager.cpp +++ b/src/mbgl/renderer/image_manager.cpp @@ -21,6 +21,7 @@ void ImageManager::setObserver(ImageManagerObserver* observer_) { } void ImageManager::setLoaded(bool loaded_) { + std::lock_guard readWriteLock(rwLock); if (loaded == loaded_) { return; } @@ -31,6 +32,7 @@ void ImageManager::setLoaded(bool loaded_) { for (const auto& entry : requestors) { checkMissingAndNotify(*entry.first, entry.second); } + requestors.clear(); } } @@ -40,16 +42,21 @@ bool ImageManager::isLoaded() const { } void ImageManager::addImage(Immutable image_) { + std::lock_guard readLock(rwLock); assert(images.find(image_->id) == images.end()); + // Increase cache size if requested image was provided. if (requestedImages.find(image_->id) != requestedImages.end()) { requestedImagesCacheSize += image_->image.bytes(); } + availableImages.emplace(image_->id); images.emplace(image_->id, std::move(image_)); } bool ImageManager::updateImage(Immutable image_) { + std::lock_guard readWriteLock(rwLock); + auto oldImage = images.find(image_->id); assert(oldImage != images.end()); if (oldImage == images.end()) return false; @@ -74,8 +81,10 @@ bool ImageManager::updateImage(Immutable image_) { } void ImageManager::removeImage(const std::string& id) { + std::lock_guard readWriteLock(rwLock); auto it = images.find(id); assert(it != images.end()); + // Reduce cache size for requested images. auto requestedIt = requestedImages.find(it->second->id); if (requestedIt != requestedImages.end()) { @@ -83,12 +92,14 @@ void ImageManager::removeImage(const std::string& id) { requestedImagesCacheSize -= it->second->image.bytes(); requestedImages.erase(requestedIt); } + images.erase(it); availableImages.erase(id); updatedImageVersions.erase(id); } const style::Image::Impl* ImageManager::getImage(const std::string& id) const { + std::lock_guard readWriteLock(rwLock); if (auto* image = getSharedImage(id)) { return image->get(); } @@ -96,6 +107,7 @@ const style::Image::Impl* ImageManager::getImage(const std::string& id) const { } const Immutable* ImageManager::getSharedImage(const std::string& id) const { + std::lock_guard readWriteLock(rwLock); const auto it = images.find(id); if (it != images.end()) { return &(it->second); @@ -107,6 +119,8 @@ void ImageManager::getImages(ImageRequestor& requestor, ImageRequestPair&& pair) // remove previous requests from this tile removeRequestor(requestor); + std::lock_guard readWriteLock(rwLock); + // If all the icon dependencies are already present ((i.e. if they've been addeded via // runtime styling), then notify the requestor immediately. Otherwise, if the // sprite has not loaded, then wait for it. When the sprite has loaded check @@ -133,6 +147,8 @@ void ImageManager::getImages(ImageRequestor& requestor, ImageRequestPair&& pair) } void ImageManager::removeRequestor(ImageRequestor& requestor) { + std::lock_guard readWriteLock(rwLock); + requestors.erase(&requestor); missingImageRequestors.erase(&requestor); for (auto& requestedImage : requestedImages) { @@ -141,6 +157,8 @@ void ImageManager::removeRequestor(ImageRequestor& requestor) { } void ImageManager::notifyIfMissingImageAdded() { + std::lock_guard readWriteLock(rwLock); + for (auto it = missingImageRequestors.begin(); it != missingImageRequestors.end();) { ImageRequestor& requestor = *it->first; if (!requestor.hasPendingRequests()) { @@ -153,6 +171,8 @@ void ImageManager::notifyIfMissingImageAdded() { } void ImageManager::reduceMemoryUse() { + std::lock_guard readLock(rwLock); + std::vector unusedIDs; unusedIDs.reserve(requestedImages.size()); @@ -173,11 +193,18 @@ void ImageManager::reduceMemoryUseIfCacheSizeExceedsLimit() { } } -const std::set& ImageManager::getAvailableImages() const { - return availableImages; +std::set ImageManager::getAvailableImages() const { + std::set copy; + { + std::lock_guard readWriteLock(rwLock); + copy = availableImages; + } + return copy; } void ImageManager::clear() { + std::lock_guard readWriteLock(rwLock); + assert(requestors.empty()); assert(missingImageRequestors.empty()); @@ -229,6 +256,7 @@ void ImageManager::checkMissingAndNotify(ImageRequestor& requestor, const ImageR } auto removePendingRequests = [this, missingImage] { + std::lock_guard readWriteLock(rwLock); auto existingRequest = requestedImages.find(missingImage); if (existingRequest == requestedImages.end()) { return; @@ -278,11 +306,11 @@ void ImageManager::dumpDebugLogs() const { Log::Info(Event::General, ss.str()); } -ImageRequestor::ImageRequestor(ImageManager& imageManager_) - : imageManager(imageManager_) {} +ImageRequestor::ImageRequestor(std::shared_ptr imageManager_) + : imageManager(std::move(imageManager_)) {} ImageRequestor::~ImageRequestor() { - imageManager.removeRequestor(*this); + imageManager->removeRequestor(*this); } } // namespace mbgl diff --git a/src/mbgl/renderer/image_manager.hpp b/src/mbgl/renderer/image_manager.hpp index ad6715fe6e5..8890332daa7 100644 --- a/src/mbgl/renderer/image_manager.hpp +++ b/src/mbgl/renderer/image_manager.hpp @@ -4,6 +4,7 @@ #include #include +#include #include namespace mbgl { @@ -48,7 +49,7 @@ class ImageManager { void notifyIfMissingImageAdded(); void reduceMemoryUse(); void reduceMemoryUseIfCacheSizeExceedsLimit(); - const std::set& getAvailableImages() const; + std::set getAvailableImages() const; ImageVersionMap updatedImageVersions; @@ -57,7 +58,6 @@ class ImageManager { private: void checkMissingAndNotify(ImageRequestor&, const ImageRequestPair&); void notify(ImageRequestor&, const ImageRequestPair&) const; - void removePattern(const std::string&); bool loaded = false; @@ -70,11 +70,13 @@ class ImageManager { std::set availableImages; ImageManagerObserver* observer = nullptr; + + mutable std::recursive_mutex rwLock; }; class ImageRequestor { public: - explicit ImageRequestor(ImageManager&); + explicit ImageRequestor(std::shared_ptr); virtual ~ImageRequestor(); virtual void onImagesAvailable(ImageMap icons, ImageMap patterns, @@ -87,7 +89,7 @@ class ImageRequestor { void removePendingRequest(const std::string& imageId) { pendingRequests.erase(imageId); } private: - ImageManager& imageManager; + std::shared_ptr imageManager; // Pending requests are image requests that are waiting to be dispatched to the client. std::set pendingRequests; diff --git a/src/mbgl/renderer/render_orchestrator.cpp b/src/mbgl/renderer/render_orchestrator.cpp index ecf41d8eccc..990becf88cb 100644 --- a/src/mbgl/renderer/render_orchestrator.cpp +++ b/src/mbgl/renderer/render_orchestrator.cpp @@ -121,7 +121,8 @@ RenderOrchestrator::RenderOrchestrator(bool backgroundLayerAsColor_, const std:: sourceImpls(makeMutable>>()), layerImpls(makeMutable>>()), renderLight(makeMutable()), - backgroundLayerAsColor(backgroundLayerAsColor_) { + backgroundLayerAsColor(backgroundLayerAsColor_), + threadPool(Scheduler::GetBackground()) { glyphManager->setObserver(this); imageManager->setObserver(this); } @@ -137,6 +138,13 @@ RenderOrchestrator::~RenderOrchestrator() { layer.markContextDestroyed(); } } + + // Wait for any deferred cleanup tasks to complete before releasing and potentially + // destroying the scheduler. Those cleanup tasks must not hold the final reference + // to the scheduler because it cannot be destroyed from one of its own pool threads. + constexpr auto deferredCleanupTimeout = Milliseconds{1000}; + [[maybe_unused]] const auto remaining = threadPool->waitForEmpty(deferredCleanupTimeout); + assert(remaining == 0); } void RenderOrchestrator::setObserver(RendererObserver* observer_) { @@ -180,8 +188,8 @@ std::unique_ptr RenderOrchestrator::createRenderTree( updateParameters->fileSource, updateParameters->mode, updateParameters->annotationManager, - *imageManager, - *glyphManager, + imageManager, + glyphManager, updateParameters->prefetchZoomDelta}; glyphManager->setURL(updateParameters->glyphURL); @@ -311,7 +319,7 @@ std::unique_ptr RenderOrchestrator::createRenderTree( // Create render sources for newly added sources. for (const auto& entry : sourceDiff.added) { - std::unique_ptr renderSource = RenderSource::create(entry.second); + std::unique_ptr renderSource = RenderSource::create(entry.second, threadPool); renderSource->setObserver(this); renderSources.emplace(entry.first, std::move(renderSource)); } diff --git a/src/mbgl/renderer/render_orchestrator.hpp b/src/mbgl/renderer/render_orchestrator.hpp index 8df4d0b390b..69ff4667e6b 100644 --- a/src/mbgl/renderer/render_orchestrator.hpp +++ b/src/mbgl/renderer/render_orchestrator.hpp @@ -184,8 +184,8 @@ class RenderOrchestrator final : public GlyphManagerObserver, public ImageManage ZoomHistory zoomHistory; TransformState transformState; - std::unique_ptr glyphManager; - std::unique_ptr imageManager; + std::shared_ptr glyphManager; + std::shared_ptr imageManager; std::unique_ptr lineAtlas; std::unique_ptr patternAtlas; @@ -210,6 +210,8 @@ class RenderOrchestrator final : public GlyphManagerObserver, public ImageManage RenderLayerReferences orderedLayers; RenderLayerReferences layersNeedPlacement; + std::shared_ptr threadPool; + #if MLN_DRAWABLE_RENDERER std::vector> pendingChanges; diff --git a/src/mbgl/renderer/render_source.cpp b/src/mbgl/renderer/render_source.cpp index 624d89073e0..2eb5ce900e2 100644 --- a/src/mbgl/renderer/render_source.cpp +++ b/src/mbgl/renderer/render_source.cpp @@ -12,28 +12,36 @@ #include #include + +#include #include namespace mbgl { using namespace style; -std::unique_ptr RenderSource::create(const Immutable& impl) { +std::unique_ptr RenderSource::create(const Immutable& impl, + std::shared_ptr threadPool_) { switch (impl->type) { case SourceType::Vector: - return std::make_unique(staticImmutableCast(impl)); + return std::make_unique(staticImmutableCast(impl), + std::move(threadPool_)); case SourceType::Raster: - return std::make_unique(staticImmutableCast(impl)); + return std::make_unique(staticImmutableCast(impl), + std::move(threadPool_)); case SourceType::RasterDEM: - return std::make_unique(staticImmutableCast(impl)); + return std::make_unique(staticImmutableCast(impl), + std::move(threadPool_)); case SourceType::GeoJSON: - return std::make_unique(staticImmutableCast(impl)); + return std::make_unique(staticImmutableCast(impl), + std::move(threadPool_)); case SourceType::Video: assert(false); return nullptr; case SourceType::Annotations: if (LayerManager::annotationsEnabled) { - return std::make_unique(staticImmutableCast(impl)); + return std::make_unique(staticImmutableCast(impl), + std::move(threadPool_)); } else { assert(false); return nullptr; @@ -41,7 +49,8 @@ std::unique_ptr RenderSource::create(const Immutable case SourceType::Image: return std::make_unique(staticImmutableCast(impl)); case SourceType::CustomVector: - return std::make_unique(staticImmutableCast(impl)); + return std::make_unique(staticImmutableCast(impl), + std::move(threadPool_)); } // Not reachable, but placate GCC. diff --git a/src/mbgl/renderer/render_source.hpp b/src/mbgl/renderer/render_source.hpp index 9df117c7751..4b46e07756b 100644 --- a/src/mbgl/renderer/render_source.hpp +++ b/src/mbgl/renderer/render_source.hpp @@ -17,20 +17,21 @@ namespace mbgl { +class CollisionIndex; +class ImageManager; +class ImageSourceRenderData; class PaintParameters; -class TransformState; -class RenderTile; -class RenderLayer; class RenderedQueryOptions; +class RenderItem; +class RenderLayer; +class RenderSourceObserver; +class RenderTile; +class Scheduler; class SourceQueryOptions; class Tile; -class RenderSourceObserver; class TileParameters; -class CollisionIndex; class TransformParameters; -class ImageManager; -class ImageSourceRenderData; -class RenderItem; +class TransformState; namespace gfx { class UploadPass; @@ -47,7 +48,7 @@ using RenderTiles = std::shared_ptr create(const Immutable&); + static std::unique_ptr create(const Immutable&, std::shared_ptr); ~RenderSource() override; bool isEnabled() const; diff --git a/src/mbgl/renderer/sources/render_custom_geometry_source.cpp b/src/mbgl/renderer/sources/render_custom_geometry_source.cpp index e1e78b6930f..7f054bfa2c2 100644 --- a/src/mbgl/renderer/sources/render_custom_geometry_source.cpp +++ b/src/mbgl/renderer/sources/render_custom_geometry_source.cpp @@ -7,8 +7,9 @@ namespace mbgl { using namespace style; -RenderCustomGeometrySource::RenderCustomGeometrySource(Immutable impl_) - : RenderTileSource(std::move(impl_)) { +RenderCustomGeometrySource::RenderCustomGeometrySource(Immutable impl_, + std::shared_ptr threadPool_) + : RenderTileSource(std::move(impl_), std::move(threadPool_)) { tilePyramid.setObserver(this); } diff --git a/src/mbgl/renderer/sources/render_custom_geometry_source.hpp b/src/mbgl/renderer/sources/render_custom_geometry_source.hpp index 559039e245b..4c77c6a1973 100644 --- a/src/mbgl/renderer/sources/render_custom_geometry_source.hpp +++ b/src/mbgl/renderer/sources/render_custom_geometry_source.hpp @@ -7,7 +7,7 @@ namespace mbgl { class RenderCustomGeometrySource final : public RenderTileSource { public: - explicit RenderCustomGeometrySource(Immutable); + explicit RenderCustomGeometrySource(Immutable, std::shared_ptr); void update(Immutable, const std::vector>&, diff --git a/src/mbgl/renderer/sources/render_geojson_source.cpp b/src/mbgl/renderer/sources/render_geojson_source.cpp index 024e5f96044..633df708879 100644 --- a/src/mbgl/renderer/sources/render_geojson_source.cpp +++ b/src/mbgl/renderer/sources/render_geojson_source.cpp @@ -65,8 +65,9 @@ MAPBOX_ETERNAL_CONSTEXPR const auto extensionGetters = } // namespace -RenderGeoJSONSource::RenderGeoJSONSource(Immutable impl_) - : RenderTileSource(std::move(impl_)) {} +RenderGeoJSONSource::RenderGeoJSONSource(Immutable impl_, + std::shared_ptr threadPool_) + : RenderTileSource(std::move(impl_), std::move(threadPool_)) {} RenderGeoJSONSource::~RenderGeoJSONSource() = default; diff --git a/src/mbgl/renderer/sources/render_geojson_source.hpp b/src/mbgl/renderer/sources/render_geojson_source.hpp index ef2b3a1e7cb..f0c41c2ab50 100644 --- a/src/mbgl/renderer/sources/render_geojson_source.hpp +++ b/src/mbgl/renderer/sources/render_geojson_source.hpp @@ -11,7 +11,7 @@ class GeoJSONData; class RenderGeoJSONSource final : public RenderTileSource { public: - explicit RenderGeoJSONSource(Immutable); + explicit RenderGeoJSONSource(Immutable, std::shared_ptr); ~RenderGeoJSONSource() override; void update(Immutable, diff --git a/src/mbgl/renderer/sources/render_raster_dem_source.cpp b/src/mbgl/renderer/sources/render_raster_dem_source.cpp index 041127e6877..6f41426141e 100644 --- a/src/mbgl/renderer/sources/render_raster_dem_source.cpp +++ b/src/mbgl/renderer/sources/render_raster_dem_source.cpp @@ -10,8 +10,9 @@ namespace mbgl { using namespace style; -RenderRasterDEMSource::RenderRasterDEMSource(Immutable impl_) - : RenderTileSetSource(std::move(impl_)) {} +RenderRasterDEMSource::RenderRasterDEMSource(Immutable impl_, + std::shared_ptr threadPool_) + : RenderTileSetSource(std::move(impl_), std::move(threadPool_)) {} const style::RasterSource::Impl& RenderRasterDEMSource::impl() const { return static_cast(*baseImpl); diff --git a/src/mbgl/renderer/sources/render_raster_dem_source.hpp b/src/mbgl/renderer/sources/render_raster_dem_source.hpp index 1da7d6f14a8..1599b76ef5b 100644 --- a/src/mbgl/renderer/sources/render_raster_dem_source.hpp +++ b/src/mbgl/renderer/sources/render_raster_dem_source.hpp @@ -7,7 +7,7 @@ namespace mbgl { class RenderRasterDEMSource final : public RenderTileSetSource { public: - explicit RenderRasterDEMSource(Immutable); + explicit RenderRasterDEMSource(Immutable, std::shared_ptr); std::unordered_map> queryRenderedFeatures( const ScreenLineString& geometry, diff --git a/src/mbgl/renderer/sources/render_raster_source.cpp b/src/mbgl/renderer/sources/render_raster_source.cpp index 2bfd6a23092..db99831e02c 100644 --- a/src/mbgl/renderer/sources/render_raster_source.cpp +++ b/src/mbgl/renderer/sources/render_raster_source.cpp @@ -8,8 +8,9 @@ namespace mbgl { using namespace style; -RenderRasterSource::RenderRasterSource(Immutable impl_) - : RenderTileSetSource(std::move(impl_)) {} +RenderRasterSource::RenderRasterSource(Immutable impl_, + std::shared_ptr threadPool_) + : RenderTileSetSource(std::move(impl_), std::move(threadPool_)) {} inline const style::RasterSource::Impl& RenderRasterSource::impl() const { return static_cast(*baseImpl); diff --git a/src/mbgl/renderer/sources/render_raster_source.hpp b/src/mbgl/renderer/sources/render_raster_source.hpp index 47035e41b54..c827c8a7e0f 100644 --- a/src/mbgl/renderer/sources/render_raster_source.hpp +++ b/src/mbgl/renderer/sources/render_raster_source.hpp @@ -7,7 +7,7 @@ namespace mbgl { class RenderRasterSource final : public RenderTileSetSource { public: - explicit RenderRasterSource(Immutable); + explicit RenderRasterSource(Immutable, std::shared_ptr); private: void prepare(const SourcePrepareParameters&) final; diff --git a/src/mbgl/renderer/sources/render_tile_source.cpp b/src/mbgl/renderer/sources/render_tile_source.cpp index d4eceeccd6c..e3527efc63e 100644 --- a/src/mbgl/renderer/sources/render_tile_source.cpp +++ b/src/mbgl/renderer/sources/render_tile_source.cpp @@ -369,8 +369,9 @@ void TileSourceRenderItem::updateDebugDrawables(DebugLayerGroupMap& debugLayerGr } #endif -RenderTileSource::RenderTileSource(Immutable impl_) +RenderTileSource::RenderTileSource(Immutable impl_, std::shared_ptr threadPool_) : RenderSource(std::move(impl_)), + tilePyramid(std::move(threadPool_)), renderTiles(makeMutable>()) { tilePyramid.setObserver(this); } @@ -489,8 +490,8 @@ void RenderTileSource::dumpDebugLogs() const { // RenderTileSetSource implementation -RenderTileSetSource::RenderTileSetSource(Immutable impl_) - : RenderTileSource(std::move(impl_)) {} +RenderTileSetSource::RenderTileSetSource(Immutable impl_, std::shared_ptr threadPool_) + : RenderTileSource(std::move(impl_), std::move(threadPool_)) {} RenderTileSetSource::~RenderTileSetSource() = default; diff --git a/src/mbgl/renderer/sources/render_tile_source.hpp b/src/mbgl/renderer/sources/render_tile_source.hpp index dd10bd4ce36..160ed9541da 100644 --- a/src/mbgl/renderer/sources/render_tile_source.hpp +++ b/src/mbgl/renderer/sources/render_tile_source.hpp @@ -51,7 +51,7 @@ class RenderTileSource : public RenderSource { void dumpDebugLogs() const override; protected: - RenderTileSource(Immutable); + RenderTileSource(Immutable, std::shared_ptr); TilePyramid tilePyramid; Immutable> renderTiles; mutable RenderTiles filteredRenderTiles; @@ -67,7 +67,7 @@ class RenderTileSource : public RenderSource { */ class RenderTileSetSource : public RenderTileSource { protected: - RenderTileSetSource(Immutable); + RenderTileSetSource(Immutable, std::shared_ptr); ~RenderTileSetSource() override; virtual void updateInternal(const Tileset&, diff --git a/src/mbgl/renderer/sources/render_vector_source.cpp b/src/mbgl/renderer/sources/render_vector_source.cpp index 8134f4cc815..212338db670 100644 --- a/src/mbgl/renderer/sources/render_vector_source.cpp +++ b/src/mbgl/renderer/sources/render_vector_source.cpp @@ -8,8 +8,9 @@ namespace mbgl { using namespace style; -RenderVectorSource::RenderVectorSource(Immutable impl_) - : RenderTileSetSource(std::move(impl_)) {} +RenderVectorSource::RenderVectorSource(Immutable impl_, + std::shared_ptr threadPool_) + : RenderTileSetSource(std::move(impl_), std::move(threadPool_)) {} const std::optional& RenderVectorSource::getTileset() const { return static_cast(*baseImpl).tileset; diff --git a/src/mbgl/renderer/sources/render_vector_source.hpp b/src/mbgl/renderer/sources/render_vector_source.hpp index f826603c8a7..326b392b698 100644 --- a/src/mbgl/renderer/sources/render_vector_source.hpp +++ b/src/mbgl/renderer/sources/render_vector_source.hpp @@ -8,7 +8,7 @@ namespace mbgl { class RenderVectorSource final : public RenderTileSetSource { public: - explicit RenderVectorSource(Immutable); + explicit RenderVectorSource(Immutable, std::shared_ptr); private: void updateInternal(const Tileset&, diff --git a/src/mbgl/renderer/tile_parameters.hpp b/src/mbgl/renderer/tile_parameters.hpp index e76d8d0f836..b7ada67d636 100644 --- a/src/mbgl/renderer/tile_parameters.hpp +++ b/src/mbgl/renderer/tile_parameters.hpp @@ -22,8 +22,8 @@ class TileParameters { std::shared_ptr fileSource; const MapMode mode; mapbox::base::WeakPtr annotationManager; - ImageManager& imageManager; - GlyphManager& glyphManager; + std::shared_ptr imageManager; + std::shared_ptr glyphManager; const uint8_t prefetchZoomDelta; }; diff --git a/src/mbgl/renderer/tile_pyramid.cpp b/src/mbgl/renderer/tile_pyramid.cpp index 350b4dd3fe4..55466aa3ada 100644 --- a/src/mbgl/renderer/tile_pyramid.cpp +++ b/src/mbgl/renderer/tile_pyramid.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -23,8 +24,9 @@ using namespace style; static TileObserver nullObserver; -TilePyramid::TilePyramid() - : observer(&nullObserver) {} +TilePyramid::TilePyramid(std::shared_ptr threadPool_) + : cache(std::move(threadPool_)), + observer(&nullObserver) {} TilePyramid::~TilePyramid() = default; @@ -65,13 +67,15 @@ void TilePyramid::update(const std::vector>& l // If we're not going to render anything, move our existing tiles into // the cache (if they're not stale) or abandon them, and return. if (!needsRendering) { - if (!needsRelayout) { - for (auto& entry : tiles) { + for (auto& entry : tiles) { + if (!needsRelayout) { // These tiles are invisible, we set optional necessity // for them and thus suppress network requests on // tiles expiration (see `OnlineFileRequest`). entry.second->setNecessity(TileNecessity::Optional); cache.add(entry.first, std::move(entry.second)); + } else { + cache.deferredRelease(std::move(entry.second)); } } @@ -227,11 +231,17 @@ void TilePyramid::update(const std::vector>& l auto retainIt = retain.begin(); while (tilesIt != tiles.end()) { if (retainIt == retain.end() || tilesIt->first < *retainIt) { - if (!needsRelayout) { - tilesIt->second->setNecessity(TileNecessity::Optional); - cache.add(tilesIt->first, std::move(tilesIt->second)); + // Remove the tile from the map. + // If it requires re-layout, discard it asynchronously, otherwise keep it in the cache + const auto key = tilesIt->first; + if (std::unique_ptr tile = std::move(tiles.extract(tilesIt++).mapped())) { + if (needsRelayout) { + cache.deferredRelease(std::move(tile)); + } else { + tile->setNecessity(TileNecessity::Optional); + cache.add(key, std::move(tile)); + } } - tiles.erase(tilesIt++); } else { if (!(*retainIt < tilesIt->first)) { ++tilesIt; diff --git a/src/mbgl/renderer/tile_pyramid.hpp b/src/mbgl/renderer/tile_pyramid.hpp index e50a0eee290..bfae55f0952 100644 --- a/src/mbgl/renderer/tile_pyramid.hpp +++ b/src/mbgl/renderer/tile_pyramid.hpp @@ -29,7 +29,7 @@ class SourcePrepareParameters; class TilePyramid { public: - TilePyramid(); + TilePyramid(std::shared_ptr threadPool_); ~TilePyramid(); bool isLoaded() const; diff --git a/src/mbgl/text/glyph_manager.cpp b/src/mbgl/text/glyph_manager.cpp index f651185b654..b6727d2e891 100644 --- a/src/mbgl/text/glyph_manager.cpp +++ b/src/mbgl/text/glyph_manager.cpp @@ -21,33 +21,36 @@ GlyphManager::~GlyphManager() = default; void GlyphManager::getGlyphs(GlyphRequestor& requestor, GlyphDependencies glyphDependencies, FileSource& fileSource) { auto dependencies = std::make_shared(std::move(glyphDependencies)); - // Figure out which glyph ranges need to be fetched. For each range that - // does need to be fetched, record an entry mapping the requestor to a - // shared pointer containing the dependencies. When the shared pointer - // becomes unique, we know that all the dependencies for that requestor have - // been fetched, and can notify it of completion. - for (const auto& dependency : *dependencies) { - const FontStack& fontStack = dependency.first; - Entry& entry = entries[fontStack]; - - const GlyphIDs& glyphIDs = dependency.second; - std::unordered_set ranges; - for (const auto& glyphID : glyphIDs) { - if (localGlyphRasterizer->canRasterizeGlyph(fontStack, glyphID)) { - if (entry.glyphs.find(glyphID) == entry.glyphs.end()) { - entry.glyphs.emplace(glyphID, makeMutable(generateLocalSDF(fontStack, glyphID))); + { + std::lock_guard readWriteLock(rwLock); + // Figure out which glyph ranges need to be fetched. For each range that + // does need to be fetched, record an entry mapping the requestor to a + // shared pointer containing the dependencies. When the shared pointer + // becomes unique, we know that all the dependencies for that requestor have + // been fetched, and can notify it of completion. + for (const auto& dependency : *dependencies) { + const FontStack& fontStack = dependency.first; + Entry& entry = entries[fontStack]; + + const GlyphIDs& glyphIDs = dependency.second; + std::unordered_set ranges; + for (const auto& glyphID : glyphIDs) { + if (localGlyphRasterizer->canRasterizeGlyph(fontStack, glyphID)) { + if (entry.glyphs.find(glyphID) == entry.glyphs.end()) { + entry.glyphs.emplace(glyphID, makeMutable(generateLocalSDF(fontStack, glyphID))); + } + } else { + ranges.insert(getGlyphRange(glyphID)); } - } else { - ranges.insert(getGlyphRange(glyphID)); } - } - for (const auto& range : ranges) { - auto it = entry.ranges.find(range); - if (it == entry.ranges.end() || !it->second.parsed) { - GlyphRequest& request = entry.ranges[range]; - request.requestors[&requestor] = dependencies; - requestRange(request, fontStack, range, fileSource); + for (const auto& range : ranges) { + auto it = entry.ranges.find(range); + if (it == entry.ranges.end() || !it->second.parsed) { + GlyphRequest& request = entry.ranges[range]; + request.requestors[&requestor] = dependencies; + requestRange(request, fontStack, range, fileSource); + } } } } @@ -88,39 +91,43 @@ void GlyphManager::processResponse(const Response& res, const FontStack& fontSta return; } - Entry& entry = entries[fontStack]; - GlyphRequest& request = entry.ranges[range]; + { + std::lock_guard readWriteLock(rwLock); - if (!res.noContent) { - std::vector glyphs; + Entry& entry = entries[fontStack]; + GlyphRequest& request = entry.ranges[range]; - try { - glyphs = parseGlyphPBF(range, *res.data); - } catch (...) { - observer->onGlyphsError(fontStack, range, std::current_exception()); - return; - } + if (!res.noContent) { + std::vector glyphs; + + try { + glyphs = parseGlyphPBF(range, *res.data); + } catch (...) { + observer->onGlyphsError(fontStack, range, std::current_exception()); + return; + } - for (auto& glyph : glyphs) { - auto id = glyph.id; - if (!localGlyphRasterizer->canRasterizeGlyph(fontStack, id)) { - entry.glyphs.erase(id); - entry.glyphs.emplace(id, makeMutable(std::move(glyph))); + for (auto& glyph : glyphs) { + auto id = glyph.id; + if (!localGlyphRasterizer->canRasterizeGlyph(fontStack, id)) { + entry.glyphs.erase(id); + entry.glyphs.emplace(id, makeMutable(std::move(glyph))); + } } } - } - request.parsed = true; + request.parsed = true; - for (auto& pair : request.requestors) { - GlyphRequestor& requestor = *pair.first; - const std::shared_ptr& dependencies = pair.second; - if (dependencies.unique()) { - notify(requestor, *dependencies); + for (auto& pair : request.requestors) { + GlyphRequestor& requestor = *pair.first; + const std::shared_ptr& dependencies = pair.second; + if (dependencies.unique()) { + notify(requestor, *dependencies); + } } - } - request.requestors.clear(); + request.requestors.clear(); + } observer->onGlyphsLoaded(fontStack, range); } @@ -153,6 +160,7 @@ void GlyphManager::notify(GlyphRequestor& requestor, const GlyphDependencies& gl } void GlyphManager::removeRequestor(GlyphRequestor& requestor) { + std::lock_guard readWriteLock(rwLock); for (auto& entry : entries) { for (auto& range : entry.second.ranges) { range.second.requestors.erase(&requestor); @@ -161,6 +169,7 @@ void GlyphManager::removeRequestor(GlyphRequestor& requestor) { } void GlyphManager::evict(const std::set& keep) { + std::lock_guard readWriteLock(rwLock); util::erase_if(entries, [&](const auto& entry) { return keep.count(entry.first) == 0; }); } diff --git a/src/mbgl/text/glyph_manager.hpp b/src/mbgl/text/glyph_manager.hpp index b01b630872f..23cb15f753e 100644 --- a/src/mbgl/text/glyph_manager.hpp +++ b/src/mbgl/text/glyph_manager.hpp @@ -7,6 +7,7 @@ #include #include +#include #include #include @@ -71,6 +72,8 @@ class GlyphManager { GlyphManagerObserver* observer = nullptr; std::unique_ptr localGlyphRasterizer; + + std::recursive_mutex rwLock; }; } // namespace mbgl diff --git a/src/mbgl/tile/geometry_tile.cpp b/src/mbgl/tile/geometry_tile.cpp index 297792a3180..1649b69f926 100644 --- a/src/mbgl/tile/geometry_tile.cpp +++ b/src/mbgl/tile/geometry_tile.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -159,8 +160,9 @@ GeometryTile::GeometryTile(const OverscaledTileID& id_, std::string sourceID_, c : Tile(Kind::Geometry, id_), ImageRequestor(parameters.imageManager), sourceID(std::move(sourceID_)), + threadPool(Scheduler::GetBackground()), mailbox(std::make_shared(*Scheduler::GetCurrent())), - worker(Scheduler::GetBackground(), + worker(threadPool, ActorRef(*this, mailbox), id_, sourceID, @@ -175,8 +177,15 @@ GeometryTile::GeometryTile(const OverscaledTileID& id_, std::string sourceID_, c showCollisionBoxes(parameters.debugOptions & MapDebugOptions::Collision) {} GeometryTile::~GeometryTile() { - glyphManager.removeRequestor(*this); markObsolete(); + + glyphManager->removeRequestor(*this); + imageManager->removeRequestor(*this); + + if (layoutResult) { + threadPool->runOnRenderThread( + [layoutResult_{std::move(layoutResult)}, atlasTextures_{std::move(atlasTextures)}]() {}); + } } void GeometryTile::cancel() { @@ -185,6 +194,7 @@ void GeometryTile::cancel() { void GeometryTile::markObsolete() { obsolete = true; + mailbox->abandon(); } void GeometryTile::setError(std::exception_ptr err) { @@ -193,13 +203,17 @@ void GeometryTile::setError(std::exception_ptr err) { } void GeometryTile::setData(std::unique_ptr data_) { + if (obsolete) { + return; + } + // Mark the tile as pending again if it was complete before to prevent // signaling a complete state despite pending parse operations. pending = true; ++correlationID; worker.self().invoke( - &GeometryTileWorker::setData, std::move(data_), imageManager.getAvailableImages(), correlationID); + &GeometryTileWorker::setData, std::move(data_), imageManager->getAvailableImages(), correlationID); } void GeometryTile::reset() { @@ -238,7 +252,7 @@ void GeometryTile::setLayers(const std::vector>& laye ++correlationID; worker.self().invoke( - &GeometryTileWorker::setLayers, std::move(impls), imageManager.getAvailableImages(), correlationID); + &GeometryTileWorker::setLayers, std::move(impls), imageManager->getAvailableImages(), correlationID); } void GeometryTile::setShowCollisionBoxes(const bool showCollisionBoxes_) { @@ -278,7 +292,7 @@ void GeometryTile::onGlyphsAvailable(GlyphMap glyphs) { void GeometryTile::getGlyphs(GlyphDependencies glyphDependencies) { if (fileSource) { - glyphManager.getGlyphs(*this, std::move(glyphDependencies), *fileSource); + glyphManager->getGlyphs(*this, std::move(glyphDependencies), *fileSource); } } @@ -294,7 +308,7 @@ void GeometryTile::onImagesAvailable(ImageMap images, } void GeometryTile::getImages(ImageRequestPair pair) { - imageManager.getImages(*this, std::move(pair)); + imageManager->getImages(*this, std::move(pair)); } std::shared_ptr GeometryTile::getFeatureIndex() const { diff --git a/src/mbgl/tile/geometry_tile.hpp b/src/mbgl/tile/geometry_tile.hpp index 23e4686ed2c..31aa70b668f 100644 --- a/src/mbgl/tile/geometry_tile.hpp +++ b/src/mbgl/tile/geometry_tile.hpp @@ -106,12 +106,14 @@ class GeometryTile : public Tile, public GlyphRequestor, public ImageRequestor { // Used to signal the worker that it should abandon parsing this tile as soon as possible. std::atomic obsolete{false}; - std::shared_ptr mailbox; + const std::shared_ptr threadPool; + + const std::shared_ptr mailbox; Actor worker; - std::shared_ptr fileSource; - GlyphManager& glyphManager; - ImageManager& imageManager; + const std::shared_ptr fileSource; + const std::shared_ptr glyphManager; + const std::shared_ptr imageManager; uint64_t correlationID = 0; diff --git a/src/mbgl/tile/geometry_tile_worker.cpp b/src/mbgl/tile/geometry_tile_worker.cpp index 57285f09901..5e3646eebfe 100644 --- a/src/mbgl/tile/geometry_tile_worker.cpp +++ b/src/mbgl/tile/geometry_tile_worker.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -44,7 +45,9 @@ GeometryTileWorker::GeometryTileWorker(ActorRef self_, pixelRatio(pixelRatio_), showCollisionBoxes(showCollisionBoxes_) {} -GeometryTileWorker::~GeometryTileWorker() = default; +GeometryTileWorker::~GeometryTileWorker() { + Scheduler::GetBackground()->runOnRenderThread([renderData_{std::move(renderData)}]() {}); +} /* GeometryTileWorker is a state machine. This is its transition diagram. diff --git a/src/mbgl/tile/tile.hpp b/src/mbgl/tile/tile.hpp index 77652e6e18b..c487155d5af 100644 --- a/src/mbgl/tile/tile.hpp +++ b/src/mbgl/tile/tile.hpp @@ -76,7 +76,7 @@ class Tile { // render data with the given properties. // // Returns `true` if the corresponding render layer data is present in this - // tile (and i.e. it was succesfully updated); returns `false` otherwise. + // tile (and i.e. it was successfully updated); returns `false` otherwise. virtual bool layerPropertiesUpdated(const Immutable& layerProperties) = 0; virtual void setShowCollisionBoxes(const bool) {} virtual void setLayers(const std::vector>&) {} diff --git a/src/mbgl/tile/tile_cache.cpp b/src/mbgl/tile/tile_cache.cpp index 09bfb928a8c..b4d65990fcf 100644 --- a/src/mbgl/tile/tile_cache.cpp +++ b/src/mbgl/tile/tile_cache.cpp @@ -1,4 +1,5 @@ #include +#include #include namespace mbgl { @@ -7,23 +8,66 @@ void TileCache::setSize(size_t size_) { size = size_; while (orderedKeys.size() > size) { - auto key = orderedKeys.front(); + const auto key = orderedKeys.front(); orderedKeys.remove(key); - tiles.erase(key); + + auto hit = tiles.find(key); + if (hit != tiles.end()) { + auto tile = std::move(hit->second); + tiles.erase(hit); + deferredRelease(std::move(tile)); + } } assert(orderedKeys.size() <= size); } -void TileCache::add(const OverscaledTileID& key, std::unique_ptr tile) { +namespace { + +/// This exists solely to prevent a problem where temporary lambda captures +/// are retained for the duration of the scope instead of being destroyed immediately. +template +struct CaptureWrapper { + CaptureWrapper(std::unique_ptr&& item_) + : item(std::move(item_)) {} + CaptureWrapper(const CaptureWrapper& other) + : item(other.item) {} + std::shared_ptr item; +}; +} // namespace + +void TileCache::deferredRelease(std::unique_ptr&& tile) { + tile->cancel(); + + // The `std::function` must be created in a separate statement from the `schedule` call. + // Creating a `std::function` from a lambda involves a copy, which is why we must use + // `shared_ptr` rather than `unique_ptr` for the capture. As a result, a temporary holds + // a reference until the construction is complete and the lambda is destroyed. + // If this temporary outlives the `schedule` call, and the function is executed immediately + // by a waiting thread and is already complete, that temporary reference ends up being the + // last one and the destruction actually occurs here on this thread. + std::function func{[tile_{CaptureWrapper{std::move(tile)}}]() { + }}; + + threadPool->schedule(std::move(func)); +} + +void TileCache::add(const OverscaledTileID& key, std::unique_ptr&& tile) { if (!tile->isRenderable() || !size) { + deferredRelease(std::move(tile)); return; } - // insert new or query existing tile - if (!tiles.emplace(key, std::move(tile)).second) { - // remove existing tile key + const auto result = tiles.insert(std::make_pair(key, std::unique_ptr{})); + if (result.second) { + // inserted + result.first->second = std::move(tile); + } else { + // already present + // remove existing tile key to move it to the end orderedKeys.remove(key); + // release the newly-provided item + deferredRelease(std::move(tile)); } // (re-)insert tile key as newest @@ -31,7 +75,7 @@ void TileCache::add(const OverscaledTileID& key, std::unique_ptr tile) { // purge oldest key/tile if necessary if (orderedKeys.size() > size) { - pop(orderedKeys.front()); + deferredRelease(pop(orderedKeys.front())); } assert(orderedKeys.size() <= size); @@ -49,10 +93,9 @@ Tile* TileCache::get(const OverscaledTileID& key) { std::unique_ptr TileCache::pop(const OverscaledTileID& key) { std::unique_ptr tile; - auto it = tiles.find(key); + const auto it = tiles.find(key); if (it != tiles.end()) { - tile = std::move(it->second); - tiles.erase(it); + tile = std::move(tiles.extract(it).mapped()); orderedKeys.remove(key); assert(tile->isRenderable()); } @@ -65,6 +108,9 @@ bool TileCache::has(const OverscaledTileID& key) { } void TileCache::clear() { + for (auto& item : tiles) { + deferredRelease(std::move(item.second)); + } orderedKeys.clear(); tiles.clear(); } diff --git a/src/mbgl/tile/tile_cache.hpp b/src/mbgl/tile/tile_cache.hpp index d5b295cd9ed..fe7f41f7b9f 100644 --- a/src/mbgl/tile/tile_cache.hpp +++ b/src/mbgl/tile/tile_cache.hpp @@ -9,22 +9,36 @@ namespace mbgl { +class Scheduler; + class TileCache { public: - TileCache(size_t size_ = 0) - : size(size_) {} + TileCache(std::shared_ptr threadPool_, size_t size_ = 0) + : threadPool(std::move(threadPool_)), + size(size_) {} + /// Change the maximum size of the cache. void setSize(size_t); - size_t getSize() const { return size; }; - void add(const OverscaledTileID& key, std::unique_ptr tile); + + /// Get the maximum size + size_t getMaxSize() const { return size; } + + /// Add a new tile with the given ID. + /// If a tile with the same ID is already present, it will be retained and the new one will be discarded. + void add(const OverscaledTileID& key, std::unique_ptr&& tile); + std::unique_ptr pop(const OverscaledTileID& key); Tile* get(const OverscaledTileID& key); bool has(const OverscaledTileID& key); void clear(); + /// Destroy a tile without blocking + void deferredRelease(std::unique_ptr&&); + private: std::map> tiles; std::list orderedKeys; + std::shared_ptr threadPool; size_t size; }; diff --git a/src/mbgl/tile/tile_loader.hpp b/src/mbgl/tile/tile_loader.hpp index 9709cf77278..6c07af8fd0b 100644 --- a/src/mbgl/tile/tile_loader.hpp +++ b/src/mbgl/tile/tile_loader.hpp @@ -3,6 +3,10 @@ #include #include +#include +#include +#include + namespace mbgl { class FileSource; @@ -48,6 +52,19 @@ class TileLoader { std::shared_ptr fileSource; std::unique_ptr request; TileUpdateParameters updateParameters{Duration::zero(), false}; + + /// @brief It's possible for async requests in flight to mess with the request + /// object at the same time as the loader's destructor. This construct is shared + /// with the request lambdas to ensure more tightly controlled synchronization + /// to prevent this from happening. + struct Shared { + std::shared_mutex requestLock; + std::atomic_bool aborted{false}; + }; + + // Allocated as a share_ptr so either the loader or request can outlive the + // other and still see this. + std::shared_ptr shared; }; } // namespace mbgl diff --git a/src/mbgl/tile/tile_loader_impl.hpp b/src/mbgl/tile/tile_loader_impl.hpp index cc1e50de1ce..68781d378b0 100644 --- a/src/mbgl/tile/tile_loader_impl.hpp +++ b/src/mbgl/tile/tile_loader_impl.hpp @@ -30,6 +30,9 @@ TileLoader::TileLoader(T& tile_, Resource::LoadingMethod::CacheOnly)), fileSource(parameters.fileSource) { assert(!request); + + shared = std::make_shared(); + if (!fileSource) { tile.setError(getCantLoadTileError()); return; @@ -55,7 +58,12 @@ TileLoader::TileLoader(T& tile_, } template -TileLoader::~TileLoader() = default; +TileLoader::~TileLoader() { + std::unique_lock lock(shared->requestLock); + shared->aborted = true; + tile.cancel(); + request.reset(); +}; template void TileLoader::setNecessity(TileNecessity newNecessity) { @@ -90,29 +98,36 @@ void TileLoader::loadFromCache() { } resource.loadingMethod = Resource::LoadingMethod::CacheOnly; - request = fileSource->request(resource, [this](const Response& res) { - request.reset(); - - tile.setTriedCache(); - - if (res.error && res.error->reason == Response::Error::Reason::NotFound) { - // When the cache-only request could not be satisfied, don't treat - // it as an error. A cache lookup could still return data, _and_ an - // error, in particular when we were able to find the data, but it - // is expired and the Cache-Control headers indicated that we aren't - // allowed to use expired responses. In this case, we still get the - // data which we can use in our conditional network request. - resource.priorModified = res.modified; - resource.priorExpires = res.expires; - resource.priorEtag = res.etag; - resource.priorData = res.data; - } else { - loadedData(res); - } - - if (necessity == TileNecessity::Required) { - loadFromNetwork(); - } + request = fileSource->request(resource, [this, shared_{shared}](const Response& res) { + do { + if (shared_->requestLock.try_lock_shared()) { + std::shared_lock lock(shared_->requestLock, std::adopt_lock); + if (shared_->aborted) return; + + request.reset(); + tile.setTriedCache(); + + if (res.error && res.error->reason == Response::Error::Reason::NotFound) { + // When the cache-only request could not be satisfied, don't treat + // it as an error. A cache lookup could still return data, _and_ an + // error, in particular when we were able to find the data, but it + // is expired and the Cache-Control headers indicated that we aren't + // allowed to use expired responses. In this case, we still get the + // data which we can use in our conditional network request. + resource.priorModified = res.modified; + resource.priorExpires = res.expires; + resource.priorEtag = res.etag; + resource.priorData = res.data; + } else { + loadedData(res); + } + + if (necessity == TileNecessity::Required) { + loadFromNetwork(); + } + break; + } + } while (!shared_->aborted); }); } @@ -164,7 +179,19 @@ void TileLoader::loadFromNetwork() { resource.minimumUpdateInterval = updateParameters.minimumUpdateInterval; resource.storagePolicy = updateParameters.isVolatile ? Resource::StoragePolicy::Volatile : Resource::StoragePolicy::Permanent; - request = fileSource->request(resource, [this](const Response& res) { loadedData(res); }); + + request = fileSource->request(resource, [this, shared_{shared}](const Response& res) { + do { + if (shared_->requestLock.try_lock_shared()) { + std::shared_lock lock(shared_->requestLock, std::adopt_lock); + if (shared_->aborted) return; + + request.reset(); + loadedData(res); + break; + } + } while (!shared_->aborted); + }); } } // namespace mbgl diff --git a/src/mbgl/util/thread_local.hpp b/src/mbgl/util/thread_local.hpp index 8b96dd3080c..a1b2002d817 100644 --- a/src/mbgl/util/thread_local.hpp +++ b/src/mbgl/util/thread_local.hpp @@ -11,7 +11,7 @@ class ThreadLocalBase { ThreadLocalBase(); ~ThreadLocalBase(); - void* get(); + void* get() const; void set(void*); private: @@ -27,7 +27,7 @@ class ThreadLocal : public impl::ThreadLocalBase { ThreadLocal(T* val) { set(val); } - T* get() { return reinterpret_cast(impl::ThreadLocalBase::get()); } + T* get() const { return reinterpret_cast(impl::ThreadLocalBase::get()); } void set(T* ptr) { impl::ThreadLocalBase::set(ptr); } }; diff --git a/src/mbgl/util/thread_pool.cpp b/src/mbgl/util/thread_pool.cpp index e1c18a45b58..4db3e322bee 100644 --- a/src/mbgl/util/thread_pool.cpp +++ b/src/mbgl/util/thread_pool.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -10,11 +11,16 @@ namespace mbgl { ThreadedSchedulerBase::~ThreadedSchedulerBase() = default; void ThreadedSchedulerBase::terminate() { + // Run any leftover render jobs + runRenderJobs(); + { std::lock_guard lock(mutex); terminated = true; } - cv.notify_all(); + + // Wake up all threads so that they shut down + cvAvailable.notify_all(); } std::thread ThreadedSchedulerBase::makeSchedulerThread(size_t index) { @@ -25,13 +31,18 @@ std::thread ThreadedSchedulerBase::makeSchedulerThread(size_t index) { platform::setCurrentThreadPriority(*priority); } - platform::setCurrentThreadName(std::string{"Worker "} + util::toString(index + 1)); + platform::setCurrentThreadName("Worker " + util::toString(index + 1)); platform::attachThread(); + owningThreadPool.set(this); + while (true) { std::unique_lock lock(mutex); + if (queue.empty() && !pendingItems) { + cvEmpty.notify_all(); + } - cv.wait(lock, [this] { return !queue.empty() || terminated; }); + cvAvailable.wait(lock, [this] { return !queue.empty() || terminated; }); if (terminated) { platform::detachThread(); @@ -40,20 +51,83 @@ std::thread ThreadedSchedulerBase::makeSchedulerThread(size_t index) { auto function = std::move(queue.front()); queue.pop(); + + if (function) { + pendingItems++; + } + lock.unlock(); - if (function) function(); + + if (function) { + const auto cleanup = [&] { + // destroy the function and release its captures before unblocking `waitForEmpty` + function = {}; + pendingItems--; + if (queue.empty() && !pendingItems) { + cvEmpty.notify_all(); + } + }; + try { + function(); + cleanup(); + } catch (...) { + lock.lock(); + if (handler) { + handler(std::current_exception()); + } + cleanup(); + if (handler) { + continue; + } + throw; + } + } } }); } -void ThreadedSchedulerBase::schedule(std::function fn) { +void ThreadedSchedulerBase::schedule(std::function&& fn) { assert(fn); - { - std::lock_guard lock(mutex); - queue.push(std::move(fn)); + if (fn) { + { + // We need to block if adding adding a new task from a thread not controlled by this + // pool. Tasks are added by other tasks, so we must not block a thread we do control + // or `waitForEmpty` will deadlock. + std::unique_lock addLock(addMutex, std::defer_lock); + if (!thisThreadIsOwned()) { + addLock.lock(); + } + std::lock_guard lock(mutex); + queue.push(std::move(fn)); + } + cvAvailable.notify_one(); } +} - cv.notify_one(); +std::size_t ThreadedSchedulerBase::waitForEmpty(Milliseconds timeout) { + // Must not be called from a thread in our pool, or we would deadlock + assert(!thisThreadIsOwned()); + if (!thisThreadIsOwned()) { + const auto startTime = util::MonotonicTimer::now(); + const auto isDone = [&] { + return queue.empty() && pendingItems == 0; + }; + // Block any other threads from adding new items + std::scoped_lock addLock(addMutex); + std::unique_lock lock(mutex); + while (!isDone()) { + if (timeout > Milliseconds::zero()) { + const auto elapsed = util::MonotonicTimer::now() - startTime; + if (timeout <= elapsed || !cvEmpty.wait_for(lock, timeout - elapsed, isDone)) { + break; + } + } else { + cvEmpty.wait(lock, isDone); + } + } + return queue.size() + pendingItems; + } + return 0; } } // namespace mbgl diff --git a/src/mbgl/util/thread_pool.hpp b/src/mbgl/util/thread_pool.hpp index 9791ff94601..ee0735082c9 100644 --- a/src/mbgl/util/thread_pool.hpp +++ b/src/mbgl/util/thread_pool.hpp @@ -2,18 +2,20 @@ #include #include +#include -#include +#include #include #include #include #include +#include namespace mbgl { class ThreadedSchedulerBase : public Scheduler { public: - void schedule(std::function) override; + void schedule(std::function&&) override; protected: ThreadedSchedulerBase() = default; @@ -22,9 +24,27 @@ class ThreadedSchedulerBase : public Scheduler { void terminate(); std::thread makeSchedulerThread(size_t index); + /// Wait until there's nothing pending or in process + /// Must not be called from a task provided to this scheduler. + /// @param timeout Time to wait, or zero to wait forever. + std::size_t waitForEmpty(Milliseconds timeout) override; + + /// Returns true if called from a thread managed by the scheduler + bool thisThreadIsOwned() const { return owningThreadPool.get() == this; } + std::queue> queue; + // protects `queue` std::mutex mutex; - std::condition_variable cv; + // Used to block addition of new items while waiting + std::mutex addMutex; + // Signal when an item is added to the queue + std::condition_variable cvAvailable; + // Signal when the queue becomes empty + std::condition_variable cvEmpty; + // Count of functions removed from the queue but still executing + std::atomic pendingItems{0}; + // Points to the owning pool in owned threads + util::ThreadLocal owningThreadPool; bool terminated{false}; }; @@ -36,16 +56,17 @@ class ThreadedSchedulerBase : public Scheduler { * Note: If N == 1 all scheduled tasks are guaranteed to execute consequently; * otherwise, some of the scheduled tasks might be executed in parallel. */ -template class ThreadedScheduler : public ThreadedSchedulerBase { public: - ThreadedScheduler() { - for (std::size_t i = 0u; i < N; ++i) { + ThreadedScheduler(std::size_t n) + : threads(n) { + for (std::size_t i = 0u; i < threads.size(); ++i) { threads[i] = makeSchedulerThread(i); } } ~ThreadedScheduler() override { + assert(!thisThreadIsOwned()); terminate(); for (auto& thread : threads) { assert(std::this_thread::get_id() != thread.get_id()); @@ -53,19 +74,48 @@ class ThreadedScheduler : public ThreadedSchedulerBase { } } + void runOnRenderThread(std::function&& fn) override { + std::lock_guard lock(renderMutex); + renderThreadQueue.push(std::move(fn)); + } + + void runRenderJobs() override { + std::lock_guard lock(renderMutex); + while (renderThreadQueue.size()) { + auto fn = std::move(renderThreadQueue.front()); + renderThreadQueue.pop(); + if (fn) { + fn(); + } + } + } + mapbox::base::WeakPtr makeWeakPtr() override { return weakFactory.makeWeakPtr(); } private: - std::array threads; + std::vector threads; mapbox::base::WeakPtrFactory weakFactory{this}; - static_assert(N > 0, "Thread count must be more than zero."); + + std::queue> renderThreadQueue; + std::mutex renderMutex; }; -class SequencedScheduler : public ThreadedScheduler<1> {}; +class SequencedScheduler : public ThreadedScheduler { +public: + SequencedScheduler() + : ThreadedScheduler(1) {} +}; -template -using ParallelScheduler = ThreadedScheduler<1 + extra>; +class ParallelScheduler : public ThreadedScheduler { +public: + ParallelScheduler(std::size_t extra) + : ThreadedScheduler(1 + extra) {} +}; -class ThreadPool : public ParallelScheduler<3> {}; +class ThreadPool : public ParallelScheduler { +public: + ThreadPool() + : ParallelScheduler(3) {} +}; } // namespace mbgl diff --git a/test/BUILD.bazel b/test/BUILD.bazel index 8c3c3046b26..2848fce98cf 100644 --- a/test/BUILD.bazel +++ b/test/BUILD.bazel @@ -2,9 +2,7 @@ load("//bazel:flags.bzl", "CPP_FLAGS", "MAPLIBRE_FLAGS") cc_library( name = "testutils", - hdrs = [ - "include/mbgl/test/util.hpp", - ], + hdrs = glob(["include/mbgl/test/*.hpp"]), strip_include_prefix = "include", deps = [ "//vendor/googletest:gtest", diff --git a/test/actor/actor.test.cpp b/test/actor/actor.test.cpp index 033a29e86e2..70e5f70481c 100644 --- a/test/actor/actor.test.cpp +++ b/test/actor/actor.test.cpp @@ -91,7 +91,12 @@ TEST(Actor, DestructionBlocksOnSend) { ~TestScheduler() override { EXPECT_TRUE(waited.load()); } - void schedule(std::function) final { + std::size_t waitForEmpty(Milliseconds) override { + assert(false); + return 0; + } + + void schedule(std::function&&) final { promise.set_value(); future.wait(); std::this_thread::sleep_for(1ms); diff --git a/test/include/mbgl/test/vector_tile_test.hpp b/test/include/mbgl/test/vector_tile_test.hpp new file mode 100644 index 00000000000..63b45aba639 --- /dev/null +++ b/test/include/mbgl/test/vector_tile_test.hpp @@ -0,0 +1,49 @@ +#include +#include +#include +#include +#include +#include +#include + +#include + +#include +#include +#include + +namespace mbgl { + +class VectorTileTest { +public: + std::shared_ptr fileSource = std::make_shared(); + TransformState transformState; + util::RunLoop loop; + style::Style style{fileSource, 1}; + AnnotationManager annotationManager{style}; + + const std::shared_ptr imageManager = std::make_shared(); + const std::shared_ptr glyphManager = std::make_shared(); + + Tileset tileset{{"https://example.com"}, {0, 22}, "none"}; + + const std::shared_ptr threadPool = Scheduler::GetBackground(); + + TileParameters tileParameters{1.0, + MapDebugOptions(), + transformState, + fileSource, + MapMode::Continuous, + annotationManager.makeWeakPtr(), + imageManager, + glyphManager, + 0}; + + ~VectorTileTest() { + // Ensure that deferred releases are complete before cleaning up + EXPECT_EQ(0, loop.waitForEmpty(Milliseconds::zero())); + EXPECT_EQ(0, threadPool->waitForEmpty()); + } +}; + +} // namespace mbgl diff --git a/test/map/map.test.cpp b/test/map/map.test.cpp index 81050434ef0..856fd2999e6 100644 --- a/test/map/map.test.cpp +++ b/test/map/map.test.cpp @@ -1451,11 +1451,14 @@ TEST(Map, KeepRenderData) { test.map.getStyle().loadURL("maptiler://maps/streets"); const int iterations = 3; const int resourcesCount = 4 /*tiles*/; + + requestsCount = 0; // Keep render data. for (int i = 1; i <= iterations; ++i) { test.frontend.render(test.map); EXPECT_EQ(resourcesCount, requestsCount); } + requestsCount = 0; // Clear render data. for (int i = 1; i <= iterations; ++i) { diff --git a/test/renderer/image_manager.test.cpp b/test/renderer/image_manager.test.cpp index 7eafee86a86..c5d82527cc9 100644 --- a/test/renderer/image_manager.test.cpp +++ b/test/renderer/image_manager.test.cpp @@ -33,6 +33,8 @@ TEST(ImageManager, Basic) { ASSERT_TRUE(stored); EXPECT_EQ(image->image.size, stored->image.size); } + + imageManager.dumpDebugLogs(); } TEST(ImageManager, AddRemove) { @@ -79,7 +81,7 @@ TEST(ImageManager, RemoveReleasesBinPackRect) { class StubImageRequestor : public ImageRequestor { public: - StubImageRequestor(ImageManager& imageManager_) + StubImageRequestor(std::shared_ptr imageManager_) : ImageRequestor(imageManager_) {} void onImagesAvailable(ImageMap icons, @@ -95,8 +97,9 @@ class StubImageRequestor : public ImageRequestor { TEST(ImageManager, NotifiesRequestorWhenSpriteIsLoaded) { util::RunLoop runLoop; - ImageManager imageManager; - StubImageRequestor requestor(imageManager); + auto imageManagerPtr = std::make_shared(); + auto& imageManager = *imageManagerPtr; + StubImageRequestor requestor(imageManagerPtr); bool notified = false; ImageManagerObserver observer; @@ -123,8 +126,9 @@ TEST(ImageManager, NotifiesRequestorWhenSpriteIsLoaded) { } TEST(ImageManager, NotifiesRequestorImmediatelyIfDependenciesAreSatisfied) { - ImageManager imageManager; - StubImageRequestor requestor(imageManager); + auto imageManagerPtr = std::make_shared(); + auto& imageManager = *imageManagerPtr; + StubImageRequestor requestor(imageManagerPtr); bool notified = false; requestor.imagesAvailable = [&](ImageMap, ImageMap, std::unordered_map) { @@ -160,8 +164,9 @@ class StubImageManagerObserver : public ImageManagerObserver { TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) { util::RunLoop runLoop; - ImageManager imageManager; - StubImageRequestor requestor(imageManager); + auto imageManagerPtr = std::make_shared(); + auto& imageManager = *imageManagerPtr; + StubImageRequestor requestor(imageManagerPtr); StubImageManagerObserver observer; imageManager.setObserver(&observer); @@ -203,7 +208,7 @@ TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) { ASSERT_FALSE(requestor.hasPendingRequests()); // Another requestor shall not have pending requests for already obtained images. - StubImageRequestor anotherRequestor(imageManager); + StubImageRequestor anotherRequestor(imageManagerPtr); imageManager.getImages(anotherRequestor, std::make_pair(dependencies, ++imageCorrelationID)); ASSERT_FALSE(anotherRequestor.hasPendingRequests()); @@ -215,8 +220,9 @@ TEST(ImageManager, OnStyleImageMissingBeforeSpriteLoaded) { TEST(ImageManager, OnStyleImageMissingAfterSpriteLoaded) { util::RunLoop runLoop; - ImageManager imageManager; - StubImageRequestor requestor(imageManager); + auto imageManagerPtr = std::make_shared(); + auto& imageManager = *imageManagerPtr; + StubImageRequestor requestor(imageManagerPtr); StubImageManagerObserver observer; imageManager.setObserver(&observer); @@ -251,7 +257,8 @@ TEST(ImageManager, OnStyleImageMissingAfterSpriteLoaded) { TEST(ImageManager, RemoveUnusedStyleImages) { util::RunLoop runLoop; - ImageManager imageManager; + auto imageManagerPtr = std::make_shared(); + auto& imageManager = *imageManagerPtr; StubImageManagerObserver observer; imageManager.setObserver(&observer); imageManager.setLoaded(true); @@ -276,7 +283,7 @@ TEST(ImageManager, RemoveUnusedStyleImages) { // Single requestor { - std::unique_ptr requestor = std::make_unique(imageManager); + std::unique_ptr requestor = std::make_unique(imageManagerPtr); imageManager.getImages(*requestor, std::make_pair(ImageDependencies{{"missing", ImageType::Icon}}, 0ull)); runLoop.runOnce(); EXPECT_EQ(observer.count, 1); @@ -296,7 +303,7 @@ TEST(ImageManager, RemoveUnusedStyleImages) { // Single requestor, exceed cache size limit. { - std::unique_ptr requestor = std::make_unique(imageManager); + std::unique_ptr requestor = std::make_unique(imageManagerPtr); imageManager.getImages(*requestor, std::make_pair(ImageDependencies{{"1024px", ImageType::Icon}}, 0ull)); runLoop.runOnce(); EXPECT_EQ(observer.count, 2); @@ -311,8 +318,8 @@ TEST(ImageManager, RemoveUnusedStyleImages) { // Multiple requestors { - std::unique_ptr requestor1 = std::make_unique(imageManager); - std::unique_ptr requestor2 = std::make_unique(imageManager); + std::unique_ptr requestor1 = std::make_unique(imageManagerPtr); + std::unique_ptr requestor2 = std::make_unique(imageManagerPtr); imageManager.getImages(*requestor1, std::make_pair(ImageDependencies{{"missing", ImageType::Icon}}, 0ull)); imageManager.getImages(*requestor2, std::make_pair(ImageDependencies{{"missing", ImageType::Icon}}, 1ull)); runLoop.runOnce(); @@ -330,9 +337,9 @@ TEST(ImageManager, RemoveUnusedStyleImages) { // Multiple requestors, check that image resource is not destroyed if there // is at least 1 requestor that uses it. - std::unique_ptr requestor = std::make_unique(imageManager); + std::unique_ptr requestor = std::make_unique(imageManagerPtr); { - std::unique_ptr requestor1 = std::make_unique(imageManager); + std::unique_ptr requestor1 = std::make_unique(imageManagerPtr); imageManager.getImages( *requestor, std::make_pair(ImageDependencies{{"missing", ImageType::Icon}, {"1024px", ImageType::Icon}}, 0ull)); diff --git a/test/style/source.test.cpp b/test/style/source.test.cpp index b14f53af199..0a2a52b5afd 100644 --- a/test/style/source.test.cpp +++ b/test/style/source.test.cpp @@ -62,8 +62,9 @@ class SourceTest { TransformState transformState; Style style{fileSource, 1}; AnnotationManager annotationManager{style}; - ImageManager imageManager; - GlyphManager glyphManager; + std::shared_ptr imageManager = std::make_shared(); + std::shared_ptr glyphManager = std::make_shared(); + std::shared_ptr threadPool = Scheduler::GetBackground(); TileParameters tileParameters(MapMode mapMode = MapMode::Continuous) { return {1.0, @@ -87,6 +88,8 @@ class SourceTest { transformState = transform.getState(); } + ~SourceTest() { threadPool->waitForEmpty(); } + void run() { loop.run(); } void end() { loop.stop(); } @@ -167,7 +170,7 @@ TEST(Source, RasterTileEmpty) { FAIL() << "Should never be called"; }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -203,7 +206,7 @@ TEST(Source, RasterDEMTileEmpty) { FAIL() << "Should never be called"; }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -241,7 +244,7 @@ TEST(Source, VectorTileEmpty) { FAIL() << "Should never be called"; }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -276,7 +279,7 @@ TEST(Source, RasterTileFail) { test.end(); }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -311,7 +314,7 @@ TEST(Source, RasterDEMTileFail) { test.end(); }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -348,7 +351,7 @@ TEST(Source, VectorTileFail) { test.end(); }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -384,7 +387,7 @@ TEST(Source, RasterTileCorrupt) { test.end(); }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -421,7 +424,7 @@ TEST(Source, RasterDEMTileCorrupt) { test.end(); }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -458,7 +461,7 @@ TEST(Source, VectorTileCorrupt) { test.end(); }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -492,7 +495,7 @@ TEST(Source, RasterTileCancel) { FAIL() << "Should never be called"; }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -526,7 +529,7 @@ TEST(Source, RasterDEMTileCancel) { FAIL() << "Should never be called"; }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -562,7 +565,7 @@ TEST(Source, VectorTileCancel) { FAIL() << "Should never be called"; }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -607,7 +610,7 @@ TEST(Source, RasterTileAttribution) { source.setObserver(&test.styleObserver); source.loadDescription(*test.fileSource); - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); test.run(); @@ -648,7 +651,7 @@ TEST(Source, RasterDEMTileAttribution) { source.setObserver(&test.styleObserver); source.loadDescription(*test.fileSource); - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); test.run(); @@ -741,7 +744,7 @@ TEST(Source, CustomGeometrySourceSetTileData) { FAIL() << "Should never be called"; }; - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); @@ -778,8 +781,8 @@ class FakeTileSource : public RenderTileSetSource { MOCK_METHOD1(tileSetNecessity, void(TileNecessity)); MOCK_METHOD1(tileSetMinimumUpdateInterval, void(Duration)); - explicit FakeTileSource(Immutable impl_) - : RenderTileSetSource(std::move(impl_)) {} + explicit FakeTileSource(Immutable impl_, std::shared_ptr threadPool_) + : RenderTileSetSource(std::move(impl_), std::move(threadPool_)) {} void updateInternal(const Tileset& tileset, const std::vector>& layers, const bool needsRendering, @@ -816,7 +819,7 @@ TEST(Source, InvisibleSourcesTileNecessity) { VectorSource initialized("source", Tileset{{"tiles"}}); initialized.loadDescription(*test.fileSource); - FakeTileSource renderTilesetSource{initialized.baseImpl}; + FakeTileSource renderTilesetSource{initialized.baseImpl, test.threadPool}; RenderSource* renderSource = &renderTilesetSource; LineLayer layer("id", "source"); Immutable layerProperties = makeMutable( @@ -839,7 +842,7 @@ TEST(Source, SourceMinimumUpdateInterval) { VectorSource initialized("source", Tileset{{"tiles"}}); initialized.loadDescription(*test.fileSource); - FakeTileSource renderTilesetSource{initialized.baseImpl}; + FakeTileSource renderTilesetSource{initialized.baseImpl, test.threadPool}; RenderSource* renderSource = &renderTilesetSource; LineLayer layer("id", "source"); Immutable layerProperties = makeMutable( @@ -882,8 +885,8 @@ TEST(Source, RenderTileSetSourceUpdate) { class FakeRenderTileSetSource : public RenderTileSetSource { public: - explicit FakeRenderTileSetSource(Immutable impl_) - : RenderTileSetSource(std::move(impl_)) {} + explicit FakeRenderTileSetSource(Immutable impl_, std::shared_ptr threadPool_) + : RenderTileSetSource(std::move(impl_), std::move(threadPool_)) {} MOCK_METHOD0(mockedUpdateInternal, void()); @@ -903,7 +906,7 @@ TEST(Source, RenderTileSetSourceUpdate) { VectorSource initialized("source", Tileset{{"tiles"}}); initialized.loadDescription(*test.fileSource); - FakeRenderTileSetSource renderTilesetSource{initialized.baseImpl}; + FakeRenderTileSetSource renderTilesetSource{initialized.baseImpl, test.threadPool}; LineLayer layer("id", "source"); Immutable layerProperties = makeMutable( @@ -969,7 +972,7 @@ TEST(Source, GeoJSONSourceTilesAfterDataReset) { auto geoJSONData = GeoJSONData::create(mapbox::geojson::parse( R"({"geometry": {"type": "Point", "coordinates": [1.1, 1.1]}, "type": "Feature", "properties": {}})")); source.setGeoJSONData(geoJSONData); - RenderGeoJSONSource renderSource{staticImmutableCast(source.baseImpl)}; + RenderGeoJSONSource renderSource{staticImmutableCast(source.baseImpl), test.threadPool}; CircleLayer layer("id", "source"); Immutable layerProperties = makeMutable( @@ -1026,7 +1029,7 @@ TEST(Source, SetMaxParentOverscaleFactor) { ASSERT_EQ(3, *source.getMaxOverscaleFactorForParentTiles()); source.loadDescription(*test.fileSource); - auto renderSource = RenderSource::create(source.baseImpl); + auto renderSource = RenderSource::create(source.baseImpl, test.threadPool); renderSource->setObserver(&test.renderSourceObserver); renderSource->update(source.baseImpl, layers, true, true, test.tileParameters()); diff --git a/test/tile/custom_geometry_tile.test.cpp b/test/tile/custom_geometry_tile.test.cpp index ae48d9aaa15..d80e4cd878b 100644 --- a/test/tile/custom_geometry_tile.test.cpp +++ b/test/tile/custom_geometry_tile.test.cpp @@ -27,8 +27,8 @@ class CustomTileTest { util::RunLoop loop; style::Style style{fileSource, 1}; AnnotationManager annotationManager{style}; - ImageManager imageManager; - GlyphManager glyphManager; + std::shared_ptr imageManager = std::make_shared(); + std::shared_ptr glyphManager = std::make_shared(); TileParameters tileParameters{1.0, MapDebugOptions(), diff --git a/test/tile/geojson_tile.test.cpp b/test/tile/geojson_tile.test.cpp index ad2eb682f93..2db82f01e50 100644 --- a/test/tile/geojson_tile.test.cpp +++ b/test/tile/geojson_tile.test.cpp @@ -27,8 +27,8 @@ class GeoJSONTileTest { util::RunLoop loop; style::Style style{fileSource, 1}; AnnotationManager annotationManager{style}; - ImageManager imageManager; - GlyphManager glyphManager; + std::shared_ptr imageManager = std::make_shared(); + std::shared_ptr glyphManager = std::make_shared(); Tileset tileset{{"https://example.com"}, {0, 22}, "none"}; TileParameters tileParameters{1.0, diff --git a/test/tile/raster_dem_tile.test.cpp b/test/tile/raster_dem_tile.test.cpp index 90803d97ffe..d727d3c777c 100644 --- a/test/tile/raster_dem_tile.test.cpp +++ b/test/tile/raster_dem_tile.test.cpp @@ -21,8 +21,8 @@ class RasterDEMTileTest { util::RunLoop loop; style::Style style{fileSource, 1}; AnnotationManager annotationManager{style}; - ImageManager imageManager; - GlyphManager glyphManager; + std::shared_ptr imageManager = std::make_shared(); + std::shared_ptr glyphManager = std::make_shared(); Tileset tileset{{"https://example.com"}, {0, 22}, "none"}; TileParameters tileParameters{1.0, diff --git a/test/tile/raster_tile.test.cpp b/test/tile/raster_tile.test.cpp index 34aba76f175..213801d20d1 100644 --- a/test/tile/raster_tile.test.cpp +++ b/test/tile/raster_tile.test.cpp @@ -21,8 +21,8 @@ class RasterTileTest { util::RunLoop loop; style::Style style{fileSource, 1}; AnnotationManager annotationManager{style}; - ImageManager imageManager; - GlyphManager glyphManager; + std::shared_ptr imageManager = std::make_shared(); + std::shared_ptr glyphManager = std::make_shared(); Tileset tileset{{"https://example.com"}, {0, 22}, "none"}; TileParameters tileParameters{1.0, diff --git a/test/tile/tile_cache.test.cpp b/test/tile/tile_cache.test.cpp index 64444f91cf6..51c0c92ef8f 100644 --- a/test/tile/tile_cache.test.cpp +++ b/test/tile/tile_cache.test.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -16,34 +17,16 @@ #include #include #include +#include #include #include +#include #include using namespace mbgl; -class VectorTileTest { -public: - std::shared_ptr fileSource = std::make_shared(); - TransformState transformState; - util::RunLoop loop; - style::Style style{fileSource, 1}; - AnnotationManager annotationManager{style}; - ImageManager imageManager; - GlyphManager glyphManager; - Tileset tileset{{"https://example.com"}, {0, 22}, "none"}; - - TileParameters tileParameters{1.0, - MapDebugOptions(), - transformState, - fileSource, - MapMode::Continuous, - annotationManager.makeWeakPtr(), - imageManager, - glyphManager, - 0}; -}; +namespace { class VectorTileMock : public VectorTile { public: @@ -51,16 +34,20 @@ class VectorTileMock : public VectorTile { std::string sourceID_, const TileParameters& parameters, const Tileset& tileset) - : VectorTile(id_, sourceID_, parameters, tileset) { + : VectorTile(id_, std::move(sourceID_), parameters, tileset) { renderable = true; } + + util::SimpleIdentity uniqueId; }; +} // namespace + TEST(TileCache, Smoke) { VectorTileTest test; - TileCache cache(1); - OverscaledTileID id(0, 0, 0); - std::unique_ptr tile = std::make_unique(id, "source", test.tileParameters, test.tileset); + TileCache cache(Scheduler::GetBackground(), 1); + const OverscaledTileID id(0, 0, 0); + auto tile = std::make_unique(id, "source", test.tileParameters, test.tileset); cache.add(id, std::move(tile)); EXPECT_TRUE(cache.has(id)); @@ -70,18 +57,36 @@ TEST(TileCache, Smoke) { TEST(TileCache, Issue15926) { VectorTileTest test; - TileCache cache(2); - OverscaledTileID id0(0, 0, 0); - OverscaledTileID id1(1, 0, 0); - std::unique_ptr tile1 = std::make_unique(id0, "source", test.tileParameters, test.tileset); - std::unique_ptr tile2 = std::make_unique(id0, "source", test.tileParameters, test.tileset); - std::unique_ptr tile3 = std::make_unique(id1, "source", test.tileParameters, test.tileset); + TileCache cache(test.threadPool, 2); + const OverscaledTileID id0(0, 0, 0); + const OverscaledTileID id1(1, 0, 0); + auto tile1 = std::make_unique(id0, "source", test.tileParameters, test.tileset); + auto tile2 = std::make_unique(id0, "source", test.tileParameters, test.tileset); + auto tile3 = std::make_unique(id1, "source", test.tileParameters, test.tileset); + auto tile4 = std::make_unique(id0, "source", test.tileParameters, test.tileset); + const auto tile1Id = tile1->uniqueId; + // add cache.add(id0, std::move(tile1)); EXPECT_TRUE(cache.has(id0)); + + // adding a key already present doesn't replace the existing item cache.add(id0, std::move(tile2)); + EXPECT_EQ(tile1Id, static_cast(cache.get(id0))->uniqueId); + + // Evict on add cache.setSize(1); cache.add(id1, std::move(tile3)); EXPECT_FALSE(cache.has(id0)); EXPECT_TRUE(cache.has(id1)); + + // Evict due to size limit change + cache.setSize(2); + cache.add(id0, std::move(tile4)); + EXPECT_TRUE(cache.has(id0)); + EXPECT_TRUE(cache.has(id1)); + cache.setSize(1); + // older item should be evicted + EXPECT_TRUE(cache.has(id0)); + EXPECT_FALSE(cache.has(id1)); } diff --git a/test/tile/vector_tile.test.cpp b/test/tile/vector_tile.test.cpp index 375d6017a6b..0c1a8286bd2 100644 --- a/test/tile/vector_tile.test.cpp +++ b/test/tile/vector_tile.test.cpp @@ -15,35 +15,13 @@ #include #include #include +#include #include #include using namespace mbgl; -class VectorTileTest { -public: - std::shared_ptr fileSource = std::make_shared(ResourceOptions::Default(), - ClientOptions()); - TransformState transformState; - util::RunLoop loop; - style::Style style{fileSource, 1}; - AnnotationManager annotationManager{style}; - ImageManager imageManager; - GlyphManager glyphManager; - Tileset tileset{{"https://example.com"}, {0, 22}, "none"}; - - TileParameters tileParameters{1.0, - MapDebugOptions(), - transformState, - fileSource, - MapMode::Continuous, - annotationManager.makeWeakPtr(), - imageManager, - glyphManager, - 0}; -}; - TEST(VectorTile, setError) { VectorTileTest test; VectorTile tile(OverscaledTileID(0, 0, 0), "source", test.tileParameters, test.tileset); diff --git a/test/util/thread.test.cpp b/test/util/thread.test.cpp index 5ea98b36b6f..01bfce2304b 100644 --- a/test/util/thread.test.cpp +++ b/test/util/thread.test.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include #include @@ -134,6 +135,11 @@ TEST(Thread, Concurrency) { unsigned numMessages = 100000; std::atomic_uint completed(numMessages); + auto& settings = platform::Settings::getInstance(); + if (!settings.get(platform::EXPERIMENTAL_THREAD_PRIORITY_WORKER).getDouble()) { + settings.set(platform::EXPERIMENTAL_THREAD_PRIORITY_WORKER, 0.5); + } + Actor poolWorker(Scheduler::GetBackground()); auto poolWorkerRef = poolWorker.self(); @@ -328,3 +334,108 @@ TEST(Thread, DeleteBeforeChildStarts) { // Should process the queue before destruction. ASSERT_TRUE(flag); } + +TEST(Thread, PoolWait) { + auto pool = Scheduler::GetBackground(); + + constexpr int threadCount = 10; + for (int i = 0; i < threadCount; ++i) { + pool->schedule([&] { std::this_thread::sleep_for(Milliseconds(100)); }); + } + + EXPECT_EQ(0, pool->waitForEmpty()); +} + +TEST(Thread, PoolWaitRecursiveAdd) { + auto pool = Scheduler::GetBackground(); + + pool->schedule([&] { + // Scheduled tasks can add more tasks + pool->schedule([&] { + std::this_thread::sleep_for(Milliseconds(10)); + pool->schedule([&] { std::this_thread::sleep_for(Milliseconds(10)); }); + }); + std::this_thread::sleep_for(Milliseconds(10)); + }); + + EXPECT_EQ(0, pool->waitForEmpty()); +} + +TEST(Thread, PoolWaitAdd) { + auto pool = Scheduler::GetBackground(); + auto seq = Scheduler::GetSequenced(); + + // add new tasks every few milliseconds + std::atomic addActive{true}; + std::atomic added{0}; + std::atomic executed{0}; + seq->schedule([&] { + while (addActive) { + pool->schedule([&] { executed++; }); + added++; + } + }); + + // Wait be sure some are added + while (added < 1) { + std::this_thread::sleep_for(Milliseconds(10)); + } + + // Add an item that should take long enough to be confident that + // more items would be added by the sequential task if not blocked + pool->schedule([&] { std::this_thread::sleep_for(Milliseconds(100)); }); + + EXPECT_EQ(0, pool->waitForEmpty()); + + addActive = false; + EXPECT_EQ(0, pool->waitForEmpty()); +} + +TEST(Thread, PoolWaitTimeout) { + auto pool = Scheduler::GetBackground(); + + std::mutex mutex; + { + std::lock_guard outerLock(mutex); + pool->schedule([&] { std::lock_guard innerLock(mutex); }); + + // should always time out + EXPECT_EQ(1, pool->waitForEmpty(Milliseconds(100))); + } + + EXPECT_EQ(0, pool->waitForEmpty()); +} + +TEST(Thread, PoolWaitException) { + auto pool = Scheduler::GetBackground(); + + std::atomic caught{0}; + pool->setExceptionHandler([&](const auto) { caught++; }); + + constexpr int threadCount = 3; + for (int i = 0; i < threadCount; ++i) { + pool->schedule([=] { + std::this_thread::sleep_for(Milliseconds(i)); + if (i & 1) { + throw std::runtime_error("test"); + } else { + throw 1; + } + }); + } + + // Exceptions shouldn't cause deadlocks by, e.g., abandoning locks. + EXPECT_EQ(0, pool->waitForEmpty()); + EXPECT_EQ(threadCount, caught); +} + +#if defined(NDEBUG) +TEST(Thread, WrongThread) { + auto pool = Scheduler::GetBackground(); + + // Asserts in debug builds, silently ignored in release. + pool->schedule([&] { EXPECT_EQ(0, pool->waitForEmpty()); }); + + EXPECT_EQ(0, pool->waitForEmpty()); +} +#endif