Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hi,
while rendering blocks, some useless block cache queries could be prevent by retrieving cache per block region rather than per block individually.
But of course, a better solution should be to retrieve all blocks caches at once...
And unfortunetly, as block rendering process is designed (per region individually rather than a list of regions), we can't load all blocks caches at once without rewriting it completly...
Thanks
Comments
Comment #1
chaby CreditAttribution: chaby commentedHere is a simple patch just for testing and sharing...
Comment #2
chaby CreditAttribution: chaby commentedAnd BTW, an another patch based on the previous (#1) to give a chance to integrator/developer to force block cache even if grant node is enable. Indeed, block module could't know how to deals with, but integrator could...(duplicate issue with #1930960).
Comment #3
pounard+1
RTBCEDIT: Needs some minor coding standard fixes.This patch saves a lot of cache queries when you have a lot of blocs in a single region.
Comment #4
pounardNote that #2 patch also include a variant of #1930960: Block caching disable hardcoded on sites with hook_node_grant() causes serious performance troubles when not necessary which I marked as a duplicate.
Comment #5
pounardComment #6
pounardFixed a few coding standard issues
Polished the comments and admin form wording.
Added the
block_cache_bypass_node_grants
variable documentation into settings.php file.No credits please.
Comment #7
pounardRTBC: Patch is straightforward and propose two nice performance improvements for the block module.
The only discutable thing is the node grants cache disable bypass variable name, I don't want to fight for a name so any name will be OK for me. This feature is performance critical for some projects abusing of core Drupal block.
Please all credits to @chaby
Comment #8
pounardAs requested per David Rothstein on IRC, I am splitting this patch into 2 separates issues, this one remains for block caching improvement only, and I'm reopening #1930960: Block caching disable hardcoded on sites with hook_node_grant() causes serious performance troubles when not necessary.
The reason is that the block caching improvement is valid for Drupal 7 only: Drupal 8 now uses plugins and the rendering pipeline is very different and splits from
_block_render_blocks()
. In Drupal 8 it has been renamed_block_get_renderable_region()
and the algorithm changes: caching kicks in later at the rendering time using the#cache
element, on which we cannot apply the same algorithm.Considering the other patch, it is applyable to Drupal 8, that's why I'm reopening it.
If tests pass, I still consider it as RTBC.
As stated uper, please don't credit me, all the work has been done by chaby.
Comment #9
pounardComment #10
pounardComment #11
David_Rothstein CreditAttribution: David_Rothstein commentedLooks like a good improvement in the case where you have a lot of blocks in a single region. (Might be a tiny performance hit if you don't, due to the duplicate _block_get_cache_id() calls? - in theory there could be a followup to refactor the cache array a bit so that the duplicate call isn't necessary. But overall looks great.)
Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/43c8918
@pounard, I did credit you on the commit also since you worked on the patch, and in addition reviewed it which counts too :)
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedFor what it's worth, here's the followup patch to avoid the duplicate _block_get_cache_id() calls and therefore basically avoid any possibility of a performance regression here at all.
I haven't tested this at all, though.
Comment #14
David_Rothstein CreditAttribution: David_Rothstein commentedAh, so the reason that failed is that the $cid variable is being used later on in the function (to write data to the cache) and needs to be defined.
Which actually points out a problem with the committed patch. It's defining $cid even when $cacheable is FALSE; therefore, data will sometimes be written to the cache when it shouldn't be.
Therefore, I rolled back the earlier commit for now and let's see if we can fix this up a bit.
Comment #15
David_Rothstein CreditAttribution: David_Rothstein commentedOK, let's try this as a new patch to work from for this issue. It should resolve everything discussed above.
Comment #16
pounardThe _block_get_cache_id() does basically nothing more than a variable_get(), an array_merge() and an implode() which represents only 3 functions calls per block: this is a lot less than what a cache_get() would do.
Nevertheless I'm not against removing those function calls, but I'm not sure why you are messing with an additional array where you could directly index the $cids array with the $key value: it would be a much simpler change.
Attaching patch.
I wrote it without testing, but it should pass. Notice I put the $cids variable declaration in a higher scope (outside of the if) so I didn't need to do additional check lower in the foreach() loop and remove the extra $cacheable check.
Comment #18
pounardOk, simple error, $cids is passed per reference so it ends up being modified by the getMultiple() method: we loose the $cid information along the way for cached blocks and they end up not being found in the rest of the algorightm. Now I understand why you'd mess up with another array! Using array_values() should fix that instead and keeps the algorithm smaller and more readable, but that's probably a matter of taste.
Comment #19
pounardComment #20
pounard@David_Rothstein this is ready for review/commit.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedThat works for me, however:
I think this will cause E_STRICT warnings (about only variables being passed by reference). And it could probably use a code comment anyway.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedFixing that issue. Does this look good to you?
Comment #23
pounardThis did not cause the E_STRICT to raise on my box, but I'm fine with keeping that fix. IMO this patch is ready. Thanks!
Comment #24
chaby CreditAttribution: chaby commentedRTBC for me too. Thanks !
Comment #25
pounardReviving this patch.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/19dc0fc
Comment #27
pounardThank you very much!