Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Huge performance issues! #148

Open
baptistebriel opened this issue Jan 20, 2016 · 4 comments
Open

Huge performance issues! #148

baptistebriel opened this issue Jan 20, 2016 · 4 comments

Comments

@baptistebriel
Copy link

Hello,

I've been working with tether and tether-select, and decided to made a quick performance test using Chrome's timeline tab..

Looks like there's a lot of painful operations going on; I made a test with ~ 20 custom <select> within the page, and every frames takes ~ 65ms to render. This is way too huge!

Every frame should be ~ 16ms.

screen shot 2016-01-20 at 14 32 02

We can see here that for every <select> element on the page, the functions are being called:

  • position
  • cache
  • getScrollBarSize

Theses functions cost a lot because of;

position (line 1003):
if (document.body.scrollHeight > window.innerHeight) {
getScrollBarSize (line 172):
var widthScroll = inner.offsetWidth;

screen shot 2016-01-20 at 14 41 11

screen shot 2016-01-20 at 14 41 28

Kinda related to this issue from tether-select: HubSpot/select#43

I really think this has to be fixed. Just to be sure, I do not have any other onscrollevents; everything on the screenshots come from tether.js.

@adamschwartz
Copy link
Contributor

Thanks for such a thorough look. We’ll definitely be looking into this. cc @zackbloom

@yairEO
Copy link

yairEO commented Feb 6, 2016

Sadly this isn't uncommon when there are developers lacking sufficient knowledge in browsers and performance. yes, Tether is awesome and I love it, and been using it for a long time, but.. the code is an absolute horrible mess :(

A quick solution would be using fastDOM everywhere in the code (this is have huge benefits, but is a very tedious thing to incorporate since there are tons of lines of code which touch the DOM without apparent order),

@baptistebriel
Copy link
Author

First of all I think it would be nice to cache the scrollbar size widthScroll variable somewhere else, so there's no look-up for the offsetWidth on every scroll event fired.

I guess this would also work for the position detection: cache the document.body.scrollHeight > window.innerHeight test somewhere and call it again only on resize.

@TrevorBurnham
Copy link
Contributor

@baptistebriel I would welcome a PR to make Tether more efficient. Can the result of getScrollBarSize be cached without introducing positioning bugs? I see that this value currently is nominally cached, but clearCache is called at the top of position, which is the 'scroll' event handler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants