Optimize redirect chain retrieval in VariationCache

Created on 13 March 2025, 5 days ago

Problem/Motivation

In 📌 Create placeholders for more things Active and on Slack we (@berdir, @catch and myself) did some digging and came to the conclusion that, when trying to get something that was invalidated from VariationCache, we follow the previously set redirect chain twice: Once for GET and once for SET.

As redirect chains are never invalidated, only overwritten during SET (self-healing), we can store the chain we checked during GET in memory to reuse during SET. This should net us an overall reduction of cache lookups. We should clear the chain from memory whenever we do a SET.

The only thing to be careful about is concurrency. If we store the redirect chain in memory during request A and request B sets something in the cache, then request A may use a stale chain to set whatever it is trying to save. In theory this is fine, because the worst thing that could happen here is that we undo a piece of self-healing from request B, making that request's recently stored item unfindable and forcing it to be recalculated.

Steps to reproduce

Check performance results after MR

Proposed resolution

Store redirect chain in memory until the next SET.

Remaining tasks

N/A

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

Feature request
Status

Active

Version

11.0 🔥

Component

cache system

Created by

🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @kristiaanvandeneynde
  • 🇨🇭Switzerland berdir Switzerland
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Let's see what testbot has to say.

  • Pipeline finished with Failed
    5 days ago
    Total: 94s
    #447490
  • Pipeline finished with Failed
    5 days ago
    Total: 89s
    #447582
  • Pipeline finished with Failed
    5 days ago
    Total: 351s
    #447599
  • Pipeline finished with Failed
    5 days ago
    Total: 322s
    #447627
  • 🇬🇧United Kingdom catch

    The results here look great. The only possible change I would make would be instead of validating that cache contexts haven't changed, maybe keying the cache IDs by the actual request object using SplObjectStorage - that ought to allow the logic to still work if there are multiple subrequests then.

  • 🇬🇧United Kingdom catch

    #7 isn't enough because we can't guarantee that cache context changing things will correctly create/pop requests on/off the request stack, so let's just do the validation for now, and it'll be on the very long list of things to solve to make async rendering in frankenphp work when we get there - but a possible risk of cache misses, not incorrect rendering.

  • 🇨🇭Switzerland berdir Switzerland

    I pushed a commit to try and support getMultiple() as well. It's a bit tricky due to the loops and behavior of getMultiple() I thought I gotten it working, but not yet seeing a change in StandardPerformanceTest.

  • Pipeline finished with Failed
    4 days ago
    Total: 120s
    #447846
  • 🇨🇭Switzerland berdir Switzerland

    Note: By not affecting tests, I meant when combined with the other two issues. On it's own it probably won't do much yet, as we're not yet pushing enough things into CachedStrategy.

  • Pipeline finished with Failed
    4 days ago
    Total: 343s
    #447852
  • 🇨🇭Switzerland berdir Switzerland

    This might be doing too much. It's now a full static cache of all variation cache lookups. That will keep those things in memory and it's also causing _tons_ of test fails, for example because it's not checking cache tags.

    My idea was to restrict this to lookups that end in a cache miss, either immediately or after redirects, Then there are fewer problems with invalidations.

  • Pipeline finished with Failed
    4 days ago
    Total: 225s
    #447930
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    LOL, @berdir beat me to it. I lost sleep last night thinking we're caching too much on cache HITS because we know redirects are permanent but the end of the chain on a hit is not.

    So either we switch to a MemoryCache for the redirect chain, which would turn this whole thing into a no-op and a glorified chained backend or we just make sure we pop a cache HIT off the chain and store only the parts leading up to it.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Refresh status change...

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    I think we can do one better in getRedirectChain(). Rather than not setting a cache on a cache HIT, we can still store the path leading up to the hit and reuse that next time we construct a redirect chain. At least that way we can skip requesting all the redirects again.

  • Pipeline finished with Failed
    4 days ago
    Total: 651s
    #448198
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Updated the code, we still get good results on performance tests. Just 3 cache gets more than the initial improvement.

    I also temporarily reverted the getMultiple() changes so we can first see which tests fail on the single changes. Easier to debug that way.

  • Pipeline finished with Failed
    4 days ago
    Total: 510s
    #448219
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    That's looking a lot better already. I suggest we fix these test fails and then introduce the same logic in getMultiple().

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    I think NodeTest might be failing for the same reasons we need refreshVariablesTrait. One request has a MISS, the other makes it so something is cached, but the cache in the test's thread still thinks there's a MISS and returns as such.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Yeah I got a fix, but got something urgent to attend to.

  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Pushed some fixes already, you can see the pattern is exactly like how we use refreshVariables()

  • Pipeline finished with Failed
    4 days ago
    Total: 153s
    #448413
  • Pipeline finished with Failed
    4 days ago
    Total: 629s
    #448449
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Fixed final test fail locally and the other went green locally. So if all goes green we can focus on reintroducing the static cache in getMultiple().

  • Pipeline finished with Success
    4 days ago
    Total: 901s
    #448492
  • 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium

    Okay, all green :) Now for the final hurdle...

  • Pipeline finished with Success
    1 day ago
    Total: 829s
    #449522
Production build 0.71.5 2024