I'd like to propose an alternative to the work being done in #1601044: Content Access in D7?

Rather than patching core to allow block caching for sites with custom node access functionality, I'd like to propose we follow moshe's advice in #186636: Block caching when node access modules are enabled. and simply piggy-back on render cache.

...the workaround is easy for Contrib or for your own site. In core, the only model for this is in the forum block. The info for the block needs to be DRUPAL_CACHE_CUSTOM (going by memory now) and then in hook_block_view() you change the render array to have a #cache array on it. Then you leverage the render cache instead of block cache. Your cache keys can then be custom and include the organic group or whatever your node access criteria are. The hooks involved are hook_block_view_alter() and hook_block_info_alter().

I think the plan would be to use the existing cache constants stored by block cache alter, rather than DRUPAL_CACHE_CUSTOM. Additionally, we may want to make it opt-in (so that the user recognizes that enabling the render cache fallback has content access consequences for which he would be responsible).

I've been experimenting with this and it works quite well. There are just a few caveats; notably, render cache includes the whole content of the block as rendered (including, for example, contextual links), so the constants don't behave exactly the same as core block cache. We should probably mitigate this for contextual links (which seems to be the only incompatibility with core).

Comments

iamEAP’s picture

Status: Active » Needs review
StatusFileSize
new4.49 KB

Here's a patch that takes Contextual Links into account.

iamEAP’s picture

Status: Needs review » Needs work
+++ b/blockcache_alter.module
@@ -110,3 +120,86 @@ function blockcache_alter_block_info_alter(&$blocks, $theme, $code_blocks) {
+    foreach ($results as $block) {
+      $blockcache_alter[$block->module . '-' . $block->delta] = $block;
+    }

So... Turns out this is problematic. I threw this in because the block ID given in hook_block_view_alter() isn't the actual block ID, it's module-delta. Unfortunately, module-delta isn't unique enough, so a bunch of block cache alter configurations are truncated/overwritten.

+++ b/blockcache_alter.module
@@ -110,3 +120,86 @@ function blockcache_alter_block_info_alter(&$blocks, $theme, $code_blocks) {
+      $blockcache_alter = blockcache_alter_load_cache_configurations();
+      if (isset($blockcache_alter[$block->bid])) {
+        $block->cache = $blockcache_alter[$block->bid]->cache;
+      }
iamEAP’s picture

Have to add the theme to the mix.

iamEAP’s picture

Ran some benchmarks and this patch is not looking very promising. On a real-world page with 3 menu blocks, 3 node blocks, and 4 view blocks--all set to cache globally--the performance gain with this patch is extremely minimal (between 0-3% improvement). Makes sense because all of the menu/view/nodeblock processing still occurs, we're just pulling rendered HTML from cache at the last minute. A full render array is generated as if there were no caching; the short circuit only happens right at drupal_render().

When I compared the same page under the same conditions, but with a core patch that allowed for standard block cache, there's no comparison: the performance gain was on the order of 30-50%.

AB results, for reference:

No block cache at all

Concurrency Level:      2
Time taken for tests:   108.145 seconds
Complete requests:      300
Total transferred:      21020900 bytes
HTML transferred:       20798900 bytes
Requests per second:    2.77 [#/sec] (mean)
Time per request:       720.966 [ms] (mean)
Time per request:       360.483 [ms] (mean, across all concurrent requests)
Transfer rate:          189.82 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        2    2   0.2      2       4
Processing:   607  718  75.2    701    1060
Waiting:      563  669  73.9    653    1015
Total:        609  720  75.1    703    1062

Percentage of the requests served within a certain time (ms)
  50%    703
  66%    724
  75%    752
  80%    773
  90%    824
  95%    861
  98%    962
  99%    985
 100%   1062 (longest request)

Blocks cached via render cache (patch to Block Cache Alter)

Concurrency Level:      2
Time taken for tests:   104.409 seconds
Complete requests:      300
Total transferred:      21021390 bytes
HTML transferred:       20799390 bytes
Requests per second:    2.87 [#/sec] (mean)
Time per request:       696.061 [ms] (mean)
Time per request:       348.031 [ms] (mean, across all concurrent requests)
Transfer rate:          196.62 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        2    4   3.1      2      16
Processing:   591  692  78.1    667    1057
Waiting:      550  643  76.9    621    1002
Total:        593  696  78.1    672    1058

Percentage of the requests served within a certain time (ms)
  50%    672
  66%    701
  75%    723
  80%    739
  90%    807
  95%    880
  98%    928
  99%    978
 100%   1058 (longest request)

Block cache enabled, Core Block module hacked to allow block caching

Concurrency Level:      2
Time taken for tests:   58.849 seconds
Complete requests:      300
Total transferred:      20022222 bytes
HTML transferred:       19800222 bytes
Requests per second:    5.10 [#/sec] (mean)
Time per request:       392.327 [ms] (mean)
Time per request:       196.164 [ms] (mean, across all concurrent requests)
Transfer rate:          332.26 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        2    4   2.8      2      13
Processing:   306  387  71.3    366     722
Waiting:      270  351  70.0    329     678
Total:        308  391  71.7    369     727

Percentage of the requests served within a certain time (ms)
  50%    369
  66%    389
  75%    405
  80%    417
  90%    518
  95%    553
  98%    584
  99%    653
 100%    727 (longest request)
fabianx’s picture

Interesting approach, I will probably add a compatibility layer via render_cache module though.

fabianx’s picture

Status: Needs review » Postponed

render_cache 7.x-2.x-dev now supports the possibility to cache all core blocks with render_caching and it takes block cache alter values into account.

Also there is now a 7.33 block_cache_bypass_node_grants option, which makes that possible.

So I think we can postpone this for now. Thanks for the patch!

fabianx’s picture

Status: Postponed » Closed (won't fix)

As core supports this, we don't need to fix it here.