Block caching

This patch is a different approach to block level caching. I think it is superior to the ideas implemented at http://drupal.org/node/72617.
This patch takes a more centralized approach to block caching. The idea is to free the module block author of as many decisions as possible.

There are four possible cache settings for each block:

BLOCK_NOT_CACHABLE: Don't cache this block at all. Usefull for highly dynamic blocks

BLOCK_ALWAYS_CACHED: Always cache this block. Usefull for static blocks

BLOCK_CACHED_PER_USER: Cache ths block for each user, usefull for personalized blocks.

BLOCK_CACHED_PER_ROLE: Cache this block per role. Usefull for permission dependend blocks.

In addition, there is also a time to live setting for each block's cache.

As an example, here the settings for the user module's blocks:

$blocks[0]['info'] = t('User login');
$blocks[0]['cache_mode'] = BLOCK_ALWAYS_CACHED;
$blocks[0]['cache_ttl'] = 0;
$blocks[1]['info'] = t('Navigation');
// The navigation block isn't cachable as it also depends on the
// page where it appears
$blocks[1]['cache_mode'] = BLOCK_NOT_CACHABLE;
$blocks[2]['info'] = t('Who\'s new');
$blocks[2]['cache_mode'] = BLOCK_ALWAYS_CACHED;
$blocks[2]['cache_ttl'] = 900;
$blocks[3]['info'] = t('Who\'s online');
$blocks[3]['cache_mode'] = BLOCK_NOT_CACHABLE;

The user module should probably invalidate the "Who's new" block's
cache when a new user is created.

To discuss:

- Should the defaults specified by the module author be editable
through the block admin interface?

- What to do with user crated blocks?

- Is this a good idea at all?

This patch is a proof of concept it lacks SQL updates and the appropriate settings for other blocks.

Attachment	Size
cache_block.patch_0.txt	4.78 KB

Some discussion in #drupal revealed that there is probably not enough of a use case to edit the default values.

The only use case where you would need to change the cache type to "per user" for all cachable blocks would be for modules like OG. However, this can be easily achieved by a second _submit function to block_admin_display.

The logic in _block_rehash might need some tweaking to allow for this, see also Gordon's patch:

http://drupal.org/node/80963

Awesome idea!

Ok so first off, I think there are too many options, 4 types of cache modes + a TTL. I think we can optimize this a bit better.

If a block is cacheable, it's TTL should determine how frequently it is updated:

TTL < 0 -- block is always cached
TTL = 0 -- block is never cached
TTL > 0 -- block is cached but updated ever 600s (or 5 min)

This should be an admin setting--for both module created blocks and custom blocks. A numeric value with help text below it--prepopulated by module created blocks.

Now for the cache mode, I'm not sure if PER_ROLE, PER_USER, and GENERAL make any sense. What is the advantage of this?

If we leave it just to TTL, it's simple, effective, and can be controlled at the module and interface levels.

We recently had a very high profile client that relied on some super complex blocks--most of which could be cached for 15 min then had to be regenerated. Setting a TTL of 15min on these custom blocks would have reduced SQL and CPU usage considerably.

+1

Shoot, looks like I lost some formating, meant to say:

Ugh, I keep losing my input, doh!

"Now for the cache mode, I'm not sure if PER_ROLE, PER_USER, and GENERAL
make any sense. What is the advantage of this?

If we leave it just to TTL, it's simple, effective, and can be
controlled at the module and interface levels. "

We need these different modes because the content of a block can vary depending on different parameters. One is the time at which the block was generated (this is covered by ttl), another one is for which user the block was generated.

Imagine you have a block that generates its output based on two different permissions. If you have both permissions you will see a different block than if you have only one. For this we need different cached copies and thus the "per role" setting.

The "per user" setting is needed if you use a module like OG.module. The "my groups" block is different for each user and thus needs to be cached per user.

The "one size fits all" cache will be the most efficient as you only need one copy for all users. The "per role" cache will need several copies depending on how many roles you have.

The "per user" cache will be least efficient as you need one copy per user, but it will still be usefull if the block is expensive to compute and displayed often.

