• 
      

    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:
    Completed
    Change Summary:
    Pushed to release-3.x (615f5dc)