Block caching

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:

TTL < 0   -- block is always cached (static blocks)
TTL = 0   -- block never cached (super dynamic blocks)
TTL > 0   -- block is cached but updated ever 5 min (keeping the interface in terms of minutes makes the most sense)

Ugh, I keep losing my input, doh!

TTL < 0   -- block is always cached (static)
TTL = 0   -- block is never cached (dynamic)
TTL > 0 -- block is cached but updated ever 5 min (best to keep in terms of minutes most user friendly)

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.

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.

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.

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?

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.

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

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.

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

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

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.

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

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?! ;)

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

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.

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.

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

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

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:

+    // 'per role' and 'per user' are mutually exclusive :
+    // we favor the less expensive 'per role'.
+    if ($block->cache & BLOCK_CACHE_PER_ROLE) {
+      $cid_parts[] = 'r_'. implode(',', array_keys($user->roles));
+    }
+    elseif ($block->cache & BLOCK_CACHE_PER_USER) {
+      $cid_parts[] = "u_$user->uid";
+    }

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:

/**
 * This block should not get cached
 */
define('BLOCK_NO_CACHE', -1);

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

/**
 * This block can change on each page of the site 
 */
define('BLOCK_CACHE_PER_PAGE', 0x0002);

/**
 * This block can change for each user 
 */
define('BLOCK_CACHE_PER_USER', 0x0004);

/**
 * This block can change for each user role
 */
define('BLOCK_CACHE_PER_ROLE', 0x0008);

/**
 * This block uses custom criteria to determine how it gets cached
 */
define('BLOCK_CACHE_CUSTOM', 0x0010);

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

cache_set($cid, $array, 'cache_block', variable_get('cache_lifetime', 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:

/**
 * Flags defining cache granularity for cached blocks.
 * 
 * Modules can return a combination of these flags from the hook_block('list')
 * 
 * Example:
 *  $blocks[0] = array('info' => t('Mymodule block #1 shows ...'),
 *     'weight' => 0, 'cache' => BLOCK_CACHE_PER_ROLE | BLOCK_CACHE_PER_PAGE);
 *
 */

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 replaced them with (less visually striking) dots (.).

2) PER_USER / PER_ROLE precedency : well, setting both flags for a given block is basically broken code, and reveals the module author does not fully get the notion of caching patterns. In that case, I thought it best to 'babysit' by picking the harmless 'PER_ROLE'. 'PER_USER' can by a resource drag, so "If you do need PER_USER, say so, but don't be equivocal...". I did not change that part for now, and improved code comments. Let me know if you do want PER_USER to win :-)

3) BLOCK_CACHE_CUSTOM : This comes from IRC chat with chx, who envisioned bare block caching simply by asking modules for a string to use as cache id for their blocks. This patch predefines generic caching patterns (per user, per role...), but does allow more specific ones. A typical use case would be 'cache per organic group' for og blocks, but og being a node_access module it will disable block caching anyway... Well, I'll try to come up with a better example...

4) $schema['cache_block'] = drupal_get_schema_unprocessed('system', 'cache') : This is the way filter.module's {cache_filter} and update.module's {cache_update} tables are created. Lets cache_* tables use the proper schema without duplicating the code.

5) tabs in the code : fixed.

6) remaining 'cache_mode' instead of 'cache' : fixed. This in fact does *not* invalidate Calebg's benchmarks results : the expected $block['cache'] being found empty, the default PER_ROLE pattern was applied, which happened to be what the faulty code intended :-)

7) 'recalculating' : right, fixed.

8) $blocks[n]['cache'] = BLOCK_NO_CACHE : see #83.
New patch adds some comments about PER_ROLE being the default cache pattern, and removes $blocks[n]['cache'] = BLOCK_CACHE_PER_ROLE lines (patch involves less files :-) )

New patch also includes Jjeff's documentation for the constants in #86 - I guess the example provided in #88 would fit best in http://api.drupal.org/api/function/hook_block.

Maybe more efficient defaults are better for a couple of reasons: a) in most use case there will be a performance increase b) any time Drupal performs better people like it better.

