Bug 66135 – CSSMutableStyleDeclaration should not add a property if there is already a existing same property

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Johnny(Jianning) Ding]

When debugging some webpages, I found current CSSMutableStyleDeclaration add a property (removed it at first) regardless whether there is already existing same property in the CSSMutableStyleDeclaration.

For example, Some pages may repeatedly set element.style.display="" to ensure the element displayed as default display style in a timer, the script author may not intend to set the value repeatedly but it happens. A small investigation (nytimes 30+,msnbc 20+, abc 140+,espn 150+) shows it is common observation(only tests home page).

I understand the style changes only call scheduleStyleRecalc, and the real style calculation is done in styleRecalcTimer. I also understand reflow will not be happened since style recalculation can identify a property change is replaced by same value. But we can skip scheduleStyleRecalc and unnecessary style recalculation if we find out they are equal in the beginning in CSSMutableStyleDeclaration::setProperty, which help improve rendering performance even it's just a tiny improvement.

A patch will be provided to fix this issue, but I want to get some inputs at first.

[Johnny(Jianning) Ding]

Hi Adam, would you please introduce the people who is/are right person(s) to take a look at this bug?

[Adam Barth]

Hi jnd.  I'm not sure who knows this area best.  I've added Eric to the CC.  Hopefully he'll know.

[Eric Seidel (no email)]

You'd need to demonstrate that this affects performance.  Do you have a pathological example?

[Johnny(Jianning) Ding]

Created attachment 104012 [details]
pathological example

(In reply to comment #3)
> You'd need to demonstrate that this affects performance.  Do you have a pathological example?

Yes, I have written a example test which is bases on a indexeddb layout test. The example is to open a database, write thousands records into the database and read them out.
Since the example will generate lots of outputs into the page, to reduce the layout cost, the output visibility is controlled by a display flag (see parameter "isDisplay" in method "output"), most of calls of output set "none" to the display property of two panels until test finished (at that time, the display is set to default). So this example test actually sets same value to panels's display property thousands of times, it may trigger thousands of style recalculation which should be unnecessary.

When running the example test by clicking "run test" button with and without the patch (provided later), you can see the around 10% percent improvement when skipping processing the redundant style changes.

I understand it may have less impact in real world on desktops, but I personally think it still worth doing and may have visible improvement the mobile device.

[Johnny(Jianning) Ding]

Created attachment 104019 [details]
patch candidate

patch for pre-review since I haven't written test for it.  (I don't know what should we test for this CL as well).

[Eric Seidel (no email)]

Adding CSS folks.

[Brent Fulgham]

This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.