_imagecache_get_presets() can be called multiple times per page request when combining imagecache and imagefield's views capabilities. For every view instance on a page views calls imagefield_field_formatter() which makes unnecessary SQL queries.

We can eliminate these duplicate queries with the attached patch which uses static variable caching. Should apply to 4.7 and 5 branches.

CommentFileSizeAuthor
#7 imagecache_presets.patch4.27 KBquicksketch
#5 108672_0.patch3.11 KBdopry
#3 108672.patch3.04 KBdopry
#2 imagecache_presetcache_0.patch1.01 KBquicksketch
imagecache_presetcache.patch955 bytesquicksketch

Comments

RobRoy’s picture

RobRoy commented
Status: Needs review » Needs work

With D5's ease of programatic form submission, we need to have all functions with static caching have an optional $reset parameter to clear the static cache. I've run into this doing some D5 installation profiles work with taxonomy_get_tree() (see http://drupal.org/node/106015). Imagine something contrived like this:

- Build a view with some imagecache images (static presets are now set)
- Programmatically submit a preset and some images with drupal_execute()
- Build another view or whatever (the static cache is now out-of-date, we need to be able to call _imagecache_get_presets(TRUE) to reset it before we build this second view.

quicksketch’s picture

quicksketch
he/him
commented
StatusFileSize
new imagecache_presetcache_0.patch1.01 KB

Cool, sounds like a plan. I had forgotten that such a situation could occur. Updated patch attached.

dopry’s picture

dopry commented
StatusFileSize
new 108672.patch3.04 KB

How do you guys feel about taking it a step further like the attached patch and using drupal's cache support, and static caching... Updates the other load functions to take advantage of a modified _imagecache_get_presets...

quicksketch’s picture

quicksketch
he/him
commented

This seems like a good idea overall. Most pages I use 2-4 presets during a single request! Knocking them down to one would be awesome. Combining with the cache table should yield a small performance gain also.

I don't quite understand this part of the patch however:

+    // Grab from cache saves building the array. Plus its a frequently used table. 
+    $presets =  cache_get('imagecache:presets');
+    
+    // Fall through if presets is empty.
+    if (is_array($presets)) {
+      return $presets = cache_get('imagecache:presets');    
+    }

Why call cache_get('imagecache:presets') twice in a row?

dopry’s picture

dopry commented
Status: Needs work » Needs review
StatusFileSize
new 108672_0.patch3.11 KB

That was an oversight. Try the attached patch.

quicksketch’s picture

quicksketch
he/him
commented

Here's a completely functional version of dopry's patch. There's a couple changes:

- cache_get and set serialize the data before caching
- the preset cache only uses the numeric ids as keys, rather than both names and ids as keys
- adds 'content_clear_type_cache()' which clears the views formatters

I noticed whenever I made a new preset in imagecache, I needed to empty the drupal cache (via devel) in order for the new preset to show up as a formatter in views. The content_clear_type_cache() empties the cache of content types and in turn causes all views formatters to be cleared.

I tested a couple times over and should be good for review.

quicksketch’s picture

quicksketch
he/him
commented
StatusFileSize
new imagecache_presets.patch4.27 KB

Oh... and the patch.

dopry’s picture

dopry commented

committed to 4.7 dev and 5.0 dev.

dopry’s picture

dopry commented
Status: Needs review » Fixed

committed to 4.7 dev and 5.0 dev.

quicksketch’s picture

quicksketch
he/him
commented

Thanks dopry, this kicks serious ass. :D

kkaefer’s picture

kkaefer commented
Title: Eliminate Duplicate Preset Queries » Eliminate Duplicate Preset Queries, but don't use non-core functions
Status: Fixed » Active

I’m getting Fatal error: Call to undefined function content_clear_type_cache() when I go to the administration page. Obviously, this function is a CCK function and not in Drupal core. However, CCK is not required for this module, is it?

quicksketch’s picture

quicksketch
he/him
commented

Oops, right you are. Here's an inline patch for Drupal 5:

Index: /Users/nate/Sites/drupal5/sites/all/modules/imagecache/imagecache.module
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/imagecache/imagecache.module,v
retrieving revision 1.19.2.13
diff -u -r1.19.2.13 imagecache.module
--- imagecache.module	25 Jan 2007 04:11:16 -0000	1.19.2.13
+++ imagecache.module	27 Jan 2007 17:38:43 -0000
@@ -191,7 +191,9 @@
   }
   cache_set('imagecache:presets', 'cache', serialize($presets));
   // Clear the content.module cache (refreshes the list of formatters provided by imagefield.module)
-  content_clear_type_cache();
+  if (module_exists('content')) {
+    content_clear_type_cache();
+  }
   return $presets;
 }

If applied to the Drupal 4.7 branch, be sure to use 'module_exist' instead of 'module_exists';

quicksketch’s picture

quicksketch
he/him
commented
Status: Active » Needs review

Setting to review, this still needs to go in.

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs review » Fixed

Commited content_clear_type_cache() fix to 4.7 and 5.x branches.

bjaspan’s picture

bjaspan commented
Priority: Normal » Critical
Status: Fixed » Needs work

You forgot to follow your own advice in #12: "If applied to the Drupal 4.7 branch, be sure to use 'module_exist' instead of 'module_exists'." Now, imagecache causes a fatal error on 4.7.

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs work » Fixed

Thanks bjaspan. I've corrected this in the 4.7.x branch.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)