Bug 98982 - Setting className attribute causes slow performance
Summary: Setting className attribute causes slow performance
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-10 20:06 PDT by Brad Vogel
Modified: 2022-07-30 13:39 PDT (History)
12 users (show)

See Also:


Attachments
The test case (574 bytes, text/html)
2012-10-10 20:07 PDT, Brad Vogel
no flags Details
WIP (6.81 KB, patch)
2012-10-17 20:41 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
New test case with className += ' ' (797 bytes, text/html)
2013-04-05 14:36 PDT, Brad Vogel
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brad Vogel 2012-10-10 20:06:59 PDT
UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/536.26.14 (KHTML, like Gecko) Version/6.0.1 Safari/536.26.14

Steps to reproduce the problem:
1. Run the attached test case.
2. View the console.

In the console you'll see:
with setting className: 342.205ms
without setting className: 1.680ms

What went wrong?
Setting the className to it same value should be a no-op. However, webkit sets the node as dirty even though the className is the same. It would be a nice performance enhancement if webkit compared the new value to the old value before setting the dirty flag to re-render the node.
Comment 1 Brad Vogel 2012-10-10 20:07:37 PDT
Created attachment 168131 [details]
The test case
Comment 2 Alexey Proskuryakov 2012-10-11 13:32:07 PDT
Do you expect this pattern to be used on actual web sites? If this never happens, then even the tiny overhead of checking whether className didn't change would be detrimental.
Comment 3 Ryosuke Niwa 2012-10-15 08:39:38 PDT
My expectation is that things like this happen a lot in a large Web where there are multiple layers of abstractions. Having said that, it seems like this can be easily resolved in script side as well. My gut tells me we should implement this optimization just like we for setAttribute.
Comment 4 Takashi Sakamoto 2012-10-17 20:41:48 PDT
Created attachment 169330 [details]
WIP
Comment 5 Brad Vogel 2012-11-05 15:05:10 PST
Yes, this is extremely useful. A lot of Javascript libraries implement a naive addClass() function like:
function addClass(newClass){
     element.className = element.className + ' ' + newClass;
}

If newClass already exists in element.classList, then it'll still dirty the node.
Comment 6 Brad Vogel 2013-04-04 16:52:52 PDT
Cross-filed for Blink at https://code.google.com/p/chromium/issues/detail?id=226854
Comment 7 Erik Arvidsson 2013-04-05 07:20:55 PDT
(In reply to comment #5)
> Yes, this is extremely useful. A lot of Javascript libraries implement a naive addClass() function like:
> function addClass(newClass){
>      element.className = element.className + ' ' + newClass;
> }

This case will always be slow since it is always changing the attribute and the list.
Comment 8 Brad Vogel 2013-04-05 10:37:06 PDT
In the above example, I was hoping that if the CSS class being added happened to already be on the element, no relayout would occur. A lot of JS libraries are naive and always append the class to the className attribute. Also, a lot of web developers use these libraries to add classes that already exist on the element, usually in a loop (e.g. striping a table). Other attributes such as those set by setAttribute have logic to only dirty the DOM if needed. Fixing className would provide nice symmetry.
Comment 9 Antti Koivisto 2013-04-05 12:52:25 PDT
We should already have do-nothing optimization for adding the same class again. Have you verified recently it is not working?
Comment 10 Erik Arvidsson 2013-04-05 13:14:56 PDT
(In reply to comment #9)
> We should already have do-nothing optimization for adding the same class again. Have you verified recently it is not working?

The case that Brad is talking about is changing the class attribute but not in a way that impacts style resolution. For example `element.className += ' '`. This would of course impact `[class='abc']` selectors but I believe we already keep track of attribute selectors.
Comment 11 Antti Koivisto 2013-04-05 13:48:10 PDT
(In reply to comment #10)
> (In reply to comment #9)
> > We should already have do-nothing optimization for adding the same class again. Have you verified recently it is not working?
> 
> The case that Brad is talking about is changing the class attribute but not in a way that impacts style resolution. For example `element.className += ' '`. This would of course impact `[class='abc']` selectors but I believe we already keep track of attribute selectors.

We do put some effort into optimizing class list change. Check Element::classAttributeChanged() and checkSelectorForClassChange() to see if there is something obvious we could still do.
Comment 12 Brad Vogel 2013-04-05 14:36:50 PDT
Created attachment 196683 [details]
New test case with className += ' '

> The case that Brad is talking about is changing the class attribute but not in a way that impacts style resolution.

Yes, that's correct. I added a new test case with the `className += ' '` example. My results from the console in Version 28.0.1466.0 canary:
    with setting className in a way that does not affect styles: 227.365ms
    without setting className: 9.272ms

I'd expect the "with setting className" performance to be much closer to "without setting className" since it's not changing styles or requiring a relayout.
Comment 13 Ahmad Saleem 2022-07-30 07:52:00 PDT
*** Safari 15.6 on macOS 12.5 ***

with setting className in a way that does not affect styles: 230.043ms
without setting className: 0.814ms


*** Firefox Nightly 105 ***

with setting className in a way that does not affect styles: 63ms
without setting className: 1ms

*** Chrome Canary 106 ***

with setting className in a way that does not affect styles: 276.712890625 ms
without setting className: 2.52392578125 ms

____________

Safari is faster than Chrome but slow than Firefox Nightly (might be due to Rust based Stylo). Might be something worth investigating. Thanks!