theme_image() and theme_image_style() access the filesystem. This is really terrible for performance.

Every time theme_image() is called, by default it will check that the file to be served actually exists and, if so, goes on to read the file to determine the width and height attributes for the Only local images are allowed. tag.

This is incredibly bad for performance and is something that theme_image() should not be capable of doing. If the filesystem is mounted over the network or the file is in the cloud then it will be immensely worse.

If the code calling theme_image() isn't smart enough to know the dimensions of the image, then it should be left up to the web browser to determine.

Many functions in core set $variables['getsize'] to FALSE to prevent this from happening, but it should never happen at all. Its a really bad practice.

theme_image_style() always checks if the file exists - its not necessary.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, theme-image-no-io.patch, failed testing.

jbrown’s picture

Damn! This passes on my machine.

jbrown’s picture

Status: Needs work » Needs review

theme-image-no-io.patch queued for re-testing.

jbrown’s picture

FileSize
12.39 KB
Damien Tournoud’s picture

Agreed for theme_image().

Please remove the hunk about theme_image_style() it is currently necessary, part of a bad design that we need to change.

jbrown’s picture

Title: Image theming functions perform I/O » theme_image() performs I/O
FileSize
11.22 KB
moshe weitzman’s picture

+1 from me

jbrown’s picture

Anyone want to RTBC this?

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

This is one step in the good direction :)

jbrown’s picture

#6: theme-image-no-io.patch queued for re-testing.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Good idea. Committed to CVS HEAD. Thanks.

David_Rothstein’s picture

Status: Fixed » Needs work
Issue tags: +API change

This is an API change, and the immediate effect of it right now is that all <img> tags produced by Drupal core no longer have width and height attributes.

That's a bad idea. (It can lead to odd behavior in browsers, and in JavaScript.)

Why do we need to change this default behavior so late in the cycle? For one thing, if you have a site or module where this is actually a serious performance problem, you can implement preprocess logic to always force 'getsize' to FALSE, or you can reimplement the theme function entirely. I don't see any reason to silently change this for everyone.

ksenzee’s picture

Having width and height attributes on <img> tags is indeed a Good Thing™. Don't you find it annoying when you start reading an article before the page has finished loading, only to have the text you're reading jump down on the page when some big header image finally loads? That's a pretty big perceived performance issue. In fact, if you consider perceived performance, this is likely to be a net performance loss for the great majority of sites.

I'd argue in favor of rolling this back. As David points out, sites with network filesystems where this causes actual problems can easily fix those problems.

Jeff Burnz’s picture

Does this mean that all images add via image fields no longer have the width and height available?

Google thinks this is important enough to put in their client side performance docs:

http://code.google.com/speed/page-speed/docs/rendering.html#SpecifyImage...

Damien Tournoud’s picture

Status: Needs work » Closed (fixed)

Does this mean that all images add via image fields no longer have the width and height available?

They never were in Drupal 7, neither for plain images nor for image derivatives. getimagesize() never worked for remote files and getsize was explicitly set to FALSE in theme_image_style().

The image toolkit and theme_image_style() needs to change, and we could then restore the "output width and height as soon as the image derivative has been generated", but that's unrelated to this issue.

jbrown’s picture

Totally agree with Damien.

Even for local files, reading a file every time you want to serve it is really slow.

Yes - it is obviously better for lots of reasons to have width and height specified in the tag, but not this is not the way to do it.

The image styles system can be improved to populate these attributes without the massive performance problems.

Jeff Burnz’s picture

OK thanks jbrown and Damian, would it be best to make this a feature request for D8?

jbrown’s picture

I'm thinking 7.1

sun’s picture

Status: Closed (fixed) » Needs work

um, wow.

Yes - it is obviously better for lots of reasons to have width and height specified in the tag, but not this is not the way to do it.

Then it is the responsibility of this patch to to fix width and height accordingly for all images throughout Drupal core.

This is definitely not how we do API changes at this stage of the release cycle.

Damien Tournoud’s picture

Status: Needs work » Closed (fixed)

Then it is the responsibility of this patch to to fix width and height accordingly for all images throughout Drupal core.

Again, this patch doesn't change anything in the most common use case, ie. images output by image fields. As I said, getimagesize() doesn't support remote files and theme_image_style() also had an explicit getsize => FALSE.

So point me toward what is to fix. As far as I know, nothing.

sun’s picture

Status: Closed (fixed) » Needs work

Did you look at your site's logo? No width, no height. So the first and foremost image on *every* site is causing a slowdown in rendering and/or odd page jumping effects now (depends on the browser being used).

How can the most common use-case of theme_image() be image fields? Especially if, as you say, image fields don't use it in the first place?