But, on the other hand a more conservative default setting, while being less efficient, avoids potential issues of confusion for end users who may be clueless as to why their block is not functioning properly after they set their site up in a way that does not complement a more liberal block caching method.

So who wins that tug of war?

@Jjeff : about cache persistence and cache clearing :
Currently, the patch does nothing about automatic block cache clearing (node create / update, user login, etc...). Cron is the only time when cache_block entries are cleared. A block using CACHE_CUSTOM pattern can 'obsolete' a cached entry by changing the cache_id it returns...

Updated patch fixes my erroneous conception about cache_lifetime variable. It is used internally by cache_clear_all when wiping all entries to decide if cached entries should actually be cleared or 'extended'. Using cache_set($expire = variable_get('cache_lifetime', CACHE_TEMPORARY)) as the previous patch did made no sense at all...

Thinking about this a little more, maybe we should clear the block cache on the same occasions that trigger a page cache refresh ? Any thoughts welcome. (I should try to grab chx on IRC, I think he was the one who advised me to follow the simple road.)

*Block Cache Lifetime*

I would really like to see a way that administrators could choose how often to refresh particularly dynamic blocks such as "who's online" and "who's new". With the current patch, these blocks are completely not cached. With blockcache module it was possible for an administrator to say "refresh this block every 2 minutes", and it seems like there's nothing like that possible with the current patch. Am I understanding correctly that the only way we could get this type of thing to happen would be to run the cron job every two minutes?

I wonder if there should be two cache values returned by hook_block('list') instead of just one, one for 'cache_type' and one for 'cache_default_lifetime' which would be a count in seconds for the lifetime of the block in the cache. For instance, I think 60 seconds is probably a reasonable default for refreshing the "new users" block. Blocks could also return -1 to not refresh until an explicit clearing of the cache. We could also add a field to the admin/build/block/configure pages to allow administrators to tweak these lifetimes. Likely, we would need to add another column to the block table to accommodate this lifetime value.

+1 to everything that jjeff describes. I'm very familiar with this functionality through using the block cache contrib module, and the cache expiration settings are indeed very nice parameters to have - but should we fold that in now, or after we get this initial api into core and then work on that as a separate issue. Dries?

Good questions.

1. I think we can proceed with the current patch, and add caching control in a second round. Users will automatically expect the cache to be "smart" and to invalidate when new content becomes available (like the page cache). Unfortunately, that is not the case. It might be better to change:

function cache_clear_all($cid = NULL, $table = NULL, $wildcard = FALSE) {
  global $user;

  if (!isset($cid) && !isset($table)) {
    cache_clear_all(NULL, 'cache_page');
    return;
  }

into:

function cache_clear_all($cid = NULL, $table = NULL, $wildcard = FALSE) {
  global $user;

  if (!isset($cid) && !isset($table)) {
    cache_clear_all(NULL, 'cache_page');
    cache_clear_all(NULL, 'cache_block'); // added
    return;
  }

and to eliminate the block_cron(). That is going to be more "predicatable" and it actually makes us re-use the cache-lifetime settings of the page cache (I believe). If that works, we can probably hold-off more advanced expiration settings until D7.

2. One thing is clear though; for developers, we'll want to better document how long blocks are going to be cached, what invalidates the cache, etc. I think we should add that to the code comments.

3. It's probably also worth mentioning that you don't want to cache really simple blocks. The syndicate block or the search block might be a good examples of where you want to use BLOCK_NO_CACHE (instead of the current BLOCK_CACHE_GLOBAL); although I don't have the benchmark results to back up this claim, it wouldn't surprise me if the actual SQL query to look up the block from the cache and to unserialize the data is more expensive than re-generating the block. After all, for those blocks, caching introduces additional SQL queries rather than removing SQL queries ... Is that something we should investigate more, and that we should mention in the documentation of BLOCK_NO_CACHE? Providing some additional guidance might be helpful here.

4. In the PHPdoc of _block_get_cache_id() it's probably worth adding one sentence to describe how this function works, i.e. provide an high-level overview of how the unique string is computed and why we do it like this. It will help people understand the caching mechanism, which in turn, helps them set the right block cache settings. Someone might wonder if *he* needs to do per-theme caching; it doesn't hurt to be a little bit more explicit about this.

5. Why don't we put the $base_cid in the $cid_parts[] array? I think it would be slightly nicer if we put everything in the array first.

6. I don't understand this code comment: "Menu blocks might be different even for the same path." It would be great if you could re-phrase or extend that in the code comments.

7. "a redirect url" should be "a redirect URL".

I think we're almost there ... :)

I think "Menu blocks might be different even for the same path." means that for different users, different items are viewable. That block is not cacheable at all as the items are callback access checked. And the menu caches what's cacheable anyways.

Maybe we should write: "The menu block can't be cached because each menu item can have a custom access callback."

Dries wrote:

Maybe we should write: "The menu block can't be cached because each menu item can have a custom access callback."

Perhaps it is me, but if I read this kind of comments, I can't help thinking: "Why? What happens if we do? Why only with custom access callbacks?"

Stefan

New patch as per Dries comments - some of the requested help texts are still TODO as this always takes me a while, and I have to run for now. Note that some of the comments also belong in hook_book doc on a.d.o

Patch also refines cache settings for core blocks :
- Not cached :
too simple : custom blocks, login, search, syndicate, language switcher
too dynamic : who's online, popular content (statistics.module)
- 'Per page' + 'per role' : author profile, book navigation (not sure about that one - chx / pwolanin ?)
- *all other blocks* are cached 'per role'
I'm sure we can finetune / debug those settings if needed once the patch is committed.

Chx has a good point. There are so many things that can affect menu blocks that it's probably not smart to assume that they'll stay the same. A developer could conceivably create a menu item that shows the time of day. If the new menu system is caching this stuff, let's certainly let it do its thing.

I haven't spent much time with the D6 menu system, but under D5, there were a lot of advantages to caching a menu block due to all of the url() lookups on each menu item. On a page with lots of menu items, I was able to reduce the db calls considerably. I once did a demo for a client where I put all of the blocks into blockcache.module blocks and brought the number of queries to build the page down from ~150 to ~50. I need to replicate this again in a more controlled environment, but everyone in the room (including myself!) was quite shocked.

So even for simple blocks that contain a lot of links, there still might be an advantage to caching them.

the 'lots of queries due to lots of links' is being worked on in another issue. anyway, the menu system now stores the alias locally so it never has to do a lookup for any of its links.

Moshe, actually menu does not store aliased paths at this moment (it did but that was before language hit) we are trying to add it back at http://drupal.org/node/166393

yched, caching the menu block is impossible, even per user and per page is not enough, what about a 'happy birthday' link? Or a 'new private message' link? It can be time bound. Let menu system optimize itself.

I agree that caching the menu blocks by default is probably a bad idea. However, we should probably provide an easy way for a contrib module to be able to do this.

Here's a quick before/after queries output from devel.module on a single node page.

In the before, the Navigation block is not cached and it takes 66 queries to build the page (26.16ms):

http://www.lullabot.com/files/nodepage-before.jpg

After putting the Navigation block into the cache (using blockcache.module), we lose 26 queries called by drupal_lookup_path(). It takes 40 queries to build the page (21.8ms).

http://www.lullabot.com/files/nodepage-after.jpg

Disclaimers: This is tested under D5 as I have not ported blockcache to D6. I realize that on a listing page with 300 queries, 26 becomes a less significant number, but I just wanted to follow through on my last post... and also, I made these pretty pictures... so I thought I'd share 'em. :-)

Not being able to cache the navigation block is not a show-stopper for this patch and is something we could work on in a follow-up patch. The only way to solve this is by providing a setting in the navigation block's configuration page, or to create a contributed module that alters the cache-field in the blocks table.
That or you could edit one line of code. Let's focus on what we have now, and then consider another iteration of improvements. I don't think these improvements will change the API -- they'd merely add configuration settings.

Actually, the latest patch in #98 already uses BLOCK_NO_CACHE for menu blocks and the Navigation block - code comment explains why and states that optimization is left to the menu system.

I simply forgot those when posting the summary of current block settings. Sorry about that ;-).

