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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chaby’s picture

Status: Active » Needs review
FileSize
1.65 KB

Here is a simple patch just for testing and sharing...

chaby’s picture

And 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).

pounard’s picture

Issue tags: -Performance

+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.

pounard’s picture

pounard’s picture

Title: block cache multiple per region instead of per single block » Use a single cache_get_multiple() call per region instead of a cache_get() per block in _block_render_blocks()
Issue tags: +Performance
pounard’s picture

Fixed 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.

pounard’s picture

Status: Needs review » Reviewed & tested by the community

RTBC: 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

pounard’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
1.82 KB

As 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.

pounard’s picture

Status: Needs work » Needs review
pounard’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Looks 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 :)

David_Rothstein’s picture

Status: Fixed » Needs review
FileSize
1.24 KB

For 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.

Status: Needs review » Needs work

The last submitted patch, 1956914-12-block-cache_get_multiple-FOLLOWUP.patch, failed testing.

David_Rothstein’s picture

Category: support » task
FileSize
1.26 KB

Ah, 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.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
2.48 KB

OK, let's try this as a new patch to work from for this issue. It should resolve everything discussed above.

pounard’s picture

The _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.

Status: Needs review » Needs work

The last submitted patch, 1956914-16-block-cache_get_multiple.patch, failed testing.

pounard’s picture

Status: Needs review » Needs work
FileSize
1.86 KB

Ok, 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.

pounard’s picture

Status: Needs work » Needs review
pounard’s picture

Status: Needs work » Reviewed & tested by the community

@David_Rothstein this is ready for review/commit.

David_Rothstein’s picture

That works for me, however:

+      $cached_blocks = cache_get_multiple(array_values($cids), 'cache_block');

I think this will cause E_STRICT warnings (about only variables being passed by reference). And it could probably use a code comment anyway.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.03 KB

Fixing that issue. Does this look good to you?

pounard’s picture

Status: Needs review » Reviewed & tested by the community

This 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!

chaby’s picture

RTBC for me too. Thanks !

pounard’s picture

Reviving this patch.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
pounard’s picture

Thank you very much!

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