The only code that has been changed accordingly by this patch was code that purposively set 'get_size' to FALSE already. All other code was not updated.

David_Rothstein’s picture

Well, we could update lots of code in core (and contrib) to explicitly call getimagesize() first, but then the end result would be an API change for no reason at all :)... Perhaps there can be a way to have the best of both worlds (calculate the width/height once and store it in the file_managed table, something like that)? But that really sounds like a Drupal 8 thing.

For Drupal 7, I think we should really just roll this back. It makes sense to have the default behavior here be optimized for front-end (not server) performance, and there are many ways to override that default if you need to.

Jacine’s picture

Issue tags: +Needs documentation

I agree this should be rolled back for D7 unless a "best of both worlds" solution arises.

If it isn't going to be rolled back, it definitely needs to be documented on the update pages for both themes and modules.

jbrown’s picture

@sun garland and bartik don't use theme_image() to render the site logo, so that is unaffected by this patch.

Getting the dimensions out of the file every time it needs to be in an img tag is a textbook example of what not to do. Its wrong wrong wrong. Drupal is supposed to be suitable for the enterprise.

The only places we were using the get_size functionality was for icons - tablesort, feed, update report and aggregator. And these image sizes should be hard coded in their theme functions! No other code (EDIT: in core) used it - presumably because it was so damn slow.

Arguably any user supplied image should be rendered through an image style and theme_image_style() should be populating the dimension attributes automatically.

I say we leave this is as it is.

I have three feature requests:

  1. site logos should be rendered as an image style
  2. theme_image_style() should be outputting dimensions automatically
  3. theme functions that output shipped images should have the dimensions hardcoded

The first two are too late for 7.0

ksenzee’s picture

This change does not make Drupal any better suited for the enterprise. People developing high-performance sites are perfectly able to override the default behavior along with the host of other changes they routinely make to Drupal's defaults in such environments.

David_Rothstein’s picture

One option would be to roll back the patch, but also do @jbrown's third suggestion. I agree it is pretty ridiculous that we are calling getimagesize() on files that are shipped as part of the codebase and whose dimensions we already know...

And then leave anything else for Drupal 8.

David_Rothstein’s picture

Status: Needs work » Needs review
FileSize
11.79 KB

Here's a straight rollback, untested.

(It would be easier to review if we do this one first, and then add the image dimensions for files shipped in the codebase as a separate followup patch.)

alippai’s picture

Why don't we cache width and height as we do at 'files'? We store filesize and filemime for the same reasons, aren't we?

jbrown’s picture

Status: Needs review » Needs work

I'm preparing the patch that will hard code the dimensions of images shipped with core into their theme() variables. I'm travelling at the moment, so I wont have this posted for a few days, but its a really simple patch. The only complication is that the watchdog-* icons aren't all the same size! Most shipped images in core are displayed using CSS.

Once this is committed, the getsize functionality wouldn't be used _anywhere_ in core if my previous patch were rolled back. It would be bad practice for a contrib module to use the getsize functionality because of its performance problems.

I see no reason to revert the patch.

Drupal should be auto-populating the dimensions for non-shipped images, but implementing this is in a performant manner is non-trivial, out of scope for this issue and surely too late for D7.0 .

grendzy’s picture

Status: Needs work » Reviewed & tested by the community

I would miss the auto-size feature of theme_image, despite having been bitten a few times by the I/O performance, and also being occasionally bothered that I have to explicitly set getsize=>false for external URLs.

Despite it's warts, this is just reckless! It's a needless API change, and we're so far beyond code freeze it's not even funny.

+1 for rollback, please.

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs review

Let's give jbrown some time to publish a new page.

As explained in #24, this was only used for minor UI related elements. There is no rush in reverting this.

jbrown’s picture

Status: Needs work » Needs review
FileSize
318 bytes
375 bytes
780 bytes
7.33 KB

Okay - I created the patch on the plane to New York. Git is fantastic!

The attached watchdog-*.png files are all 18x18. These need to replace the existing ones in misc, as two of them are 17x17. I have also shrunk the file size down as much as possible.

I'll document the API change.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Works for me.

webchick’s picture

Sorry, but IMO that approach is awful. That requires me as a module developer to go and measure all of my icons and images precisely to know what size they are in order to include them (and in a lot of cases, have to call imagegetsize() myself). We never imposed this type of headache on module developers before, I have no desire to start imposing it now, after code freeze, and after beta.

I'm not quite sure why this patch was committed in the first place, especially at the time that it was, since enterprise sites are perfectly capable of simply overriding theme_image() to do whatever they want inside, no?

I'm leaving as RTBC for Dries to take a look at, since he committed the original patch. But IMO we need to roll this back. See #12 and beyond.