A quick calculation: On drupal.org we usually have in between 1000 and 1500 cached menus. Assuming the blocks have a similar life time and all blocks (17 in total) would need to be cached per user (they don't), we would need 17*1500 = 25500 cache entries for a site the size of drupal.org. That's not too bad (much less than for example for the filter cache). This calculation assumes we'd use OG or a similar node access module. Since we don't, we will have much less entries.

I agree that it might make sense to make the cache ttl changable by admins.

Ok, the PER ROLE and PER USER blocks makes a lot of sense. Totally forgot you could have blocks that appear different for each user/role/etc.

Ok I'm cool with that then. However, can we use the TTL to determine these two:

BLOCK_NOT_CACHABLE: Don't cache this block at all. Usefull for highly dynamic blocks
BLOCK_ALWAYS_CACHED: Always cache this block. Usefull for static blocks

If we use my negative, 0, positive TTL scale I think these might be redundant, no?

Block caching is sorely needed and the approach looks sound.

Likely the defines should be general rather than block-specific, and should be in cache.inc, so that we can reuse them for other caching. I'd say maybe:

CACHE_SCOPE_ALL;
CACHE_SCOPE_NONE;
CACHE_SCOPE_PER_USER;
CACHE_SCOPE_PER_ROLE;

Erm, seriously flawed. Not going to work as is:

1. The content of all blocks can be translated based on personal locale and/or i18n settings.
2. Due to the db_rewrite_sql function, most of the blocks can be modified on the fly (and under the radar of the declaring module).
3. Because users can have multiple roles, there is no such thing as per-role settings. Users have the combination of all permissions in all of the roles assigned to them. Hence, "per role" really means: all the possible combinations of all available roles.

In your example patch, you cache two blocks. In addition to the reasons specified above, they are broken for two more reasons:

1. The who's new block can't be cached because it would ignore various permissions such as 'access user profiles'. For some users, the usernames show up as links, for other users they show up as plain text.
2. The 'user login' block can't be cached because of the hidden 'destination' field. It's a dynamic element with a different value on each different page.

The best you can do is per-user caching, and to drop the per-role caching.

1. The content of all blocks can be translated based on personal locale and/or i18n settings.

True I forgot about that. Easily fixed by appending the current locale ID to the cache ID as we do already for the menu cache.

2. Due to the db_rewrite_sql function, most of the blocks can be modified on the fly (and under the radar of the declaring module).

For the blocks using db_rewrite_sql, the module which implements the db_rewrite_sql hook should change the affected (or all) blocks to be cached "per user" as described in my example for OG.

3. Because users can have multiple roles, there is no such thing as per-role settings. Users have the combination of all permissions in all of the roles assigned to them. Hence, "per role" really means: all the possible combinations of all available roles.

Look at the code, I've already taken care of this by constructing the cache ID appropriately:

$cid = "$block->module:$block->delta:". serialize($user->roles);

This would give us some extra cache entries for people with different role combinations, but I think we could live with that.

In your example patch, you cache two blocks. In addition to the reasons specified above, they are broken for two more reasons:

1. The who's new block can't be cached because it would ignore various permissions such as 'access user profiles'. For some users, the usernames show up as links, for other users they show up as plain text.

No, since these permissions are connected to the user's roles that's an excellent use case for caching a block per role. :)

2. The 'user login' block can't be cached because of the hidden 'destination' field. It's a dynamic element with a different value on each different page.

Ah, yes, I forgot about this. Too bad.

I'd rather not drop the per role caching. It will improve performance on sites like drupal.org where we don't use db_rewrite_sql.

new patch.

Changes:

- Changed default to "not cachable", this way all blocks will act as before.
- Added useful (?) defaults for all core blocks
- Use existing cache_lifetime setting for content related blocks(!)
- user created blocks will use default, ie non-cachable (needs to be discussed)
- added sql scripts
- added cron hook to expire stale cache entries

Please review

Attachment	Size
block_cache.patch_1.txt	15.46 KB

The previous version also fixed the locale issue.

This version in addition will change all "per role" cache settings to "per user" if a node_access module is used. Only works when you re-save the admin/block form. Not sure this makes sense.

Attachment	Size
block_cache.patch_2.txt	15.79 KB

this line is not so readable IMO:
+ $block['cache_mode'] = isset($block['cache_mode']) ? (($block['cache_mode'] == BLOCK_CACHED_PER_ROLE && $node_access) ? BLOCK_CACHED_PER_USER : $block['cache_mode']) : BLOCK_NOT_CACHABLE;

technically, a node_access module could give a grant for certain users only on wednesdays and the per user logic would be incorrect. but i've never seen that, and i don't think we need to design for it. a module could always add a submit handler to block form and edit as needed.

That line isn't readable at all. :p

The idea behind adding it was that core should provide services to contrib modules. The service in this case would be "toggle state of block caching". The problem is that it requires a visit to the block admin page (which isn't really obvious). I guess that modle authors could document it, though. Then again, they probably could make the neccessary update part of their .install files.

As can be seen, I am not 100% clear on whether it is a good idea.

What do you you (and others) think about sings such as:

+ if ($ttl = variable_get('cache_lifetime', 0)) {
+ $blocks[0]['cache_mode'] = BLOCK_CACHED_PER_ROLE;
+ $blocks[0]['cache_ttl'] = $ttl;
+ $blocks[1]['cache_mode'] = BLOCK_CACHED_PER_ROLE;
+ $blocks[1]['cache_ttl'] = $ttl;
+ }
+ else {
+ $blocks[0]['cache_mode'] = BLOCK_NOT_CACHABLE;
+ $blocks[1]['cache_mode'] = BLOCK_NOT_CACHABLE;
+ }
+

makes sense?

What do you you (and others) think about sings such as:

If we intend cache_lifetime to provide a universal override--a variable that, if set to 0, prevents caching of blocks - presumably we should implement that test once, rather than separately for each block. A given block might return BLOCK_CACHED_PER_ROLE (or CACHED_PER_ROLE), but it would not be cached if cache_lifetime evaluated to 0.

Looks like this is being worked on.

nedjo: yes, that seems like a good idea. The question is: Do we want sich a central cache variable for registered users?

As a reference the form descrption of that variable:

'On high-traffic sites it can become necessary to enforce a minimum cache lifetime. The minimum cache lifetime is the minimum amount of time that will go by before the cache is emptied and recreated. A larger minimum cache lifetime offers better performance, but users will not see new content for a longer period of time.'

My path would extend this variable to apply for registered users and blocks.

Example: Here on drupal.org, that variable is set to 10 minutes. Do we want eg the "new forum" block to be delayed by that amount of time?

Should we have a separate variable?

00:09 < Bart> what happens when a block is set to cache per role but two users zith that
role are using a different theme?
00:09 < killes> Bart: hmmm.
00:09 < killes> Bart: let me see
00:15 < killes> Bart: yeah, that might be a potention problem.
00:15 < killes> Bart: but we can circumnavigate this by putting the current theme into the
cache ID
00:15 < killes> Bart: same as for current locale.

have been talking to redLED and from his experience in running large communtiy sites it will be very important to allow the site admin to regulate the cache settings for both admin created and module created blocks.

Hi,
I just want to express my support for block caching in core (maybe for 5.1, 5.0 is frozen I know).

Since starting to work with drupal I thought about serious block caching, as this will benefit also registered users and us webmasters will not "fear" to add many blocks to a page as we know it will have minimum impact on CPU usage.

I like the idea of a global setting + per block custom settings accessible from the blocks page. Custom block parameters could have a "Use default (default option)" "Never cached" "Always cached" and a cache life "XX seconds" options.

Version x.y.z?

So long as this method enables admins to specify whether a block is dynamic/static etc and give (some) control through the block admin page then it gets a +1 from me :)

