Progressive draw might be slower on fennec

RESOLVED FIXED in Firefox 42

Status

()

Core
Graphics: Layers
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jnicol, Assigned: jnicol)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)

Updated

3 years ago
Whiteboard: gfx-noted
(Assignee)

Comment 1

2 years ago
Profiling shows that scrolling cnn.com on fennec with progressive draw enabled is slower than with progressive draw disabled.

http://people.mozilla.org/~bgirard/cleopatra/#report=405e018ffed0573fc406f7b386f9d0f7faaf2fdf&selection=0,1,196,275,276,281,288,289,290,291

That profile shows long rasterisation stages. I added a profiler label to FrameLayerBuilder::RecomputeVisibilityForItems() and we can see a lot of time is being spent in that function. This is because it is being called for every call to FrameLayerBuilder::DrawPaintedLayer(). It should be possible to compute the item visibility once per layer manager transaction rather than each progressive paint.
(Assignee)

Comment 2

2 years ago
Here are two profiles of planet.mozilla.org. Both capturing a quick scroll event faked with the command "adb shell input touchscreen swipe 540 1440 540 480 50"

http://people.mozilla.org/~bgirard/cleopatra/#report=86689951165c94caa6f74bce369cad0299323b29&selection=0,1,59,60,61,78,99,100,101,102

http://people.mozilla.org/~bgirard/cleopatra/#report=6b9d91a3d7e57292feea60230e731717632f1ef4&selection=0,1,143,144,145,180,215,216,217,223

The first is the current behaviour, and you can see that a lot of time is spent in FrameLayerBuilder::RecomputeVisibilityForItems(). The second is with a patch applied to only call that function once per transaction, and you can see that the overall rasterisation time is reduced.

Unscientifically, it feels to me like there is less checkerboarding with the patch applied.
(Assignee)

Comment 3

2 years ago
Created attachment 8629407 [details] [diff] [review]
Only recompute PaintedLayer item visibility when display list has changed

This calls RecomputeItemsVisibility() for the entire visible region of the layer when the display list is changed. Another approach, I believe, would be to call it for the display port region on every transaction. I do not know if that would be better or worse, but this way is a less intrusive code change and seems to work well.
(Assignee)

Updated

2 years ago
Assignee: nobody → jnicol
Status: NEW → ASSIGNED
Comment on attachment 8629407 [details] [diff] [review]
Only recompute PaintedLayer item visibility when display list has changed

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

::: gfx/layers/Layers.h
@@ +1850,5 @@
>     * The residual translation components are always in the range [-0.5, 0.5).
>     */
>    gfxPoint GetResidualTranslation() const { return mResidualTranslation; }
>  
> +  bool IsItemsVisibilityValid() const { return mIsItemsVisibilityValid; }

Can we put this state on the PaintedDisplayItemLayerUserData instead, since it's data internal to FrameLayerBuilder and doesn't really need to be exposed to the layers API.

I'd prefer a name similar to mNeedsRecomputeVisibility, not a huge deal though.

Please also add a comment explaining what this is used for. Something like:

Returns true if the display items for this layer have changed and we need a call to RecomputeVisibility before painting them. This can be false during the latter iterations of progressive painting.
(Assignee)

Comment 5

2 years ago
Created attachment 8629937 [details] [diff] [review]
Patch v2

Reworked patch
(Assignee)

Updated

2 years ago
Attachment #8629407 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8629937 - Flags: review?(matt.woodrow)
Attachment #8629937 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 6

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b762d4914d9
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=61dc2d62af81
(Assignee)

Comment 8

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e2f54cc8323
(Assignee)

Comment 9

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2239936b503
That's pretty interesting. What about the 250ms for processing display lists?
(Assignee)

Comment 11

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b38d13ccb97d
(Assignee)

Comment 12

2 years ago
Created attachment 8633590 [details] [diff] [review]
Patch v3

Previous patch caused invalidation reftests to fail due to caling
RecomputeVisibilityForItems on too large a region (the layer's visible
region). This was also inefficient, and actually regressed performance
when progressive paint was disabled. This version passes the region on
which to call RecomputeVisibilityForItems as a parameter to
DrawPaintedLayer.
Attachment #8633590 - Flags: review?(matt.woodrow)
(Assignee)

Updated

