Bug 66135 - CSSMutableStyleDeclaration should not add a property if there is already a existing same property
Summary: CSSMutableStyleDeclaration should not add a property if there is already a ex...
Status: RESOLVED CONFIGURATION CHANGED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-12 06:10 PDT by Johnny(Jianning) Ding
Modified: 2022-07-13 15:32 PDT (History)
10 users (show)

See Also:


Attachments
pathological example (6.51 KB, text/html)
2011-08-16 00:15 PDT, Johnny(Jianning) Ding
no flags Details
patch candidate (1.67 KB, patch)
2011-08-16 03:29 PDT, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Johnny(Jianning) Ding 2011-08-12 06:10:52 PDT
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.
Comment 1 Johnny(Jianning) Ding 2011-08-12 06:12:48 PDT
Hi Adam, would you please introduce the people who is/are right person(s) to take a look at this bug?
Comment 2 Adam Barth 2011-08-12 11:45:03 PDT
Hi jnd.  I'm not sure who knows this area best.  I've added Eric to the CC.  Hopefully he'll know.
Comment 3 Eric Seidel (no email) 2011-08-12 12:00:35 PDT
You'd need to demonstrate that this affects performance.  Do you have a pathological example?
Comment 4 Johnny(Jianning) Ding 2011-08-16 00:15:56 PDT
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.
Comment 5 Johnny(Jianning) Ding 2011-08-16 03:29:13 PDT
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).
Comment 6 Eric Seidel (no email) 2011-08-17 11:24:37 PDT
Adding CSS folks.
Comment 7 Brent Fulgham 2022-07-13 15:32:42 PDT
This code has been significantly refactored since this patch was proposed. There doesn't seem to be any action we can take here.