Problem/Motivation

#2232375: Support CacheOptionalInterface in BlockViewBuilder, use it in language switcher block to prevent max-age 0 bubbling prevents the language switcher block being cached in the dynamic page cache.

We can also prevent it being rendered by bigpipe using #placeholder_strategy_denylist, this will mean that after #3508508: Allow big pipe to run for session-less users it won't prevent pages going into the page cache (external or internal).

Additionally because the language switcher block is often placed at the top of the page and is very cheap to render, it will additionally prevent jank/content layout shift issues from it being rendered by JavaScript.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3554414

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:

Comments

catch created an issue. See original summary.

catch’s picture

Title: User #placeholder_strategy_denylist in the language switcher block to avoid big pipe » User #placeholder_strategy_denylist for CacheOptionalInterface blocks to prevent them being rendered by BigPipe
Status: Active » Needs review

Re-titling since this is a generic change even if the language switcher block is the only one it will affect so far.

berdir’s picture

Status: Needs review » Active

This has a bigger impact than I expected and skipping several config and even bootstrap caches, we probably want to investigate exactly why that is.

berdir’s picture

Status: Active » Needs work
berdir’s picture

The Drupal\Tests\language\Functional\LanguageSwitching test fails I think means we're running into similar "complications" with #3349201: Big Pipe, logged-in users and 4xx pages as in the CacheOptionalIssue, some implementations there also "solved" that problem and I think this now does as well.

catch’s picture

I did a before/after with xhprof, and have a partial answer. Umami + Navigation - Toolbar logged in as user/1 and visiting https://drupal-dev.ddev.site/en/recipes/super-easy-vegetarian-pasta-bake after a drush cr.

Before the MR there are 553 calls to Drupal\Core\Asset\LibraryDependencyResolver::getLibrariesWithDependencies, after the MR there are 524 - this will be because by skipping BigPipe, we don't need to prepare the AJAX response when replacing the big pipe placeholder.

So instead of ten calls to AssetResolver::getJsAssets() there are nine calls etc. etc.

The assets to prepare for each big pipe placeholder can be different (e.g. different libraries attached to the different elements in the replacement) so these will often be separate lookups, although they could also be the same. In Umami it's a mixture of both.

There is already an optimization in ::getJsAssets() to skip the library resolving when both assets and settings are empty, however when Contextual module is enabled, settings always has this:

 array:1 [▼
  "contextual" => array:1 [▼
    "theme" => "umami"
  ]
]

So we always get past that and do the full library resolving for empty libraries and nearly empty settings, which means hook_js_alter() and etc. run, which in turn to do other things.

We might be able to optimize this further with some static caching, could be a decent saving if so especially with more placeholders. #3414398: AssetResolver::getJsAssets() and AssetResolver::getCssAssets() can repeatedly try to calculate the same set of assets was my previous attempt to fix this that obviously did not do enough.

The administrator node test is new, and I assume it's the only test that shows this because it's the only user we test with that has access to contextual links. Without contextual links we'd hit the empty settings path.

I don't know if that explains everything but it definitely explains some of it. I looked through the xhprof diff and couldn't find anything that wasn't related to this, so we'd probably need to fix it and then try an xhprof diff again to see if there's something else - although even that might be another side-effect of creating one less AJAX response.

catch’s picture

Status: Needs work » Needs review

Most of #7 is wrong in that there aren't less operations to account for. We do skip the additional big pipe ajax response, one call to hook_js_settings_alter() etc. but the different cache operations in that test failure were due to a random failure which I can't reproduce locally. AssetResolver already uses a chained backend so adding an extra static cache there has minimal effect and neither does skipping one call to it.

Updating one test that definitely does fail - should be no big_pipe.js + htmx for authenticated warm cache which is the intent of the overall change.

Would like to know why the random test failure resulted in less things happening rather than more but hard to track that down when it's random.

catch’s picture

#7/#8 mystery is solved by #3554646: Stabilize OpenTelemetryAuthenticatedPerformanceTest.

With that there's no cache/database change here at all, but we do save loading about 50kb of JavaScript on an authenticated warm cache request - because Big Pipe is taken out of the equation.

The other benefit is that pages that don't require big pipe are varnish/CDN-cacheable, which enables #3508508: Allow big pipe to run for session-less users.

berdir’s picture

Component: language system » block.module
Status: Needs review » Reviewed & tested by the community

The changes make sense to me. Moving to block as it's not language specific.

The comment changes to the test where the behavior now changes for this specific block are IMHO fine, it doesn't solve the issue, it just no longer means that this specific block is affected by it.

alexpott’s picture

Issue tags: +Needs followup

This change looks great - the amount of bytes saved in the performance test is amazing. I think we need to document how blocks use CacheOptionalInterface so block plugin authors are aware. Plus as @catch points out

we really need this to be understood is in all the page builders, none of which do any of this.

(page builders are canvas, layout builder...)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 6209c29211c to 11.x and eb51f3b6652 to 11.3.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed eb51f3b6 on 11.3.x
    Issue #3554414 by catch, berdir: User #placeholder_strategy_denylist for...

  • alexpott committed 6209c292 on 11.x
    Issue #3554414 by catch, berdir: User #placeholder_strategy_denylist for...
catch’s picture

Title: User #placeholder_strategy_denylist for CacheOptionalInterface blocks to prevent them being rendered by BigPipe » Use #placeholder_strategy_denylist for CacheOptionalInterface blocks to prevent them being rendered by BigPipe

Status: Fixed » Closed (fixed)

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