Cache filter/format queries

looks good to me as well.

It would be really great if we had a proper API for those optimizations that made this behavior more explicit.

+++ modules/filter/filter.module	13 Jul 2010 06:56:08 -0000
@@ -375,14 +375,20 @@ function filter_modules_disabled($module
+    if ($cached = cache_get('formats_all')) {

Bad namespace -- this cache entry is from Filter module, so must be prefixed with "filter[_:]"

Effectively questioning, why not simply __FUNCTION__ ?

+++ modules/filter/filter.module	13 Jul 2010 06:56:08 -0000
@@ -625,9 +633,15 @@ function filter_list_format($format_id) 
+    if ($cached = cache_get('filters_all')) {

Same here, although the key is (almost) in the correct namespace this time. However, "filters_all" is not very descriptive.

Again, __FUNCTION__ would be cleaner here.

Powered by Dreditor.

Furthermore, when caching data that is otherwise retrieved through a query using ->addTag('translatable'), then the cache cid key needs to have a language suffix.

Attached patch should be ready to fly -- and is also a requirement for #903730: Missing drupal_alter() for text formats and filters

Sorry, forgot that it's $language->language now.

+  cache_clear_all('filters_all', 'cache', TRUE);

Should be

+  cache_clear_all('filter_list_format', 'cache', TRUE);

Ah, thanks! - stupid me.

#9: drupal.filter-format-cache.9.patch queued for re-testing.

Rerolled

Thanks!

This seems like a sensible performance improvement.

Committed to HEAD. Thanks!

GRRRRUUUUMMMMMMMBBBBBLLLLLEEEE.

You guys snuck this by me. This went from needs to work to committed in 90 mins.

The queries we are saving here are again single table, fully indexed queries. They are stupid simple. Once again, you guys are just afraid of relying on your DB layer and using caching layer instead. This adds a lot of complexity to Drupal (cache expiration, stampede, general developer WTF when stuff won't change).

Please rollback. It is *not* a performance improvement, as webchick says. This is killing Drupal, one knife cut at a time.

EEK! Don't GRRUMMBLLEEE at me! :D

Ok, sorry. I didn't realize this would be contentious.

Rolled back. Marking back to "needs review" so we can come to a consensus about what to do here.

sigh. we are not afraid of the database. the database is slow. memcache is fast. if you want to postpone to Drupal 8, oh well. I want memcache in core for D8 just fyi.

For a site using the default database cache, I wonder if this was a performance hit rather than an improvement (it swaps one database query for another, plus then you have the overhead of unserialize in the caching layer).

Even for Drupal 7, is there some hack we could use to make drupal_static() swappable? Then a contrib module could override drupal_static() and allow some of those "static" variables to persist between page requests and be stored in memcache, etc, but other sites wouldn't have to deal with it.

@David: That's exactly what the patch in #924616: Make database cache leverage static cache by default intended to do.

While this patch could be considered as improvement for D7, it is too late to consider it now, especially as it seems to require more discussion. Thus, moving to D8. It may be backported later, if we happen to do this performance improvement for D8. For the current D7 queue, we only consider absolutely required patches.

I agree the other issue would allow that, but would also require bigger changes (lots of code would have to switch to using cache_get, etc). The idea here would be to simply leave things as they are, but allow contrib to hook into the existing drupal_static() mechanism and turn some of the static caches into persistent caches if it wanted to. As a stopgap for Drupal 7.

Difficult to do though, I guess - since drupal_static() is called so often and so early, it would probably be hard to make it cleanly swappable or alterable.

I disagree with @moshe that patches like this are killing Drupal. One might argue that this is a 'misuse' of the cache API, substituting a cache_get() that is *potentially* slower than a straight query. But in reality the cache API is meant to be pluggable and changes like this allow sites that care about scalability to scale without hacking or forking core. Let's keep 7.x in mind, this is not a huge patch.

I'm attaching a new patch, please if you have some time, read http://drupal.org/node/903730#comment-3515902 to understand the reasons for this patch.

If we're going to add alterability to these queries (I agree with catch this should be discussed in a separate issue), we already have a hook_query_alter(). No need to add a new hook IMO, just a tag on the query.

I spoke with catch on IRC. I'll now stop obstructing these patches for D7 since we have a plan for D8: use exportables (code+DB override, with persistent cache). I still can't mark the original patch RTBC without gagging.

#13: drupal.filter-format-cache.13.patch queued for re-testing.

Moshe: I don't like these patches either so I'd love to learn more about the exportables idea. Somehow I question that the exportables concept will make it _easier_ ... :)

I don't think this particular issue has anything to do with exportables, really. It's more about being able to serve Drupal pages directly from memcache without hitting the DB. The ability to *alter* this query so that instead of it grabbing a filter ID it grabs a machine name enables exportables, but that discussion has been moved to #903730: Missing drupal_alter() for text formats and filters.

Well not just alter the query. Load text formats from code requires a drupal_alter too.

#13: drupal.filter-format-cache.13.patch queued for re-testing.

#13 is still RTBC then and currently blocks #903730: Missing drupal_alter() for text formats and filters, as there is no clear target code to run a patch against.

Committed to CVS HEAD. Thanks.