- Issue created by @kristiaanvandeneynde
- Merge request !11463Update variation cache so chain lookups are stored in memory until the next... → (Open) created by kristiaanvandeneynde
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Let's see what testbot has to say.
- 🇬🇧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.
- 🇨🇭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.
- 🇨🇭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.
- 🇧🇪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
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.
- 🇧🇪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.
- 🇧🇪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()
- 🇧🇪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().
- 🇧🇪Belgium kristiaanvandeneynde Antwerp, Belgium
Okay, all green :) Now for the final hurdle...