Open Bug 1415586 Opened 7 years ago Updated 2 years ago

MotionMark CSS bouncing clipped rectanges generates many identical clips

Categories

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

enhancement

Tracking

()

People

(Reporter: kats, Unassigned)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [wr-reserve])

Attachments

(2 files)

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.
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
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
Attached file Display list dump
Here's a display list dump. The masks aren't even siblings, they are nested inside nsDisplayTransform items.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
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
Assignee: nobody → ethlin
Status: NEW → ASSIGNED
Priority: P2 → P1
Assignee: ethlin → kechen
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 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  &
FWIW, I'm not sure this work needs to be P1. We shouldn't need this to get reasonable speeds on MotionMark.
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
Flags: needinfo?(bugmail)
(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.
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.
Flags: needinfo?(bugmail)
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?
Flags: needinfo?(bugmail)
Sure
Assignee: kechen → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(bugmail)
Priority: P1 → P3
Whiteboard: [wr-mvp] → [wr-reserve]
No longer blocks: stage-wr-nightly
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: