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 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.
Comment | File | Size | Author |
---|---|---|---|
#64 | misc_images-908282-64.patch | 4.87 KB | pillarsdotnet |
#61 | theme_image-remove_unnecessry_io-908282-61.patch | 11.85 KB | pillarsdotnet |
#60 | theme_image-remove_unnecessry_io-908282-60.patch | 12.76 KB | pillarsdotnet |
#42 | hardcode-image-dimensions.patch | 6.43 KB | jbrown |
#32 | hardcode-image-dimensions.patch | 7.33 KB | jbrown |
Comments
Comment #2
jbrown CreditAttribution: jbrown commentedDamn! This passes on my machine.
Comment #3
jbrown CreditAttribution: jbrown commentedtheme-image-no-io.patch queued for re-testing.
Comment #4
jbrown CreditAttribution: jbrown commentedComment #5
Damien Tournoud CreditAttribution: Damien Tournoud commentedAgreed 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.
Comment #6
jbrown CreditAttribution: jbrown commentedComment #7
moshe weitzman CreditAttribution: moshe weitzman commented+1 from me
Comment #8
jbrown CreditAttribution: jbrown commentedAnyone want to RTBC this?
Comment #9
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is one step in the good direction :)
Comment #10
jbrown CreditAttribution: jbrown commented#6: theme-image-no-io.patch queued for re-testing.
Comment #11
Dries CreditAttribution: Dries commentedGood idea. Committed to CVS HEAD. Thanks.
Comment #12
David_Rothstein CreditAttribution: David_Rothstein commentedThis 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.
Comment #13
ksenzeeHaving 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.
Comment #14
Jeff Burnz CreditAttribution: Jeff Burnz commentedDoes 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...
Comment #15
Damien Tournoud CreditAttribution: Damien Tournoud commentedThey never were in Drupal 7, neither for plain images nor for image derivatives.
getimagesize()
never worked for remote files andgetsize
was explicitly set to FALSE intheme_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.
Comment #16
jbrown CreditAttribution: jbrown commentedTotally 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.
Comment #17
Jeff Burnz CreditAttribution: Jeff Burnz commentedOK thanks jbrown and Damian, would it be best to make this a feature request for D8?
Comment #18
jbrown CreditAttribution: jbrown commentedI'm thinking 7.1
Comment #19
sunum, wow.
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.
Comment #20
Damien Tournoud CreditAttribution: Damien Tournoud commentedAgain, 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.
Comment #21
sunDid 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.
Comment #22
David_Rothstein CreditAttribution: David_Rothstein commentedWell, 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.
Comment #23
JacineI 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.
Comment #24
jbrown CreditAttribution: jbrown commented@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:
The first two are too late for 7.0
Comment #25
ksenzeeThis 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.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedOne 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.
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedHere'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.)
Comment #28
alippai CreditAttribution: alippai commentedWhy don't we cache width and height as we do at 'files'? We store filesize and filemime for the same reasons, aren't we?
Comment #29
jbrown CreditAttribution: jbrown commentedI'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 .
Comment #30
grendzy CreditAttribution: grendzy commentedI 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.
Comment #31
Damien Tournoud CreditAttribution: Damien Tournoud commentedLet'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.
Comment #32
jbrown CreditAttribution: jbrown commentedOkay - 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.
Comment #33
Damien Tournoud CreditAttribution: Damien Tournoud commentedWorks for me.
Comment #34
webchickSorry, 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.
Comment #35
catch#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.
Comment #36
webchickOk, well in any case, it sounds like this is needs work.
Comment #37
mattyoung CreditAttribution: mattyoung commented>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.
Comment #38
jbrown CreditAttribution: jbrown commentedIts very important that 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:
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.
Comment #39
jbrown CreditAttribution: jbrown commented#32: hardcode-image-dimensions.patch queued for re-testing.
Comment #41
Jeff Burnz CreditAttribution: Jeff Burnz commentedTotally 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.
Comment #42
jbrown CreditAttribution: jbrown commentedReroll of #32.
Images from #32 also need committed.
Comment #43
webchickFor 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.
Comment #44
jbrown CreditAttribution: jbrown commentedOkay - 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()
Comment #45
jbrown CreditAttribution: jbrown commented#42: hardcode-image-dimensions.patch queued for re-testing.
Comment #46
MustangGB CreditAttribution: MustangGB commentedSo #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
Comment #47
JacineWhat 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.
Comment #48
bfroehle CreditAttribution: bfroehle commentedSee also #1015150: Clean-up 'getsize' remnants since it has been removed. which aims to remove all mentions of 'getsize' from D7 core.
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedShouldn'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.
Comment #50
Eric_A CreditAttribution: Eric_A commentedThe 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
I would be happy to do this in a new issue a few days from now.
@Jacine: would this work for you?
Comment #51
Eric_A CreditAttribution: Eric_A commentedOf 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...
Comment #52
Eric_A CreditAttribution: Eric_A commentedIn 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.)
Comment #53
David_Rothstein CreditAttribution: David_Rothstein commentedI 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.
Comment #54
Eric_A CreditAttribution: Eric_A commentedI 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.
Comment #55
David_Rothstein CreditAttribution: David_Rothstein commentedIf 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.
Comment #56
mikeytown2 CreditAttribution: mikeytown2 commentedFYI I just created a module for D6 that accomplishes this same goal but for theme_imagecache.
http://drupal.org/project/imageinfo_cache
Comment #57
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #58
catchComment #59
Dries CreditAttribution: Dries commentedI committed #42 to 8.x. To me that seems like the right thing to do. Moving to 7.x.
Comment #60
pillarsdotnet CreditAttribution: pillarsdotnet commentedAdjusted title and tags accordingly, and re-rolled according to current patch standards.
Comment #61
pillarsdotnet CreditAttribution: pillarsdotnet commentedSorry; didn't mean to include my local patch to mail.inc. Trying again:
Comment #62
jbrown CreditAttribution: jbrown commented#1129642: Populate HTML image tags with dimension attributes (like D6 imagefield) without re-introducing I/O
Comment #63
catchDoesn't look like the images from #32 were committed to 8.x.
Comment #64
pillarsdotnet CreditAttribution: pillarsdotnet commentedBinary patch for 8.x
Comment #65
jbrown CreditAttribution: jbrown commentedPatch in #64 is correct.
Comment #66
Dries CreditAttribution: Dries commentedCommitted to 8.x -- http://drupalcode.org/project/drupal.git/commit/aeb7b43 seems to list the new files now.
Comment #67
pillarsdotnet CreditAttribution: pillarsdotnet commentedPatch in #61 still applies cleanly and is RTBC for 7.x.
Comment #68
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #69
webchickSo 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()?
Comment #70
webchickNot sure why that tag was reverted.
Comment #71
Eric_A CreditAttribution: Eric_A commented@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.
Comment #72
webchickGrumble, 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.
Comment #74
dropbydrop CreditAttribution: dropbydrop commentedwhen drupal 7 will print image width and height at html alt attributes?