I tested yched's latest patch and it applies cleanly, everything works. Am marking RTBC on the basis of Dries comments.

I, and I'm sure others, will be glad to help with documentation, feature improvements, and/or UI additions in subsequent threads/handbook pages.

Not yet unfortunately, some code comments Dries requested are still TODO :-)

talking with chx and pwolanin on IRC, book navigation block should be safely cached (per role + per page), so patch #98 should be OK on this regard.

Eh, there was a couple 'hunk line offset' error is the previous patch. Same patch as before but just to allow for the line differences so that it applies without the error messages.

Sorry, yched posted his comment and changed status while I was typing...

I've reviewed the most recent patch a bit.

The logic in block_list is a bit faulty. If you use a node_access module, there will be items put into the cache that will never get used. Should be easy to rework a bit.

What I don't like at all is the way the cache is cleared. I'd exceedingly prefer if each block would take care of that itself.

For example: I've added caching of the "latest forum topics" to drupal.org. There is no reason to clear that cache if there's a new project issue. Hence, I only clear it if there is a new or edited forum topic.

The current implementation would probably cut the hit/miss ration for this block's cache into half.

The reason for this patch is to have a better cache, a more granular patch. Clearing a cache is alost as important as setting it in the right way to get the best performance.

Ok, I missed:
+ if (isset($cid)) {

so the logic is ok.

About cache clearing : just like the 'time_to_live' feature Jjeff mentioned, there's no doubt additional settings would make a better block cache feature. Can we hold them off until D7, or do we consider the current patch plain not suitable as is, at the risk of letting D6 miss the feature and the gains it currently buys us because of feature creep) ?

(from IRC : for drupal.org, the patch as it is now is certainly not usable.)

After talking with killes on IRC, here are a few possible options if we really want to improve cache clearing now :

- killes' suggestion : drop the current 'default' cache clearing in cache_clear_all() altogether and let each module handle cache clearing for their blocks themselves (through hook_nodeapi, hook_user, whatever). This raises the cost of having your module's blocks get cached : you have to write some code to handle cache clearing. With the current patch, you have nothing to do to have your blocks benefit on caching. The risk IMO is to have lots of contrib modules overlook the block cache feature, and end up with limited gains.

- intermediate solution : let modules specify a BLOCK_CACHE_CLEAR_CUSTOM flag for their blocks, meaning they will handle cache clearing for the corresponding blocks themselves. In cache_clear_all(), we clear block cache _only_ for blocks that do not use this flag.
This lets us commit the current patch (more or less) pretty soon, and finetune tricky blocks in subsequent patches.

Both solutions probably mean we lose cache_lifetime feature for block cache, BTW, since cache clear will never happen for *all* blocks (cache_clear_all($cid = NULL, 'cache_block') ) anymore.

Dries, what do you think ?

edit (tag filter mess up) - the IRC quote is from killes :
<killes> for drupal.org, the patch as it is now is certainly not usable.

I didn't realize that any additional features would have to wait for Drupal 7. Based on my experience and use of the block cache module it seems like would be a real shame to not be able to customize the way blocks expire their cache in some/any form or fashion for Drupal 6 (though admittedly much better than nothing). By having the ability to set when blocks expire one could set block caching for things they might otherwise have to pass on if the option wasn't available.

A big +1 one the 'intermediate' solution you proposed yched, and I'm glad to offer my help in any way that is useful.

In general, I agree with Gerhard/killes -- maintaining the cache on a per-block basis makes it possible to cache more aggressively. And potentially a _lot_ more aggressively.

However, it also comes at a cost: it is more difficult for newbie Drupal developers, requires more code, and is less robust (prone to programming mistakes). I'd prefer not to get into the whole cache clearing business at this point of the release cycle. We're weeks into the code freeze, and we're about to roll a first D6 beta.

I prefer to stick with an approach that is (mostly) transparent for existing code, that gives us a big gain, and that just works. Introducing complex caching strategies is something we can work on during the Drupal 7 release cycle? We could then look into making both the block and the page cache more aggressive/fine-grained?

Plus, what Gerhard describes is already possible. You can by-pass the block caching API (i.e. use BLOCK_NO_CACHE), and implement your own block caching strategy using the regular cache API. If we want all the flexibility and power of the world, we don't need the block caching API to begin with.

Updated patch - simply adds the code comments that were still marked TODO, so the patch should be ready now.

The patch also reorders the definitions of BLOCK_CACHE_* constants to make it clearer that PER_ROLE is the default, and thus changes their values. People who tested a previous version simply need to revisit the blocks overview page to get the values right.

Please also note that with the current upgrade path, block caching will not be effective until the blocks overview page is displayed. Not sure if / how we should enhance this.

When this gets in, I'll need to update the doc for hook_block accordingly.

I reviewed the code and I don't have any major comments. The only thing is that we don't write a space before ':' but other than that, this patch looks good to me. Feel free to mark this RTBC after careful testing and/or an additional code review. When you review the patch, make sure that the documentation is clear.

Thanks for the hard work.

One more thing: this warrants a line in the CHANGELOG.txt in my opinion.

space before ':' - right, that is french typography... Fixed, plus added a line in CHANGELOG.

Tested the patch extensively and it works well with no errors. Attaching a patch that only corrects for hunk line offset errors, and marking RTBC.

I've put so much extraneous stuff in this thread I thought I'd spare everyone and put the new/corrected findings about this patch in an article instead...

Over 60% increase for authenticated users; this is another serious killer feature for Drupal 6. Congratulations guys.

New patch, fixes a few missing tidbits :
- populate the {blocks}.cache column when a new theme is enabled
- (D5 upgrade path) Signature for db_create_table() in update function was wrong
- (D5 upgrade path) With the previous patch, once update.php was run, block caching was only effective after visiting the blocks overview page. The update function now ensures block caching is possible (not enabled, though) straight ahead by running _block_rehash() for every theme in order to populate the {blocks}.cache column.
This means that any existing setting for contrib blocks (that are supposed to be disabled when upgrading core) will be lost. Not sure what our policy is on this regard.

rerolled after latest commits

Nice additions yched. Tested thoroughly and it works as fast and as cleanly as ever. Am marking RTBC. Patch #18 applies cleanly as of tonight.

Obviously people's schedules have been strained with, um, new arrivals =) but since this now involves 15 separate files maybe this can get committed soon...

(and Yched, please don't mark from rtbc again!) ;)

Yes, new code in the patch, so I figured another round of 'needs review' was reasonable. So, thanks Caleb :-)

I esp. think the upgrade path needs the approval of core folks (see #123 above).

One more request: for consistency, can we change $op = 'cache_id' to $op = 'cache'?

Actually, I'm still not convinced by the custom cache IDs. So far, everyone failed to come up with a valid use case for it. This leaves me to believe it might be over-engineering. It might be better to introduce more defines as we discover more fine-grained needs. In other words, in absence of a clear use case, I'd vote to remove the support for custom cache ID.

And do you think the upgrade path is OK ? - see my comment in #123

(buying some time for the custom cache id...)

A few arguments in favor of the 'custom cache id' :
- (suggested by Crell) A block whose content depend on a notion of 'section' (a site divided in 3 or 4 'sections', for instance based on the first element of the alias). For that, the PER_PAGE pattern will be highly sub-optimal.
- There are many sites that use OG without its n_a features - can be turned on / off in the module settings. I think there might be a way to retain block caching for those sites (provided there are no other actual n_a modules), and then a 'per group id' pattern will definitely be useful.

Anyway, here's a new patch *with* the 'custom cache id' feature (renamed hook_block op from 'cache_id' to 'cache').
I'll post a version without in the next post, you'll pick the one you want in :-)

Without BLOCK_CACHE_CUSTOM

I've committed the patch without the custom block cache stuff. If you want to do custom stuff, you might just as well use cache_get/cache_set directly. It's as many lines of code, and probably easier to learn. Good job, guys.

If you want to do custom stuff, you might just as well use cache_get/cache_set directly. It's as many lines of code, and probably easier to learn

Well, not if you want to combine your specific pattern with some of the existing per_role or per page (then need to reimplement them).
Besides, the _block_get_cache_id() cid builder takes care of automatic per language and per theme differentiation, that will be easily overlooked by contribs doing their own block caching stuff.

All in all, I still think the 'custom cid' is a non-critical but clean, valuable and non expensive feature. Well, let me know if you change your mind, putting it back is a 10 lines patch :-)

