Optimize various small display list building tasks

NEW
Assigned to

Status

()

Core
Layout: Web Painting
8 months ago
7 months ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(firefox54 affected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

59 bytes, text/x-review-board-request
mstange
: review+
Details | Review
59 bytes, text/x-review-board-request
mstange
: review+
Details | Review
59 bytes, text/x-review-board-request
sinker
: review+
Details | Review
59 bytes, text/x-review-board-request
mstange
: review+
Details | Review
59 bytes, text/x-review-board-request
mstange
: review+
Details | Review
59 bytes, text/x-review-board-request
mstange
: review+
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
mstange
: review+
Details | Review
(Assignee)

Description

8 months ago
Bug 1340226 has profiles that showed a bunch of time being spent in various display list building tasks.

Upcoming patches try to make it a bit better.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
How much do your patches improve performance on this testcase? Did you grab a copy of perf-html.io before I changed it to use a canvas?
Thanks for using mozreview for these patches!

Comment 11

8 months ago
mozreview-review
Comment on attachment 8844265 [details]
Bug 1344971 - Part 1: Create OutOfFlowDisplayData for the parent of the OOF frame so they can be shared.

https://reviewboard.mozilla.org/r/117772/#review119708

I don't understand what this is doing. Can you put a description into the commit message?

Comment 12

8 months ago
mozreview-review
Comment on attachment 8844266 [details]
Bug 1344971 - Part 2: Only modify mValidRegion if we actually invalidated something.

https://reviewboard.mozilla.org/r/117774/#review119710

\o/
Attachment #8844266 - Flags: review?(mstange) → review+

Comment 13

8 months ago
mozreview-review
Comment on attachment 8844268 [details]
Bug 1344971 - Part 4: Don't compute a region for nsDisplayBorder when we only want a rect.

https://reviewboard.mozilla.org/r/117778/#review119714
Attachment #8844268 - Flags: review?(mstange) → review+

Comment 14

8 months ago
mozreview-review
Comment on attachment 8844269 [details]
Bug 1344971 - Part 5: Don't track multiple DLBI rects for nsDisplayBorder.

https://reviewboard.mozilla.org/r/117780/#review119718

It would be nice to rely less on style-triggered frame invalidations instead of more, but as long as we're not taking advantage of DLBI by removing those frame invalidation hints, we might as well might DLBI cheaper. (E.g. bug 1272174 is an example where style-induced repaint hints hurt us when padding-top changes; one could imagine a similar usecase with border-top-width instead of padding-top.)
Attachment #8844269 - Flags: review?(mstange) → review+

Comment 15

8 months ago
mozreview-review
Comment on attachment 8844270 [details]
Bug 1344971 - Part 6: Make the nsDisplayListBuilder arena size bigger.

https://reviewboard.mozilla.org/r/117782/#review119720

This is the perfect size.

No, seriously, how did you determine this? Were there measurable improvements? Did you count the number of arena reallocs before and after or something?
Attachment #8844270 - Flags: review?(mstange) → review+

Comment 16

8 months ago
mozreview-review
Comment on attachment 8844271 [details]
Bug 1344971 - Part 7: Remove AssertDisplayItemData since it has a large runtime cost.

https://reviewboard.mozilla.org/r/117784/#review119722

What about bug 1331928 comment 0?

Comment 17

8 months ago
mozreview-review
Comment on attachment 8844272 [details]
Bug 1344971 - Part 8: Share DisplayItemData lookups when we can in FrameLayerBuilder.

https://reviewboard.mozilla.org/r/117786/#review119728

::: layout/painting/FrameLayerBuilder.cpp:4769
(Diff revision 1)
> +  if (!oldData) {
> +    oldData = GetDisplayItemDataForManager(aItem, mRetainingManager);
> +  }

I'd prefer making the aData argument mandatory for all callers, instead of letting them get away with accidentally not sharing DisplayItemData lookups when they could.
Attachment #8844272 - Flags: review?(mstange) → review+
(Assignee)

Comment 18

8 months ago
(In reply to Markus Stange [:mstange] from comment #9)
> How much do your patches improve performance on this testcase? Did you grab
> a copy of perf-html.io before I changed it to use a canvas?

It removed roughly 1/3 of the time spent in DL+FLB (1500ms -> 1000ms).


I did not unfortunately, wasn't aware that you were doing that.(In reply to Markus Stange [:mstange] from comment #11)
> Comment on attachment 8844265 [details]
> Bug 1344971 - Part 1: Create OutOfFlowDisplayData for the parent of the OOF
> frame so they can be shared.
> 
> https://reviewboard.mozilla.org/r/117772/#review119708
> 
> I don't understand what this is doing. Can you put a description into the
> commit message?

We currently store the OutOfFlowDisplayData objects on the out-of-flow frame itself, and if there are multiple OOF frames on a given containing block, then we're storing the data on each of them, even though it's almost completely identical.

This patches moves the frame property up to the containing block, so that we only store it once and then the OOF frames do the lookup via their parent (and adjust the dirty rect into local coords).

It should also improve lookup performance since we're more likely to get hits on the FramePropertyTable lookup cache.

I'll improve the commit message.

(In reply to Markus Stange [:mstange] from comment #14)
> Comment on attachment 8844269 [details]
> Bug 1344971 - Part 5: Don't track multiple DLBI rects for nsDisplayBorder.
> 
> https://reviewboard.mozilla.org/r/117780/#review119718
> 
> It would be nice to rely less on style-triggered frame invalidations instead
> of more, but as long as we're not taking advantage of DLBI by removing those
> frame invalidation hints, we might as well might DLBI cheaper. (E.g. bug
> 1272174 is an example where style-induced repaint hints hurt us when
> padding-top changes; one could imagine a similar usecase with
> border-top-width instead of padding-top.)

Indeed, it's only worth adding the cost to DL building if we can actually take advantage of it during painting. WR is likely to change the calculus for this sort of improvement too.


(In reply to Markus Stange [:mstange] from comment #15)
> Comment on attachment 8844270 [details]
> Bug 1344971 - Part 6: Make the nsDisplayListBuilder arena size bigger.
> 
> https://reviewboard.mozilla.org/r/117782/#review119720
> 
> This is the perfect size.
> 
> No, seriously, how did you determine this? Were there measurable
> improvements? Did you count the number of arena reallocs before and after or
> something?

It's just as arbitrary as the previous number really. Over the time the previous number was determined we've started using the arena for a lot more things (clip chains, AGRs, Bas is adding more etc), and I've noticed that arena resizing has shown up in profiles a few times.

I just doubled the increment amount, and it both reduced the number of allocations and improved performance. It's possible that we're wasting memory on pages with tiny display lists, but it's still not a huge amount.

(In reply to Markus Stange [:mstange] from comment #16)
> Comment on attachment 8844271 [details]
> Bug 1344971 - Part 7: Remove AssertDisplayItemData since it has a large
> runtime cost.
> 
> https://reviewboard.mozilla.org/r/117784/#review119722
> 
> What about bug 1331928 comment 0?

Hmm, good point. We really need to figure this bug out, it's beating me at the moment.

(In reply to Markus Stange [:mstange] from comment #17)
> Comment on attachment 8844272 [details]
> Bug 1344971 - Part 8: Share DisplayItemData lookups when we can in
> FrameLayerBuilder.
> 
> https://reviewboard.mozilla.org/r/117786/#review119728
> 
> ::: layout/painting/FrameLayerBuilder.cpp:4769
> (Diff revision 1)
> > +  if (!oldData) {
> > +    oldData = GetDisplayItemDataForManager(aItem, mRetainingManager);
> > +  }
> 
> I'd prefer making the aData argument mandatory for all callers, instead of
> letting them get away with accidentally not sharing DisplayItemData lookups
> when they could.

Ok, I'll move it up for the other callers too.

Comment 19

8 months ago
mozreview-review
Comment on attachment 8844267 [details]
Bug 1344971 - Part 3: Check NS_FRAME_MAY_BE_TRANSFORMED as part of Extend3DContext.

https://reviewboard.mozilla.org/r/117776/#review120364

looks good!
Attachment #8844267 - Flags: review?(tlee) → review+

Comment 20

7 months ago
mozreview-review
Comment on attachment 8844265 [details]
Bug 1344971 - Part 1: Create OutOfFlowDisplayData for the parent of the OOF frame so they can be shared.

https://reviewboard.mozilla.org/r/117772/#review121744

Looks good! Feels like it should be a good optimization in some cases.
Attachment #8844265 - Flags: review?(mstange) → review+

Comment 21

7 months ago
mozreview-review
Comment on attachment 8844271 [details]
Bug 1344971 - Part 7: Remove AssertDisplayItemData since it has a large runtime cost.

https://reviewboard.mozilla.org/r/117784/#review122202
Attachment #8844271 - Flags: review?(mstange)
You need to log in before you can comment on or make changes to this bug.