Page MenuHome

BLI: New basic CacheMutex.
ClosedPublic

Authored by Jacques Lucke (JacquesLucke) on Nov 8 2022, 1:00 PM.

Details

Summary

This patch introduces a new CacheMutex which makes it easy to implement lazily computed caches in e.g. Curves or Mesh. I experimented with this in D16283 and the abstraction turned out to work well. It simplified existing code quite a bit.

The naming seems ok since I'd describe it using "cache" and "mutex". Possible alternatives would be something like CacheProtector or CacheLock.

/**
 * A #CacheMutex is used to protect a lazily computed cache from being computed more than once.
 * Using #CacheMutex instead of a "raw mutex" to protect a cache has some benefits:
 * - Avoid common pitfalls like forgetting to use task isolation or a double checked lock.
 * - Cleaner and less redundant code because the same locking patterns don't have to be repeated
 *   everywhere.
 * - One can benefit from potential future improvements to #CacheMutex of which there are a few
 *   mentioned below.
 *
 * The data protected by #CacheMutex is not part of #CacheMutex. Instead, the #CacheMutex and its
 * protected data should generally be placed next to each other.
 *
 * Each #CacheMutex protects exactly one cache, so multiple cache mutexes have to be used when a
 * class has multiple caches. That is contrary to a "custom" solution using `std::mutex` where one
 * mutex could protect multiple caches at the cost of higher lock contention.
 *
 * To make sure the cache is up to date, call `CacheMutex::ensure` and pass in the function that
 * computes the cache.
 *
 * To tell the #CacheMutex that the cache is invalidated and to be re-evaluated upon next access
 * use `CacheMutex::tag_dirty`.
 *
 * This example shows how one could implement a lazily computed average vertex position in an
 * imaginary `Mesh` data structure:
 *
 * \code{.cpp}
 * class Mesh {
 *  private:
 *   mutable CacheMutex average_position_cache_mutex_;
 *   mutable float3 average_position_cache_;
 *
 *  public:
 *   const float3 &average_position() const
 *   {
 *     average_position_cache_mutex_.ensure([&]() {
 *       average_position_cache_ = actually_compute_average_position();
 *     });
 *     return average_position_cache_;
 *   }
 *
 *   void tag_positions_changed()
 *   {
 *     average_position_cache_mutex_.tag_dirty();
 *   }
 * };
 * \endcode
 *
 * Possible future improvements:
 * - Avoid task isolation when we know that the cache computation does not use threading.
 * - Try to use a smaller mutex. The mutex does not have to be fair for this use case.
 * - Try to join the cache computation instead of blocking if another thread is computing the cache
 *   already.
 */

Diff Detail

Repository
rB Blender
Branch
basic-cache-mutex (branched from master)
Build Status
Buildable 24534
Build 24534: arc lint + arc unit

Event Timeline

Jacques Lucke (JacquesLucke) requested review of this revision.Nov 8 2022, 1:00 PM
Jacques Lucke (JacquesLucke) created this revision.
Sergey Sharybin (sergey) requested changes to this revision.Nov 8 2022, 1:15 PM

Please pay more attention presenting the work and design you're doing.

What this CacheMutex is? How to use it? When to use it? What are the drawbacks? Is it the best naming?

Use inspiration of the famous Ingredients of a Patch on the Wiki, and comments in the BLI_task.h. Make it so that usage of such fundamental primitives is well documented and clear to the entire team, without this "grep the code for usage examples".

I would really like to see people to pay more attention on making clear and well documented APIs. It helps both the review process and people using the API.

This revision now requires changes to proceed.Nov 8 2022, 1:15 PM

Thanks, so much better!

Some suggestions on the wording.

source/blender/blenlib/BLI_cache_mutex.hh
18
25

Not sure "reset" is correct here. Also, you are not resetting the cache (that's how the sentence currently reads) since the CacheMutex does not hold the cache.

To tell the CacheMutex that the cache is invalidated and to be re-evaluated upon next access use `CacheMutex::tag_dirty()`.
This revision is now accepted and ready to land.Nov 8 2022, 2:27 PM
source/blender/blenlib/BLI_cache_mutex.hh
67

I get the point of putting the comment at the top of the file, but it's too bad we won't get comments on hover like this. What do you think about moving it right above CacheMutex?

70

exists is slightly misleading, since the cache can "exist" even if it's filled with garbage data or old data. This class doesn't manage the existence of the cache, just its computation and dirty state.

74
74–76

Simpler wording suggestion:

This calls `compute_cache` once to update the cache (which is stored outside of this class) if it is dirty, otherwise it does nothing.
Brecht Van Lommel (brecht) requested changes to this revision.Nov 8 2022, 2:45 PM

I think it's great to encapsulate this.

source/blender/blenlib/intern/cache_mutex.cc
11–16

What is the logic behind the relaxed/acquire usage here?

When I check for example here they are swapped.
https://en.wikipedia.org/wiki/Double-checked_locking

Which makes some sense to me, using acquire when not holding a lock to ensure that reading threads will not read cache_exists_ == true while reading an outdated value of the cached data right after that.

And then when you already hold a lock, relaxed is enough because the acquiring the lock already includes a fence.

At least that is my understanding, have not looked very closely at this.

This revision now requires changes to proceed.Nov 8 2022, 2:45 PM
Jacques Lucke (JacquesLucke) marked 4 inline comments as done.
  • rename vars
  • improve comment
source/blender/blenlib/BLI_cache_mutex.hh
67

I'm generally not against that, but that decision can be done separately as it applies to many other files as well.

Thanks for doing this!

source/blender/blenlib/BLI_cache_mutex.hh
67

Okay, for later!

  • improve memory orders
Jacques Lucke (JacquesLucke) marked an inline comment as done.Nov 8 2022, 2:56 PM
Jacques Lucke (JacquesLucke) added inline comments.
source/blender/blenlib/intern/cache_mutex.cc
11–16

Yes that does sound reasonable, thanks.

This revision is now accepted and ready to land.Nov 8 2022, 3:00 PM
This revision was automatically updated to reflect the committed changes.
Jacques Lucke (JacquesLucke) marked an inline comment as done.