As an admin I am always adding the number of blocks on my site in fear of CPU over kill. Block caching needs work and this could be an answer in the right direction. I like the fact the issue of caching has really been elevated in recent weeks and discussion has broken out in various nodes.

Feature requests go against 6.x-dev.

so, is this still desireable? if so, anyone up for it?

Yes, I think this patch still makes sense. I'd be willing to work on it if I knew I wouldn't work on something that is deemed unimportant and thus won't end up in core...

I think any performance improvement project is very important, in particular if it improves performance for logged in users.
I bet this kind of project will make it in core.

Actually, this project alone would be for me the main reason to convert to drupal 6 and i'm sure i'm not the only one.

I'd love to see this and will try to help test where possible.

I currently use the blockcache module, a lot.

Stuff I like about it:
1. caching per page - great for contextual blocks like related nodes/views with arguments - those can be quite expensive so it'd be nice to keep them cachable by page as an option.
2. update when node added/user logs in etc. - allows me to set long cache times but know that'll be overridden by new content.

Stuff I don't like:
really, really long block list since everything is duplicated due to the wrapper. Simply having a cache/don't cache option for a block, instead of 'cached version' 'uncached version' would be a massive improvement, and a great reason to move this out of contrib and into core.

killes : "I'd be willing to work on it if I knew I wouldn't work on something that is deemed unimportant and thus won't end up in core..."

I think Dries wrote in his 'D6 code extension' post / mail that any patch improving performances would get high attention - or something like that.
Block caching seems the perfect candidate - lots of gains to be expected for any/most 'authenticated users' based sites.
Even more so now that Views is one of the basic bricks for drupal site building - easy to add new blocks, equally easy to seriously mess your site's performance.

big +1 from me anyway.

Subscribing.

yched and I are going to try and wire this into my per-user caching patch at http://drupal.org/node/152901
This should be able to deal with the per-role and long lived per-user based caches.

One option with the interface is to simply not have any, and have modules provide their best guess at if something should be cached or not - with a timed expiry, cleared on a node save, or perhaps self cleared using a block_cache_clear API function. We could provide a hook (or allow some FAPI magic) to allow contrib modules to provide an interface to allow admins to tweak this on a per block level and improve their cache performance.

Blocks could be cached individually or en-mass (i.e. all cacheable blocks in the $blocks array). The latter is obviously much faster (remember that users would share the same cache if it is identical), but makes any smart cache invalidation strategies (such as a block_cache_clear API) pretty much impossible.

OK, after starting from jjeff's block_cache contrib, comparing with killes' patch above, discussing with both on IRC and getting ideas and advice from chx, I actually made a sort of round trip and ended with something quite similar to killes' patch in #12.

Keeping any UI clutter from the individual blocks settings pages, notably, was deemed important by all to keep this feature the best chances of getting in. With that in mind, the attached patch does its best to provide a flexible feature nonetheless.

Differences are :
- updated against current D6 :-)
- per chx suggestion, simplified the per block 'time to live' setting - we use variable_get('cache_lifetime', CACHE_TEMPORARY)
- merged chx original idea of a generic hook_block('cache_id') op, allowing modules to define their own granularity option (think 'per OG'...). We simply provide default PER_PAGE, PER_USER, PER_ROLE settings on top of that.
- unless a block is explicitly marked as not cacheable, we use PER_ROLE as a default caching pattern, as this is the most current case (see patch in #12).
- disabled block caching altogether for user 1 - that one seems too specific and brings too many chances of getting unwanted output cached and then displayed to other users.
- added a general option to switch off block caching on Performance settings page - Help text is still TODO.

PS - Grugnog : I currently lack time and braincells to figure how this could play with your generic user caching patch, sorry :-) But this seems rather independent actually, AFAICT.

Attachment	Size
block_cache.patch	9.78 KB

reviews welcome indeed :-)

Previous patch was missing a serialize.

I also forgot to add that the patch includes the FAPI3 fix for blocks overview form I posted in http://drupal.org/node/152585 - without it, block functionality is not really testable...

Attachment	Size
block_cache_0.patch	9.79 KB

Actually patch relies on an additional 'cache_mode' column in {blocks}, that I still had in my db from previous attempts, but left out of this patch because I thought it was not needed anymore - Silly me.

Too late too roll a new patch tonight, will update tomorrow...

OK, rerolled with the correct db upgrade : added 'cache_mode' column in {blocks}

The patch also disables caching for core blocks containing forms (since caching breaks form submission, of course)

This should really be up for review now.

Attachment	Size
block_cache_1.patch	14.07 KB

