Optimize redirect chain retrieval in VariationCache

kristiaanvandeneynde → credited berdir → .

kristiaanvandeneynde → credited catch → .

Let's see what testbot has to say.

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.

#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.

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.

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.

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.

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.

Refresh status change...

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.

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.

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

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.

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

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

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().

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