From 0d90208265f787d5c8e4742c4300ca4be154b58a Mon Sep 17 00:00:00 2001 From: Tim Sylvester Date: Wed, 20 Nov 2024 13:49:57 -0800 Subject: [PATCH] back out capture changes --- CMakeLists.txt | 1 - bazel/core.bzl | 1 - include/mbgl/util/unique_function.hpp | 71 --------------------------- src/mbgl/tile/tile_cache.cpp | 38 +++++++------- 4 files changed, 21 insertions(+), 90 deletions(-) delete mode 100644 include/mbgl/util/unique_function.hpp diff --git a/CMakeLists.txt b/CMakeLists.txt index b7b2b3c77bc..b5a5a2792dd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -435,7 +435,6 @@ list(APPEND INCLUDE_FILES ${PROJECT_SOURCE_DIR}/include/mbgl/util/traits.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/type_list.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/unitbezier.hpp - ${PROJECT_SOURCE_DIR}/include/mbgl/util/unique_function.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/util.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/variant.hpp ${PROJECT_SOURCE_DIR}/include/mbgl/util/vectors.hpp diff --git a/bazel/core.bzl b/bazel/core.bzl index c6781ca3cfd..472fde18105 100644 --- a/bazel/core.bzl +++ b/bazel/core.bzl @@ -861,7 +861,6 @@ MLN_CORE_HEADERS = [ "include/mbgl/util/traits.hpp", "include/mbgl/util/type_list.hpp", "include/mbgl/util/unitbezier.hpp", - "include/mbgl/util/unique_function.hpp", "include/mbgl/util/util.hpp", "include/mbgl/util/variant.hpp", "include/mbgl/util/vectors.hpp", diff --git a/include/mbgl/util/unique_function.hpp b/include/mbgl/util/unique_function.hpp deleted file mode 100644 index 880748ad79e..00000000000 --- a/include/mbgl/util/unique_function.hpp +++ /dev/null @@ -1,71 +0,0 @@ -#pragma once - -#include - -namespace mbgl { -namespace util { - -// C++20 non-copying lambda capture, pending C++23 `move_only_function` -// Adapted from https://stackoverflow.com/a/52358928/135138 -template -class unique_function : public std::function { - using base = std::function; - -public: - unique_function() noexcept = default; - - unique_function(std::nullptr_t) noexcept - : base(nullptr) {} - - template - requires requires(Fn f) { - { f() } -> std::same_as>; - } - unique_function(Fn &&f) - : base(wrapper>{std::forward>(f)}) {} - - unique_function(unique_function &&) = default; - - unique_function &operator=(unique_function &&) = default; - - template - requires requires(Fn f) { - { f() } -> std::same_as>; - } - unique_function &operator=(Fn &&f) { - base::operator=(wrapper>{std::forward>(f)}); - return *this; - } - - using base::operator(); - -private: - template - struct wrapper; - - // specialization for MoveConstructible-only Fn - template - struct wrapper && std::is_move_constructible_v>> { - Fn fn; - - wrapper(Fn &&fn_) - : fn(std::forward(fn_)) {} - wrapper(wrapper &&) = default; - wrapper &operator=(wrapper &&) = default; - - // these two functions are instantiated by std::function and are never called - // We can't delete this or `fn` is uninitialized for non-DefaultContructible types - wrapper(const wrapper &rhs) - : fn(const_cast(rhs.fn)) { - throw std::runtime_error{{}}; - } - wrapper &operator=(const wrapper &) = delete; - - template - auto operator()(Args &&...args) { - return fn(std::forward(args)...); - } - }; -}; -} // namespace util -} // namespace mbgl diff --git a/src/mbgl/tile/tile_cache.cpp b/src/mbgl/tile/tile_cache.cpp index 3bbaa2b083f..c0c534a2c75 100644 --- a/src/mbgl/tile/tile_cache.cpp +++ b/src/mbgl/tile/tile_cache.cpp @@ -2,7 +2,6 @@ #include #include -#include #include @@ -41,17 +40,17 @@ void TileCache::setSize(size_t size_) { } namespace { -// Capturing `std::vector>` directly produces an error related to -// copying, but this somehow avoids the same problem. It may be possible to eliminate it. +/// This exists solely to prevent a problem where temporary lambda captures +/// are retained for the duration of the scope instead of being destroyed immediately. struct CaptureWrapper { - std::vector> releases; - - CaptureWrapper(std::vector>&& x) - : releases(std::move(x)) {} + CaptureWrapper(std::vector>&& items_) + : items(items_.size()) { + std::ranges::move(items_, items.begin()); + } CaptureWrapper(CaptureWrapper&&) = default; - CaptureWrapper& operator=(CaptureWrapper&&) = default; - CaptureWrapper(CaptureWrapper const&) = delete; - CaptureWrapper& operator=(CaptureWrapper const&) = delete; + CaptureWrapper(const CaptureWrapper& other) + : items(other.items) {} + std::vector> items; }; } // namespace @@ -79,18 +78,23 @@ void TileCache::deferPendingReleases() { CaptureWrapper wrap{std::move(pendingReleases)}; pendingReleases.clear(); - threadPool.schedule(util::unique_function{[this, wrap_{std::move(wrap)}]() mutable { + // 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(wrap)}}, this]() mutable { MLN_TRACE_ZONE(deferPendingReleases lambda); MLN_ZONE_VALUE(wrap_.releases.size()); + tile_.items.clear(); - // Run the deletions - wrap_.releases.clear(); - - // Wake up a waiting destructor - std::lock_guard counterLock{deferredSignalLock}; + std::lock_guard counterLock(deferredSignalLock); deferredDeletionsPending--; deferredSignal.notify_all(); - }}); + }}; + threadPool.schedule(std::move(func)); } void TileCache::add(const OverscaledTileID& key, std::unique_ptr&& tile) {