dmitrig01 marked as 'needs work' because of discussion on IRC about node_access issues.

Attached patch handles node_access this way : if node_access modules are enabled, we force the PER_USER bit for every cach_enabled block.

Back to 'needs review' :-)

Attachment	Size
block_cache_2.patch	14.31 KB

Comments need fixing. "disabled block caching altogether for user 1 - that one seems too specific and brings too many chances of getting unwanted output cached and then displayed to other users." -- add this to the code. "menu blocks might be different on every page " -- while this is true some more about how even on the same path (path and page is easy to mix up!) and for the same user the menu block can change. "Can't be cached because it contains a form. " -- this needs further examination, anon users can log in from a cached page. Lose serialize/unserialize , no longer needed.

@yched - if you change 'force' to 'default' in your lastr post then i think your proposal is fine. node_access modules often vary by role and there is no need to degrade performance by forcing per user. this is just the sort of thing that can give node_access the reputation of "don't - it slows down your site".

Updated patch adresses chx remarks in #38.
'Forms in block' is fixed by bypassing cache read / write when request method is not GET (just like regular page cache for anon users)

@moshe : I know the current 'per user' fallback for all (cachable) blocks when node acces modules are around is kind of harsh.
Problem is, as you pointed on IRC, that only the node access module can tell exactly what blocks it affects or not. But you probably can't expect node access modules to be able to say something about every other block in core or contrib. And this becomes a can of worms if several node access modules are present...

Best thing that comes to my mind (other than my current 'per user' fallback), is to provide per block 'disable caching' chackboxes. Maybe only if a node_access module is enabled. But this would slightly complicate the patch, and several people have expressed the feeling that a UI would kill the chances of getting this in...

Attachment	Size
block_cache_3.patch	13.73 KB

Moshe, I expect node access to slow down my site -- naturally there is more work done by the system if I want more fine grained control. I have no problems with this. And yched is right, anyways :)

If we could add support for my per user caching API patch I think that may go some way to avoiding performance problems when using node access. The reason for that is because users for whom the block is identical could share caches (because the hash would be the same) - so you don't need to store an additional cache for every single site user. This helps prevents having massively tall cache tables, and so should help keep performance up.

We could probably simplify this patch a lot by just automatically caching all blocks using the per user API - even if node access is off - without any major penalty. We should benchmark both ways of course, but there is the potential for significant simplification of the block caching code by using this approach. I would also like to experiment with caching things at a much higher level (i.e. all cachable blocks in the $blocks array, per user), to see how that works out.

I'll try and see if I can figure out how to mesh these ideas into the existing patch, or I may just need to take a different route.

Problem is, as you pointed on IRC, that only the node access module can tell exactly what blocks it affects or not.

As discussed in IRC, if node access causes a fall back to per user caching, I think there could be an "aggressive block caching" setting in performance - since as well as the modules, (at least some) admins should know what blocks they've got enabled and whether they'll be affected.

That checkbox would revert behaviour to the default without node_access modules enabled - with a help text that says "this may interfere with access control provided by the following modules, use at own risk etc" - then if I as an admin know that I've not got any blocks which are being altered by node_access I can use it whilst still getting the performance benefits. This would mean one checkbox as opposed to doing it per-block, and you can still set blocks to display by role etc. in block settings if you want to have it both ways.

|Having said that, grugnog's ideas sound very interesting and could negate this issue altogether.

@chx - i would expect node_access to slow down a site just as much as is necessary and no more. this patch slows down the site more than necessary, because many modules restrict access based on role.

i think we should pursue grugnug's suggestion that we do per_user for all blocks with hashes to mitigate the storage growth.

If we are afraid of the impact of forced 'per user' block caching when node_access modules enabled, I think catch's suggestion in #43 could be worthwhile :
'regular' (no node access module) options for block caching on admin/settings/performance are :
- disabled (by default)
- enabled

when a node access module is present, these become :
- disabled
- enabled - might cause performance issues due to the presence of node access modules
- aggressive - might interfere with access control provided by the following modules : (list of node access modules)

*Or* we could also simply say that block caching is automatically disabled when node_access modules are enabled.

Either way, i'd like to point out that we still gain lots of performance boost when no access module is used. At worst, sites with node_access modules stay on the same level of performance as they are now in D5. We lose nothing, we gain a lot a many sites.

