theme_imagecache_formatter is doing a full load of the node passed into the element so it can pass the node to imagecache_field_formatter. imagecache_field_formatter only uses that node object in the case that it is creating an link and only to reference the nid.

The attached patch is a simple fix without changing the definition of imagecache_field_formatter. Alternatively, you could pass only the nid into imagecache_field_formatter.

CommentFileSizeAuthor
#11 str_vs_preg.php_.txt786 bytesdrewish
#9 str_vs_preg.php.txt743 bytesneclimdul
#8 imagecache_437098.patch2.92 KBdrewish
#7 imagecache_module.patch2.25 KBjcmarco
#4 imagecache_437098.patch6.97 KBdrewish
#2 imagecache_437098.patch3.62 KBdrewish
imagecache_cheaper_formatter.patch1.49 KBneclimdul

Comments

zroger’s picture

zroger commented

+1. This node_load causes some really hard to track performance issues.

Consider the case where you have a view that outputs a list of imagecache formatted images. If you selected 'Fields' for the views row-style, you would expect the view to be output without any additional database overhead. But instead theme_imagecache_formatter does a full node_load on every row, wreaking havoc on what should have been an optimized view.

drewish’s picture

drewish commented
StatusFileSize
new imagecache_437098.patch3.62 KB

the first part of that patch looks good but the second seems kind of odd. i went ahead and grabbed the current code from imagefield's formatters since i know a lot of code is now handled by the cck sanitize op now so we should be able to do less in our formatters. make sure you're running the latest imagefield.

drewish’s picture

drewish commented
Status: Needs review » Needs work

actually that needs a little work.

drewish’s picture

drewish commented
Status: Needs work » Needs review
StatusFileSize
new imagecache_437098.patch6.97 KB

Okay this does a pretty big overhaul on the formatters to split them up into their own theme functions so they can specialize in just what they do and give someone an easy way to override the just nodelink formatter.

drewish’s picture

drewish commented

marking #283922: Imagecache field formatter imagelink theme function as a duplicate since my last patch gives themers equivalent control.

drewish’s picture

drewish commented
Status: Needs review » Fixed

committed to HEAD.

jcmarco’s picture

jcmarco commented
Status: Fixed » Needs review
StatusFileSize
new imagecache_module.patch2.25 KB

This patch breaks existing imagecache configurations!

If you have an configuration with an _ in the presetname by example photo_node when the theme function explode the name into $presetname and $style then it takes presetname=photo and style=node_linked, in the case of a linked style.

In this case the directory and the presetname are wrong and no image is shown.

It should break the string giving the presetname all the first part of the $element['#formatter'] and the last string after last _ to the style string.

I attach a patch, probably there should be some cleanest way to do it.

drewish’s picture

drewish commented
StatusFileSize
new imagecache_437098.patch2.92 KB

Whoops, totally hadn't considered that case. Thanks for testing out the -dev release. If we can avoid a preg_match() I'd prefer that for performance reasons. The attached uses some more verbose string functions and just hard codes the strings where it makes sense.

Please test it out and let me know how it works for you.

neclimdul’s picture

neclimdul
He/Him
commented
StatusFileSize
new str_vs_preg.php.txt743 bytes

I'm not sure we actually should avoid using preg in this case. I made quick test script and it was actually faster to use preg_match than these combined string functions I'm pretty sure strrpos is a pretty slow function.

I've attached the script I ran but here's the results of 100k iterations over the two options splitting "100_box_default."

String function result: 100_box
preg_match result: Array
(
    [0] => 100_box_default
    [1] => 100_box
    [2] => default
)
string function calls:	3.2901763916E-5
preg_match:		1.50203704834E-5
neclimdul’s picture

neclimdul
He/Him
commented

I should also note that this was the /best/ result for the string functions. In the 10 tests I did it was generally around 8E-5.

drewish’s picture

drewish commented
StatusFileSize
new str_vs_preg.php_.txt786 bytes

neclimdul, wow, i ran your script and get basically the opposite results:

string function calls:	3.19480895996E-5
preg_match:		5.96046447754E-6

edit: I should note that I'm on OSX. I'd guess this is pretty platform dependant.

drewish’s picture

drewish commented

I went ahead and committed the preg free version just to get this working. i'd be open to further benchmarking though.

drewish’s picture

drewish commented
Status: Needs review » Fixed

actually lets do any further benchmarking in a better named task ;)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.