Cleanup

Author: rschu1ze

src/Interpreters/Cache/QueryConditionCache.cpp

@@ -18,34 +18,6 @@ QueryConditionCache::QueryConditionCache(const String & cache_policy, size_t max
 {
 }
 
-inline bool QueryConditionCache::needUpdate(const std::shared_ptr<Entry> & entry, const MarkRanges & mark_ranges, size_t marks_count, bool has_final_mark) const
-{
-    // If no marks to process, we may only need to update the final mark
-    if (mark_ranges.empty()) {
-        // Only acquire the lock and check final mark when has_final_mark is true
-        std::shared_lock read_lock(entry->mutex);
-        // Return true (update needed) only if final mark needs to be set to false
-        return has_final_mark && entry->matching_marks[marks_count - 1];
-    }
-    else {
-        // Acquire shared lock for read access to the matching_marks vector
-        std::shared_lock read_lock(entry->mutex);
-
-        // Check if any mark within the ranges is still true
-        for (const auto & mark_range : mark_ranges) {
-            if (std::find(
-                entry->matching_marks.begin() + mark_range.begin,
-                entry->matching_marks.begin() + mark_range.end,
-                true) != (entry->matching_marks.begin() + mark_range.end)) {
-                    return true;  // Found at least one true mark in range, need update
-                }
-        }
-
-        // If all marks in ranges are already false, check if final mark needs update
-        return has_final_mark && entry->matching_marks[marks_count - 1];
-    }
-}
-
 void QueryConditionCache::write(
     const UUID & table_id, const String & part_name, size_t condition_hash, const String & condition,
     const MarkRanges & mark_ranges, size_t marks_count, bool has_final_mark)
@@ -55,11 +27,29 @@ void QueryConditionCache::write(
     auto load_func = [&](){ return std::make_shared<Entry>(marks_count); };
     auto [entry, inserted] = cache.getOrSet(key, load_func);
 
-    // Skip update if not needed - optimization to avoid unnecessary locks and updates
-    if (!needUpdate(entry, mark_ranges, marks_count, has_final_mark))
-        return;
+    /// Try to avoid acquiring the RW lock below (*) by early-ing out. Matters for systems with lots of cores.
+    {
+        std::shared_lock shared_lock(entry->mutex); /// cheap
+
+        bool need_not_update_marks = true;
+        for (const auto & mark_range : mark_ranges)
+        {
+            /// If the bits are already in the desired state (false), we don't need to update them.
+            need_not_update_marks = std::all_of(entry->matching_marks.begin() + mark_range.begin,
+                                                entry->matching_marks.begin() + mark_range.end,
+                                                [](auto b) { return b == false; });
+            if (!need_not_update_marks)
+                break;
+        }
+
+        /// Do we either have no final mark or final mark is already in the desired state?
+        bool need_not_update_final_mark = !has_final_mark || entry->matching_marks[marks_count - 1] == false;
 
-    std::lock_guard lock(entry->mutex);
+        if (need_not_update_marks && need_not_update_final_mark)
+            return;
+    }
+
+    std::lock_guard lock(entry->mutex); /// (*)
 
     chassert(marks_count == entry->matching_marks.size());
 
@@ -164,4 +154,5 @@ size_t QueryConditionCache::QueryConditionCacheEntryWeight::operator()(const Ent
     size_t memory = (entry.matching_marks.capacity() + 7) / 8; /// round up to bytes.
     return memory + sizeof(decltype(entry.matching_marks));
 }
+
 }

src/Interpreters/Cache/QueryConditionCache.h

@@ -16,7 +16,7 @@ namespace DB
 ///
 /// Note: The cache may store more than the minimal number of matching marks.
 /// For example, assume a very selective predicate that matches just a single row in a single mark.
-/// One would expect that the cache records just the single mark as potentially matching:
+/// One would expect that the cache records just a single mark as potentially matching:
 ///     000000010000000000000000000
 /// But it is equally correct for the cache to store this: (it is just less efficient for pruning)
 ///     000001111111110000000000000
@@ -51,14 +51,13 @@ class QueryConditionCache
 
         /// (*) You might wonder why Entry has its own mutex considering that CacheBase locks internally already.
         ///     The reason is that ClickHouse scans ranges within the same part in parallel. The first scan creates
-        ///     and inserts a new Key + Entry into the cache, the 2nd ... Nth scan find the existing Key and update
+        ///     and inserts a new Key + Entry into the cache, the 2nd ... Nth scans find the existing Key and update
         ///     its Entry for the new ranges. This can only be done safely in a synchronized fashion.
 
         /// (**) About error handling: There could be an exception after the i-th scan and cache entries could
         ///     (theoretically) be left in a corrupt state. If we are not careful, future scans queries could then
         ///     skip too many ranges. To prevent this, it is important to initialize all marks of each entry as
         ///     non-matching. In case of an exception, future scans will then not skip them.
-
     };
 
     struct KeyHasher
@@ -71,20 +70,6 @@ class QueryConditionCache
         size_t operator()(const Entry & entry) const;
     };
 
-    /**
-     * Check if we need to update the cache entry.
-     * This function determines whether a cache update is necessary by examining:
-     * 1. Whether the mark ranges are empty
-     * 2. Whether marks in specified ranges are already set to false
-     * 3. Whether the final mark (if applicable) is already set correctly
-     *
-     * @param entry The cache entry to check
-     * @param mark_ranges The ranges of marks to potentially update
-     * @param marks_count Total number of marks in the entry
-     * @param has_final_mark Flag indicating if the final mark needs special handling
-     * @return true if an update is needed, false if we can skip the update
-     */
-    inline bool needUpdate(const std::shared_ptr<Entry> & entry, const MarkRanges & mark_ranges, size_t marks_count, bool has_final_mark) const;
 
 public:
     using Cache = CacheBase<Key, Entry, KeyHasher, QueryConditionCacheEntryWeight>;