Dropping the approach in this thread in favor of grugnog's user caching is fine by me if it's deemed a better road, of course (I won't be able to help on that though, i have to drive some sites live in the next few days). But is that really attainable for D6 ?

Hi guys,
so did this feature made it to drupal 6.... ? I didn't saw it mentionned in the log.

even though we are still accepting performance patches, this one is looking unlikely for D6. anyone want to push it in? or we should defer to 7.

Well, I would love to push this one of course, but it looks we're still unclear on how we could handle the 'node_access' issue. See #43 above for my latest thoughts on the matter - aside from that, the patch should be ready.

My take would be that we disable block caching when n_a modules are enabled. No gain / no loss for sites with n_a modules, huge gain for sites without n_a modules. I understand the argument about bad publicity for n_a system, but should we let that hold us back for the gains otherwise ?

think yched's proposal to skip caching for sites with node_access enabled is reasonable for D6.

OK, here's a rerolled patch, with block caching disabled when n_a modules are around.

Attachment	Size
block_cache_4.patch	10.4 KB

Update : If I trust system_update_6026, the call to _drupal_initialize_schema is not needed anymore.

Attachment	Size
block_cache_5.patch	10.35 KB

I'm still in favour of a performance setting "aggressive block caching" to allow this to be used on n_a sites with suitable warning.

I'm still in favour of a performance setting "aggressive block caching" to allow this to be used on n_a sites with suitable warning.

I think there are valid use cases where this will be good to have. It also follows with the existing style on the Performance page so Drupal admins will already be familiar with what they're getting themselves into when messing with "aggessive caching".

So at this point what exactly needs to happen to move this patch forward?

I propose - let's get something in - anything that improves performance for many use cases and doesn't break things would be a lot better than what Drupal has now. Dries has announce his support for getting it - so let's get something workable within the deadline together (e.g., not letting "perfection" be the enemy of the "good"). And then once something gets committed, work can continue on refinements.

Come on - who wouldn't want to be part of getting a performance patch committed to core?! ;)

tried to test this patch, but its throwing lots of warnings about undefined block cache constants.

added the constants defined in the patch at #40 back to this patch and updated so that it applies without offsets.

Attachment	Size
block_cache_6.patch	10.87 KB

@justinrandell : oops, my bad - thanks.

Updated patch with help text on the 'performance settings' page (was still TODO in the previous patch)

@Caleb G : "So at this point what exactly needs to happen to move this patch forward?"
- Reviews stating that it works OK (it should AFAICT), and if so, someone to push the RTBC button.
- Probably the opinion of core committers on the "node_access modules / UI" question :
Currently, block caching is disabled when n_a modules are enabled - no UI. This is sufficient IMO, esp for a late D6 inclusion...
Possible ways to improve this :
Catch suggests an 'aggressive block caching' option (= cache even if n_a module are enabled). Not sure how to word this in a non-confusing way.
I'd probably be more in favor of a per block 'cache' checkbox on the blocks overview page, much like the 'throttle' checkbox (maybe this could even replace the 'throttle' checkbox ?).

Attachment	Size
block_cache_7.patch	11.95 KB

Thanks for the new patch and the suggestions yched. I'll try and get some testing done on this patch soon.

Some benchmarks on the patch that yched on #56. I used the instructions/parameters described here.

* 2,000 users
* 5,000 nodes
* 5,000 path aliases
* 10,000 comments
* 250 terms
* 15 vocabularies

The patch is a performance approvement all the way around - that much is clear. I hope I am not presumptious to mark this RTBC, but all the work above by some of the best Drupal minds seems to be working well.

+1 to getting this in so that patches don't have to keep being re-rolled against HEAD and so that people can start adapting other parts of core/contrib to the change. Of course, once/if it's been committed new threads can be opened up to explore the possibilities of extending the current functionality/optimization of the current patch.

===

NO CACHING

Document Length: 15145 bytes

Concurrency Level: 5
Time taken for tests: 82.177 seconds
Complete requests: 500
Failed requests: 0
Broken pipe errors: 0
Total transferred: 7814000 bytes
HTML transferred: 7572500 bytes
Requests per second: 6.08 [#/sec] (mean)
Time per request: 821.77 [ms] (mean)
Time per request: 164.35 [ms] (mean, across all concurrent requests)
Transfer rate: 95.09 [Kbytes/sec] received

===

REGULAR CACHE OFF - BLOCK CACHE ON

Concurrency Level: 5
Time taken for tests: 80.552 seconds
Complete requests: 500
Failed requests: 0
Broken pipe errors: 0
Total transferred: 7814000 bytes
HTML transferred: 7572500 bytes
Requests per second: 6.21 [#/sec] (mean)
Time per request: 805.52 [ms] (mean)
Time per request: 161.10 [ms] (mean, across all concurrent requests)
Transfer rate: 97.01 [Kbytes/sec] received

===

REGULAR CACHE ON - BLOCK CACHE OFF

Concurrency Level: 5
Time taken for tests: 14.159 seconds
Complete requests: 500
Failed requests: 0
Broken pipe errors: 0
Total transferred: 7852418 bytes
HTML transferred: 7617935 bytes
Requests per second: 35.31 [#/sec] (mean)
Time per request: 141.59 [ms] (mean)
Time per request: 28.32 [ms] (mean, across all concurrent requests)
Transfer rate: 554.59 [Kbytes/sec] received

===

REGULAR CACHE OFF - BLOCK CACHE ON

Concurrency Level: 5
Time taken for tests: 13.431 seconds
Complete requests: 500
Failed requests: 0
Broken pipe errors: 0
Total transferred: 7852333 bytes
HTML transferred: 7617935 bytes
Requests per second: 37.23 [#/sec] (mean)
Time per request: 134.31 [ms] (mean)
Time per request: 26.86 [ms] (mean, across all concurrent requests)
Transfer rate: 584.64 [Kbytes/sec] received

Btw, for anyone trying to recreate - the only modules turned on were the ones that come turned on by default - the devel module and the devel generate module were the only exceptions. The only other change I made was to turn on all of the blocks that come with the default configuration and/or devel (a total of 9 blocks). This made sense to me given that we are trying to test blocks...

Thanks for the benchmarks Caleb G!

So block caching alone provides even further performance than Drupal's regular node caching. This is VERY interesting. Is it possible to do one extra test with Regular Caching ON and Block Caching ON? It would complete the picture and get my +2 :)

For having turned on all blocks, this patch doesn't seem to save a lot of cycles. As I understand, it buys us about 2-3%?

Some extra comments:

1. Rename 'cache_mode' to 'cache'.

2. Isn't the presence of module_implements('node_grants') a case for 'custom' block caching?

3. Before this gets committed, it would be nice to add caching support to some additional blocks. The blocks that are currently getting cached, aren't particularly expensive. It would be good to see if we could cache the forum module related blocks or maybe the comment module related blocks. I'd prefer to see us focus some more on blocks that are worth caching. It will help me validate the proposed API.

Dries: Caleb G's benchmarks don't include all core blocks, only those available by default + devel.

the only modules turned on were the ones that come turned on by default - the devel module and the devel generate module were the only exceptions.

- i.e. those core modules and their blocks which aren't on by default weren't included, including the expensive forum and comment blocks.

Bioalien:

Gah - I mislabeled the last result. The last one IS with blockcache and normal cache on. The difference in this case was a 5% improvement rather than the 2-3% Dries was thinking based on my blunder.

I'll see if I can get anywhere with Dries comment #3 (re: adding other blocks to caching). If someone else can offer their thoughts/code about comment #2 that would be great (since I have no idea what this means, atm).

Thanks for these benchmarks, Caleb.
Ultimately, the performance improvements depend a lot of the blocks themselves - I'm not sure there are many expensive blocks in core besides 'latest forum threads'. Views-generated blocks might gain a lot, though...

Dries :
1- sure, why not - I'm having troubles with my ISP at home and can't roll a patch right now - Anyone ?

2- not sure what you mean, but I don't think n_a issues can be handled with 'custom caching'. If module_a defines a block that lists nodes, and module_b defines node access restrictions : module_a knows nothing about module_b access rules, and module_b knows nothing about module_a blocks (and does not have the ability to alter their caching mechanism anyway...). Ultimately, it's the admin who knows if a given block is affected by n_a rules...
A previous patch (#36) added 'PER_USER' cahing pattern for all blocks when n_a modules were around, but this was deemed too expensive (probably rightly so...)

3- please note that with the latest patch, blocks are cached "BY_ROLE" by default, unless they explicitely specify a pattern or "BLOCK_NO_CACHE"

Before this gets committed, it would be nice to add caching support to some additional blocks. The blocks that are currently getting cached, aren't particularly expensive."

After re-reading that I'm a bit confused - blocks can specifically be excluded from block caching either in modules themselves or in the presence of n-a modules, but otherwise they are automatically cached, right? If so what other caching support would there be to add? (maybe Dries meant just to go ahead and turn on the forum module as well as the forum block for the test...)

Also, I just realized the comment block (which was enabled for the test) appears not to be working at all in my sandbox. Whether this is a patch induced issue or not, I'll have to investigate further.

Just to clarify these are the blocks that were enabled for benchmarking, though as mentioned the comment block wasn't functioning. When I do the tests again I'll add the forum block to it.

Recent comments
Devel
Navigation
User login
Execute PHP
Switch user
Who's new
Syndicate
Who's online

Changed 'cache_mode' to 'cache'.

Attaching new patch.

Attachment	Size
block_cache_8.patch	11.87 KB

There is no problem with comment block. Apparently the comments published by the devel module are missing something that makes the block show up. As soon as I put a manual comment in the block showed up right away. I test this phenomenon on an install that did not have the patch applied.

So more benchmarking - this time with the forum module and both forum blocks turned on, as well as a functional comment block.

Unfortunately, the results of the tests we less favorable than last time - at least once site-wide caching was turned on. Basically there was no difference between the results when normal caching and block cache was on, and when only normal caching was on. Something about those additional blocks it didn't like...

===

ALL CACHING TURNED OFF
Concurrency Level: 5
Time taken for tests: 82.185 seconds
Complete requests: 500
Failed requests: 0
Broken pipe errors: 0
Total transferred: 8335000 bytes
HTML transferred: 8093500 bytes
Requests per second: 6.08 [#/sec] (mean)
Time per request: 821.85 [ms] (mean)
Time per request: 164.37 [ms] (mean, across all concurrent requests)
Transfer rate: 101.42 [Kbytes/sec] received

===

NORMAL CACHE OFF - BLOCK CACHE TURNED ON
Concurrency Level: 5
Time taken for tests: 79.488 seconds
Complete requests: 500
Failed requests: 0
Broken pipe errors: 0
Total transferred: 8335000 bytes
HTML transferred: 8093500 bytes
Requests per second: 6.29 [#/sec] (mean)
Time per request: 794.88 [ms] (mean)
Time per request: 158.98 [ms] (mean, across all concurrent requests)
Transfer rate: 104.86 [Kbytes/sec] received

===

NORMAL CACHE ON - BLOCK CACHE ON
Concurrency Level: 5
Time taken for tests: 13.301 seconds
Complete requests: 500
Failed requests: 0
Broken pipe errors: 0
Total transferred: 8376459 bytes
HTML transferred: 8142061 bytes
Requests per second: 37.59 [#/sec] (mean)
Time per request: 133.01 [ms] (mean)
Time per request: 26.60 [ms] (mean, across all concurrent requests)
Transfer rate: 629.76 [Kbytes/sec] received

===

NORMAL CACHE ON - BLOCK CACHE OFF
Concurrency Level: 5
Time taken for tests: 13.323 seconds
Complete requests: 500
Failed requests: 0
Broken pipe errors: 0
Total transferred: 8376459 bytes
HTML transferred: 8142061 bytes
Requests per second: 37.53 [#/sec] (mean)
Time per request: 133.23 [ms] (mean)
Time per request: 26.65 [ms] (mean, across all concurrent requests)
Transfer rate: 628.72 [Kbytes/sec] received

The fact there is no notable performance gain is because we're caching the wrong blocks. In my previous comment I suggested that we try to use this API to cache the forum topic blocks and/or the recent comments block. When you do, make sure to create a couple of path aliases, as that will kick the path alias system in action. (When there are no path aliases, it doesn't try to lookup paths.)

In short:
1. Let's cache some expensive blocks in Drupal core rather than the inexpensive ones
2. Let's do some better tests (i.e. enabling a bunch of things like path aliasing)

After a conversation in irc with Killes I think I *finally* understand what needs to be done. Will try and have a new patch posted which implements the blockcache api for the forum and comment modules, then I'll create some path aliases, and then re-benchmark (while adjusting for the admonishments that Killes gave for interpreting the results ;) ).

For anyone watching along at home or documenting this, blockcaching is done on a per-module-level-basis, not on a system-wide/automatic basis as I had mistakenly imagined.

After implementing caching for the forum blocks and the recent comments blocks (I checked the db to make sure it was actually working) and including some path aliases, the benchmarks don't show any improvement at all between block caching on and block caching off if normal caching is turned on. If normal caching is off and there's about a 4% improvement between block caching being on or not. I'm not sure why block caching doesn't result in a noticeable improvement when the normal cache is on, but either way it doesn't seem to be the numbers everyone might have hoped for yet. Am setting issue status to 'code needs work'.

Btw, for the forum blocks I set them to 'BLOCK_CACHE_PER_PAGE & BLOCK_CACHE_PER_USER' and for the comments I set 'BLOCK_CACHE_PER_ROLE'.

Is 4% really not worth it?

Isn't block caching supposed to work for loged in users, thus providing improvement that the normal cache can't provide since it doesn't cache page for loged in users ?

Hmm, I really hoped for a 20%+ improvement with this. Maybe if we carried out benchmarks on larger and more complex sites (scratch.d.o)?

Caleb G: Did you benchmark logged in at all?

Setting back CNR in case you didn't, since those tests could change those results a fair bit.

Also, it's not clear why you used 'BLOCK_CACHE_PER_PAGE & BLOCK_CACHE_PER_USER' for the forum topic blocks. For a site like drupal.org, that's probably not aggressive enough. Maybe upload a new version of the patch so we can have a closer looks, and maybe try to replicate your benchmark results.

First my apologies - I did that last post close to midnight. After sitting up in bed thinking about this I realize a few things:

• 1. There apparently is a good reason that performance doesn't improve with regular cache and block cache on. Testing this morning shows that even with CSS aggregation AND JS aggregation AND block cache on at the same time, it does not improve the benchmarks obtained through normal caching alone. I don't know enough about Drupal caching to know why this is so, or if it's a good thing or a bad thing, but definitely it appears that the only thing that can improve on benchmarks for normal caching is 'aggressive caching' or alternate methods (memcache, etc). So apparently block cache is not unique or problematic in this regard.
• 2. With the help of #drupal I figured out how to benchmark as a logged in user (at shell do ab -n500 -c5 -C SESSION-NAME=SESSION-VALUE http://your-url/index.php )

  The results we're confounding - no need to post the results because they were identical across the board (meaning that any/all caching turned off benchmarks were the same as any/all caching turned on benchmarks). There must be some kind of global switch that kills any caching for logged in users?

• 3. A 4% improvement does seem respectable, especially if we could achieve it for logged in users. (right now the 4% improvement only applies to a situation whereby normal caching is off, block cache is on and the user is anonymous - not a very likely scenario for most sites)

I've rolled a patch which includes the changes I made for the forum and comment modules. I wasn't sure if those were good settings or not but I tried to err on the side of being conservative with them. As of this morning the patch applies correctly to a fresh checkout of HEAD.

Attachment	Size
block_cache_9.patch	13.65 KB

I'm sorry if I miss the point but why would one want to enable block caching on loged out cached page ? From my understanding, entire page is cached for loged out users so of course there would be no improvement because entire page cached > only blocks cached.

In my humble view, I thought that block caching would be enabled ONLY to loged in users, loged out user would still have the entire page cached just as it is now.

I can't imagine why cached blocks for loged in users would not provide a significant improvement, particularly for heavy resource blocks.

Cordially,
Brakkar

Isn't the proper way to set both BLOCK_CACHE_PER_PAGE and BLOCK_CACHE_PER_USER, to use the bitwise OR operator "|"?

I believe the effect of the code below is to set neither bit:

$blocks[1]['cache'] = BLOCK_CACHE_PER_PAGE & BLOCK_CACHE_PER_USER;

This is saying to set the cache to any bits that are common in both (of which there are none).

It should probably be:

$blocks[1]['cache'] = BLOCK_CACHE_PER_PAGE | BLOCK_CACHE_PER_USER;

Which will return bits that are in either BLOCK_CACHE_PER_PAGE or BLOCK_CACHE_PER_USER. Doesn't bitwise math make your head hurt? :-)

This might affect the benchmarks.

Also, setting a block to cache both per-page and per-user is going to fill up the cache table FAST. For a site (like drupal.org) where users are jumping around to a lot of pages and not hitting the same pages over and over, it will be of very little value.

Just to be clear, if page caching is enabled, caching blocks will have basically NO performance improvement for anonymous users.

The current patch says: "Enabling the block cache will enhance performance for pages requested by any user (anonymous or authenticated). It can be enabled or disabled indenpendently of the page cache."

It should say: "Enabling the block cache can bring a performance increase for all users by preventing blocks from recalculating on every page load. If page cache is enabled, this performance increase will mainly affect authenticated users." ... or something like that.

Good catch with the binary operators. Good thing we have Jeff. :-)

Even for anonymous users not all pages are cached. When we have a 'cache miss' (a request for a page that wasn't cached yet), the block caching can still help for anonymous users. On a site like drupal.org with thousands of pages, there is a fairly high cache miss rate. Last time I checked (more than 1 year ago), more than 30% of the requests from anonymous users still required the page to be generated.

Yay! For both anonymous AND for authenticated users I noted a nearly identical 15.5% improvement with block cache on vs. block cache off. (benchmarks attached to save spamming the thread with them)

Notes:

• After figuring out that I was stupidly testing authenticated users with uid 1, which is excluded from getting cached blocks, as well as making the syntax change jjeff mentioned - benchmarking worked much better. ;)
• For the forum.module I changed from BLOCK_CACHE_PER_PAGE | BLOCK_CACHE_PER_USER to just BLOCK_CACHE_PER_ROLE.
• Enabled 'view' and 'post comments' for anonymous user. I hadn't realized those settings were disabled in my previous tests.
• No benchmarks include the 'normal Drupal cache' anymore as it had no effect one way or the other.
• UI comments changed to what jjeff mentioned, "Enabling the block cache can bring a performance increase ..."
• As I have mysql query_cache off, per the instructions for benchmarking, the authenticated user tests actually started to show a bunch of failed requests in the results. I checked all variables repeatedly and this event only occurred on auth user tests when no block caching was enabled. Apparently, the non-cached tests had to much overhead for my system to keep up with. I'd say that further validates the improvements achieved by the patch. :)

Attachment	Size
blockcache_benchmarks.txt	2.86 KB

Here is the patch which applies successfully to head as of right now. Not the easiest patch to keep up with - 9 files. Perhaps with such good results this can get into core soon so that it doesn't have to keep getting re-rolled? :)

Attachment	Size
block_cache_10.patch	13.62 KB

Great. Now we know the patch works as designed, I figured I'd do a more in-depth code review.

1. _block_get_cache_id uses underscores when building the cache ID -- I think the other cache ids use : as their delimiter. I might be worth double-checking.

2. I'm not sure I agree with:

If the block can change on a per-user basis, we should prefer that. If a block is personalized on a per-user basis, we should never cache it on a per-role basis. If you think I'm mistaken, I'd like to see the explanation in the code to be expanded.

3. It would be great if we could add some extra documentation about the BLOCK_CACHE_CUSTOM and the cache_id hook. Are we sure this is required? What would be a good use case? Can we summarize a good use case in the PHPdoc?

4. +  $schema['cache_block'] = drupal_get_schema_unprocessed('system', 'cache'); -- why was that introduced?

5. There are tabs in the code.

6. The database field is now correctly called 'cache', but certain blocks still use ''cache_mode' -- how can this result in correct benchmark results? Can we rename all 'cache_mode's to 'cache' for consistency.

7. "by preventing blocks from recalculating on every page load" -- I'm not convinced by the word "recalculating".

8. + $blocks[3]['cache'] = BLOCK_NO_CACHE; Wouldn't this be redundant? I'd expect this to default to BLOCK_NO_CACHE.

Sorry I was forced to stay behind for the last few days (internet access down + vacations). Loads of thanks to Caleb for taking this up. I should be able to get back actively on this later today.

Just a quick note :
Dries : "+ $blocks[3]['cache'] = BLOCK_NO_CACHE; Wouldn't this be redundant? I'd expect this to default to BLOCK_NO_CACHE."
As I mentioned in #64 above (and a few times before that, I think), the patch uses PER_ROLE caching as the default behaviour for blocks that do not specify any caching settings.
I thought this made more sense, as there are only a few blocks that have a good reason _not_ to be cached, and PER_ROLE is the 'generic' drupal behaviour (the roles your belong to define what you see), and is in fact the right pattern for most blocks. This gives you block caching for free when porting your module.

and & vs | binary operators : big oops from me, those came from the last patch I posted before going silent... Thx Jjeff

I'm trying to get a little more clear on the flags and how/when cache refreshing happens with this patch. First, I've added some documentation for the constants. Please let me know if I'm misunderstanding any of these:

Second, it appears that all blocks are stored with an expiration of CACHE_TEMPORARY.

Perhaps I'm not clear on exactly what CACHE_TEMPORARY means. This gets cleared whenever a new node or comment is created on the site? And this is 'cache_lifetime' variable from the 'Minimum cache lifetime' section in the page cache settings? I'm inclined to think that there should be a lifetime setting for each block on its configuration page. If not, we should make it clear that 'Minimum cache lifetime' affects blocks too.

It also appears that block_cron() clears the block cache whenever its called. What purpose is this serving?

/**
* Block is the same for every user on every page of the site.
* Only affected by locale (language) selection and multisite *and theme*.
*/
define('BLOCK_CACHE_GLOBAL', 0x0001);

Oops. I had also added this comment above the constant definitions to explain what they do and how they should be used. Forgot to grab it:

Updated patch, and response to Dries' comment #83 :

1) '_' delimiters : no, _block_get_cache_id does use ':' as a delimiter (return "$base_cid:". implode(':', $cid_parts);).
Underscores were used in 'r_{role_ids}' and 'u_{uid}' sub-patterns (the 'u' and 'r' letters are there to differentiate between 'cache for user 2' and 'cache for role 2' sub patterns). I