AssetResolver::getJsAssets() and AssetResolver::getCssAssets() can repeatedly try to calculate the same set of assets

Can just early return when there's no libraries to load. This will help any big pipe response that doesn't return unique css/js that's already loaded - makes a decent reduction in the number of cache operations in both standard and umami performance tests. Won't be a measurable speed improvement, but less to do is good.

MR also tidies up the cache IDs to add colons between each part consistently. This doesn't resolve any bugs, it was just a debugging aid before I realised exactly what was going on. However I don't think it does any harm here.

Apparently not that easy.

📌 Add authenticated user umami performance tests Fixed landed.

Seems like a good cleanup, reviewing the code for assetResolver and using the variable seems to still work. Number going down too was neat.

Rebased after 📌 Separate cache operations from database queries in OpenTelemetry and assertions Fixed .

Nice — trivial improvement thanks to static caching, in a code path where I see no reason to ever need static cache clearing? Ship it! 🚢

Do we have any numbers? YES WE DO! 🤩 Thanks to core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php and core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryAuthenticatedPerformanceTest.php 👏

If we add a static cache, we save a couple of cache gets.

Devil's advocate: why does this matter? Aren't we just spreading out the places where we "keep stuff"? The cache backend already does static caching, so why does this matter?

It'd be more meaningful if this was about unique cache gets. Wouldn't it be more meaningful to only track cache gets for unique cache IDs? 🤔

The cache backend already does static caching, so why does this matter?

cache.data doesn't do any static caching, and it also doesn't use chained fast (which would go into apcu by default so be extremely cheap although not static as such) - so this is either a database query or a round trip to redis/memcache depending on the site.

fwiw the situation where this actually seems to happen is for a list of empty assets (i.e. where the AJAX request either attaches nothing, or only things that have already been attached - in fact any other situation ought to be a unique cache ID). I tried an early return when the list is empty and that broke lots of things, but I think we could special case the static caching and do it only when there's an empty list - it would be a bit more logic, but we can easily tell from the tests whether it'll still work like that.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

catch → changed the visibility of the branch 3414398-asset-resolving-runs to hidden.

Figured out what was wrong with the early return in the first attempt - new MR with no static caching and early returns that work this time. Saves a lot of cache gets when bigpipe is enabled.

Code wise seems like a good cleanup. Not sure how else to best test so leaning on performance tests.

We need to do the early return and the static cache here to get the best results. Updated MR incoming.

No it's a false alarm, sometimes we cache the same thing with different cache keys, but to figure that out, we'd have to run at least some of the logic we're caching, so just static caching doesn't help with that. The MR is relatively simple as it is and gets more complicated with static caching, and performance tests don't change when I implement it, so... moving back to RTBC.

Going back on #19, I think there might be one more thing we can do here.

Meh moving back to RTBC again, might open a follow-up for this last bit.

What happens is when you have e.g. the announce toolbar library, which only has a single CSS file in it, the end result of that in ::getJsAssets() is the same as if there are no libraries to load at all.

However we can only determine this by getting the full library definition from LibraryDiscovery::getLibraryByName(), which involves running some logic, which involves getting the library definitions from the cache collector, which.. potentially undermines caching in the first place. So changing that logic would need a fair bit of manual profiling I think.

I reverted the change @Berdir asked about in the MR, since that's only removing something out of scope, back to RTBC (again).

This looks great.

Committed and pushed e2840d7270 to 11.x and 557022fcdd to 10.3.x. Thanks!

alexpott → credited Berdir → .

Got an idea how to resolve what I was trying to figure out above, but opened 📌 Optimize AssetResolver caching Fixed , will put a stacked MR on there since it'll build on the changes here.

Whoops cross-post but that means I don't need to stack the other issue now!

Automatically closed - issue fixed for 2 weeks with no activity.