Hooray!

No that this is in, I'm wondering if there is some way that a module can set a time limit for the caching rather than by per-page/user/whatever? Or is this even needed?

The use case I'm wondering about is, say for instance, a busy site that has lots of new comments being added. It they do per-anything won't cache have to be dumped and re-created every time a new comment is posted? If so isn't that potentially a lot of overhead - enough that it could actually be detrimental? If this isn't an issue and or is already possible to do with BLOCK_CACHE_CUSTOM please forgive me.

Yeah it'd be cool if say blockcache module could re-use most of this code but provide the extra lifetime and "log in/out/node/comment" cache expiry settings from contrib that are currently available with it. Or for that matter provide the option for node_access override as well. That'd require extra hooks and hence api change though right?

Added an item on the D5 -> D6 upgrade page, and updated hook_block() documentation in http://cvs.drupal.org/viewvc.py/drupal/contributions/docs/developer/hook... (should be visible shortly on http://api.drupal.org/api/function/hook_block/6).

@Calebg :
It they do per-anything won't cache have to be dumped and re-created every time a new comment is posted?
This is the current behaviour : block cache entries are cleaned whenever cache_clear_all() (no arguments) is called - that includes node, comment, user, taxonomy term added / updated / deleted.
See discussion in #94, #11, #112, #115 above.

