Closed Bug 1377571 Opened 7 years ago Closed 7 years ago

fallback path for layers free

Categories

(Core :: Graphics: WebRender, enhancement)

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

Details

Attachments

(1 file)

Bug 1377571 - Add fallback path for layers-free.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
Currently I haven't added the fallback path for layers free. According to the bug 1372118 comment 69, we probably have some invalidation / performance issues here. We should evaluate these things in this bug or the follow up bugs.
Per discussion with Morris, with the patches in bug 1376855, basically I just need to call PushItemAsBlobImage for fallback items. The result looks fine but some pages are pretty slow. I believe this path is recreating many resources for each transaction. I'll profile it and see how to make it better. Also I should implement non-BlobImage path too.
We should be able to get a bunch of performance improvements by adding very simple invalidation for the blob image display items. Basically we just need to do a minimal version of what DLBI does and track whether a particular blob image has changed.
The code for this lives in FrameLayerBuilder::ComputeGeometryChangeForItem(). With some version of this we should be able to see a big improvement.
After some investigation, I think we could reuse the nsDisplayGeometry. We can get the invalidate region by comparing the nsDisplayGeometry and repaint the blob image if the invalidate region is not empty. The jam problem got improved after adding this mechanism.
So basically I take each display item as an image and check invalidate region to see if repaint is needed.

The advantages are:
1. This way doesn't need to union the invalidate regions and intersect with each display items.
2. We don't need to repaint an item when another overlapped item changes. The original painted layer have to repaint both items since they share the same buffer.
3. For scrolling, no need rotation buffer or tiling. The repaint region is still small.

The disadvantages are:
1. We'll have many external images or blob images, not sure if there is additional overhead cost. We probably have to push hundreds image. Normally the total area of images is larger than the origin area of painted layer.
2. The worst case is everything needs repaint. When this happens, I suppose the performance is worse than the original gecko.
Almost done. I think we could use this method for now. There is room for improvement, like computing the opacity region to paint less or only rendering the invalidate region. I would like to test these optimizations when investigating svg performance issue.
(In reply to Ethan Lin[:ethlin] from comment #7)
> Almost done. I think we could use this method for now. There is room for
> improvement, like computing the opacity region to paint less or only
> rendering the invalidate region. I would like to test these optimizations
> when investigating svg performance issue.

I noticed you switched from a blob image to a regular image. What motivated the switch? Presumably, we'll want support for both?
(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> (In reply to Ethan Lin[:ethlin] from comment #7)
> > Almost done. I think we could use this method for now. There is room for
> > improvement, like computing the opacity region to paint less or only
> > rendering the invalidate region. I would like to test these optimizations
> > when investigating svg performance issue.
> 
> I noticed you switched from a blob image to a regular image. What motivated
> the switch? Presumably, we'll want support for both?

Right, the function support for both image type with my patch, so we can use the preference "webrender.blob-images" to switch.
I tested some pages. I feel that this approach is slower than the original WebRenderPaintedLayerBlob. Maybe there is some overhead cost to create blob images. Need more investigation.
(In reply to Ethan Lin[:ethlin] from comment #10)
> I tested some pages. I feel that this approach is slower than the original
> WebRenderPaintedLayerBlob. Maybe there is some overhead cost to create blob
> images. Need more investigation.

There shouldn't be much overhead to creating blob images. Are you creating them more often then with WebRenderPaintedLayerBlob?
Comment on attachment 8882701 [details]
Bug 1377571 - Add fallback path for layers-free.

https://reviewboard.mozilla.org/r/153784/#review159924
Attachment #8882701 - Flags: review?(jmuizelaar) → review+
I ran this patch locally. It looks like there are some bugs around how much of the display item should be drawn. Instead of doing invalidation like PaintedLayerBlob we should probably draw the entire display item into a blob image and only redraw if it changes. That should help with performance some.

Also, I guess one reason that the performance is worse is because we have more blob images then we did PaintedLayerBlob because there's no merging.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> I ran this patch locally. It looks like there are some bugs around how much
> of the display item should be drawn.

One of the things we noticed was that text items on https://www.mozilla.org/en-US/ would get clipped incorrectly when they were scrolled partially offscreen towards the top. We were testing with APZ off. It's easier to see if you turn on webrender.highlight-painted-layers - instead of just the top being clipped, these items were also clipped at the bottom.
And as the clip of an item changes, the whole item's blob was repainted. It would be nice if we could always paint the entire item into the blob (and have the blob image's bounds be the whole, unclipped item), and then apply the clip on the outside.
Webrender should still be able to only paint the parts of the blob that are visible.
This would hopefully make it easier to avoid the invalidations as the clip changes.
(In reply to Markus Stange [:mstange] from comment #15)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> > I ran this patch locally. It looks like there are some bugs around how much
> > of the display item should be drawn.
> 
> One of the things we noticed was that text items on
> https://www.mozilla.org/en-US/ would get clipped incorrectly when they were
> scrolled partially offscreen towards the top. We were testing with APZ off.
> It's easier to see if you turn on webrender.highlight-painted-layers -
> instead of just the top being clipped, these items were also clipped at the
> bottom.
> And as the clip of an item changes, the whole item's blob was repainted. It
> would be nice if we could always paint the entire item into the blob (and
> have the blob image's bounds be the whole, unclipped item), and then apply
> the clip on the outside.
> Webrender should still be able to only paint the parts of the blob that are
> visible.
> This would hopefully make it easier to avoid the invalidations as the clip
> changes.

I will fix the text problem. I think I should shift the top-left after clipping.

About the invalidations, some display items do the clipping in the Paint function, such as nsDisplayText, though I'm not sure which line is doing that. Some display items doesn't, such as nsDisplayBorder. I think if we could always paint the whole item into a blob image would be great. Currently I have to clip first since the size of a display item may be very large. The buffer size may be over the limit of a texture. If I simply pass the clip region to webrender, this problem still exists. Unless blob image supports this mechanism, and we could just update the clip region to parent if nothing else changes (though the parent side may have to paint again). Is this correct? Please let me know if I misunderstand.

Even there are invalidations which are due to the clip changes, the repaint region is still small when scrolling. I'll check why it takes so much time to render these items. I guess the time cost may be proportional to the number of blob images but not just about the area. Maybe we should merge these items.
I fixed the wrong clipping problem. There is another position problem for text-shadow. The bounds of the text with shadow seems to be wrong. I would like to ignore it for now since it's individual display item.
I also profiled for the blob image: https://perfht.ml/2tCWcuA. We spend lots of time on font data. It lookes like each transaction and each DrawTargetRecording serialize the font data again. I suppose all DrawTargetRecording could share the font data.
Yep, I'm working on fixing the font stuff in bug 1380014.
Fixed try failures. Try result is good now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cab9b8a25d4465dfee4ff28858f41b4e521ea59.
The fallback path couldn't handle all cases. Some items are container type and they don't have a paint function. I would like to push the patch first and then wire up other things.
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a62238e43aca
Add fallback path for layers-free. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/a62238e43aca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: