wnr / element-resize-detector

Optimized cross-browser resize listener for elements.
MIT License
1.32k stars 118 forks source link

[Needs verification] scroll strategy performs bad in FireFox and IE relative to other implementations #60

Closed KingSora closed 7 years ago

KingSora commented 8 years ago

Hello!

I've tested your plugin and there is one Problem. Actually the "scroll" strategy is just faster in case of loading performance. The "object" strategy is much faster in runtime.

I've created a fiddle: https://jsfiddle.net/930mvaux/4/

The first element has the object strategy and the second has the scroll strategy. If you will start resizing both elements, you will notice how mich faster the object strategy actually is.

Maybe you have some improvements.

Best regards, Sora

eliseumds commented 8 years ago

Hi @KingSora. Have you profiled it? I can't notice any performance difference between both methods by simply resizing the elements on latest Chrome (v53) for Mac.

KingSora commented 8 years ago

I've tested it on the latest Firefox. Chrome seems to be fine on windows too.

KingSora commented 8 years ago

Well, I've found a plugin which is using the "scroll strategy" too, but it has way more performance than yours (testet in Firefox 49.0.1).

Your Plugin: https://jsfiddle.net/930mvaux/6/ Other Plugin: https://jsfiddle.net/930mvaux/5/

Maybe you can figure out what is causing the performance issues in Firefox. (I think its the fact, that youre resizing the "expand" div every time the div is resized)

Best regards.

wnr commented 8 years ago

Hi and thanks for the detailed bug report, it's much appreciated :)

I'll look into it. I think expanding the div is necessary in order to handle render-tree detaching (e.g., display:none correctly). It's great that you have found a similar approach so that I can have a look at it.

I'll investigate this further, and keep you updated on the progress.

wnr commented 8 years ago

Hi again.

I've been profiling the samples you provided in FireFox 47, but when I benchmark the two scroll approach implementations I get roughly the same result: ~30ms for a resize.

Could you describe more in depth how you perform the benchmarks?

Regarding resizing the expand div, it's actually mandatory to do so in order for the algorithm to work. I remembered it wrong when I said it was due to handling display:none before. For the record, the other implementation also resizes the divs on resizes https://github.com/ShimShamSam/ResizeListener/blob/master/ResizeListener.js#L211

KingSora commented 8 years ago

I am just resizing the blue div with the mouse really fast in all directions. You will notice the lags in your plugin (FPS drops to 1-2 in total). When you test it the other plugin the FPS drops too, but not as much as they drop in your Plugin. Maybe I am able to record this.

wnr commented 8 years ago

Did you manage to record it? Could you also try removing the logic in the onResize callback, so that it doesn't access the offsetWidth/offsetHeight and see if the problem still exists?

thedavidmeister commented 7 years ago

FYI The article linked from the readme (which i assume means it is reasonably reliable 🙂) has a performance chart in section 5.1 that shows object approach is faster at runtime across all browsers tested:

https://arxiv.org/pdf/1511.01223v1.pdf

Indeed in FF the difference is the largest at about 2.3x according to the article:

screen shot 2017-01-22 at 10 35 38 pm

Keep in mind that the article is advocating responding to element dimensions for many (hundreds) of elements being dynamically modified (potentially injected/removed in an SPA setting) and resized. i.e. they're proposing using this as a significant or complete replacement of global media queries, not just an ad-hoc technique for tweaks here and there (which might change your mind about whether the tradeoff of object vs. scroll is a good one).

From the article (referring to using objects):

This approach has good browser compatability and excellent resize detection performance, but imposes severe performance impacts during injection since object elements use a significant amount of memory as shown in Section 5.1.

The problem with the object method wasn't the runtime performance (which they said was excellent) but that there were two deal breakers for their vision of dimension-aware-elements-everywhere:

screen shot 2017-01-22 at 10 41 12 pm
wnr commented 7 years ago

@thedavidmeister Hi, and thank you for your comment. Yes, I am aware of that the object approach performs better at detection. However, only slightly so if you consider few elements (when I do manual testing in FF 50 I get ~30ms vs ~20ms). My reasoning has been that the normal case for resize detection is that the elements doesn't usually resize and therefore the installation time is more important. If you do have a case where you know that there are few elements that will resize often, perhaps it is more suitable to use the object approach (which is why I allow to choose both with the "strategy" option). I know that the object approach is marked as deprecated, but it is still supported and works just fine. If there are real use cases for it, I should perhaps unmark it as deprecated.

@KingSora I think this library seems to perform worse in your fiddle due to your script accessing element.offsetWidth/offsetHeight in the callback. If you comment that out I think it will perform as good as the other lib you linked to. The other lib is actually sending some additional resize-data with the callback, which I think is a good solution to this. If you could verify this theory, I would happily implement this in this library as well :)

thedavidmeister commented 7 years ago

@wnr yeah i tend to agree that installation time is more important. IMO it could be clearer in the readme, etc. about what the 37x faster claim refers to specifically and yeah, i don't think the object method should really be deprecated as it is a perfectly reasonable tradeoff to want to make provided you realise what the tradeoff is.

elements can resize very often in a SPA setting where page layouts are highly dynamic, I don't think you can assume that the calculations are being triggered rarely.

KingSora commented 7 years ago

It seems like the problem was fixed. My Firefox has now the version 52.0 and I cant reproduce the bug anymore.

May someone has a older FF version (between 45 and 52) to check in which version this behavior was fixed?

wnr commented 7 years ago

Yeah, it seems to have been fixed. I'll close this. Thanks for the report 👍

fengyun2 commented 6 years ago

There are still performance issues under ie11, you can see the window keeps shaking.