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

Files: 
CommentFileSizeAuthor
#22 1956914-22-block-cache_get_multiple.patch2.03 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,529 pass(es).
[ View ]
#18 1956914-18-block-cache_get_multiple.patch1.86 KBpounard
PASSED: [[SimpleTest]]: [MySQL] 40,387 pass(es).
[ View ]
#16 1956914-16-block-cache_get_multiple.patch1.84 KBpounard
FAILED: [[SimpleTest]]: [MySQL] 40,062 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#15 1956914-15-block-cache-get-multiple.patch2.48 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,311 pass(es).
[ View ]
#14 1956914-14-block-cache_get_multiple-FOLLOWUP.patch1.26 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1956914-14-block-cache_get_multiple-FOLLOWUP.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 1956914-12-block-cache_get_multiple-FOLLOWUP.patch1.24 KBDavid_Rothstein
FAILED: [[SimpleTest]]: [MySQL] 40,200 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#8 1956914-8-block-cache_get_multiple.patch1.82 KBpounard
PASSED: [[SimpleTest]]: [MySQL] 40,408 pass(es).
[ View ]
#6 1956914-6-block-cache_get_multiple.patch4.41 KBpounard
PASSED: [[SimpleTest]]: [MySQL] 40,324 pass(es).
[ View ]
#2 block-cache_multiple-1956914-2.patch3.26 KBchaby
PASSED: [[SimpleTest]]: [MySQL] 40,129 pass(es).
[ View ]
#1 block-cache_multiple-1956914-1.patch1.65 KBchaby
PASSED: [[SimpleTest]]: [MySQL] 40,170 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.65 KB
PASSED: [[SimpleTest]]: [MySQL] 40,170 pass(es).
[ View ]

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

StatusFileSize
new3.26 KB
PASSED: [[SimpleTest]]: [MySQL] 40,129 pass(es).
[ View ]

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

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.

Title:block cache multiple per region instead of per single blockUse a single cache_get_multiple() call per region instead of a cache_get() per block in _block_render_blocks()
Issue tags:+Performance

StatusFileSize
new4.41 KB
PASSED: [[SimpleTest]]: [MySQL] 40,324 pass(es).
[ View ]

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.

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

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new1.82 KB
PASSED: [[SimpleTest]]: [MySQL] 40,408 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

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

Status:Fixed» Needs review
StatusFileSize
new1.24 KB
FAILED: [[SimpleTest]]: [MySQL] 40,200 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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.

Category:support» task
StatusFileSize
new1.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1956914-14-block-cache_get_multiple-FOLLOWUP.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.48 KB
PASSED: [[SimpleTest]]: [MySQL] 40,311 pass(es).
[ View ]

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

StatusFileSize
new1.84 KB
FAILED: [[SimpleTest]]: [MySQL] 40,062 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work
StatusFileSize
new1.86 KB
PASSED: [[SimpleTest]]: [MySQL] 40,387 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs work» Reviewed & tested by the community

@David_Rothstein this is ready for review/commit.

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.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.03 KB
PASSED: [[SimpleTest]]: [MySQL] 40,529 pass(es).
[ View ]

Fixing that issue. Does this look good to you?

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!

RTBC for me too. Thanks !

Reviving this patch.

Status:Reviewed & tested by the community» Fixed

Thank you very much!

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