Introduce drupal_render() cache pattern. Start using it for blocks

Tagging.

Too sick to review but subscribing.

Regarding #2, do you need the query with values replaced or without? If you just want the prepared statement itself, that's easy. Just cast the query object to a string. With, well, doesn't exist.

Actually now that I think about it, you're right, hook_query_alter runs in execute so it won't actually affect just a straight cast. Hmmm... We'll need some sort of check to ensure that a query object doesn't get altered multiple times.

subscribe

Very interesting.
As a pattern, it's still a little complex to implement, though. I'm wondering how much of this could be tied directly into drupal_render() and triggered by a couple #-properties ?

+ minor : having the query built in forum_block_view() makes it easily overlooked that it's only actually executed in the case of a cache miss.
[edit: OK, except that the query generates the cache cid - nevermind]

Thank you for working on this! I can barely contain my excitement! Marking http://drupal.org/node/300935 as duplicate.

Can you confirm that this system also would allow to display dynamic content to anonymous users when cache is enabled? Like a 'this post was posted 5 minutes ago' depending on their local time, or choose to display advertisements or not depending on their provenance (might not want to display ads to users who access the site directly and have a cookie showing they frequent the site often, for example)?

The implications for this patch are huge for Drupal adoption by complex websites!

I thought that you could cache all elements that are non-dynamic in the pre render phase as you suggest, leaving only the other ones being calculated at page view without page cache.

Good work, Moshe.

If this is to go in, we'll want to clean-up it up some more, make it easier to use, and make sure we can accommodate different scenarios such as those outlined by Fireang3l. It has to become a nice simple pattern that is optional.

That said, I think it OK to have this code in 'prototype stage' for a while. Let's not get obsessed with corner cases or developer experience just yet. First, we have to proof that it is worth it, and that it is better than the block caching we have. Let's be numbers driven so we can explore the upside -- and then worry about other things.

Question. If we go this route, do we any longer need #186636: Block caching when node access modules are enabled.? Should I bump that one down to 6.x?

If this gets in, then we just need to remove that check entirely I think. Although it'll have to be clear that modules specifying cache per page/role without using this system are going to break node_access.

Awesome! Subscribing.

This looks incomplete:

+ * specified. Use BLOCK_CACHE_CUSTOM to disable standard block cache and 
+ * implement

I'm concerned about this current approach because it doesn't give us flexibility to expand this to support edge-side inclusion (ESI), which is becoming a necessary feature for items like blocks on sites with lots of pages and traffic.

The basic idea of ESI is that, instead of including content directly on a page, we provide a tag with the URL, which an ESI-compliant proxy server uses to download the actual content. ESI-supporting proxy servers send headers letting us know when this is possible, so we can intelligently choose between sending ESI tags or the actual content.

The key advantage to ESI is decoupling the caching of block content from general page content, which ties directly into the goals of this issue. Currently, if a site wants a block on every page to show the latest forum topics, the cache lifetime for all pages is limited to the maximum lifetime of that block. As more blocks get added, the page cache lifetime becomes a race to the bottom. With ESI, the page and block cache lifetimes have no bearing on each other. Thousands of cached pages can have long lifetimes even if the forum topics block gets updated every five minutes. ESI also synchronizes the block content across all pages, avoiding the awkwardness of seeing a block in various stages of staleness from page to page (e.g. 30 sec old on one page and 10 min old on another).

Now, we could make developers manually implement ESI as they need to, but we have a great opportunity here to integrate (or provide the foundations to integrate) ESI support into our general content/block rendering system.

What we need to support this is an architecture that allows generation of cached content purely on the basis of the cache key. This cache key must contain (in readable, non-hashed form) the basic arguments required to identify and start generating the content (e.g. a block with module X and delta Y) and relevant key/value pairs for the session (e.g. role M and user ID N).

By carefully defining the cache keys, edge servers (e.g. reverse proxy caches) can request content that Drupal can generate with no other context than the key.

