Performance issues with large number imagecache presets and content types

I'm not sure what lightbox2 can do about this. Yes there are a lot of formatters created, which combine every possible usage of imagecache presets. So every time a new preset is added, the number of lightbox2 cck formatters grows exponentially. At present cck formatters have no configuration associated with them, so I don't see how I can shorten the list. I don't believe the number of content types would have an effect on this however.

Well, I can see the problem, but formatter settings will definitely not happen in CCK D6.
1M is an insufficient max packet size to run drupal anyway.

As far as CCK is concerned, this is a won't fix

D7 Field API already has formatter settings. The UI implications, though, are not resolved yet.

Here's an idea that maybe lightbox2 could implement. I'm not sure how lightbox2 works, though.

In the field settings panel there are global settings, and widget settings. Maybe lightbox2 could implement widget settings so that users can customize defaults for the lightbox2 formatters. That way, maybe just one formatter could be needed.

Note that widget settings are not shared, even if you use the same field shared with different content types. So you can implement different formatter settings per widget here.

I used this technique for the Money CCK field ("Currency display mode" and "Decimals display mode" really are formatter options). :)

Yes, in the absence of formatter settings in D6, some modules put their display settings in 'widget settings', which in D6 are the only 'instance specific' settings.
Problems with this approach, though :
- conceptually awful (but well, CCK D6 has nothing better to propose...)
- you need to own the widget to add widget settings :-(, which is not the case of lightbox2 - #417122: Allow drupal_alter() on Field and Widget Settings should work around this
- you cannot specify different settings for different build modes (teaser, full...) - unless you add one set of settings per build mode in the widget settings form, basically duplicating the 'Display fields' page. Yuck.

I'm not sure how well a lightbox2 widget would work, considering lightbox2 has to be able to handle images, pdfs, files, videos, web page links, etc. It's possibly worth considering, but I think I'd prefer to add support for the #417122: Allow drupal_alter() on Field and Widget Settings approach.

Ah, that issue is in a state that needs review. I think it would be nice to avail this opportunity to join forces with the people working on that patch so maybe it can be included in CCK as soon as it can be assured it works for more that one use-case (filefield and lightbox2 related extensions).

@stella: The previously mentioned patch had been committed to CVS, so you can now embed your own settings to any field/widget. Hence this is now doable in contrib. I'm afraid CCK for D6 cannot do more.

Re-assigning to lightbox2 for further review / development

I'd like to emphasize the negative impact of this default behavior on large Drupal setups.

As explained in this frontpage case study http://drupal.org/node/614014, Lightbox2 alone (with its automatically-generated formatter and theme declarations that are stored in cache and fetched on every page) was responsible of 80% of the SQL traffic, i.e. 1Gbit/s out of 1.25Gbit between apache and mysql.

It would be really great to have these formatters generated only if we want them, e.g. with a config page.

bump

Anything we can expect here? Thx.

subscribe

subscribe

subscribing. I

have a site with about 20 imagecache presets, wich leads to an overall generation of about 2000 registered theme functions :-( And this unfortunately kills my database server.

Hope there will be a fix soon, as I can't install the security release as it provides a lot of new theme functions wich lead to this problem

Cheers

hctom

UPDATE: after increasing my max_allowed_packet size, everything works fine again... except that about 3MB of memory is used more per page request because of all of the new functions.

Isn't it possible to use templates with suggestions instead?

subscribing

Here's a really quick patch based on #21. It is against 6.x-1.9 ... sorry don't have time right now to roll against HEAD. I have only done really brief testing but by removing the formatters for Lightshow and Lightframe, and only allowing one preset for the Lightbox view I have reduced my theme registry from 1.2MB to 500KB.

Basically this just adds some settings to the bottom of admin/settings/lightbox2. Please test and improve

Whoops, was so excited I forgot to attach. Here we go

Again, I was rather rushed. I only remembered to update it in one spot instead of all of them. There should prob be a function that returns this.

Correct behaviour for variable_get is in this part of the patch

+  $imagecache_presets = imagecache_presets();
+  $presets = array();
+  foreach($imagecache_presets as $key => $preset) {
+    $presets[$key] = $preset['presetname'];
+  }
+  $form['advanced_options']['theme_registry_settings']['lightbox2_theme_fullsize_formats'] = array(
+    '#type' => 'checkboxes',
+    '#title' => t('Fullsize presets'),
+    '#default_value' => variable_get('lightbox2_theme_fullsize_formats', array_keys($presets)),
+    '#options' => $presets
+  );
+

Reroll of #26 with comments from #27

subscribing

Subscribe.

We had the same issue in noderef_image_helper.

Our solution was to add checkboxes to enable presets as candidates for formatters.
#1009456: limit imagecache presets

Instead of adding some similar feature like this, imagecache (or an additional module) should allow us to define consistent per-preset settings about their application.

subscribing

Providing some updated patch.
Note the repo meanwhile introduced way more formatters (compact variants). The code starts to become very heavy... (for a lightbox).
So it is very important we cleanup the theme registry. I think currently it's already twice as heavy as initially calculated.
It starts to become pretty critical.

Much more i'd like to propose a pair definition.
In ImageCache we should be able to define pairs of presets or even groups. Thus we could define

small => big
small2 => big2

If not imagecache itself, then lightbox settings could define such setups.
However the UI will be pretty heavy for such a generally simple task.

Alternatively - to avoid implementing a complex UI - we could also introduce hidden variables that define pairs.

$presets = array(
 array('small', 'big'),
 array('small', 'big2'),
 array('small2', 'big2'),
);

If nothing predefined, we can fallback to current any2any case.
What do you think?

Providing a better version. The other had some issues...

Latest patch applied cleanly to latest Lightbox2, thanks!

I haven't been able to check performance, really, but will post if I notice anything out of the ordinary.

subscribing

Excellent last patch, thanks! I have reduced my theme registry from 2.6MB to 400KB.

thanks!

subscribing

+1 for #37

sub

I'm sorry, but the patch in #37 does not solve the problem at all. By the way: it even does not work, as there is a $presets variable used in hook_theme() that is nowhere loaded. Just apply the patch and check the lines 962 and 1037 of the lightbox2.module file!

Another problem is, that I have to enable both the source as well as the target preset as "Fullsize presets" in order to get the correct theme hooks created... and as you may guess: This leads to the same big theme registry as before!

So I guess here is some more work needed :(

When using memcached with Drupal this is a problem, because we are limited to 1Mb per cache object. So, no cache on this query :-(

If one is not aware of the performance impact of this issue, it can cause considerable pain. Due to the massive amount of formatters generated, your theme registry can easily hit 1MB, and you will then (probably) be hit by either the default packet size in MySQL, or the max object size in Memcache(d), which are both 1MB.
Our site has 25 presets, appx. 20 content types and Lightbox formatters in the thousands, which caused significant CPU load on the webserver, due to the theme registry never being cached - and thus regenerated on every page load.

Not really the fault of this module, but I do think there should be some sort of warning or information about this on the module's frontpage. Ie. that this may cause your theme_registry variable to explode, thus rendering it nigh uncacheable.

Just hit this on a client site. They pushed some new code and the theme registry stopped being cached. I remembered this bug report and checked the size of the theme registry on dev, there were over 5,000 entries in the array. Added a custom hook_theme_registry_alter() to strip lightbox2 entries out and this brought it down to just over 700.

Subscribe

I've made a patch with different approach. The patch adds formatter settings in the variable. The setting defines possible combinations of image presets for the lightbox2 image formatters. It works so that only enabled combinations generate formatters & theme functions.
There's no UI cause it's significant amount of work and I'm not sponsored to do that. Note that there's no check if the combination of settings is a valid: it's a job for UI to check that before saving.
Setting to 'needs work' because of no UI but basically this needs review.
Example variable setting (for settings.php):

$conf['lightbox2_image_formatter_options'] = array(
  array(
    'type' => 'lightbox2',
    'compact' => FALSE,
    'source' => 'inline_image_600',
    'dest' => 'lightbox_image_popup'
  ),
  array(
    'type' => 'lightbox2',
    'compact' => FALSE,
    'source' => 'profile_photo_200',
    'dest' => 'profile_photo_lightbox'
  ),
);

Also nice approach, crea.

I think a UI makes only sense if it's a pattern - which all other modules with similar issues should follow...
However this is something that really should be defined.

The config minimizes the issues and is a real option to me. :-)

I can't follow, in how filtering this could reduce the theme registry size.
Please test this yourself first, if you think it might help.

The issue was always primarily about the theme registry size.

Now i get it. I missed the cck formatter issue completely.
Actually i think one should always array_filter before persisting.

I'm also thinking about an imagecache helper module to support per-imagepreset options.. In a uniform style for all extending modules.

Per-preset options would be wrong solution.
Say, you want to enable to show inline images using preset A with popup preset B. and C with popup preset D
How are you going to configure that using per-preset options ?
- If you implemented "use this for lightbox" checkboxes, you would have too many lighbox directions enabled: all combinations of A,B,C,D
- If you implemented "use this for image sources" and "use this for image popups" checkboxes, you would still have too many lighbox directions: both A->D and C->B would be enabled.
- If you implemented "use this as a source preset for B popup preset, but not for D popup preset" type settings, you would have nightmare UI in the end.

I hope it's clear ;)

Indeed. But similar A-to-B are needed for other cases too.
However in D7 this is no more needed since formatters support per-instance settings.
This leads me to the suggestion of a UI less minimum implementation but ASAP...

I think the fix for the registry issue would be to provide suggestions instead of registering an individual theme hook for each combination. That might only require one or two entries in the theme registry. Should this be split into two issues?

I think there's now enough discussion to build a clean solution for "the issue" without a split.

Both
Configurable (non ui) formatter suggestions as by crea #49.
array_filter of the formatter chosen in cck as by Alan D #53.

This will fix all issues and is minimum intrusive.

Reviewed again #51 and it's great.

However, for the second issue, cck display formatter array_filter... i'm very confused... I can no mor reproduce this. Isn't this the job of CCK to filter the options?
Adding the filter with a validate finally is very complicated. Note also that fapi of D6 doesn't support element validation. Instead you'll need to validate the form and build a lot of access code.

Any better idea to bringing in the element validator?

With the latest 6.x version i cannot reproduce the behaviour Alan D. is addressing.
It seems to me, cck possibly already fixed the redundancy issue.

There's absolutely none of the other (unchosen) imagecache present in the registry data i've seen.
Thus nor need to array_filter and nor need to investigate the issue from lightbox2 side...
I might have been missing something..?!#

I'm switching this to "needs review". Whoever can tell me how to reproduce the issue cleanly, please do. If reproduction is not possible, we should fix the issue asap with optional configurability as in #51 and done.

Tagging

Subscribing...

Subscribe

The patch #37 is great and reduced in our case the memory for the theme registry from 1.5mb to 400kb and added some important admin settings.

But the patch has bugs with the imagecache preset-names. I generated a new patch which replaces the patch #37 and resolves this bugs.
The patch #37 has no bugs! I applied the patch by mistake to an older dev version which had different code in the hook_theme() function.

Do not use this patch (#64)!

What are the bugs exactly? Can you be more specific?

See my comment above. Thanks for the question!! :-)

Weri, finally - since the UI opens some questions and limitations - i would prefer to make things go via settings like #51 suggested.
In standard case everything works without limitation.

You can then define some array structures in settings.php with exact controle. That's the best result i can think of currently.

Since in D7 everything is differently i recommend to go for that way ASAP, as discussions about the UI and (still limited) improvements to it make no sense to me.
Would you please review #51 and test the way via settings?

I'm glad I found this issue!

I noticed just what Alan D and others experienced, but probably in even grander detail. We happen to have 53 imagecache presets and 19 nodetypes, too many modules and around 70 000 nodes.. Just found out the biggest problem is the themeregistry cache. This has been described already, but I'll say it in my words:
The cache entry was unusuable by memcache (1M limit) => theme registry is recreated on every page view => massive performance problems and headbanging.

After disabling lightbox2 the themeregistry shrinked from 8.1M to 0.9M!

But we can't just disable it in production, it's being used a lot.

Thus, I guess I will try to use the patch in #49, since there are no newer actual patches.

Please try that patch out and let us know how it works. Once people have tested and verified it works as expected, we should make this RTBC and hope that it gets committed to the module soon!

I can commit a patch like this, but i do not have any real big site in drupal 6 now, so i need everybody that have this issue(which is important), test the patch, report if it works and do not break anything else, because i do not have this issue to test the patch.

I'm using #49 in production, and it's working well. The site is a lot faster. An UI would be really nice, but I can live without it. However, this issue made me decide to not use lightbox in future projects.

Anyway, I'm marking this as RTBC.

Working on site performance and found that cache_content content_type_info was >3100k! This became a problem when it wouldn't fit into memcache. After removing lightbox additions it was only 389k. We have 53 imagecache presets on this site. After looking at this issue I checked the size of this site's theme registry - 6672k before, 378k after.

Tested #49. We're going to put it into production soon. UI will be nice, but I don't want to hold up this patch. RTBC.

Haha, 10000+ theme preprocess entries. 4MB of cache data. About to apply the patch :)

#37 worked a treat. Our server load went from 7 to 2.

It's a real pity this has not been applied yet.

Also in D7, even there are amazing new features available from Drupal Core about field formatter settings, Lightbox doesn't use them. It still messes up with registering a ton of formatters... Just checked the code and it's still unmaintained.

 807 function lightbox2_field_formatter_info() {
 808   $formatters = array();
 809   if (module_exists('image')) {
 810     $image = $lightbox = image_styles();
 811     $types = array('lightbox', 'lightshow');
 812     foreach ($types as $type) {
 813       foreach ($image as $image_key => $image_value ) {
 814         $formatters['lightbox2__' . $type . '__' . $image_key . '__original'] = array(
 815           'label' => 'Lightbox2: ' . $type . ': ' . $image_key . '->original',
 816           'field types' => array('image'),
 817         );
 818         foreach ($lightbox as $lightbox_key => $lightbox_value ) {
 819           $formatters['lightbox2__' . $type . '__' . $image_key . "__" . $lightbox_key] = array(
 820             'label' => 'Lightbox2: ' . $type . ': ' . $image_key . '->' . $lightbox_key,
 821             'field types' => array('image'),
 822           );
 823         }

This should really go ALL into options! No need to cycle with foreach!

Checkout colorbox how beautiful it can be to set options...
http://drupal.org/project/colorbox
If i look at stats, most people switched to colorbox anyway. :-(

Tested #49. We put in into production. Page generation time went from 5s to 2s.

Look good

Good for me.

#49 pushed into the 6.x-1.x
Thanks