Problem/Motivation

Don't fully understand why yet, but i noticed on our project with redis monitor that there are quite a few identical cache lookups:

"HGETALL" "prefix:data:css:THEME_NAMEenaxHQwg5UBhW9FfRUylgqlvQ43K67H9VuTW3QXt27iZU1"
"HGETALL" "prefix:data:js:THEME_NAMEen:D9AaXzT3MlnOQcjEJMKZEOqJpT9sVkpufRFf2FceqFM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:NXhscRe0440PFpI5dSznEVgmauL25KojD7u4e9aZwOM11"
"HGETALL" "prefix:data:js:THEME_NAMEen:D9AaXzT3MlnOQcjEJMKZEOqJpT9sVkpufRFf2FceqFM11"

There are two unique hashes, the D9A... one is actually all-the-libraries, the NXh hash is an empty array of $libraries_to_load and settings.

Not sure how common this situation is, this also also on 10.4, haven't verified yet if 11.x changes anything.

I'm not sure what this is doing exactly with those assets. Took me a while to find where exactly it's looping, it's in \Drupal\big_pipe\Render\BigPipe::sendNoJsPlaceholders(), which has 23 fragments, all or most seem to be toolbar parts (classic toolbar with admin_toolbar).

Not sure what should be done in this scenario. statically cache the cache? if it's all identical, do we even need to process it or could we somehow return early somewhere?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 3504559-nr-bot.txt91 bytesneeds-review-queue-bot
#8 3504559-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3504559

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

#3493911: Add a CachedPlaceholderStrategy to optimize render cache hits and reduce layout shift from big pipe is also not helping here, because those placeholders aren't cacheable.

catch’s picture

catch
he/him
Primary language English
commented

I think I tried to get rid of this in #3494354: AssetResolver::getJsAssets cache id doesn't consider attached settings, leads to ajax issues 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.

berdir opened merge request !11121

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review

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

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3504559-nr-bot.txt91 bytes

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.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

Rebased.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

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

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3504559-nr-bot.txt91 bytes

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.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

Rebased, minor conflict on CacheTagIsValidCount.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

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!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

Rebased and test assertions updated, green again.

smustgrave’s picture

smustgrave commented

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Okay in that case going to remark it

  • catch committed 407e7645 on 11.x 
    Issue #3504559 by berdir: AssetResolver may load the same cache several...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

catch closed merge request !11121

Status: Fixed » Closed (fixed)

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