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

Attributes: addСlass and removeСlass performance improvements #1632

Closed
wants to merge 1 commit into from
Closed

Attributes: addСlass and removeСlass performance improvements #1632

wants to merge 1 commit into from

Conversation

romamatusevich
Copy link

It would be better for performance to cache element.className in variable: http://jsperf.com/cache-elem-classname-vs-non-cache-elem-classname

@dmethvin
Copy link
Member

Is this a meaningful difference, especially considering most classname changes involve applying styles at minimum and a reflow at worst?

@romamatusevich
Copy link
Author

Yes, it`is not a huge increase of performance, but for example jsperf testcase for IE 10 on my pc has 20% profit. Not bad, I think.

@thinkingmedia
Copy link

@dmethvin the code has elem = this[ i ]; so why not also elemClassName = elem.className;. The two are basically done for the same reason.

But if you're looking for performance to fix. This part of the code is the real bottleneck.

            while ( (clazz = classes[j++]) ) {
                if ( cur.indexOf( " " + clazz + " " ) < 0 ) {
                    cur += clazz + " ";
                }
            }

Is there a faster alternative than this?

@thinkingmedia
Copy link

This would help when elements don't have any classes already.

        finalValue = value;
        if(cur != " ")
        {
            while ( (clazz = classes[j++]) ) {
                if ( cur.indexOf( " " + clazz + " " ) < 0 ) {
                    cur += clazz + " ";
                }
            }
            finalValue = jQuery.trim( cur );
        }

@romamatusevich
Copy link
Author

As for the while loop, just using the for instead shows good results, especially for FireFox http://jsperf.com/addclass-for-vs-while

I can add that improvement too.

@dmethvin
Copy link
Member

Nonononono. These kind of perf "improvements" are off in the weeds. Take a look at mathiasbynens/jsperf.com#158 for a bunch of links, and the videos in particular are good. Again, in the context of what happens after the class changes, these are tiny little trivial differences even if it turns out they are real--and even that is in doubt.

The best way to fix perf issues is to find places in real web pages and apps that are bottlenecked and see if jQuery is the cause. Then see what can be done to fix that. Nobody has come to us saying addClass is killing their page's performance, and if it were it would likely be to reapplying styles and reflows.

We do have a bunch of things that you could help out with, see the bug tracker for what we are working on with 1.12/2.2. Some of them are relatively simple changes and would be great starter projects if you're interested in getting your feet wet.

@romamatusevich
Copy link
Author

Ok, position is clear. Thanks for the clarification.

@hamishdickson
Copy link

Hi Dave,

Your tiny link is broken (I get a 404 anyway) - for reference do you mean
this link? http://bugs.jquery.com/milestone/1.12/2.2

I'm looking for something to help out with, so it would be good to know

On Wed, Jul 30, 2014 at 7:21 PM, Dave Methvin notifications@github.com
wrote:

Nonononono. These kind of perf "improvements" are off in the weeds. Take a
look at mathiasbynens/jsperf.com#158
mathiasbynens/jsperf.com#158 for a bunch of
links, and the videos in particular are good. Again, in the context of what
happens after the class changes, these are tiny little trivial differences
even if it turns out they are real--and even that is in doubt.

The best way to fix perf issues is to find places in real web pages and
apps that are bottlenecked and see if jQuery is the cause. Then see what
can be done to fix that. Nobody has come to us saying addClass is killing
their page's performance, and if it were it would likely be to reapplying
styles and reflows.

We do have a bunch of things that you could help out with, see
http://goo.gl/NsAfgV/ for what we are working on with 1.12/2.2. Some of
them are relatively simple changes and would be great starter projects if
you're interested in getting your feet wet.


Reply to this email directly or view it on GitHub
#1632 (comment).

@dmethvin
Copy link
Member

dmethvin commented Aug 1, 2014

Yes, that link has most of the tickets we're working on for the next release. If you see one you'd like to do, comment in the ticket. Perhaps http://bugs.jquery.com/ticket/15073 ?

@hamishdickson
Copy link

Great - will do, thank you. (I've actually just created a jsFiddle for that issue so I'll stop spamming this one)

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants