1415586 - MotionMark CSS bouncing clipped rectanges generates many identical clips

Status: NEW

(Core :: Graphics: WebRender, enhancement, P2)

Description

[Kartikaya Gupta (email:[e-mail])]

Spinoff from bug 1415326 comment 3.

Apparently the motionmark test (see URL field) has a bunch of rectangles with masks, and all the masks are the same. We generate a bunch of separate clips in WR for this, and we should look into generating one and reusing it.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

There's an nsDisplayMask for each rect so we need to look inside of it and hang the mask image off of the css-clip path

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Attachment: Display list dump

Here's a display list dump. The masks aren't even siblings, they are nested inside nsDisplayTransform items.

Comment 3

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)
> Created attachment 8926667 [details]
> Display list dump
> 
> Here's a display list dump. The masks aren't even siblings, they are nested
> inside nsDisplayTransform items.

Right, they belong to different frames. The addresses of the style structures are also different. I'm not sure how to make them share same style object. Maybe we could create a hash table to cache the shape properties and share the mask image for the items that have the same style.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Yeah that might work. I looked a little bit at this last night but I couldn't find where the shape property is actually stored.

Comment 5

[Ethan Lin[:ethlin]]

It should be this one[1]. There are five properties, but the mCoordinates is an array. For the MotionMark, the type is polygon.

[1] https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/layout/style/nsStyleStruct.h#2348

Comment 6

[Ethan Lin[:ethlin]]

I already have a prototype to reuse the mask image. But I was still trying to understand why I often got different scores in each run. Kevin will take over this bug and I will upload my prototype soon.

Comment 7

[Ethan Lin[:ethlin]]

Attachment: Bug 1415586 - share mask image (WIP)

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

Comment 8

[Code Review Bot [:reviewbot]]

Comment on attachment 8932792 [details]
Bug 1415586 - share mask image (WIP)

https://reviewboard.mozilla.org/r/203850/#review209328


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/layers/wr/WebRenderCommandBuilder.cpp:696
(Diff revision 1)
> +    maskimage->type = svgReset->mClipPath.GetType();
> +    const UniquePtr<StyleBasicShape>& basicShape = svgReset->mClipPath.GetBasicShape();
> +    maskimage->basicShape = new StyleBasicShape(StyleBasicShapeType::Polygon);
> +    *(maskimage->basicShape) = *basicShape;
> +
> +    for (auto mask: mClipCacheManager.mMaskList) {

Warning: Loop variable is copied but only used as const reference; consider making it a const reference [clang-tidy: performance-for-range-copy]

    for (auto mask: mClipCacheManager.mMaskList) {
              ^
         const  &

Comment 9

[Jeff Muizelaar [:jrmuizel]]

FWIW, I'm not sure this work needs to be P1. We shouldn't need this to get reasonable speeds on MotionMark.

Comment 10

[Kevin Chen[:kechen] (UTC + 8)]

To check if the bottleneck of this test is these identical clips, I skipped all the mask generations[1](which is the best case we can do in this bug) and compared it to the normal case.

In the test for profiling[2], I fixed the number of stars and compared their fps. I ran three times for each case to get the average result.

The following is the results:
                                         w/ WR                             w/ WR, skip mask generation
Firfox 59 / debug / linux    |  6.2 fps / 6.21 fps / 6.37 fps       |    6.61 fps / 6.76 fps / 6.73 fps
Firfox 59 / release / linux  |  9.42 fps / 10.83 fps / 10.87 fps    |   12.09 fps / 11.95 fps / 11.85 fps

Decreasing the number of mask generation do brings better fps, but the performance gain is not that obvious.
Maybe we should consider other possibilities for the bottleneck.
Kats, do you have any thought about this?

[1] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/layout/painting/nsDisplayList.cpp#9586
[2] http://browserbench.org/MotionMark/developer.html?test-interval=30&display=minimal&tiles=big&controller=fixed&frame-rate=50&kalman-process-error=1&kalman-measurement-error=4&time-measurement=performance&suite-name=HTMLsuite&test-name=CSSbouncingclippedrects&complexity=1000

Comment 11

[Ethan Lin[:ethlin]]

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #10)
> To check if the bottleneck of this test is these identical clips, I skipped
> all the mask generations[1](which is the best case we can do in this bug)
> and compared it to the normal case.
> 
> In the test for profiling[2], I fixed the number of stars and compared their
> fps. I ran three times for each case to get the average result.
> 
> The following is the results:
>                                          w/ WR                            
> w/ WR, skip mask generation
> Firfox 59 / debug / linux    |  6.2 fps / 6.21 fps / 6.37 fps       |   
> 6.61 fps / 6.76 fps / 6.73 fps
> Firfox 59 / release / linux  |  9.42 fps / 10.83 fps / 10.87 fps    |  
> 12.09 fps / 11.95 fps / 11.85 fps
> 
> Decreasing the number of mask generation do brings better fps, but the
> performance gain is not that obvious.
> Maybe we should consider other possibilities for the bottleneck.
> Kats, do you have any thought about this?
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 8839daefd69087d7ac2655b72790d3a25b6a815c/layout/painting/nsDisplayList.
> cpp#9586
> [2]
> http://browserbench.org/MotionMark/developer.html?test-
> interval=30&display=minimal&tiles=big&controller=fixed&frame-rate=50&kalman-
> process-error=1&kalman-measurement-error=4&time-
> measurement=performance&suite-name=HTMLsuite&test-
> name=CSSbouncingclippedrects&complexity=1000

According to the wr profiler on my device, I think the bottleneck is renderer backend. You can take a look https://bugzilla.mozilla.org/show_bug.cgi?id=1421543#c2.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

I haven't tried profiling this myself so I'm not sure what the bottleneck would be., but it sounds like what you're seeing matches what Jeff is seeing, that it's the renderer backend rather than the duplicated masks that are the problem here.

Comment 13

[Kevin Chen[:kechen] (UTC + 8)]

Hello Kats,

Since the bottleneck of this benchmark is not identical clips, maybe it advisable to mark this as a P3 bug and do it in the future?

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Sure