Small performance improvements and code cleanup

[rolandschuetz]

[stowball]

When I put this code in jsperf, I get an error "image is undefined", so it never runs.

I took the top (easy) parts from your pull request to compare, and it was consistently slower, but my laptop's screen did timeout on the "043" test: http://jsperf.com/rwd-042-rolandschuetz

Also, I have no idea why this code is soooooooooo slow to run in jsperf. In the browser I don't (think) I see these terrible speeds. Any idea?

[stowball]

Hmmm, I think it could be eval related. I've removed that in the 043 test and the test runs so much quicker than 042 (seconds instead of minutes), but the actual end results aren't wildy different. Perhaps jsperf really hates eval

[rolandschuetz]

Hello,

version 4.3 seems to be faster then my fixes.

My fixes improved Version 4.2.

• First change clearly improved the result: http://jsperf.com/rwd-042-rolandschuetz/2#results Showing that a function outside the loop outperforms the functon-recreation inside the loop is easy: http://jsperf.com/rwdimages-function-outside-loop#results
• The second one just fixed unclean js.
• The third one improved performance a little, but mainly cleaned up the code and removed the global variable

So about Version 4.3:

• First and second fix seems to be in it
• the getComputed still shouldn't be a global

I tried to create a test of all different versions, but jsperf always crashes. Seems like a memory leak or something like that.

---

Would be great if you could incorporate the following changes into v0.4.3: http://jsperf.com/rwdimages-rolandschuetz-imagelooping#results

Cheers, Roland

[stowball]

Hey Roland

I've written a new version here: https://gist.github.com/stowball/07f6a2cc5f50403e261a.

It fixes IE8 images when using enquire.js, drops IE7 support and simplifies & improves its performance.

Here's a comparison to 0.4.2: http://jsperf.com/rwdimage-043c and here's a comparison against your performance test: http://jsperf.com/rwdimages-rolandschuetz-imagelooping/2

FYI rwdImageChangeSrc() needs to be a global because it's there for you to use if you're lazy-loading images. You add the lazy-loaded class, then call the function to change its src.

The main performance test is still a bit slow, and there could be a memory leak as you say, but I don't see these speed issues in a real project - so I'm going to ignore it for now :)

Anyway, test it and let me know what you think, because in my opinion, this is a real release candidate.

Cheers

[rolandschuetz]

Hello Matt,

looks good!

I could not find where rwdImageChangeSrc is called from outside the scope, you're not providing an API, do you?

Seems like a release candidate to me as well!

Cheers, Roland

[stowball]

Cool! :)

I use rwdChangeImgSrc in my framework's carousel to lazy-load the each slide's image as required https://github.com/izilla/Suzi/blob/master/js/site/all.js#L502

[rolandschuetz]

Ah, I see.

Maybe refactor this to a cleaner API?

I would place this at the top of the file to make it more obvious that this is an API function, and not just an internal function which leaked to the outside and may be removed at any time.

window.rwdImages = window.rwdImages || {};
/**
 * Retrieves this currently matching media query image and applies it as new image source.
 * If using Enquire.js this is done automatically and you don't need to use this function.
 */
rwdImages.updateImageSrc = function() { ... };

[stowball]

Very good point

[rolandschuetz]

A bit off topic, what was actually the reason that you chose to implement your own solution and not write a polyfill for the picture element?

[rolandschuetz]

Do you know advantages of rwdimages compared to this picture polyfill?