AssetResolver may load the same cache several time on a single page with bigpipe

I can reproduce this on 11.x by installing admin_toolbar_tools, but I assume this might also happen if you have lots of placeholders without that.

I can see that this also happens out of the box on standard profile, for example with comments. To reproduce, create an article with a few comments, helps to filter redis monitor to just ":js:" then:

ddev redis-cli monitor | grep ":js:"
"HGETALL" "core:data:js:olivero:en:7zukb8t4ZfBhCx251RER8wVVOq1uHT4RK1bzyN-Zs4k10"
"HGETALL" "core:data:js:olivero:en:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM10"
"HGETALL" "core:data:js:olivero:en:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM10"
"HGETALL" "core:data:js:olivero:en:7zukb8t4ZfBhCx251RER8wVVOq1uHT4RK1bzyN-Zs4k10"
"HGETALL" "core:data:js:olivero:en:3tuqg80w5eAzmKR6QJQIP9lFpLGh3emPlFtUgncnLic10"
"HGETALL" "core:data:js:olivero:en:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM10"

7zukb8t4ZfBhCx251RER8wVVOq1uHT4RK1bzyN is repeated twice and NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM10 three times, that's on an article with 4 comments or so. Adding more comments doesn't directly correlate to the number of repetitions it seems.

Unsure how to proceed on this one. I could add a memory cache or something, but there might be something more useful to be done in this case?

I also tested with 🐛 AssetResolver::getJsAssets cache id doesn't consider attached settings, leads to ajax issues Active , doesn't really change this.

📌 Render the navigation toolbar in a placeholder Active is also not helping here, because those placeholders aren't cacheable.

I think I tried to get rid of this in 🐛 AssetResolver::getJsAssets cache id doesn't consider attached settings, leads to ajax issues Active by skipping the cache get on empty libraries altogether, but it is not entirely skippable due to alters etc.

Statically caching result of the cache get only in the case of empty libraries is probably the simplest thing to do here.

Injecting the cache chain now and updated the test. This will of course conflict with several other issues.

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.

Rebased.

This looks great, I don't see anything else to do.

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.

Rebased, minor conflict on CacheTagIsValidCount.

Appears to need a rebase

If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!

Rebased and test assertions updated, green again.

Will add to my list for tomorrow but since it was previously RTBC wonder if it’s good to go back?

It should be, nothing changed except performance metric conflicts. One reason I didn't set it back myself is that there are a lot of performance issues right now and it's not as important/big of an impact as some others, but any improvement helps...

Okay in that case going to remark it

Committed/pushed to 11.x, thanks!

catch → closed merge request !11121

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