1594768 - Too many blob layers on https://www.goodfour.us/

Status: NEW

(Core :: Graphics: WebRender, defect, P3)

Description

[Nicolas Silva [:nical]]

Just above halfway on the page, "Why Four Hours?" animation).

Comment 1

[Jim Blandy :jimb]

Attachment: Somewhat reduced: HTML file that includes the slow animation from github

Comment 2

[Jim Blandy :jimb]

Attachment: A single layer from the animation

This is a reduced version of the animation on the original page, including only a single layer. This is a Lottie animation that decodes to an SVG consisting of a large square of solid color, masked to a small circle. The two seem to move together.

The full animation includes many layers, all using a similar technique. Firefox doesn't recognize that although the path in the layer is large, the visible portion of it is much smaller, so it ends up allocating a much larger backing pixel buffer than necessary, and drawing more area than necessary.

(Masking is expensive because it ostensibly requires us to fully render the underlying image (the large square), render the mask, compute a luminance value for each pixel, and then trim the underlying image according to the luminance values, a per-pixel multiplication.)

Firefox could render this faster by computing a bounding box for the portion of the mask with non-zero luminance, intersecting that with the bounding box of the underlying image, and using the result to decide what portion to render, cache, and draw.

Comment 3

[Jim Blandy :jimb]

Attachment: Attempt to make the single-layer version viewable.

Comment 4

[Jim Blandy :jimb]

Attachment: Attempt to make the single-layer version viewable.

Comment 5

[Jim Blandy :jimb]

Attachment: Attempt to make the single-layer version viewable.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

So I believe we could get a significant improvement here if the computed geometry for the nsDisplayItemMasksAndClip included the bounds of the mask as is computed by ComputeMaskGeometry (https://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#9803).

Miko do you think intersecting the bounds of the mask in nsDisplayItemMasksAndClip is practical?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

It would also be interesting to try disabling active layers to see how much worse they're making performance here.

Comment 8

[Jim Blandy :jimb]

Attachment: Attempt to make the single-layer version viewable.

Comment 9

[Miko Mynttinen]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)

Miko do you think intersecting the bounds of the mask in nsDisplayItemMasksAndClip is practical?

Unfortunately I'm not familiar enough with masking to answer this. Markus probably knows this code better.

Comment 10

[Jim Blandy :jimb]

My laptop has a high-DPI screen and integrated Intel graphics. We observed the WRWorkerLP threads spending most of their time waiting for malloc locks, so we doubled the size of the tiles (to quarter the number of allocations) and lowered the number of worker threads to match physical cpus, since hyperthreading is unlikely to benefit blitting tasks, and they're spending their time waiting for the malloc lock anyway.

This lowers the time a WRWorkerLP spends blocked on malloc to 20%, but still gives poor frame rates, with 28% of the time spent on mask operations. The way to improve that time is to shrink the area of the mask, so we don't need to create the mask, compute its luminance, and so on. If we can use the mask region to choose smaller bounding rectangles into the blob image recordings for the SVG, that would reduce these costs.

Comment 11

[Jim Blandy :jimb]

If we force jemalloc to use a thread-local arena even for large allocations, the malloc lock overhead disappears from the profiles.

Perhaps we should change mozjemalloc.cpp's choose_arena to let each thread specify its own size threshold for redirecting requests to the default arena. Then WRWorkerLP threads could specify a large threshold, to remove contention over allocations like this, whereas other threads could continue to operate under the same parameters they do now.

Comment 12

[Bert Peers [:bpeers]]

Interesting. How large is 'large'? One would expect large allocations to be infrequent unless we're consuming enormous amount of memory, and therefor not conflict a lot, assuming all the small cruft goes to a per-thread SBA. Is the problem perhaps that we're constantly allocating and freeing these large chunks? If so, a per-thread re-use / delayed-free strategy could also be an idea?

Comment 13

[Markus Stange [:mstange]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)

So I believe we could get a significant improvement here if the computed geometry for the nsDisplayMasksAndClip included the bounds of the mask as is computed by ComputeMaskGeometry (https://searchfox.org/mozilla-central/source/layout/painting/nsDisplayList.cpp#9803).

So the way this is supposed to work is the following:

• The nesting in the display list is the following: We have a nsDisplayMasksAndClip item, and inside of that we have the display items for the masked contents.
• When we build the display items for the mask and the contents, we try to compute the mask bounds and apply them as a clip to the masked contents.
• Now there is a clip on the masked contents, but not on the wrapping nsDisplayMasksAndClip item.
• However, the clip on the contents affects the bounds of the nsDisplayMasksAndClip item. This happens as follows: The bounds of the nsDisplayMasksAndClip item is computed by nsDisplayWrapList::UpdateBounds, which calls nsDisplayList::GetClippedBoundsWithRespectToASR which unions the clipped bounds of its child items. This is where the intersection with the clip we computed above is performed.

We should verify that the call to ComputeClipForMaskItem in BuildDisplayListForStackingContext is hit and computes an appropriate rectangle.

Comment 14

[Markus Stange [:mstange]]

The bounds and clips of the Gecko display items are printed as part of a Gecko display list dump. You can get one from your testcase by setting the pref layout.display-list.dump-content to true and running the testcase; this pref works in any Firefox build.

Comment 15

[Jim Blandy :jimb]

Attachment: A single frame of the animation.

Comment 16

[Markus Stange [:mstange]]

Jeff traced the rect computation to a call to GetBBox. GetBBox takes into account clip-paths but not masks, as per spec (see the "If clipped is true" pieces). So if we want to take masks into account we have to additionally intersect the result of GetBBox with the mask bounds, and we may need a way to compute those mask bounds if we don't already have one.

Comment 17

[Jim Blandy :jimb]

Okay, this makes sense. I'd observed GetBBox behaving that way but didn't realize it was per spec.

Comment 18

[Jim Blandy :jimb]

ComputeClipForMaskItem, however, is calling GetAndObserveMasks, suggesting that it already intends to take masks into account; however, it only consults the masks of the top item; in the example, the top item has a clip, but its sole child has a mask.