catch’s picture

#32 and #27 combined would remove the API change, but also remove the stupid I/O issue from core (and that's really not just 'enterprise' sites - any site with i/o issues is only going to have worse ones due to this).

#27 but with getimagesize() defaulting to FALSE is also an option, but one which doesn't seem to have been discussed.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Ok, well in any case, it sounds like this is needs work.

mattyoung’s picture

>since enterprise sites are perfectly capable of simply overriding theme_image() to do whatever they want inside, no?

How is this possible if by overriding is to disable file access to do auto size calc. This will break core and everything else that expects size to be auto calculated.

jbrown’s picture

Status: Needs review » Needs work
Issue tags: +Needs documentation, +API change

Its very important that Only local images are allowed. tags have the width and height tags specified, but it is also very important that we don't read a file _every_ time it needs to be served as the performance hit of doing so is catastrophic. This isn't just related to enterprise sites. Core barely used the getsize functionality at all - only for some shipped icons (and these should have their dimensions hardcoded - #32).

There are two types of image files:

  • static shipped - the dimensions should be specified either in css or img tag (theme_image() variables). Blindly putting a static image on the page without knowing exactly what size it is is a really bad practice. It needs to match up with nearby / parent elements.
  • user / admin submitted and shipped defaults (e.g. site logo, image field) - these should always be rendered through an image style and that system should determine the dimensions automatically. It wasn't utilising the getsize functionality because it was so slow.

The image styles system can be improved in a later release of 7 to automatically populate the dimensions variables without performing I/O every time.

In D8, site logos should be passed through an image style and therefore have dimensions in img tags for the first time ever.

For reasons specified above, it's bad practice for contrib modules to use the getsize functionality and for that reason I don't advise it being rolled back, but I respect whatever Dries chooses to do. If it is rolled back then getsize should default to FALSE. getsize really is disgusting.

Patch in #32 should be applied regardless.

jbrown’s picture

Status: Needs work » Needs review
Issue tags: -Needs documentation, -API change

#32: hardcode-image-dimensions.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, hardcode-image-dimensions.patch, failed testing.

Jeff Burnz’s picture

Totally another issue but these watchdog icons should be removed and the protocons used instead, which are all either 24x24 or 16x16, there are two sets in /misc.

jbrown’s picture

Status: Needs work » Needs review
FileSize
6.43 KB

Reroll of #32.

Images from #32 also need committed.

webchick’s picture

Priority: Normal » Major

For evidence of developers running aground of the WTF nature of this change, see #1012416: Image styles don't add width/height attributes.

Right now we have a getsize param that does nothing, and core isn't printing height/width attributes on any images which slows down front-end rendering. Escalating to major.

jbrown’s picture

Okay - I'm looking back on this fresh.

I stand by my comments in #38.

What needs to happen is that theme_image_formatter() determines the dimensions automatically.

The image field should store source dimensions in the field_data table. It can then be determined based on the style configuration what the dimensions are - no I/O is required at all. Unlike getimagesize() this would also work for remote stream wrappers and would also work for images that are being generated for the first time.

This should be implemented for a later release of D7 as it is very easy to do.

In the mean time, I would recommend having getimagesize() hard coded in theme_image_formatter() for local stream wrappers.

There should be no getsize parameter for theme_image_style()

jbrown’s picture

#42: hardcode-image-dimensions.patch queued for re-testing.

MustangGB’s picture

So #44 is essentially saying calculate width/height on image upload, store these in database, then use database values when rendering?
If so, then +1 to this approach
This is definitely needed in D7

Jacine’s picture

What about cases where there is no chance for database storage? This patch pretty much rendered theme_image() useless in the theme layer or anywhere else that for people that care about improving front end performance. It also makes adding height and width attributes to something like a site logo in a contrib theme impossible, since we will not know what the size is. Are we supposed to work getsize in manually? Sheesh.

If I have to hard code pixel widths of images, why bother using the theme function in the first place. I really don't understand why this needs to be imposed on everyone. If a module developer wants to make this sort of optimization he/she do so by overring the theme function instead of ruining it for everyone else.

bfroehle’s picture

See also #1015150: Clean-up 'getsize' remnants since it has been removed. which aims to remove all mentions of 'getsize' from D7 core.

David_Rothstein’s picture

Shouldn't we still do what we discussed a few months ago, i.e. revert this patch? (Or actually just add back the 'getsize' parameter; the rest is OK.) Removing 'getsize' from core needed way more discussion, and was too late for Drupal 7 at the time it was committed.

The other stuff is important too but is kind of a separate issue, or at least a followup issue once the behavior of theme_image() itself is resolved.

Eric_A’s picture

Are we supposed to work getsize in manually? Sheesh.

If I have to hard code pixel widths of images, why bother using the theme function in the first place.

The getimagesize() code would sit well in a render element type. Then we can have both a lightweight theme layer counterpart and an easy way to have special image functionality. You would do something like

$image = array(
  '#type' => 'get_image_size',
  '#path' => 'path',
  '#title' => 'title',
  '#attributes' => array('class' => array('class1')),
);
print drupal_render($image);

I would be happy to do this in a new issue a few days from now.
@Jacine: would this work for you?

Eric_A’s picture

Of course it is not just about moving code to a better place.
The big idea is that render elements have caching these days. If we could put that to work for our case, we would lighten some of the performance implications mentioned before.

EDIT:
Maybe a #pre_render is too early for doing such resources eating work... If theme_image() is going to stay as lightweight as it is now, then maybe we could add theme_image__get_size() with a preprocessor that tries to determine width and height if not set...

Eric_A’s picture

Status: Needs review » Reviewed & tested by the community

In September Damian Tournoud RTBC'ed adding width and height attributes for shipped images, and it looks like there is consensus here that maintaining hard coded width and height attributes for shipped *core* images is a good idea.

Committing this will improve Drupal without interfering with:
1) the discussion on theme_image_formatter(), theme_image() and theme_image_style();
2) the discussion about reverting #6. I think that the release of Drupal 7.0 now *really* makes it impossible to revert #6, but in any case committing #42 does not interfere.
The patch in #42 assumes all status images in the watchdog table to be 18x18, implying the replacement of the two 17x17 images. (New images attached in #32) Replacing watchdog images seems doable after 7.0. (If not, then #42 needs to be rerolled to account for the current 17x17 images.)

