Improve page performance by sorting cache contexts/tags on-demand

Following, will have a look.

I've seen this too and I agree that we can definitely improve things. Especially in big tables, the merging and so on that is going on is immense.

That said, blackfire tends to massively over-report quick function calls that are called many times. The time you can gain from it is likely a lot less impressive than what blackfire says. That said, even just getting rid of the noise in blackfire reports will make it easier to identify possibly . And it does cost something.

I think the comment and/or assertion needs updating in EntityUnitTest, otherwise this is looking promising.

Should we also try to figure out why that other test doesn't work with assertEqualsCanonicalizing?

I would recommend you compare with something like plain ApacheBench to get a better understanding of the real difference and make sure you disable both blackfire and xdebug extensions. Because those absolute numbers are pretty wild, is that page really *that* slow?

And if you have enough cache tags to to have a problematically large dataset for sort, then you have a other problems too. Because querying the checksum backend to verify that they are still up to date is going to be far slower than a sort. It's more likely to be many, many calls to sort with a small array.

159,000 calls to Cache::mergeTags() is quite a lot. I don't know how many tags there are on average here, but some sample runs of sort() alone with different data set sizes:

$ time php -r '$a = range(0, 100); for ($i = 0; $i < 159000; $i++) sort($a);'

real	0m0.497s
user	0m0.488s
sys	0m0.008s

$ time php -r '$a = range(0, 200); for ($i = 0; $i < 159000; $i++) sort($a);'

real	0m1.177s
user	0m1.157s
sys	0m0.020s

$ time php -r '$a = range(0, 500); for ($i = 0; $i < 159000; $i++) sort($a);'

real	0m3.428s
user	0m3.404s
sys	0m0.024s

$ time php -r '$a = range(0, 1000); for ($i = 0; $i < 159000; $i++) sort($a);'

real	0m8.019s
user	0m7.999s
sys	0m0.020s

If we can eliminate all these then it would seem to be a good improvement, even in the 100 tags case.

> They tested actual implementation instead of expected behavior because in a given context it should/does not matter if cache tags are bar config:baz entity:foo or entity:foo bar config:baz , what matters is that cacheability is set/inherited/calculated properly and the expected cache tags or contexts are available/set.

Agreed and IMHO several of my commands go in that direction. (Exception are headers, where we still sort and should also test it like that IMHO) That we change the expected order instead of asserting in an order-agnostic way, like you did with many of the tests. The difference is that so far, tests could assume an alphabetic order and hardcode that, now they need to be explicitly not explicit about the order ;)

> * exposing this information on an admin UI where proper cacheability information "does not matter" so it is not calculated (at least I see a tendency here and there for this)

Note: Cacheability metadata is always collected, even if we decide that we don't cache it. regular render cache is active everywhere, only dynamic page cache explicitly excludes admin pages. But that's just the final decision on whether or not it _uses_ that info. It's there.

So one page where you should see a benefit with just core would be the permission table or views tables with a lot of rows.

Maybe you misunderstood me. I'm trying to point out that this will actually help on /admin pages too and might have a measurable impact on certain pages out of the box, without having the same rather special use case/scenario as you do. Which will make it easier get this in, if it's not just for pages that maybe shouldn't be built the way they are.

We don't count data structures as API, and the order of cache tags/contexts is barely a data structure, so this could land in a minor release. Caveat that I haven't actually reviewed the patch yet.

All review points have been addressed; ultimately this is a simple change to the runtime code with lots of changes to supporting tests, so over to core committers to see what they think.

Patch looks good to me, I added a change record to help out any failing contrib tests.

While it's tricky to quantify the exact impact of removing function calls on raw performance due to profiling exaggerating the savings, removing hundreds or thousands of function calls from requests also removes noise from profiling - making it easier to spot other issues. So it's generally worth doing either way.

Committed 6951f4d and pushed to 9.3.x. Thanks!

Very nice!

We intentionally did this back in #2429257: Bubble cache contexts to optimize for DX until proven otherwise. It was proven otherwise here, in pretty extreme situations.

Also: TIL assertEqualsCanonicalizing()!