cc: Add mutex annotations to SoftwareImageDecodeCache.

Add mutex annotations to replace load-bearing comments in
SoftwareImageDecodeCache. This requires adding the proper annotation to
Lock::AssertAcquired(), which is also done in this CL. As a consequence, this CL
does a few things:

In SoftwareImageDecodeCache:
- Convert comments to annotations
- Make a member const instead of annotating it
- Change a test-only method to make compile-time checks pass
- Fix "git cl lint" warnings (include what you use)

In Lock:
- Add a mutex annotation to Lock::AssertAcquired()

Bug: 1013082, 831825, 983348
Change-Id: If779a0f6453a2060ed3e7dedf7ad7f6823ebac68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1939749
Commit-Queue: Benoit L <lizeb@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#720164}

base/synchronization/lock.h

@@ -26,8 +26,8 @@ class LOCKABLE BASE_EXPORT Lock {
   ~Lock() {}
 
   // TODO(lukasza): https://crbug.com/831825: Add EXCLUSIVE_LOCK_FUNCTION
-  // annotation to Acquire method and similar annotations to Release, Try and
-  // AssertAcquired methods (here and in the #else branch).
+  // annotation to Acquire method and similar annotations to Release and Try
+  // methods (here and in the #else branch).
   void Acquire() { lock_.Lock(); }
   void Release() { lock_.Unlock(); }
 
@@ -38,7 +38,7 @@ class LOCKABLE BASE_EXPORT Lock {
   bool Try() { return lock_.Try(); }
 
   // Null implementation if not debug.
-  void AssertAcquired() const {}
+  void AssertAcquired() const ASSERT_EXCLUSIVE_LOCK() {}
 #else
   Lock();
   ~Lock();
@@ -63,7 +63,7 @@ class LOCKABLE BASE_EXPORT Lock {
     return rv;
   }
 
-  void AssertAcquired() const;
+  void AssertAcquired() const ASSERT_EXCLUSIVE_LOCK();
 #endif  // DCHECK_IS_ON()
 
   // Whether Lock mitigates priority inversion when used from different thread

cc/tiles/software_image_decode_cache.cc

@@ -6,6 +6,10 @@
 
 #include <stdint.h>
 
+#include <algorithm>
+#include <string>
+#include <utility>
+
 #include "base/bind.h"
 #include "base/format_macros.h"
 #include "base/metrics/histogram_macros.h"
@@ -267,7 +271,6 @@ void SoftwareImageDecodeCache::AddBudgetForImage(const CacheKey& key,
   TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                "SoftwareImageDecodeCache::AddBudgetForImage", "key",
                key.ToString());
-  lock_.AssertAcquired();
 
   DCHECK(!entry->is_budgeted);
   DCHECK_GE(locked_images_budget_.AvailableMemoryBytes(), key.locked_bytes());
@@ -280,7 +283,6 @@ void SoftwareImageDecodeCache::RemoveBudgetForImage(const CacheKey& key,
   TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                "SoftwareImageDecodeCache::RemoveBudgetForImage", "key",
                key.ToString());
-  lock_.AssertAcquired();
 
   DCHECK(entry->is_budgeted);
   locked_images_budget_.SubtractUsage(key.locked_bytes());
@@ -297,7 +299,6 @@ void SoftwareImageDecodeCache::UnrefImage(const DrawImage& image) {
 }
 
 void SoftwareImageDecodeCache::UnrefImage(const CacheKey& key) {
-  lock_.AssertAcquired();
   auto decoded_image_it = decoded_images_.Peek(key);
   DCHECK(decoded_image_it != decoded_images_.end());
   auto* entry = decoded_image_it->second.get();
@@ -340,7 +341,6 @@ SoftwareImageDecodeCache::DecodeImageIfNecessary(const CacheKey& key,
   TRACE_EVENT1(TRACE_DISABLED_BY_DEFAULT("cc.debug"),
                "SoftwareImageDecodeCache::DecodeImageIfNecessary", "key",
                key.ToString());
-  lock_.AssertAcquired();
   DCHECK_GT(entry->ref_count, 0);
 
   if (key.target_size().IsEmpty())
@@ -550,7 +550,6 @@ DecodedDrawImage SoftwareImageDecodeCache::GetDecodedImageForDrawInternal(
                "SoftwareImageDecodeCache::GetDecodedImageForDrawInternal",
                "key", key.ToString());
 
-  lock_.AssertAcquired();
   auto decoded_it = decoded_images_.Get(key);
   CacheEntry* cache_entry = nullptr;
   if (decoded_it == decoded_images_.end())
@@ -695,13 +694,17 @@ void SoftwareImageDecodeCache::OnMemoryPressure(
 
 SoftwareImageDecodeCache::CacheEntry* SoftwareImageDecodeCache::AddCacheEntry(
     const CacheKey& key) {
-  lock_.AssertAcquired();
   frame_key_to_image_keys_[key.frame_key()].push_back(key);
   auto it = decoded_images_.Put(key, std::make_unique<CacheEntry>());
   it->second.get()->mark_cached();
   return it->second.get();
 }
 
+size_t SoftwareImageDecodeCache::GetNumCacheEntriesForTesting() {
+  base::AutoLock lock(lock_);
+  return decoded_images_.size();
+}
+
 // MemoryBudget ----------------------------------------------------------------
 SoftwareImageDecodeCache::MemoryBudget::MemoryBudget(size_t limit_bytes)
     : limit_bytes_(limit_bytes), current_usage_bytes_(0u) {}

cc/tiles/software_image_decode_cache.h

@@ -9,11 +9,13 @@
 
 #include <memory>
 #include <unordered_map>
+#include <vector>
 
 #include "base/containers/mru_cache.h"
 #include "base/memory/memory_pressure_listener.h"
 #include "base/memory/ref_counted.h"
 #include "base/numerics/safe_math.h"
+#include "base/thread_annotations.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "cc/cc_export.h"
 #include "cc/paint/draw_image.h"
@@ -72,7 +74,7 @@ class CC_EXPORT SoftwareImageDecodeCache
   bool OnMemoryDump(const base::trace_event::MemoryDumpArgs& args,
                     base::trace_event::ProcessMemoryDump* pmd) override;
 
-  size_t GetNumCacheEntriesForTesting() const { return decoded_images_.size(); }
+  size_t GetNumCacheEntriesForTesting();
 
  private:
   using CacheEntry = Utils::CacheEntry;
@@ -98,69 +100,67 @@ class CC_EXPORT SoftwareImageDecodeCache
   using ImageMRUCache = base::
       HashingMRUCache<CacheKey, std::unique_ptr<CacheEntry>, CacheKeyHash>;
 
-  // Actually decode the image. Note that this function can (and should) be
-  // called with no lock acquired, since it can do a lot of work. Note that it
-  // can also return nullptr to indicate the decode failed.
-  std::unique_ptr<CacheEntry> DecodeImageInternal(const CacheKey& key,
-                                                  const DrawImage& draw_image);
-
   // Get the decoded draw image for the given key and paint_image. Note that
-  // this function has to be called with no lock acquired, since it will acquire
-  // its own locks and might call DecodeImageInternal above. Note that
   // when used internally, we still require that DrawWithImageFinished() is
   // called afterwards.
-  DecodedDrawImage GetDecodedImageForDrawInternal(
-      const CacheKey& key,
-      const PaintImage& paint_image);
+  DecodedDrawImage GetDecodedImageForDrawInternal(const CacheKey& key,
+                                                  const PaintImage& paint_image)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
   // Removes unlocked decoded images until the number of decoded images is
   // reduced within the given limit.
-  void ReduceCacheUsageUntilWithinLimit(size_t limit);
+  void ReduceCacheUsageUntilWithinLimit(size_t limit)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
-  void OnMemoryPressure(
-      base::MemoryPressureListener::MemoryPressureLevel level);
+  void OnMemoryPressure(base::MemoryPressureListener::MemoryPressureLevel level)
+      LOCKS_EXCLUDED(lock_);
 
   // Helper method to get the different tasks. Note that this should be used as
   // if it was public (ie, all of the locks need to be properly acquired).
   TaskResult GetTaskForImageAndRefInternal(const DrawImage& image,
                                            const TracingInfo& tracing_info,
-                                           DecodeTaskType type);
+                                           DecodeTaskType type)
+      LOCKS_EXCLUDED(lock_);
 
-  CacheEntry* AddCacheEntry(const CacheKey& key);
+  CacheEntry* AddCacheEntry(const CacheKey& key)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
   TaskProcessingResult DecodeImageIfNecessary(const CacheKey& key,
                                               const PaintImage& paint_image,
-                                              CacheEntry* cache_entry);
-  void AddBudgetForImage(const CacheKey& key, CacheEntry* entry);
-  void RemoveBudgetForImage(const CacheKey& key, CacheEntry* entry);
-  base::Optional<CacheKey> FindCachedCandidate(const CacheKey& key);
+                                              CacheEntry* cache_entry)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  void AddBudgetForImage(const CacheKey& key, CacheEntry* entry)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  void RemoveBudgetForImage(const CacheKey& key, CacheEntry* entry)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  base::Optional<CacheKey> FindCachedCandidate(const CacheKey& key)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
-  void UnrefImage(const CacheKey& key);
+  void UnrefImage(const CacheKey& key) EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
-  // The members below this comment can only be accessed if the lock is held to
-  // ensure that they are safe to access on multiple threads.
-  // The exception is accessing |locked_images_budget_.total_limit_bytes()|,
-  // which is const and thread safe.
   base::Lock lock_;
-
   // Decoded images and ref counts (predecode path).
-  ImageMRUCache decoded_images_;
+  ImageMRUCache decoded_images_ GUARDED_BY(lock_);
 
-  std::unique_ptr<base::MemoryPressureListener> memory_pressure_listener_;
+  std::unique_ptr<base::MemoryPressureListener> memory_pressure_listener_
+      GUARDED_BY(lock_);
 
   // A map of PaintImage::FrameKey to the ImageKeys for cached decodes of this
   // PaintImage.
   std::unordered_map<PaintImage::FrameKey,
                      std::vector<CacheKey>,
                      PaintImage::FrameKeyHash>
-      frame_key_to_image_keys_;
+      frame_key_to_image_keys_ GUARDED_BY(lock_);
 
+  // Should be GUARDED_BY(lock_), except that accessing
+  // |locked_images_budget_.total_limit_bytes()| is fine without the lock, as
+  // it is const and thread safe.
   MemoryBudget locked_images_budget_;
 
   const SkColorType color_type_;
   const PaintImage::GeneratorClientId generator_client_id_;
 
-  size_t max_items_in_cache_;
+  const size_t max_items_in_cache_;
 };
 
 }  // namespace cc