Closed Bug 1343596 Opened 3 years ago Closed 3 years ago

Investigate performance improvements for nsDisplayItem::ZIndex() and nsDisplayList.cpp::Sort

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: miko, Assigned: miko)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(4 files)

Some performance profiles (see bug 1340226) show nsDisplayItem::ZIndex() and Sort() functions taking considerable amount of time during the display list creation.
This bug tracks the investigation of improving the performance of those functions. Possible solutions include, for example, caching ZIndex() values or making it a field in nsDisplayItem.
Blocks: 1340226
No longer depends on: 1340226
Blocks: 1204549
It seems that storing ZIndex() values during nsDisplayList::Sort can yield very nice performance improvements, especially with larger display lists.

I used the attached benchmark and script to analyze display list creation times. The following mean measurements with n=999 were obtained from the profiles recorded with 0.03ms interval. Unfortunately, this interval makes benchmarking creation of smaller display lists tricky, but especially the >1ms display list creation times seem consistent.

divs		old sorting	new sorting	diff
------------------------------------------------------
20		0.202ms		0.204ms		+1.3%
50		0.343ms		0.299ms		-12.8%
100		0.517ms		0.480ms		-7.0%
200		0.830ms		0.755ms		-9.0%
500		1.852ms		1.732ms		-6.5%
1000		3.808ms		3.134ms		-17.7%
5000		15.176ms		12.133ms		-20.1%
Comment on attachment 8846771 [details]
Bug 1343596 - Part 1: Remove trailing whitespace

https://reviewboard.mozilla.org/r/119768/#review121656
Attachment #8846771 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8846772 [details]
Bug 1343596 - Part 2: Refactor nsDisplayList sorting

https://reviewboard.mozilla.org/r/119770/#review121660

::: layout/painting/nsDisplayList.cpp:2500
(Diff revision 1)
>  
> -static bool IsContentLEQ(nsDisplayItem* aItem1, nsDisplayItem* aItem2,
> -                         void* aClosure) {
> -  nsIContent* commonAncestor = static_cast<nsIContent*>(aClosure);
> +struct ZSortItem {
> +  nsDisplayItem* item;
> +  int32_t zIndex;
> +
> +  ZSortItem(nsDisplayItem* aItem)

I think you need 'explicit' for this ctor to pass static analysis.
Attachment #8846772 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Comment on attachment 8846772 [details]
> Bug 1343596 - Part 2: Refactor nsDisplayList sorting
> 
> https://reviewboard.mozilla.org/r/119770/#review121660
> 
> ::: layout/painting/nsDisplayList.cpp:2500
> (Diff revision 1)
> >  
> > -static bool IsContentLEQ(nsDisplayItem* aItem1, nsDisplayItem* aItem2,
> > -                         void* aClosure) {
> > -  nsIContent* commonAncestor = static_cast<nsIContent*>(aClosure);
> > +struct ZSortItem {
> > +  nsDisplayItem* item;
> > +  int32_t zIndex;
> > +
> > +  ZSortItem(nsDisplayItem* aItem)
> 
> I think you need 'explicit' for this ctor to pass static analysis.

Thanks for the review!

Good catch, made this change.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/72067b245d41
Part 1: Remove trailing whitespace r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/e2d9ac61dd7d
Part 2: Refactor nsDisplayList sorting r=mattwoodrow
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/72067b245d41
https://hg.mozilla.org/mozilla-central/rev/e2d9ac61dd7d
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1379610
Depends on: 1568554
You need to log in before you can comment on or make changes to this bug.