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
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
Comment #2
catchRe-titling since this is a generic change even if the language switcher block is the only one it will affect so far.
Comment #4
berdirThis has a bigger impact than I expected and skipping several config and even bootstrap caches, we probably want to investigate exactly why that is.
Comment #5
berdirComment #6
berdirThe 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.
Comment #7
catchI 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: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.
Comment #8
catchMost 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.
Comment #9
catch#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.
Comment #10
berdirThe 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.
Comment #11
alexpottThis 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
(page builders are canvas, layout builder...)
Comment #12
alexpottCommitted and pushed 6209c29211c to 11.x and eb51f3b6652 to 11.3.x. Thanks!
Comment #16
catchOpened #3556377: Better documentation for placeholder strategy denylist/CacheOptionalInterface for block plugin authors/consumers.
Comment #17
catch