Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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!