Consolidate cache_memoize state and functionality into a CacheContext.

Review Request #12377 — Created June 15, 2022 and submitted

Information

Djblets
release-3.x

Reviewers

We pass a lot of state around the various functions that comprise
cache_memoize() and cache_memoize_iter(), and we have a lot of key
building that we duplicate everywhere. Some upcoming work will be making
a lot of this even more complex, so in preparation for that, this change
cleans up much of this state and logic.

There's now an internal _CacheContext class, which stores the state
being used for a caching operation. This includes:

  • The cache connection
  • The base key name passed to cache_memoize()/cache_memoize_iter()
  • the full key name generated through make_cache_key()
  • The timeout
  • Whether to compress large data

This simplifies a lot of function signatures.

It then provides functions for generating suitable keys (wrapping
make_cache_key(), subkeys (used for storing chunks), loading cached
data, and storing cached data. Right now, these are mostly very simple
wrappers, but they'll soon be extended for additional checks and
functionality, hence the consolidation of this logic.

There's also a performance improvement/race condition fix here. Both
cache_memoize() and cache_memoize_iter() previously checked if a
value was in cache, and then pulled it out. This requires two hits to
cache instead of one, and it's possible (though unlikely) that the value
could disappear in the meantime. In practice, we'd gracefully handle the
failure, but the two cache hits weren't necessary.

We now pull the value out, defaulting the result to a _NO_RESULTS
sentinel if not found. We then make a determination based on this.

Aside from the performance improvement, there are no changes to how
the cache is utilized. This does make extensive improvements to our
documentation, and improves some logging (adding a missing "Cache miss"
for cache_memoize() to go along with that in cache_memoize_iter()),
and logging full cache keys).

All Djblets and Review Board unit tests pass.

Summary ID
Consolidate cache_memoize state and functionality into a CacheContext.
We pass a lot of state around the various functions that comprise `cache_memoize()` and `cache_memoize_iter()`, and we have a lot of key building that we duplicate everywhere. Some upcoming work will be making a lot of this even more complex, so in preparation for that, this change cleans up much of this state and logic. There's now an internal `_CacheContext` class, which stores the state being used for a caching operation. This includes: * The cache connection * The base key name passed to `cache_memoize()`/`cache_memoize_iter()` * the full key name generated through `make_cache_key()` * The timeout * Whether to compress large data This simplifies a lot of function signatures. It then provides functions for generating suitable keys (wrapping `make_cache_key()`, subkeys (used for storing chunks), loading cached data, and storing cached data. Right now, these are mostly very simple wrappers, but they'll soon be extended for additional checks and functionality, hence the consolidation of this logic. There's also a performance improvement/race condition fix here. Both `cache_memoize()` and `cache_memoize_iter()` previously checked if a value was in cache, and then pulled it out. This requires two hits to cache instead of one, and it's possible (though unlikely) that the value could disappear in the meantime. In practice, we'd gracefully handle the failure, but the two cache hits weren't necessary. We now pull the value out, defaulting the result to a `_NO_RESULTS` sentinel if not found. We then make a determination based on this. Aside from the performance improvement, there are no changes to how the cache is utilized. This does make extensive improvements to our documentation.
4bf8f00c076d10063aadcb3158a7888311a59070
Description From Last Updated

Since this is purely internal API, do we really need to document changes?

daviddavid
david
  1. 
      
  2. djblets/cache/backend.py (Diff revision 1)
     
     
     
     
    Show all issues

    Since this is purely internal API, do we really need to document changes?

    1. Probably not. I just felt, does more good than harm to record when this changed, since I was updating docs anyway.

  3. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.x (615f5dc)
Loading...