The other advantage is that it opens the door for preemptive refreshing of the cache, like my Preempt module did. For blocks taking 5+ seconds to render, it's not very nice to make random visitors wait around to regenerate cache items. Preemptive caching has identical architecture requirements to ESI support.

I realize I'm suggesting a radical departure from what's being done here, but I want to lay foundations if we're going to bother making a change this big in the API.

Finally, I think the change suggested here is mostly an attempt to layer caching on the current API rather than create an inherently cache-friendly API. There's nothing inherently wrong with layering, but I think this API is worse for that design trait. This feels like the kind of cache support a contrib module would awkwardly provide.

Discussed with Moshe on IRC. I'll be spending tonight banging my head against the problem of an API that supports ESI without too much absurdity. It's a rather interesting architecture problem.

We really really need to encourage folks to use #attached_css and #attached_js instead of drupal_add_css() and drupal_add_js(). Those functions are side effects that are not easily cached.

Where is the documentation on #attached_js and #attached_css? I've had people asking me for it and am at a loss to help them. It also seems like this paradigm shift was introduced without the knowledge of the majority of Drupal front-end developers, because they've been working on making drupal_add_js() and drupal_add_css() better (alterable, weightable, able to pull in external JS, etc.).

Getting everyone on the same page here seems like a pretty critical issue.

It went in with vertical tabs, which the same people working on drupal_add_js() and drupal_add_css() changes worked on. Also documented at http://drupal.org/node/224333#attached_js with a comment covering the caching issue. I think we're going to need a section in the module updates to make it clear this should be used for any renderable thing though.

Oooh yes, I like, I like!

I'm dedicating tonight to working on an ESI-compatible version of this that works for at least anonymous users. (That should not require anything special to work with Squid and Varnish.) Really, my final goal of ESI support for even authenticated users is too far down the road to worry about now.

subscribing