2 years ago
Attachment #8629937 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
:snorp, That is also something that should be looked at. Happens regardless of progressive paint being enabled, so should I create a new ticket?
(In reply to Jamie Nicol [:jnicol] from comment #13)
> :snorp, That is also something that should be looked at. Happens regardless
> of progressive paint being enabled, so should I create a new ticket?

I think so, yeah.
(Assignee)

Comment 15

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3887f868227c
Comment on attachment 8633590 [details] [diff] [review]
Patch v3

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1360,5 @@
> +
> +  // True if the display items for this layer have changed and we need a call
> +  // to RecomputeVisibilityForItems before painting them. This can be false
> +  // during the latter iterations of progressive painting.
> +  bool mNeedsRecomputeVisibility;

I'd feel better if this was initialized (to... false?) in the constructor.
Comment on attachment 8633590 [details] [diff] [review]
Patch v3

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

::: layout/base/FrameLayerBuilder.h
@@ +285,5 @@
>     */
>    static void DrawPaintedLayer(PaintedLayer* aLayer,
>                                gfxContext* aContext,
>                                const nsIntRegion& aRegionToDraw,
> +                              const nsIntRegion& aDirtyRegion,

Add to the comment with why we might not be trying to paint the entire dirty region at this point.
Attachment #8633590 - Flags: review?(matt.woodrow) → review+
The other thing that wasn't clear to me - what is the relationship of the dirty region to the region to draw?  Is one always enclosed in another, are they usually, but not always the same, do they just overlap but one is not a subset of another, etc. I'm guessing this last one, but it'd be useful to be explicit.
OK, it took me a whole day because a debug build takes a while to run the reftests, but on my Windows 7 machine, latest debug build, the layout/svg reftests pass without this change, and fail with this change.
(In reply to Milan Sreckovic [:milan] from comment #18)
> The other thing that wasn't clear to me - what is the relationship of the
> dirty region to the region to draw?

I think it depends where you are in the code.

In the snippet used above, the layer system can tell layout that it needed a region of the layer to become invalid and be repainted later. I'm not sure if it's still needed or used?

Under the tiling code IIRC I think it depends on the paint strategy we're using. We might invalidate/dirty a region but we might decide to (progressively) paint only a subset. So the dirty region that isn't covered by the paint region shouldn't be presented and would become checkerboard until the paint for that region comes in.
(Assignee)

Comment 21

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1c6207daeb7
(Assignee)

Comment 22

2 years ago
The aDirtyRegion is the total region that needs to be drawn, but this might happen over multiple calls to DrawPaintedLayer. Whereas the aRegionToDraw is the region that DrawPaintedLayer is supposed to draw in that call. Often they'll be equal but aDirtyRegion might be a superset (like when progressive paint is enabled). I'll definitely add a comment explaining that.

Quite an interesting cause of failures due to the last patch. In ClientPaintedLayer::PaintThebes(), instead of just drawing state.mRegionToDraw, we call RotatedContentBuffer::BorrowDrawTargetForPainting() iteratively and paint the returned regions separately. So I assumed I should use state.mRegionToDraw as aDirtyRegion and iter.mDrawRegion as aRegionToDraw. I think this was the right assumption, and it mostly works.

However, on D2D (which explains why the reftest failures are accelerated windows 7 only?) BorrowDrawTargetForPainting() calls SimplifyOutwardByArea() on iter.mDrawRegion. This results in aRegionToDraw being larger than aDirtyRegion. Meaning that RecomputeVisibilityForItems() marks some items as invisible as they are outwith aDirtyRegion, but we then paint (the background colour?) in their place as they are within aRegionToDraw.

I've kicked off a try run which does not call SimplifyOutwardsByArea() to confirm this. But presumably we call SimplifyOutwardsByArea() on D2D for a reason, so that's not a proper solution.
(Assignee)

Comment 23

2 years ago
Created attachment 8636464 [details] [diff] [review]
Patch v4

In RotatedContentBuffer::BorrowDrawTargetForPainting() call
SimplifyOutwardsByArea on state.mRegionToDraw rather than
iter.mDrawRegion. This ensures that state.mRegionToDraw remains larger
than iter.mDrawRegion, which fixes the previous test failures.

This is a try run with the latest patch that looks good to me:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1489f3e3af29
Attachment #8636464 - Flags: review?(matt.woodrow)
(Assignee)

Updated

2 years ago
Attachment #8633590 - Attachment is obsolete: true
Attachment #8636464 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 24

2 years ago
another try run - https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4c812e935b0
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9116170485f2
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9116170485f2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Can someone link me to a m-i build where this landed, I'm not having any luck finding a build to test with.

I suspect this may have caused: https://bugzilla.mozilla.org/show_bug.cgi?id=1187619
Depends on: 1187619
Depends on: 1200729
You need to log in before you can comment on or make changes to this bug.