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

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[Miko Mynttinen]

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.

Comment 1

[Miko Mynttinen]

Attachment: Script used to measure displaylist creation times

Comment 2

[Miko Mynttinen]

Attachment: Benchmark to trigger displaylist creation

Comment 3

[Miko Mynttinen]

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 4

[Miko Mynttinen]

Attachment: Bug 1343596 - Part 1: Remove trailing whitespace

Review commit: https://reviewboard.mozilla.org/r/119768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119768/

Comment 5

[Miko Mynttinen]

Attachment: Bug 1343596 - Part 2: Refactor nsDisplayList sorting

Review commit: https://reviewboard.mozilla.org/r/119770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119770/

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8846771 [details]
Bug 1343596 - Part 1: Remove trailing whitespace

https://reviewboard.mozilla.org/r/119768/#review121656

Comment 7

[Matt Woodrow (:mattwoodrow)]

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.

Comment 8

[Miko Mynttinen]

Comment on attachment 8846771 [details]
Bug 1343596 - Part 1: Remove trailing whitespace

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119768/diff/1-2/

Comment 9

[Miko Mynttinen]

Comment on attachment 8846772 [details]
Bug 1343596 - Part 2: Refactor nsDisplayList sorting

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/119770/diff/1-2/

Comment 10

[Miko Mynttinen]

(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.

Comment 11

[Miko Mynttinen]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea7fc8b624a323b2b6433d756b04b2a2107b3565

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/72067b245d41
https://hg.mozilla.org/mozilla-central/rev/e2d9ac61dd7d