And BLOCK_CACHE_CUSTOM did not make it :-) : #127, #129, #131, #132

@catch : not sure how we could simple and sound hooks - might need some thinking. Not sure Dries is willing to let something else in for D6 either :-)

Yched, sorry for not grokking this info earlier, as I'm sure it was stated earlier in the thread (sometimes I seem to need to read something more than once to 'get it', apparently), but as I understand it, this:

"block cache entries are cleaned whenever cache_clear_all() (no arguments) is called - that includes node, comment, user, taxonomy term added / updated / deleted."

...will result in a a race-like condition on sites with frequent updates, and will hence be a detriment rather than a help for those kind of sites. I was wondering why Killes said that 'in its current for this patch as it stands now wouldn't not be useful for a site like D.O., but now I think I understand.

Rather than make suggestions, I think I'll ask for some -- suggestions? (and yes, I'm glad to help out - even with coding if need be)

A typo in an insert query currently prevents you from adding a custom block in HEAD.

Attached patch fixes. Simple typo, so I marked it RTBC. Hope that's ok.

Committed webchick's bugfix. Thanks.

Let's keep talking about how this system can be improved, or why it could be suboptimal. Now we have the foundations in core, we can continue to improve this.

Resetting issue status. Thanks, Dries!

OK, here are a few proposals to improve what we now have - mainly throwing ideas, I will most probably have very limited time to actually work on this myself.

1) about cache smarter cache invalidation and better hit ratio :

Back in #112, I proposed this : "let modules specify a BLOCK_CACHE_CLEAR_CUSTOM flag for their blocks, meaning they will handle cache clearing for the corresponding blocks themselves. In cache_clear_all(), we clear block cache _only_ for blocks that do not use this flag."
I still think this is our best option currently. We still gain 'all bocks from all contribs benefit on block caching without effort', and we let more tricky blocks clear their cache wisely (using hook_nodeapi, hook_comment, whatever), without requiring them to reimplement the mechanism we have put in block_list() / _block_get_cache_id().
The additional logic in cache_clear_all() will cost us a few cycles and db queries on 'general' cache clear, and it also means we lose the 'cache_lifetime' feature, that only operates on full cache clearance. *Unless* we store cached entries for BLOCK_CACHE_CLEAR_CUSTOM blocks in a separate table...

2) About not penalizing n_a modules :

