Closed Bug 1407938 Opened 7 years ago Closed 7 years ago

Improve the memory management for the fallback path

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected

People

(Reporter: ethlin, Assigned: ethlin)

References

Details

(Whiteboard: [wr-mvp] [gfx-noted])

Attachments

(3 files, 1 obsolete file)

Bug 1407938 - Part1. Add nsDisplaySVGWrapper for webrender as a fallback display item.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1407938 - Part2. Extract paint function for filter and svg wrapper.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1407938 - Part3. Update reftest annotations after the fix.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
For WR fallback, we paint each item separately. If a item doesn't change, then we will reuse the last image key. But for some svg or certain web page, some fallback items keep changing. Then we will keep reallocating the memory and repainting. Either non-blob or blob image has this problem. I think the original gecko doesn't have this problem is because normally the size of a painted layer is equal to the viewport size, which doesn't change at all. I will use the web page[1] as a reference to measure the performance in this bug. [1] https://swizec.github.io/react-fractals/
Blocks: stage-wr-trains
status-firefox57: --- → unaffected
status-firefox58: --- → unaffected
Priority: -- → P3
Whiteboard: [gfx-noted]
I have a patch that makes an attempt at this.
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [gfx-noted] → [wr-mvp] [gfx-noted]
(In reply to Jeff Muizelaar [:jrmuizel] from comment #1) > I have a patch that makes an attempt at this. Great. Will you land the patch soon? Should I keep working on this?
Flags: needinfo?(jmuizelaar)
(In reply to Ethan Lin[:ethlin] from comment #2) > (In reply to Jeff Muizelaar [:jrmuizel] from comment #1) > > I have a patch that makes an attempt at this. > > Great. Will you land the patch soon? Should I keep working on this? Do you have a patch already? I can dig up what I have tomorrow.
Flags: needinfo?(jmuizelaar)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #3) > Do you have a patch already? I can dig up what I have tomorrow. I just have some ideas. I will do some testing first. It would be good to refer to your patch.
oh..maybe you mean the WIP in bug 1388842. I think that's a correct way to handle SVG, though we need to deal with various items. If you are not going to look into this soon, I want to pick up the WIP and see how to make it work.
Maybe we can use FrameLayerBuilder to paint the nsDisplaySVGWrapper, like we did for nsDisplayFilter.
Blocks: 1408822
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad64cc00e5c2e8431a7a026bb8a00ff1833c6cc9&selectedJob=137185893 With the fix, the svg talos test is normal now. The performance of the web page[1] is same as original gecko. Most of the orange tests in the try push are unexpected-pass or about fuzz number changing. Before landing the patches, I need to do some investigations about a failure[2] in R7 which is about SVG blending and the R8 failures. I would like to send review request first to make sure if we want this way to handle SVG. I also have some ideas about enhancing SVG performance based on this approach. I'll do some local test first. [1] https://swizec.github.io/react-fractals/ [2] layout/reftests/svg/blend-difference-stacking.html
Bug 1388842 is the plan that I have for doing SVG. We could potentially take this approach as short term solution but I'd like a good reason to do so. I'm curious what ideas you have for enhancing SVG performance based on this approach though.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #10) > Bug 1388842 is the plan that I have for doing SVG. We could potentially take > this approach as short term solution but I'd like a good reason to do so. I've just uploaded a new patch to bug 1388842. It still doesn't build yet though. I'd encourage you to take a look and let me know what you think.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #11) > I've just uploaded a new patch to bug 1388842. It still doesn't build yet > though. I'd encourage you to take a look and let me know what you think. Thanks. I already copied the nsDisplaySVGWrapper to my patches. I think the nsDisplaySVGWrapper is necessary for WR fallback. Looks like the grouping things are complicated. That's why I want to use FLB for now. But I think eventually we should get rid of FLB for WR. (In reply to Jeff Muizelaar [:jrmuizel] from comment #10) > Bug 1388842 is the plan that I have for doing SVG. We could potentially take > this approach as short term solution but I'd like a good reason to do so. > > I'm curious what ideas you have for enhancing SVG performance based on this > approach though. This approach can fix some reftests and also fixes the SVG talos test. The SVG performance/memory will be same as the original gecko. There is not much change. So I think we can take this as a short term solution, as the bottom line. About the ideas, I'm just thinking if we could convert some SVG items to WR commands. For example, the SVG file[1] in talos creates 2500 rectangles and we could convert them to color layers or wr rects. For layers-full, we probably don't want so many layers since the overhead of layers is high. For WR, it should be fine. And we can do it for some other SVG shapes too. But the problem is we may split a SVG content into several parts. The worst case may be similar to the current WR result. So we may need some rules to decide when to take the this optimization and when to do full fallback for the whole nsDisplaySVGWrapper. [1] testing/talos/talos/tests/svg_opacity/big-optimizable-group-opacity-2500.svg
Attachment #8918804 - Flags: review?(jmuizelaar)
Attachment #8918805 - Flags: review?(jmuizelaar)
This approach fixes current SVG performance/memory problems and many reftests. So I think we can land this first. We may keep using the nsDisplaySVGWrapper in the future either we want to group items on gecko side or we want to have more WR geometry api.
Comment on attachment 8918804 [details] Bug 1407938 - Part1. Add nsDisplaySVGWrapper for webrender as a fallback display item. https://reviewboard.mozilla.org/r/189648/#review196058 It might be worth getting mattwoodrow to ok this as well.
Attachment #8918804 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8919636 [details] Bug 1407938 - Part3. Update reftest annotations after the fix. https://reviewboard.mozilla.org/r/190536/#review196062
Attachment #8919636 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8918805 [details] Bug 1407938 - Part2. Extract paint function for filter and svg wrapper. https://reviewboard.mozilla.org/r/189650/#review196060
Attachment #8918805 - Flags: review?(jmuizelaar) → review+
Attachment #8919635 - Flags: review?(jmuizelaar) → review?(matt.woodrow)
I tried to convert SVG Geometry to wr rect and I tested some SVG web pages which only have rect. The performance is poor. I'll do some more investigation for that.
Comment on attachment 8919635 [details] Bug 1407938 - Part3. Make sure the SVG wrapper contains the background items for the blending. https://reviewboard.mozilla.org/r/190534/#review197052 What is this needed for? Is this the case where the blend mode is within the SVG and the blend container is outside? Do we support that? Or is the blend mode on the svg itself (not within it), but we build nsDisplayMixBlendMode inside nsDisplaySVGWrapper? If it's the latter, then I think we should try fix the ordering instead of doing this.
Attachment #8919635 - Flags: review?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #23) > Comment on attachment 8919635 [details] > Bug 1407938 - Part3. Make sure the SVG wrapper contains the background items > for the blending. > > https://reviewboard.mozilla.org/r/190534/#review197052 > > What is this needed for? > > Is this the case where the blend mode is within the SVG and the blend > container is outside? Do we support that? > > Or is the blend mode on the svg itself (not within it), but we build > nsDisplayMixBlendMode inside nsDisplaySVGWrapper? If it's the latter, then I > think we should try fix the ordering instead of doing this. It's the former one case. The fix only effects one testcase. I would like to create another bug for this issue.
Which test? The current patch is going to break with the retained-dl patches that I just landed, so I'd rather avoid doing it this way if possible.
(In reply to Matt Woodrow (:mattwoodrow) from comment #25) > Which test? > > The current patch is going to break with the retained-dl patches that I just > landed, so I'd rather avoid doing it this way if possible. The testcase is "layout/reftests/svg/blend-difference-stacking.html". IIRC, with the Part1 patch, the display list is something like: nsDisplayBlendContainer nsDisplayBackgroundColor nsDisplaySVGWrapper nsDisplayBlendMode nsDisplayBackgroundColor
Attachment #8919635 - Attachment is obsolete: true
Pushed by ethlin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0eeb0113f966 Part1. Add nsDisplaySVGWrapper for webrender as a fallback display item. r=jrmuizel https://hg.mozilla.org/integration/autoland/rev/6552b096baf0 Part2. Extract paint function for filter and svg wrapper. r=jrmuizel https://hg.mozilla.org/integration/autoland/rev/763e524b44f6 Part3. Update reftest annotations after the fix. r=jrmuizel
Depends on: 1413680
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: