thread_safe_memoizer.h reword

Author: dconeybe

Firestore/core/src/core/composite_filter.cc

@@ -142,16 +142,13 @@ const FieldFilter* CompositeFilter::Rep::FindFirstMatchingFilter(
   return nullptr;
 }
 
-const std::vector<FieldFilter>& CompositeFilter::Rep::GetFlattenedFilters()
-    const {
-  return memoized_flattened_filters_.memoize([&]() {
-    auto flattened_filters = absl::make_unique<std::vector<FieldFilter>>();
-    for (const auto& filter : filters())
-      std::copy(filter.GetFlattenedFilters().begin(),
-                filter.GetFlattenedFilters().end(),
-                std::back_inserter(*flattened_filters));
-    return flattened_filters;
-  });
+std::shared_ptr<std::vector<FieldFilter>> CompositeFilter::Rep::CalculateFlattenedFilters() const {
+  auto flattened_filters = absl::make_unique<std::vector<FieldFilter>>();
+  for (const auto& filter : filters())
+    std::copy(filter.GetFlattenedFilters().begin(),
+              filter.GetFlattenedFilters().end(),
+              std::back_inserter(*flattened_filters));
+  return flattened_filters;
 }
 
 }  // namespace core

Firestore/core/src/core/composite_filter.h

@@ -138,7 +138,7 @@ class CompositeFilter : public Filter {
       return filters_.empty();
     }
 
