From 12f596ce7dfeabd22e9360707fbf95928e8bedaa Mon Sep 17 00:00:00 2001 From: Tim Sylvester Date: Fri, 13 Dec 2024 10:21:48 -0800 Subject: [PATCH] Batch up scheduling of deferred deletions (#3030) --- src/mbgl/renderer/tile_pyramid.cpp | 3 ++ src/mbgl/tile/tile_cache.cpp | 56 ++++++++++++++++++++++++------ src/mbgl/tile/tile_cache.hpp | 16 ++++----- src/mbgl/vulkan/context.cpp | 2 +- src/mbgl/vulkan/descriptor_set.cpp | 12 ++++--- 5 files changed, 62 insertions(+), 27 deletions(-) diff --git a/src/mbgl/renderer/tile_pyramid.cpp b/src/mbgl/renderer/tile_pyramid.cpp index 4533e6f7549..3e79657b5bf 100644 --- a/src/mbgl/renderer/tile_pyramid.cpp +++ b/src/mbgl/renderer/tile_pyramid.cpp @@ -82,6 +82,7 @@ void TilePyramid::update(const std::vector>& l tiles.clear(); renderedTiles.clear(); + cache.deferPendingReleases(); return; } @@ -279,6 +280,8 @@ void TilePyramid::update(const std::vector>& l tile.usedByRenderedLayers |= tile.layerPropertiesUpdated(layerProperties); } } + + cache.deferPendingReleases(); } void TilePyramid::handleWrapJump(float lng) { diff --git a/src/mbgl/tile/tile_cache.cpp b/src/mbgl/tile/tile_cache.cpp index 2bbde03a82b..5104c3ea6ef 100644 --- a/src/mbgl/tile/tile_cache.cpp +++ b/src/mbgl/tile/tile_cache.cpp @@ -1,10 +1,22 @@ #include + #include #include + #include namespace mbgl { +TileCache::~TileCache() { + MLN_TRACE_FUNC(); + + clear(); + pendingReleases.clear(); + + std::unique_lock counterLock{deferredSignalLock}; + deferredSignal.wait(counterLock, [&]() { return deferredDeletionsPending == 0; }); +} + void TileCache::setSize(size_t size_) { MLN_TRACE_FUNC(); @@ -26,16 +38,20 @@ void TileCache::setSize(size_t size_) { } 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(std::vector>&& items_) + : items(items_.size()) { + std::ranges::move(items_, items.begin()); + } + CaptureWrapper(CaptureWrapper&&) = default; + + /// This copy constructor is required to build, but doesn't seem to be called. CaptureWrapper(const CaptureWrapper& other) - : item(other.item) {} - std::shared_ptr item; + : items(other.items) {} + + std::vector> items; }; } // namespace @@ -43,6 +59,25 @@ void TileCache::deferredRelease(std::unique_ptr&& tile) { MLN_TRACE_FUNC(); tile->cancel(); + pendingReleases.push_back(std::move(tile)); +} + +void TileCache::deferPendingReleases() { + MLN_TRACE_FUNC(); + + constexpr std::size_t scheduleThreshold = 1; + if (pendingReleases.size() < scheduleThreshold) { + return; + } + + // Block destruction until the cleanup task is complete + { + std::lock_guard counterLock{deferredSignalLock}; + deferredDeletionsPending++; + } + + CaptureWrapper wrap{std::move(pendingReleases)}; + pendingReleases.clear(); // 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 @@ -51,17 +86,16 @@ void TileCache::deferredRelease(std::unique_ptr&& tile) { // 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)}}, this]() mutable { - tile_.item = {}; + std::function func{[tile_{CaptureWrapper{std::move(wrap)}}, this]() mutable { + MLN_TRACE_ZONE(deferPendingReleases lambda); + MLN_ZONE_VALUE(wrap_.releases.size()); + tile_.items.clear(); std::lock_guard counterLock(deferredSignalLock); deferredDeletionsPending--; deferredSignal.notify_all(); }}; - std::unique_lock counterLock(deferredSignalLock); - deferredDeletionsPending++; - threadPool.schedule(std::move(func)); } diff --git a/src/mbgl/tile/tile_cache.hpp b/src/mbgl/tile/tile_cache.hpp index db9f8658c46..fea185a32ba 100644 --- a/src/mbgl/tile/tile_cache.hpp +++ b/src/mbgl/tile/tile_cache.hpp @@ -18,15 +18,7 @@ class TileCache { TileCache(const TaggedScheduler& threadPool_, size_t size_ = 0) : threadPool(threadPool_), size(size_) {} - - ~TileCache() { - clear(); - - std::unique_lock counterLock(deferredSignalLock); - while (deferredDeletionsPending != 0) { - deferredSignal.wait(counterLock); - } - } + ~TileCache(); /// Change the maximum size of the cache. void setSize(size_t); @@ -43,13 +35,17 @@ class TileCache { bool has(const OverscaledTileID& key); void clear(); - /// Destroy a tile without blocking + /// Set aside a tile to be destroyed later, without blocking void deferredRelease(std::unique_ptr&&); + /// Schedule any accumulated deferred tiles to be destroyed + void deferPendingReleases(); + private: std::map> tiles; std::list orderedKeys; TaggedScheduler threadPool; + std::vector> pendingReleases; size_t deferredDeletionsPending{0}; std::mutex deferredSignalLock; std::condition_variable deferredSignal; diff --git a/src/mbgl/vulkan/context.cpp b/src/mbgl/vulkan/context.cpp index 9f54ebed28b..e3055c4054f 100644 --- a/src/mbgl/vulkan/context.cpp +++ b/src/mbgl/vulkan/context.cpp @@ -596,7 +596,7 @@ void Context::buildUniformDescriptorSetLayout(vk::UniqueDescriptorSetLayout& lay for (size_t i = 0; i < uniformCount; ++i) { bindings.push_back(vk::DescriptorSetLayoutBinding() - .setBinding(i) + .setBinding(static_cast(i)) .setStageFlags(stageFlags) .setDescriptorType(vk::DescriptorType::eUniformBuffer) .setDescriptorCount(1)); diff --git a/src/mbgl/vulkan/descriptor_set.cpp b/src/mbgl/vulkan/descriptor_set.cpp index d8d6866e5e7..f27f0a23c3e 100644 --- a/src/mbgl/vulkan/descriptor_set.cpp +++ b/src/mbgl/vulkan/descriptor_set.cpp @@ -50,7 +50,7 @@ void DescriptorSet::createDescriptorPool(DescriptorPoolGrowable& growablePool) { const auto descriptorPoolInfo = vk::DescriptorPoolCreateInfo(poolFlags).setPoolSizes(size).setMaxSets(maxSets); growablePool.pools.emplace_back(device->createDescriptorPoolUnique(descriptorPoolInfo), maxSets); - growablePool.currentPoolIndex = growablePool.pools.size() - 1; + growablePool.currentPoolIndex = static_cast(growablePool.pools.size() - 1); }; void DescriptorSet::allocate() { @@ -73,7 +73,8 @@ void DescriptorSet::allocate() { growablePool.pools.begin(), growablePool.pools.end(), [&](const auto& p) { return !p.unusedSets.empty(); }); if (unusedPoolIt != growablePool.pools.end()) { - growablePool.currentPoolIndex = std::distance(growablePool.pools.begin(), unusedPoolIt); + growablePool.currentPoolIndex = static_cast( + std::distance(growablePool.pools.begin(), unusedPoolIt)); } else #endif { @@ -83,7 +84,8 @@ void DescriptorSet::allocate() { [&](const auto& p) { return p.remainingSets >= layouts.size(); }); if (freePoolIt != growablePool.pools.end()) { - growablePool.currentPoolIndex = std::distance(growablePool.pools.begin(), freePoolIt); + growablePool.currentPoolIndex = static_cast( + std::distance(growablePool.pools.begin(), freePoolIt)); } else { createDescriptorPool(growablePool); } @@ -161,7 +163,7 @@ void UniformDescriptorSet::update(const gfx::UniformBufferArray& uniforms, .setBufferInfo(descriptorBufferInfo) .setDescriptorCount(1) .setDescriptorType(vk::DescriptorType::eUniformBuffer) - .setDstBinding(index) + .setDstBinding(static_cast(index)) .setDstSet(descriptorSets[frameIndex]); device->updateDescriptorSets(writeDescriptorSet, nullptr); @@ -197,7 +199,7 @@ void ImageDescriptorSet::update(const std::array(id)) .setDstSet(descriptorSets[frameIndex]); device->updateDescriptorSets(writeDescriptorSet, nullptr);