Closed Bug 1331342 Opened 7 years ago Closed 7 years ago

ProcessDisplayItems() spends too much time on ComputeOpaqueRect()

Categories

(Core :: Web Painting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: sinker, Assigned: sinker)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch reuse-getdisplayport.diff (obsolete) — Splinter Review
In the profile for bug 1269695 (google doc with over 200 pages), ProcessDisplayItems() takes a lot of time while the user scrolls the document.
When look into the detail, nsLayoutUtils::GetDisplayPort() called by ComputeOpaqueRect() pop out.

It is very clear that most items has the same AGR as its previous item, the result of GetDisplayPort() must be the same too.  I created a patch to keep the result of GetDisplayPort() in ContainerState for later using if the AGR of later items is the same one.
I did some test on this patch.  Load a https://en.wikipedia.org/wiki/Linux, and inject following JS code to content.
   var idxx=0;
   window.setInterval(function() {if(idxx%2) document.body.style="padding: 10px"; else document.body.style="padding: 0px"; idxx++;}, 1000);
And measure how long to run nsDisplayList::PaintRoot(), and compute the average.
It improves about 3%~%5.
This patch assumes display items in succession usually having the same AGR, we don't need to re-compute display port over and over again.

It is not easy to measure for too many factors interfering the result, but, overall, it roughly improves nsDisplayList::PaintRoot() for 3~5%.
Attachment #8827103 - Attachment is obsolete: true
Attachment #8828701 - Flags: review?(matt.woodrow)
Comment on attachment 8828701 [details] [diff] [review]
Cache and reuse the result of GetDisplayPort

Review of attachment 8828701 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet!

It might be worth moving some of the code into a 'GetDisplayPortForAnimatedGeometryRoot' helper that checks the cache, and does the expensive lookup if it fails.

That way we can just have the Contains() check once (and we can run it on the empty rect if 'sf' was nullptr).
Attachment #8828701 - Flags: review?(matt.woodrow) → review+
r=mattwoodrow
Move code to |ContainerState::GetDisplayPortForAnimatedGeometryRoot()| as the suggestion from Matt.
Attachment #8843254 - Flags: review+
Attachment #8828701 - Attachment is obsolete: true
Attachment #8827103 - Attachment is obsolete: false
Please check-in attachment 8843536 [details] [diff] [review].  Thanks!
Assignee: nobody → tlee
Keywords: checkin-needed
Keywords: checkin-needed
Attachment #8843536 - Attachment is obsolete: true
Attachment #8843603 - Flags: review+
Attachment #8827103 - Attachment is obsolete: true
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cf1aa220fc1
Cache and reuse the result of GetDisplayPort. r=mattwoodrow
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8cf1aa220fc1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.