template_preprocess_responsive_image() always creates an Image object. It only needs it to ge the height and weight, which is something that should be set in almost all cases already.
In one example profile run that I did on an empty render cache, this function took 460ms for 38 calls. That is using imagemagick, so an external process, but on the other side, it was a local file system with an SSD, if the image is on a slow network file system, this could be a lot slower.
I'm also wondering what would happen if this is called on an image style file that doesn't actually exist yet, wouldn't that fail hard?
Replace ->getSource() calls with $variables['uri'], make the argument to responsive_image_build_source_attributes() optional, so that we can pass in NULL, only create it when necessary.
Comments
Comment #2
berdirThis seems to work. I needed to do something additinal as we use file_entity and we use that formatter on a file field, not an image field, so the width/height is actually not set.
Comment #3
berdirComment #4
slashrsm commentedLooks good to me.
Comment #5
catchThe change looks good, but having the deprecated optional argument as first parameter is a bit unfortunate. When we eventually remove that parameter, all existing code calling the function will break.
Could we add a new function without an $image parameter, and deprecate this entire function instead? That way core and any contrib module which might use this will be able to write forward-compatible code and doesn't have to pass NULL all the time.
In terms of code sharing they could both call an underscore prefixed helper.
Comment #6
berdirYes, that argument is a bit annoying. Also, adding a @deprecated would mark the whole function as deprecated in IDE's, that would be annoying too.
The thing is that if this would be object oriented code, like an event listener, this code would very likely be a protected method. It's specifically built for that template, to be called with the full variables array of that template. So IMHO "core and any contrib module which might use this will be able" is a rather theoretical argument as I can't imagine anyone ever will.
At the same time, it has like 140 lines of documentation, explaining with lots of examples what it does. If we add a new method, we either have to copy that or move it and add a @see on the old one (in the patch, we'd actually add the old function elsewhere I suppose). We also need to find a new function name for it.
Those are my argument for keeping it like this :)
If you would prefer an new function without the argument, I would say we just rename it and add a wrapper that silently ignores it. It would be an overhead if someone actually calls it, but again, I can't really imagine why someone would do that.
Comment #7
catchThe solution in the last paragraph is a good one. I'd suggest we call the new function _responsive_image_build_source_attributes() - this should only have been an underscore prefixed helper in the first place.
Comment #8
berdirOk, did that, also provided a patch for 8.3 as I noticed this doesn't apply to 8.3. Not 100% sure if we can commit this to 8.2 but it is a performance bugfix and deprecating the function doesn't actually harm anyone?
I also explicitly didn't mention you should be calling the now underscore-prefixed function as it is just that.. prefixed and not meant to be called. If you really want to, you can easily see it in the function.
Comment #11
berdirForgot to call the new function..
Comment #12
catchThis looks good to me now. The very minor behaviour change in case someone happened to be calling this function seems OK for a patch release to me.
Comment #14
catchCommitted 48d5f83 and pushed to 8.3.x. Thanks!
I think this should go into 8.2.x as a straight performance improvement, however not into 8.2.2 so leaving RTBC and will cherry-pick once that's out.
Comment #16
catchCommitted/pushed to 8.2.x, thanks!