refactor(block_cache): factor out periodic_executor

cmake/libdwarfs.cmake

@@ -106,6 +106,7 @@ add_library(
   src/reader/internal/inode_reader_v2.cpp
   src/reader/internal/metadata_types.cpp
   src/reader/internal/metadata_v2.cpp
+  src/reader/internal/periodic_executor.cpp
 )
 
 add_library(

include/dwarfs/reader/internal/periodic_executor.h

@@ -0,0 +1,68 @@
+/* vim:set ts=2 sw=2 sts=2 et: */
+/**
+ * \author     Marcus Holland-Moritz (github@mhxnet.de)
+ * \copyright  Copyright (c) Marcus Holland-Moritz
+ *
+ * This file is part of dwarfs.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the “Software”), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED “AS IS”, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#pragma once
+
+#include <chrono>
+#include <functional>
+#include <memory>
+#include <mutex>
+#include <string_view>
+
+namespace dwarfs::reader::internal {
+
+class periodic_executor {
+ public:
+  periodic_executor(std::mutex& mx, std::chrono::nanoseconds period,
+                    std::string_view name, std::function<void()> func);
+
+  void start() const { impl_->start(); }
+
+  void stop() const { impl_->stop(); }
+
+  bool running() const { return impl_->running(); }
+
+  void set_period(std::chrono::nanoseconds period) const {
+    impl_->set_period(period);
+  }
+
+  class impl {
+   public:
+    virtual ~impl() = default;
+
+    virtual void start() const = 0;
+    virtual void stop() const = 0;
+    virtual bool running() const = 0;
+    virtual void set_period(std::chrono::nanoseconds period) const = 0;
+  };
+
+ private:
+  std::unique_ptr<impl const> impl_;
+};
+
+} // namespace dwarfs::reader::internal

src/reader/internal/block_cache.cpp

@@ -30,14 +30,12 @@
 #include <atomic>
 #include <cassert>
 #include <chrono>
-#include <condition_variable>
 #include <exception>
 #include <future>
 #include <iterator>
 #include <mutex>
 #include <new>
 #include <shared_mutex>
-#include <thread>
 #include <utility>
 #include <vector>
 
@@ -62,6 +60,7 @@
 #include <dwarfs/reader/internal/block_cache.h>
 #include <dwarfs/reader/internal/block_cache_byte_buffer_factory.h>
 #include <dwarfs/reader/internal/cached_block.h>
+#include <dwarfs/reader/internal/periodic_executor.h>
 
 namespace dwarfs::reader::internal {
 
@@ -231,6 +230,7 @@ class block_cache_ final : public block_cache::impl {
                std::shared_ptr<performance_monitor const> const& perfmon
                [[maybe_unused]])
       : cache_(0)
+      , tidy_runner_{mx_, {}, "tidy-blkcache", [this] { tidy_cache(); }}
       , mm_(std::move(mm))
       , buffer_factory_{block_cache_byte_buffer_factory::create(
             options.allocation_mode)}
@@ -256,9 +256,7 @@ class block_cache_ final : public block_cache::impl {
   ~block_cache_() noexcept override {
     LOG_DEBUG << "stopping cache workers";
 
-    if (tidy_running_) {
+    tidy_runner_.stop();
-      stop_tidy_thread();
-    }
 
     if (wg_) {
       wg_.stop();
@@ -362,23 +360,21 @@ class block_cache_ final : public block_cache::impl {
 
   void set_tidy_config(cache_tidy_config const& cfg) override {
     if (cfg.strategy == cache_tidy_strategy::NONE) {
-      if (tidy_running_) {
+      tidy_runner_.stop();
-        stop_tidy_thread();
-      }
     } else {
       if (cfg.interval == std::chrono::milliseconds::zero()) {
         DWARFS_THROW(runtime_error, "tidy interval is zero");
       }
 
-      std::lock_guard lock(mx_);
+      {
+        std::lock_guard lock(mx_);
+        tidy_config_ = cfg;
+      }
 
-      tidy_config_ = cfg;
+      tidy_runner_.set_period(cfg.interval);
 
-      if (tidy_running_) {
+      if (!tidy_runner_.running()) {
-        tidy_cond_.notify_all();
+        tidy_runner_.start();
-      } else {
-        tidy_running_ = true;
-        tidy_thread_ = std::thread(&block_cache_::tidy_thread, this);
       }
     }
   }
@@ -600,15 +596,6 @@ class block_cache_ final : public block_cache::impl {
     }
   }
 
-  void stop_tidy_thread() {
-    {
-      std::lock_guard lock(mx_);
-      tidy_running_ = false;
-    }
-    tidy_cond_.notify_all();
-    tidy_thread_.join();
-  }
-
   void update_block_stats(cached_block const& cb) {
     if (cb.range_end() < cb.uncompressed_size()) {
       partially_decompressed_.fetch_add(1, std::memory_order_relaxed);
@@ -749,36 +736,26 @@ class block_cache_ final : public block_cache::impl {
     }
   }
 
-  void tidy_thread() {
+  void tidy_cache() {
-    folly::setThreadName("cache-tidy");
+    switch (tidy_config_.strategy) {
+    case cache_tidy_strategy::EXPIRY_TIME:
+      LOG_DEBUG << "tidying cache by expiry time";
+      remove_block_if([tp = std::chrono::steady_clock::now() -
+                            tidy_config_.expiry_time](cached_block const& blk) {
+        return blk.last_used_before(tp);
+      });
+      break;
 
-    std::unique_lock lock(mx_);
+    case cache_tidy_strategy::BLOCK_SWAPPED_OUT: {
+      LOG_DEBUG << "tidying cache by swapped out blocks";
+      std::vector<uint8_t> tmp;
+      remove_block_if([&tmp](cached_block const& blk) {
+        return blk.any_pages_swapped_out(tmp);
+      });
+    } break;
 
-    while (tidy_running_) {
+    default:
-      if (tidy_cond_.wait_for(lock, tidy_config_.interval) ==
+      break;
-          std::cv_status::timeout) {
-        switch (tidy_config_.strategy) {
-        case cache_tidy_strategy::EXPIRY_TIME:
-          LOG_DEBUG << "tidying cache by expiry time";
-          remove_block_if(
-              [tp = std::chrono::steady_clock::now() -
-                    tidy_config_.expiry_time](cached_block const& blk) {
-                return blk.last_used_before(tp);
-              });
-          break;
-
-        case cache_tidy_strategy::BLOCK_SWAPPED_OUT: {
-          LOG_DEBUG << "tidying cache by swapped out blocks";
-          std::vector<uint8_t> tmp;
-          remove_block_if([&tmp](cached_block const& blk) {
-            return blk.any_pages_swapped_out(tmp);
-          });
-        } break;
-
-        default:
-          break;
-        }
-      }
     }
   }
 
@@ -791,9 +768,7 @@ class block_cache_ final : public block_cache::impl {
   mutable lru_type cache_;
   mutable fast_map_type<size_t, std::vector<std::weak_ptr<block_request_set>>>
       active_;
-  std::thread tidy_thread_;
+  periodic_executor tidy_runner_;
-  std::condition_variable tidy_cond_;
-  bool tidy_running_{false};
 
   mutable std::mutex mx_dec_;
   mutable fast_map_type<size_t, std::weak_ptr<block_request_set>>

src/reader/internal/periodic_executor.cpp

@@ -0,0 +1,114 @@
+/* vim:set ts=2 sw=2 sts=2 et: */
+/**
+ * \author     Marcus Holland-Moritz (github@mhxnet.de)
+ * \copyright  Copyright (c) Marcus Holland-Moritz
+ *
+ * This file is part of dwarfs.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the “Software”), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED “AS IS”, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#include <atomic>
+#include <condition_variable>
+#include <optional>
+#include <thread>
+
+#include <folly/system/ThreadName.h>
+
+#include <dwarfs/reader/internal/periodic_executor.h>
+
+namespace dwarfs::reader::internal {
+
+namespace {
+
+class periodic_executor_ final : public periodic_executor::impl {
+ public:
+  periodic_executor_(std::mutex& mx, std::chrono::nanoseconds period,
+                     std::string_view name, std::function<void()> func)
+      : mx_{mx}
+      , period_{period}
+      , name_{name}
+      , func_{std::move(func)} {}
+
+  ~periodic_executor_() override { stop(); }
+
+  void start() const override {
+    std::lock_guard lock(mx_);
+    if (!running_.load()) {
+      running_.store(true);
+      thread_.emplace(&periodic_executor_::run, this);
+    }
+  }
+
+  void stop() const override {
+    std::unique_lock lock(mx_);
+    if (running_.load()) {
+      running_.store(false);
+      lock.unlock();
+      cv_.notify_all();
+      thread_->join();
+      thread_.reset();
+    }
+  }
+
+  bool running() const override { return running_.load(); }
+
+  void set_period(std::chrono::nanoseconds period) const override {
+    {
+      std::lock_guard lock(mx_);
+      period_ = period;
+    }
+
+    if (running_.load()) {
+      cv_.notify_all();
+    }
+  }
+
+ private:
+  void run() const {
+    folly::setThreadName(name_);
+    std::unique_lock lock(mx_);
+    while (running_.load()) {
+      if (cv_.wait_for(lock, period_) == std::cv_status::timeout) {
+        func_();
+      }
+    }
+  }
+
+  std::mutex& mx_;
+  std::condition_variable mutable cv_;
+  std::atomic<bool> mutable running_{false};
+  std::optional<std::thread> mutable thread_;
+  std::chrono::nanoseconds mutable period_;
+  std::string const name_;
+  std::function<void()> func_;
+};
+
+} // namespace
+
+periodic_executor::periodic_executor(std::mutex& mx,
+                                     std::chrono::nanoseconds period,
+                                     std::string_view name,
+                                     std::function<void()> func)
+    : impl_{std::make_unique<periodic_executor_>(mx, period, name,
+                                                 std::move(func))} {}
+
+} // namespace dwarfs::reader::internal