David_Rothstein’s picture

I don't see how a partial revert that adds back the 'getsize' parameter would break any code, so it seems on the table for D7 to me.

I haven't thought through all the details though.

Eric_A’s picture

I don't see how a partial revert that adds back the 'getsize' parameter would break any code, so it seems on the table for D7 to me.

Well, adding it back with the original default value of TRUE is not on the the table anymore, because for one: it breaks existing optimizations in contrib, where values for $width and $height are being passed to theme_image(), but not FALSE for $getsize because it simply does not exist here.

David_Rothstein’s picture

If the caller has set width or height, 'getsize' would be ignored - there is no reason to try to determine the image size automatically if they have already asserted that they know what they want it to be. So it would not break the code in those examples.

mikeytown2’s picture

FYI I just created a module for D6 that accomplishes this same goal but for theme_imagecache.
http://drupal.org/project/imageinfo_cache

pillarsdotnet’s picture

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
Dries’s picture

Version: 8.x-dev » 7.x-dev

I committed #42 to 8.x. To me that seems like the right thing to do. Moving to 7.x.

pillarsdotnet’s picture

Title: theme_image() performs I/O » Remove unnecessary I/O from theme_image()
Issue tags: -Needs backport to D7 +backport
FileSize
12.76 KB

Adjusted title and tags accordingly, and re-rolled according to current patch standards.

pillarsdotnet’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.85 KB

Sorry; didn't mean to include my local patch to mail.inc. Trying again:

jbrown’s picture

catch’s picture

Version: 7.x-dev » 8.x-dev
Priority: Minor » Normal
Status: Needs review » Reviewed & tested by the community

Doesn't look like the images from #32 were committed to 8.x.

pillarsdotnet’s picture

Binary patch for 8.x

jbrown’s picture

Patch in #64 is correct.

Dries’s picture

Version: 8.x-dev » 7.x-dev

Committed to 8.x -- http://drupalcode.org/project/drupal.git/commit/aeb7b43 seems to list the new files now.

pillarsdotnet’s picture

Patch in #61 still applies cleanly and is RTBC for 7.x.

pillarsdotnet’s picture

webchick’s picture

So just so I'm clear on what I'm committing here, this doesn't do anything about reverting the controversial behaviour of no longer auto-calculating heights/widths as theme_image() did in D6 and below, but instead merely hard-codes the heights/widths of some of the default core images. So it is still the developers' responsibility to manually calculate their heights/widths before calling theme_image()?

webchick’s picture

Not sure why that tag was reverted.

Eric_A’s picture

@webchick. That's correct. The patch also changes 3 watchdog image resources. (both width and height 1 pixel and smushed).

jbrown is adding the auto-calculating at the toolkit and field level in #1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs backport to D7

Grumble, grumble. Ok, thanks.

Committed this to 7.x as a stop-gap then, and to get D7 in sync with D8. Will see everyone over in #1129642.

Status: Fixed » Closed (fixed)

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

dropbydrop’s picture

when drupal 7 will print image width and height at html alt attributes?