-    const std::vector<FieldFilter>& GetFlattenedFilters() const override;
+    std::shared_ptr<std::vector<FieldFilter>> CalculateFlattenedFilters() const override;
 
     std::vector<Filter> GetFilters() const override {
       return filters();

Firestore/core/src/core/field_filter.cc

@@ -123,13 +123,11 @@ FieldFilter::FieldFilter(std::shared_ptr<const Filter::Rep> rep)
     : Filter(std::move(rep)) {
 }
 
-const std::vector<FieldFilter>& FieldFilter::Rep::GetFlattenedFilters() const {
+std::shared_ptr<std::vector<FieldFilter>> FieldFilter::Rep::CalculateFlattenedFilters() const {
   // This is already a field filter, so we return a vector of size one.
-  return memoized_flattened_filters_.memoize([&]() {
-    auto filters = absl::make_unique<std::vector<FieldFilter>>();
-    filters->push_back(FieldFilter(std::make_shared<const Rep>(*this)));
-    return filters;
-  });
+  auto filters = std::make_shared<std::vector<FieldFilter>>();
+  filters->push_back(FieldFilter(std::make_shared<const Rep>(*this)));
+  return filters;
 }
 
 std::vector<Filter> FieldFilter::Rep::GetFilters() const {

Firestore/core/src/core/field_filter.h

@@ -117,8 +117,6 @@ class FieldFilter : public Filter {
       return false;
     }
 
-    const std::vector<FieldFilter>& GetFlattenedFilters() const override;
-
     std::vector<Filter> GetFilters() const override;
 
    protected:
@@ -140,6 +138,8 @@ class FieldFilter : public Filter {
 
     bool MatchesComparison(util::ComparisonResult comparison) const;
 
+    std::shared_ptr<std::vector<FieldFilter>> CalculateFlattenedFilters() const override;
+
    private:
     friend class FieldFilter;
 

Firestore/core/src/core/filter.h

@@ -145,21 +145,21 @@ class Filter {
 
     virtual bool IsEmpty() const = 0;
 
-    virtual const std::vector<FieldFilter>& GetFlattenedFilters() const = 0;
+    virtual const std::vector<FieldFilter>& GetFlattenedFilters() const {
+      return flattened_filters_.value();
+    }
 
     virtual std::vector<Filter> GetFilters() const = 0;
 
+   protected:
+    virtual std::shared_ptr<std::vector<FieldFilter>> CalculateFlattenedFilters() const = 0;
+
+   private:
     /**
      * Memoized list of all field filters that can be found by
      * traversing the tree of filters contained in this composite filter.
-     *
-     * Use a `std::shared_ptr<ThreadSafeMemoizer>` rather than using
-     * `ThreadSafeMemoizer` directly so that this class is copyable
-     * (`ThreadSafeMemoizer` is not copyable because of its `std::once_flag`
-     * member variable, which is not copyable).
      */
-    mutable util::ThreadSafeMemoizer<std::vector<FieldFilter>>
-        memoized_flattened_filters_;
+    mutable util::ThreadSafeMemoizer<std::vector<FieldFilter>> flattened_filters_{[&] { return CalculateFlattenedFilters(); }};
   };
 
   explicit Filter(std::shared_ptr<const Rep>&& rep) : rep_(rep) {

Firestore/core/src/core/query.cc

@@ -92,44 +92,42 @@ absl::optional<Operator> Query::FindOpInsideFilters(
   return absl::nullopt;
 }
 
-const std::vector<OrderBy>& Query::normalized_order_bys() const {
-  return memoized_normalized_order_bys_.memoize([&]() {
-    // Any explicit order by fields should be added as is.
-    auto result = absl::make_unique<std::vector<OrderBy>>(explicit_order_bys_);
-    std::set<FieldPath> fieldsNormalized;
-    for (const OrderBy& order_by : explicit_order_bys_) {
-      fieldsNormalized.insert(order_by.field());
-    }
+std::shared_ptr<std::vector<OrderBy>> Query::calculate_normalized_order_bys() const {
+  auto result = std::make_shared<std::vector<OrderBy>>(explicit_order_bys_);
 
-    // The order of the implicit ordering always matches the last explicit order
-    // by.
-    Direction last_direction = explicit_order_bys_.empty()
-                                   ? Direction::Ascending
-                                   : explicit_order_bys_.back().direction();
-
-    // Any inequality fields not explicitly ordered should be implicitly ordered
-    // in a lexicographical order. When there are multiple inequality filters on
-    // the same field, the field should be added only once. Note:
-    // `std::set<model::FieldPath>` sorts the key field before other fields.
-    // However, we want the key field to be sorted last.
-    const std::set<model::FieldPath> inequality_fields =
-        InequalityFilterFields();
-
-    for (const model::FieldPath& field : inequality_fields) {
-      if (fieldsNormalized.find(field) == fieldsNormalized.end() &&
-          !field.IsKeyFieldPath()) {
-        result->push_back(OrderBy(field, last_direction));
-      }
-    }
+  std::set<FieldPath> fieldsNormalized;
+  for (const OrderBy& order_by : explicit_order_bys_) {
+    fieldsNormalized.insert(order_by.field());
+  }
 
-    // Add the document key field to the last if it is not explicitly ordered.
-    if (fieldsNormalized.find(FieldPath::KeyFieldPath()) ==
-        fieldsNormalized.end()) {
-      result->push_back(OrderBy(FieldPath::KeyFieldPath(), last_direction));
-    }
+  // The order of the implicit ordering always matches the last explicit order
+  // by.
+  Direction last_direction = explicit_order_bys_.empty()
+                                 ? Direction::Ascending
+                                 : explicit_order_bys_.back().direction();
+
+  // Any inequality fields not explicitly ordered should be implicitly ordered
+  // in a lexicographical order. When there are multiple inequality filters on
+  // the same field, the field should be added only once. Note:
+  // `std::set<model::FieldPath>` sorts the key field before other fields.
+  // However, we want the key field to be sorted last.
+  const std::set<model::FieldPath> inequality_fields =
+      InequalityFilterFields();
+
+  for (const model::FieldPath& field : inequality_fields) {
+    if (fieldsNormalized.find(field) == fieldsNormalized.end() &&
+        !field.IsKeyFieldPath()) {
+      result->push_back(OrderBy(field, last_direction));
+        }
+  }
 
-    return result;
-  });
+  // Add the document key field to the last if it is not explicitly ordered.
+  if (fieldsNormalized.find(FieldPath::KeyFieldPath()) ==
+      fieldsNormalized.end()) {
+    result->push_back(OrderBy(FieldPath::KeyFieldPath(), last_direction));
+      }
+
+  return result;
 }
 
 LimitType Query::limit_type() const {
@@ -297,16 +295,12 @@ std::string Query::ToString() const {
   return absl::StrCat("Query(canonical_id=", CanonicalId(), ")");
 }
 
-const Target& Query::ToTarget() const& {
-  return memoized_target_.memoize([&]() {
-    return absl::make_unique<Target>(ToTarget(normalized_order_bys()));
-  });
+std::shared_ptr<Target> Query::calculate_target() const {
+  return std::make_shared<Target>(ToTarget(normalized_order_bys()));
 }
 
-const Target& Query::ToAggregateTarget() const& {
-  return memoized_aggregate_target_.memoize([&]() {
-    return absl::make_unique<Target>(ToTarget(explicit_order_bys_));
-  });
+std::shared_ptr<Target> Query::calculate_aggregate_target() const {
+  return std::make_shared<Target>(ToTarget(explicit_order_bys_));
 }
 
 Target Query::ToTarget(const std::vector<OrderBy>& order_bys) const {

Firestore/core/src/core/query.h

@@ -148,7 +148,9 @@ class Query {
    * This might include additional sort orders added implicitly to match the
    * backend behavior.
    */
-  const std::vector<OrderBy>& normalized_order_bys() const;
+  const std::vector<OrderBy>& normalized_order_bys() const {
+    return normalized_order_bys_.value();
+  }
 
   bool has_limit() const {
     return limit_ != Target::kNoLimit;
@@ -246,15 +248,19 @@ class Query {
    * Returns a `Target` instance this query will be mapped to in backend
    * and local store.
    */
-  const Target& ToTarget() const&;
+  const Target& ToTarget() const& {
+    return target_.value();
+  }
 
   /**
    * Returns a `Target` instance this query will be mapped to in backend
    * and local store, for use within an aggregate query. Unlike targets
    * for non-aggregate queries, aggregate query targets do not contain
    * normalized order-bys, they only contain explicit order-bys.
    */
-  const Target& ToAggregateTarget() const&;
+  const Target& ToAggregateTarget() const& {
+    return aggregate_target_.value();
+  }
 
   friend std::ostream& operator<<(std::ostream& os, const Query& query);
 
@@ -295,16 +301,19 @@ class Query {
   // member variable, which is not copyable).
 
   // The memoized list of sort orders.
+  std::shared_ptr<std::vector<OrderBy>> calculate_normalized_order_bys() const;
   mutable util::ThreadSafeMemoizer<std::vector<OrderBy>>
-      memoized_normalized_order_bys_;
+      normalized_order_bys_{[&] {return calculate_normalized_order_bys(); }};
 
   // The corresponding Target of this Query instance.
-  mutable util::ThreadSafeMemoizer<Target> memoized_target_;
+  std::shared_ptr<Target> calculate_target() const;
+  mutable util::ThreadSafeMemoizer<Target> target_{[&] {return calculate_target(); }};
 
   // The corresponding aggregate Target of this Query instance. Unlike targets
   // for non-aggregate queries, aggregate query targets do not contain
   // normalized order-bys, they only contain explicit order-bys.
-  mutable util::ThreadSafeMemoizer<Target> memoized_aggregate_target_;
+  std::shared_ptr<Target> calculate_aggregate_target() const;
+  mutable util::ThreadSafeMemoizer<Target> aggregate_target_{[&] {return calculate_aggregate_target(); }};;
 };
 
 bool operator==(const Query& lhs, const Query& rhs);

Firestore/core/src/util/thread_safe_memoizer.h

@@ -20,6 +20,7 @@
 #include <atomic>
 #include <functional>
 #include <memory>
+#include <utility>
 
 namespace firebase {
 namespace firestore {
@@ -32,14 +33,25 @@ namespace util {
 template <typename T>
 class ThreadSafeMemoizer {
  public:
-  ThreadSafeMemoizer()
-      : memoized_(new std::atomic<T*>(nullptr), MemoizedValueDeleter) {
+  explicit ThreadSafeMemoizer(std::function<std::shared_ptr<T>()> func) : func_(std::move(func)) {
   }
 
-  ThreadSafeMemoizer(const ThreadSafeMemoizer& other) = default;
-  ThreadSafeMemoizer& operator=(const ThreadSafeMemoizer& other) = default;
-  ThreadSafeMemoizer(ThreadSafeMemoizer&& other) = default;
-  ThreadSafeMemoizer& operator=(ThreadSafeMemoizer&& other) = default;
+  ThreadSafeMemoizer(const ThreadSafeMemoizer& other) : func_(other.func_), memoized_(std::atomic_load(&other.memoized_)) {}
+
+  ThreadSafeMemoizer& operator=(const ThreadSafeMemoizer& other) {
+    func_ = other.func_;
+    std::atomic_store(&memoized_, std::atomic_load(&other.memoized_));
+    return *this;
+  }
+
+  ThreadSafeMemoizer(ThreadSafeMemoizer&& other) noexcept : func_(std::move(other.func_)), memoized_(std::atomic_load(&other.memoized_)) {
+  }
+
+  ThreadSafeMemoizer& operator=(ThreadSafeMemoizer&& other) noexcept {
+    func_ = std::move(other.func_);
+    std::atomic_store(&memoized_, std::atomic_load(&other.memoized_));
+    return *this;
+  }
 
   /**
    * Memoize a value.
@@ -58,28 +70,24 @@ class ThreadSafeMemoizer {
    * No reference to the given function is retained by this object, and the
    * function be called synchronously, if it is called at all.
    */
-  const T& memoize(std::function<std::unique_ptr<T>()> func) {
+  const T& value() {
+    std::shared_ptr<T> old_memoized = std::atomic_load(&memoized_);
     while (true) {
-      T* old_memoized = memoized_->load();
       if (old_memoized) {
         return *old_memoized;
       }
 
-      std::unique_ptr<T> new_memoized = func();
+      std::shared_ptr<T> new_memoized = func_();
 
-      if (memoized_->compare_exchange_weak(old_memoized, new_memoized.get())) {
-        return *new_memoized.release();
+      if (std::atomic_compare_exchange_weak(&memoized_, &old_memoized, new_memoized)) {
+        return *new_memoized;
       }
     }
   }
 
  private:
-  std::shared_ptr<std::atomic<T*>> memoized_;
-
-  static void MemoizedValueDeleter(std::atomic<T*>* value) {
-    delete value->load();
-    delete value;
-  }
+  std::function<std::shared_ptr<T>()> func_;
+  std::shared_ptr<T> memoized_;
 };
 
 }  // namespace util

Firestore/core/test/unit/util/thread_safe_memoizer_test.cc

@@ -20,34 +20,33 @@
 #include "absl/memory/memory.h"
 #include "gtest/gtest.h"
 
-namespace firebase {
-namespace firestore {
-namespace util {
+namespace {
+
+using firebase::firestore::util::ThreadSafeMemoizer;
 
 TEST(ThreadSafeMemoizerTest, MultiThreadedMemoization) {
   std::atomic<int> global_int{77};
 
-  auto expensive_lambda = [&]() {
+  auto expensive_lambda = [&] {
     // Simulate an expensive operation
     std::this_thread::sleep_for(std::chrono::milliseconds(100));
     // If the lambda gets executed multiple times, threads will see incremented
     // `global_int`.
-    global_int++;
-    return absl::make_unique<int>(global_int.load());
+    ++global_int;
+    return std::make_shared<int>(global_int.load());
   };
 
-  const int num_threads = 5;
-  const int expected_result = 78;
+  constexpr int num_threads = 5;
+  constexpr int expected_result = 78;
 
   // Create a thread safe memoizer and multiple threads.
-  util::ThreadSafeMemoizer<int> memoized_result;
+  ThreadSafeMemoizer<int> memoized_result(expensive_lambda);
   std::vector<std::thread> threads;
 
   for (int i = 0; i < num_threads; ++i) {
     threads.emplace_back(
-        [&memoized_result, expected_result, &expensive_lambda]() {
-          const int& actual_result = memoized_result.memoize(expensive_lambda);
-
+        [&memoized_result, expected_result] {
+          const int& actual_result = memoized_result.value();
           // Verify that all threads get the same memoized result.
           EXPECT_EQ(actual_result, expected_result);
         });
@@ -58,6 +57,4 @@ TEST(ThreadSafeMemoizerTest, MultiThreadedMemoization) {
   }
 }
 
-}  // namespace util
-}  // namespace firestore
-}  // namespace firebase
+}  // namespace