I'm starting to lose faith that it's possible to build a widely compatible ESI-based cache. If anyone has any great ideas for this issue (including Moshe's original one), don't let my investigations hold you up.

Changing status.

I normally mark them postponed to make it obvious that they're blocked by the other issues, which is then a good reason to mark the blocking issues as critical, but maybe that's just me.

@davidstrauss: if you ever find the time, I think it would be a good thing to document (in another issue) the problems you encountered trying to implement an ESI-compatible version.

Bump. The patches in #5 both landed recently. Do you have a current patch, moshe?

OK. Attached is a new, improved patch, building on #16 but containing some new stuff.

The caching is not anymore based on a special #pre_render callback, but is native to drupal_render. All cache related properties are in a #cache property of an element that is an associative array.

Quoting from the documentation I added to drupal_render():

/**
 * drupal_render() can optionally cache the rendered output of elements to
 * improve performance. To use drupal_render() caching, set the element's #cache
 * property to an associative array with one or several of the following keys:
 *    - 'keys': An array of one or more keys that identify the element. If 'keys'
 *       is set, the cache ID is created automatically from these keys.
 *       @see drupal_render_cid_create()
 *    - 'granularity' (optional): Define the cache granularity using binary
 *       combinations of the cache granularity constants, e.g. BLOCK_CACHE_PER_USER
 *       to cache for each user seperately or
 *       BLOCK_CACHE_PER_PAGE | BLOCK_CACHE_PER_ROLE to cache seperately for each
 *       page and role. If not specified the element is cached globally for each
 *       theme and language.
 *    - 'cid': Specify the cache ID directly. Either 'keys' or 'cid' is required.
 *       If 'cid' is set, 'keys' and 'granularity' are ignored. Use only if you
 *       have special requirements.
 *    - 'expire': Set to one of the cache lifetime constants.
 *    - 'bin': Specify a cache bin to cache the element in. Defaults to 'cache'.

So: Elements do normally not specify a cache id directly but set cache 'keys' (for blocks that usually is array('module', 'delta')) and optionally 'granularity'. The latter is exactly the same as in the current block cache. For that to work out I moved the block cache constants to common.inc but did not yet rename them. How should they be named? DRUPAL_CACHE_PER_ROLE etc? This is an easy change, though.

Block cache is still unmodified and in place (but uses some code from common.inc now to create the IDs). I propose to just leave it in place.

Not that with this system, it is REALLY easy for contrib to change cache behaviour in hook_page_alter()!
All elements that load their actual data in a #pre_render hook can be made cacheable by just setting the #cache property on them in hook_page_alter(). It's also easily possible to modify caching behaviour for individual sites by modifying the 'granularity' and 'keys' properties of individual elements in hook_page_alter(). Would of course be nice to move more of core towards this system, but in this patch it's still only forum module blocks. I'd like to keep the rest to follow up patches.

Now, I will leave for vacation tomorrow morning so I won't be able to keep pushing this. Anybody interested? Would be really great to still get this in. We can still move more stuff towards this system after the freeze. But getting the base system in would be awesome.

This patch is the same as above but renames the block cache constants (so BLOCK_CACHE_PER_ROLE is now DRUPAL_CACHE_PER_ROLE etc) as they're in common.inc by now and used by drupal_render() caching as well. No other changes to #38.

Note that the patch appears much larger than it is, the actual changes are just about 100 LOCs in common.inc and a few lines in forum.module, the rest is just moving things around and some renames.

Forgot --no-prefix in the last patch. This one is better. The one in #38 is fine as well.

I like this patch, but before this gets committed, I'd really like to see some benchmark results just to make sure it works and that we don't have any regression. Looking at the code, I don't expect any regression.

I don't believe just the active forum blocks being cached this way makes anything 10% faster, but there's certainly no regression:

catch@catch-laptop:~/www/7$ ab -c1 -n500 http://d7.7/
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking d7.7 (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Finished 500 requests


Server Software:        Apache/2.2.11
Server Hostname:        d7.7
Server Port:            80

Document Path:          /
Document Length:        19585 bytes

Concurrency Level:      1
Time taken for tests:   44.673 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Total transferred:      10025000 bytes
HTML transferred:       9792500 bytes
Requests per second:    11.19 [#/sec] (mean)
Time per request:       89.345 [ms] (mean)
Time per request:       89.345 [ms] (mean, across all concurrent requests)
Transfer rate:          219.15 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    78   89  18.8     84     420
Waiting:       72   83  18.2     78     409
Total:         78   89  18.9     84     420

Percentage of the requests served within a certain time (ms)
  50%     84
  66%     87
  75%     91
  80%     93
  90%    102
  95%    120
  98%    130
  99%    132
 100%    420 (longest request)
catch@catch-laptop:~/www/7$ wget http://drupal.org/files/issues/bc3_1.diff
--2009-08-31 21:19:45--  http://drupal.org/files/issues/bc3_1.diff
Resolving drupal.org... 140.211.166.6
Connecting to drupal.org|140.211.166.6|:80... connected.
HTTP request sent, awaiting response... 200 OK
Length: 26033 (25K) [text/plain]
Saving to: `bc3_1.diff'

100%[===================================================================================================================>] 26,033      35.6K/s   in 0.7s    

2009-08-31 21:19:46 (35.6 KB/s) - `bc3_1.diff' saved [26033/26033]

catch@catch-laptop:~/www/7$ patch -p0 < bc3_1.diff
patching file includes/common.inc
patching file modules/block/block.admin.inc
Hunk #1 succeeded at 443 (offset -24 lines).
patching file modules/block/block.api.php
patching file modules/block/block.module
patching file modules/book/book.module
patching file modules/forum/forum.module
patching file modules/locale/locale.module
patching file modules/menu/menu.module
patching file modules/node/node.module
patching file modules/profile/profile.module
patching file modules/search/search.module
patching file modules/statistics/statistics.module
patching file modules/system/system.module
patching file modules/user/user.module
catch@catch-laptop:~/www/7$ ab -c1 -n500 http://d7.7/
This is ApacheBench, Version 2.3 <$Revision: 655654 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking d7.7 (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Finished 500 requests


Server Software:        Apache/2.2.11
Server Hostname:        d7.7
Server Port:            80

Document Path:          /
Document Length:        3043 bytes

Concurrency Level:      1
Time taken for tests:   39.681 seconds
Complete requests:      500
Failed requests:        0
Write errors:           0
Non-2xx responses:      500
Total transferred:      1759500 bytes
HTML transferred:       1521500 bytes
Requests per second:    12.60 [#/sec] (mean)
Time per request:       79.362 [ms] (mean)
Time per request:       79.362 [ms] (mean, across all concurrent requests)
Transfer rate:          43.30 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    60   79  16.3     74     201
Waiting:       60   79  16.3     74     201
Total:         60   79  16.3     74     201

Percentage of the requests served within a certain time (ms)
  50%     74
  66%     84
  75%     91
  80%     95
  90%    100
  95%    107
  98%    117
  99%    124

Found out why it's faster, PDO exception when forum block + block cache is enabled which bails everything out before rendering. Shoddy forum tests fail us again.

Was just a missing global $user so easy fix. I verified the caching is working with devel query log, and re-ran benchmarks to confirm no other issues. Back to RTBC.

Committed to CVS HEAD. Thanks!

documentation

Makes sense.

Shouldn't we add tests for this?

#55 is still RTBC, but in profiling I noticed we're calling drupal_render_cache_get() and drupal_render_cid_create() for every element passed to drupal_render() whether it's cacheable or not - on a node with 50 comments this is ~ 1,000 function calls.

So this patch just adds an isset($elements['#cache']) to the cache check.

Committed both patches. I still think it would be useful to have tests because this bug fixed in #55 is testable... marking to 'code needs work' for tests.

Tagging.

Downgrading from critical, see #653074: Review test coverage / outstanding tests before release - I also don't think there's a particular hole in test coverage here - or at least, we have much more pressing gaps in forum.module specifically if we were to add it to there.

.

Am I right in understanding that if drupal_add_*() is called from within a theme function or preprocess function, then this is incompatible with the render cache? And that therefore, if theme_foo() outputs markup that requires a CSS file, and if the way theme_foo() is called is via a structure like:

array(
'#theme' => 'foo',
...
);

Then every caller of the above needs to add a #attached? That really breaks encapsulation, doesn't it? I think contrib will really struggle with this.

And related, if you have:

$element['parent']['child']['#attached']['css'] = ...

And if the parent is render cached, that too fails to load the CSS file, right?

Hmmm. I'm troubled by the lack of encapsulation in #68.1. If foo.tpl.php outputs markup requiring certain CSS, how is every caller of it supposed to know that? I wonder if we can do something like add 'attached' to the theme registry, and then have drupal_render() transfer that info to $element['#attached']. The only trouble I see with that is that sometimes #theme is an array (or a string with "__" that's an implicit array), and drupal_render() shouldn't have to duplicate everything theme() does to handle that. But, the 2nd param to theme() can't be by reference, because sometimes, code calls theme('foo', array(...)), and PHP would choke on that if theme() tries to take $variables by reference. Does it make sense to add a 3rd param to theme that could be used for theme() to inform drupal_render() on what needs to be added to #attached? I know we'll fix this somehow in D8, but do you think anything along these lines is possible for D7? I've been starting to look at several contrib modules, and this problem is surfacing a lot. I don't think contrib module maintainers will move calls to drupal_add_*() out of the theme layer without some reasonable place to move them to.

Is the "needs documentation" tag on this issue still relevant?

subscribe

Documentation here: https://www.drupal.org/developing/api/8/cache