1344971 - Optimize various small display list building tasks

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Matt Woodrow (:mattwoodrow)]

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 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1344971 - Part 1: Create OutOfFlowDisplayData for the parent of the OOF frame so they can be shared.

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

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1344971 - Part 2: Only modify mValidRegion if we actually invalidated something.

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

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1344971 - Part 3: Check NS_FRAME_MAY_BE_TRANSFORMED as part of Extend3DContext.

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

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1344971 - Part 4: Don't compute a region for nsDisplayBorder when we only want a rect.

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

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1344971 - Part 5: Don't track multiple DLBI rects for nsDisplayBorder.

We shouldn't need both of those, any reflow should trigger them both to change, and a style change
would set the invalid bit on the frame.

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

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1344971 - Part 6: Make the nsDisplayListBuilder arena size bigger.

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

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1344971 - Part 7: Remove AssertDisplayItemData since it has a large runtime cost.

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

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1344971 - Part 8: Share DisplayItemData lookups when we can in FrameLayerBuilder.

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

Comment 9

[Markus Stange [:mstange]]

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?

Comment 10

[Markus Stange [:mstange]]

Thanks for using mozreview for these patches!

Comment 11

[Markus Stange [:mstange]]

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

[Markus Stange [:mstange]]

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/

Comment 13

[Markus Stange [:mstange]]

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

Comment 14

[Markus Stange [:mstange]]

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

Comment 15

[Markus Stange [:mstange]]

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?

Comment 16

[Markus Stange [:mstange]]

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

[Markus Stange [:mstange]]

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.

Comment 18

[Matt Woodrow (:mattwoodrow)]

(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

[Thinker Li [:sinker]]

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!

Comment 20

[Markus Stange [:mstange]]

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.

Comment 21

[Markus Stange [:mstange]]

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e03e3a670ac
Part 1: Create OutOfFlowDisplayData for the parent of the OOF frame so they can be shared. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/6805a1db0a08
Part 2: Check NS_FRAME_MAY_BE_TRANSFORMED as part of Extend3DContext. r=thinker
https://hg.mozilla.org/integration/mozilla-inbound/rev/5895e9f4bc68
Part 3: Don't compute a region for nsDisplayBorder when we only want a rect. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/699c28d527aa
Part 4: Don't track multiple DLBI rects for nsDisplayBorder. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbed4bf1ca00
Part 5: Share DisplayItemData lookups when we can in FrameLayerBuilder. r=mstange

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/9e03e3a670ac
https://hg.mozilla.org/mozilla-central/rev/6805a1db0a08
https://hg.mozilla.org/mozilla-central/rev/5895e9f4bc68
https://hg.mozilla.org/mozilla-central/rev/699c28d527aa
https://hg.mozilla.org/mozilla-central/rev/bbed4bf1ca00

Comment 24

[mdew]

Possibly causing regression? https://bugzilla.mozilla.org/show_bug.cgi?id=1427558