Active
Project:
Drupal core
Version:
main
Component:
cache system
Priority:
Major
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
17 Mar 2015 at 15:03 UTC
Updated:
5 Apr 2025 at 09:00 UTC
Jump to comment: Most recent
Problem/Motivation
#pre_render + #cache is great, but has one big disadvantage:
Items in listings (300 comments, 20 blocks) are gotten one item at a time.
That means 300 cache_get instead of one cache_get_multiple().
The previous block cache did a get_multiple per region and hence was faster.
Proposed resolution
Add a cache pre-fetching foreach () loop on all children to Renderer::render().
Possibly set the pre-warmed data on an internal #cache_data attribute temporarily and remove after the #cache check again.
Remaining tasks
- Write a patch
User interface changes
- None
API changes
- None, no API change at all. Pure performance optimization.
--
This is major because of the performance impact. It likely is also a pretty simple quick fix.
Issue fork drupal-2453945
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:
compare
Comments
Comment #1
wim leersAre you sure that this is a regression? The block cache was removed in #2158003: Remove Block Cache API in favor of blocks returning #cache with cache tags, and interestingly, no mention of "get multiple" — neither in the comments nor the patch.
Comment #2
wim leersThat being said, +100 to this. But since this is something that can be done at any point during the release cycle, I think we should prefer to move this to 8.1, to keep focus on the perf issues that cannot be done after 8.0, because they require API changes.
Comment #3
fabianx commentedFrom https://api.drupal.org/api/drupal/modules%21block%21block.module/functio...
( I only know this as I re-implemented a lot of the logic in render_cache module and I was especially trying to not make performance worse than core's block cache).
Actually that hunk was already gone at that time so that patch is not to blame ...
This was even gone already when #918808: Standardize block cache as a drupal_render() #cache got in 4 years ago.
And the reason is that Drupal 8 never had this change to cache_get_multiple().
It is nowhere in the repo, this is a port fail, but so probably indeed not a regression:
#1956914: Use a single cache_get_multiple() call per region instead of a cache_get() per block in _block_render_blocks() introduced this for Drupal 7.
Comment #4
wim leersAh, that'd explain it indeed (that it was already gone before).
Nevertheless, as I said, very much agree that we should do this! But… we can actually go even further.
So far, we're saying that we should look at the children of the current level, see which ones have CIDs, and do a
::getMultiple()on that. That's true. But we can go further than that: instead of looking only at the children, for every child that does not have a CID, we can actually descend further down until we hit a level in the render tree again that has CIDs.In other words: for each child, the first encountered level that has a CID.
Not sure if the extra traversing necessary would offset the advantage of doing an even bigger
::getMultiple()though.Comment #5
fabianx commentedAs edited in my last comment #1956914: Use a single cache_get_multiple() call per region instead of a cache_get() per block in _block_render_blocks() introduced this for Drupal 7.
Comment #6
berdirNoticed a lot of cache get calls from the Renderer as well, I guess this would then also apply to things like lists of nodes in a view in the same way?
Another way to address this might be to try and render cache complete regions if all blocks in a region can be cached? I started implementation something like that for #2344073: Support block caching, which I will have to update for the max-age and cache context changes now, where I automatically cache the whole page display if every block inside it can be cached. Separate issue probably, though
Comment #7
fabianx commentedYes, caching larger regions (also smartcache) definitely helps, but this should still be fixed.
Yes, lists of nodes in a view would probably be the same (if they set #cache render arrays).
Comment #8
catchRe-titling. This is definitely worth trying.
Comment #9
wim leersComment #10
fabianx commentedComment #11
wim leersSince this wouldn't require any API changes, keeping as major.
Comment #13
smccabe commentedTrying to pick this up but notes are a little outdated and my D8 caching knowledge isn't amazing, is the correct idea to add in a getMultiple to renderCache and add to either the Renderer:doRender or Renderer:render. A little nudge in the right direction would be much appreciated.
Comment #17
mxh commentedA major issue with "Quick fix" tag. Something is wrong here, guess this isn't something to quickly fix.
This one is not just a performance one, it would drastically reduce IO between PHP and Cache backend.
Comment #18
ndobromirov commentedBased on previous comment.
Comment #28
catchRender cache multiple get is now in core after #3493911: Add a CachedPlaceholderStrategy to optimize render cache hits and reduce layout shift from big pipe, we should see if there's an example in core where this would help, and then try to implement it in the renderer.
Comment #29
catchComment #30
berdirThe combination of those 4 issues means we now have efficient get multiple cache loading for placeholdered routes and we have quite a few of those now with menus, block_content, local tasks and so on being placeholdered:
#3493911: Add a CachedPlaceholderStrategy to optimize render cache hits and reduce layout shift from big pipe
#3512762: Optimize placeholder retrieval from cache
#3512766: Optimize redirect chain retrieval in VariationCache
#3437499: Use placeholdering for more blocks
So I think the benefits for blocks aren't that huge, you'd need multiple non-placeholdered blocks in the same region for this to be efficient. We might want to instead look into either placeholder even more blocks or then not cache them on their own at all ##3516051: Add CacheOptionalInterface to more blocks ).
Looking at umami, the only blocks that are not placeholdered are: umami_search (simple form, pretty fast to build), umami_branding (also pretty simple), umami_messages (just puts in a placeholder, definitely could be non-cacheable?) and umami_help (unsure, but depends on route, so high amount of variations). So, not a lot of benefit here.
But I think one scenario where this is still interesting is what I mentioned #6. If you have a view with 50 nodes that gets invalidated, then you can fetch them with a single cache get instead of 50 (with cache redirects, possibly even 2 instead of 100). To measure the benefit, we'd need to create a handful of articles for the frontpage view.
We'd need to run the children through getMultiple() and somehow flag those that have been fetched, so we don't attempt to load them again.