Not *all* n_a modules are incompatible with block caching per se :
A lot of them define access rules 'by role', which avoids the need to add the dreaded 'PER_USER' caching pattern to every block.
And a lot of sites use og without it's n_a feature (but the hook_node_grants is defined, so they lose block caching as well)

- A minimal addition would probably be the additional 'aggressive block cache' option proposed by catch in #43 : lets the user say 'I'm aware I have n_a modules on my site, but I still want block caching'. Then if your blocks expose unwanted output, you asked for it. ('aggressive' is not the right word, IMO, for the feature is different from 'aggressive (page) caching', but you get the idea).

- A more complex idea would be to let n_a modules declare what kind of access rules they define : PER_ROLE, or PER_USER. Then, the 'aggressive' (or whatever the name) block cache option adds PER_ROLE or PER_USER caching pattern for every block, and thus can still safely cache (if it's PER_USER, we inform the admin that keeping block caching could be problematic for sites with lots of users...)

- And then we also have some options involving admin choice and some additional UI : for instance, add a 'cache' checkbox for every block on the blocks overview, which lets you decide which block to cache or not. I even think these could replace the 'throttle' checkboxes, couldn't they ?

It doesn't seem that letting cache_set and cache_get suffice for custom block caching is a good idea for the reason yched mentioned (e.g., theme and language issues).

One question, Yched - will someone be able to set a time limit using the code you already created for BLOCK_CACHE_CUSTOM? (and if so what would that look like)

Assuming that BLOCK_CACHE_CUSTOM can support this then +1 for adding it back in as is. If not then I will figure out how to get that functionality included with it (or would I need to just add another parameter, e.g., BLOCK_CACHE_TIME).

Sorry to keep harping on the time-elapsed issue, but I think it's really important. Even Drupal's primary site cache uses the 'minimum cache lifetime' setting as it's main control, for instance.

Finally, replacing the throttle option with block caching doesn't seem like a such good idea. They're two really different things - blockcaching is to help a site to not get in trouble with resources, someone who wants throttling however is assumed to already be in a full-on crisis situation where they're like - 'I don't want this darn thing on at all anymore because my server is at max stress, and I'm just looking to keep it from falling over'.

will someone be able to set a time limit using the code you already created for BLOCK_CACHE_CUSTOM? (and if so what would that look like)
No, not really. BLOCK_CACHE_CUSTOM lets modules affect the cache_id, not the 'expire' property. You could write some code that inserts a notion of timestamp in the cache_id, but the data will be wiped at next cache clearing all the same.

Even Drupal's primary site cache uses the 'minimum cache lifetime' setting as it's main control, for instance.

So does block caching :-) 'minimum cache lifetime' is a general 'cache' feature that delays cache clearing when it involves the *whole* table (for any table, not just cache_page).

Well, I guess the end result is that block caching as it stand now is a whole lot better than what there was before, as well as something to build on. It's really good to know that minimum cache lifetime is applicable for block caching, and +1 is for adding back in the BLOCK_CACHE_CUSTOM for the bits of flexibility it adds.

Everyone who wants to can work on other more robust features in a more methodical, thoughtful way (probably targeting Drupal 7). Honestly, I'm a bit hammered myself with work and with moving so getting myself totally hooked into rewriting/adding on major parts to this is probably more than I can/should step up for, atm.

I might actually have a few hours available to work on possible refinements in the next few days. Dries, is there anything in the proposals in #141 that you could see in D6 ?

Was going to open another issue linking back to this one, but there's lots of good stuff in this, so seems better to mark this to active and bump it. If this is wrong, please open a new issue and close this one down of course.

http://drupal.org/node/105981 was duplicate of this or any new issue which carries on the discussion.

What exactly is the reason for opening this? "There's lots of good stuff in this" is not a reason to mark it active.

Drumm -

Dries: Let's keep talking about how this system can be improved, or why it could be suboptimal. Now we have the foundations in core, we can continue to improve this.

I think webchick probably should have set this to active when she reset status in the first place, but still, I'll split out the various improvement suggestions into different issues so the conversation can continue there.

re-setting title/version.

Feature requests for block caching when node access is enabled and cache expiry now have their own issues.