Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: ethlin, Assigned: ethlin)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(3 attachments, 7 obsolete attachments)

(Assignee)

Description

2 years ago
We converted many display items to WR items but it looks like the conversion also has some overhead cost. I used Maze Solver[1] to test the performance since there are many display items in the page. The display items in page are almost converted to WR display items. I tested it on my MBP. Test result:

Original gecko : 12 sec
WebRender with gfx.webrendest.enabled=true : 25 sec

According to the profiling data, we spent lots of time building layers. I have a prototype to remove the layer tree, which means we convert the gecko display items to WR commands directly. It's not too hard since the WebRenderDisplayItemLayer is just a shell to wrap a display item. I will upload patches and profiling data.

[1] https://testdrive-archive.azurewebsites.net/Performance/MazeSolver/Default.html
(Assignee)

Comment 2

2 years ago
(Assignee)

Comment 3

2 years ago
Posted patch Part3. Support more items. (obsolete) — Splinter Review
(Assignee)

Comment 4

2 years ago
Update the profiling result on my MBP:

1. Normal gecko
   12 sec
2. WebRender with gfx.webrendest.enabled=true
   25 sec
3. WebRender with gfx.webrendest.enabled=true and gfx.webrender.remove-layer=true
   9 sec

Morris used gecko-profiler to profile on linux:

1. Normal gecko (https://perfht.ml/2rRfMCG)
    DisplayList: 8-10ms (same as above)
    LayerBuilding: 10-12ms
    One frame: 23-25ms
    total: 17seconds

2. Normal webrender with gfx.webrendest.enabled=true (https://perfht.ml/2rRzaiY)
    DisplayList: 9.2ms
    LayerBuilding: 14-19ms
    One frame: 32-35ms
    total: 23.2 seconds

3. Webrender with gfx.webrendest.enabled=true and gfx.webrender.remove-layer=true (https://perfht.ml/2rRB3vQ)
    DisplayList: 8-10ms
    LayerBuilding: None
    One frame: 13-16ms
    total: 10.7 seconds
(Assignee)

Comment 5

2 years ago
Looks like removing layer tree helps. I guess it also benefits some talos tests. If we want to go next, we'll need to make it more completely and have a way to fallback. :kats, what do you think about removing layer?
Flags: needinfo?(bugmail)
(Assignee)

Comment 6

2 years ago
The main change is that we don't build layer tree and we generate WR commands from display items directly. So we skip ContainerState::ProcessDisplayItems() and Layer::RenderLayer(), but just calling each display item's CreateWebRenderCommands(). I still reuse the LayerManager since it's easier to make the prototype.

Known issues:
1. Fallback - I think it should be not hard. We can just do what paintedlayer is doing, accumulating items, painting to a image and pushing the image. It should be compatibly with blob image.
2. Mask layer not from nsDisplayMask - The worst case is using the fallback way. It would be better to convert it to WR commands.
3. DLBI - not sure if we still need this if most of the items are converted.
4. OMTA, APZ - I think we still can retrieve all we need from display items, not very sure if there is any obstacle.
5. Layer cache - We cache some data in layers. If there is no layer, we may need another place to keep them, maybe LayerManager or some kind of cache manager. 

Markus, if we skip ProcessDisplayItems and generate WR commands directy, do you know any possible problem we will have?
Flags: needinfo?(mstange)
Summary: Prototype of removing layer tree → Prototype of not building layer tree
I like this approach! In particular, it seems like a good idea to keep the WebRenderLayerManager for now while removing the actual layer instances, and have the layer manager directly build the WR display list. That way all external machinery (like the TabChild, widget, etc.) still have a layer manager to deal with, while internally we remove the overhead of building layers.

Looking at your prototype, it seems like you only handle a small set of display items. So I'm not sure how valid the timing comparisons are going to be. It might be worth instrumenting a little bit more to see what percent of display items get handled vs get dropped, and come up with a reasonable multiplier for the time spent. It could be that say we're only processing 50% of display items, in which case the current display list build time of 8-10ms would really be 16-20ms after we do the whole thing. At the very least it will give us some kind of estimate.

My other concern is about how to deal with OMTA and APZ stuff, but that's not specific to your approach. It was always in the overall plan to get rid of the layer tree, and so we'll have to find a way to wire up OMTA and APZ directly from the display list.
Flags: needinfo?(bugmail)
Another nice thing about this approach is that it opens the door to performing OMTA on both layerizable animations and non-layerizable ones (bug 1371101).
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> ...
> 
> My other concern is about how to deal with OMTA and APZ stuff, but that's
> not specific to your approach. It was always in the overall plan to get rid
> of the layer tree, and so we'll have to find a way to wire up OMTA and APZ
> directly from the display list.

If we're going to have time to work on this, I would suggest we start with the working APZ and slowly remove things that it doesn't need, rather than remove everything (FLB) and then try to wire it back in.  It's possible that simplifying what we have can be done if APZ is the only thing that needs to survive it.
(Assignee)

Comment 10

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> I like this approach! In particular, it seems like a good idea to keep the
> WebRenderLayerManager for now while removing the actual layer instances, and
> have the layer manager directly build the WR display list. That way all
> external machinery (like the TabChild, widget, etc.) still have a layer
> manager to deal with, while internally we remove the overhead of building
> layers.
> 
> Looking at your prototype, it seems like you only handle a small set of
> display items. So I'm not sure how valid the timing comparisons are going to
> be. It might be worth instrumenting a little bit more to see what percent of
> display items get handled vs get dropped, and come up with a reasonable
> multiplier for the time spent. It could be that say we're only processing
> 50% of display items, in which case the current display list build time of
> 8-10ms would really be 16-20ms after we do the whole thing. At the very
> least it will give us some kind of estimate.
> 

The testing page is simple, almost all items are converted. I slightly modified the page to fix the random seed and remove all fallback/dropped items. So there is no PaintedLayer for the normal webrender and no dropped item for the prototype. The result is same as I updated in comment 4. 

> My other concern is about how to deal with OMTA and APZ stuff, but that's
> not specific to your approach. It was always in the overall plan to get rid
> of the layer tree, and so we'll have to find a way to wire up OMTA and APZ
> directly from the display list.

The prototype totally skips DisplayItemData. But for OMTA, it seems like we may need to know which item is no change or changed comparing to the last transaction. For APZ, I need to investigate more. Maybe we could have a frame based check in this architecture, just checking if the frame is changed? Anyway, I don't think we should move the whole DisplayItemData mechanism after removing layer since if we paint fast enough and have OMTP, we shouldn't do too much work to decide the invalidate region or a precise calculation for culling.
I will make the prototype more complete and check APZ/OMTA issues at the same time.
This is great, Ethan. I can't think of any issues other than the ones you mentioned in comment 6.
Whenever we discussed this approach in the past, invalidation stood out as probably the most difficult aspect. I don't think adding support for APZ or OMTA should be much of a challenge.
Flags: needinfo?(mstange)
(Assignee)

Comment 12

2 years ago
Posted patch prototype of removing layer (obsolete) — Splinter Review
Refactoring and support more display items, so the completeness of wiki page is very high. Peter and I just made the OMTA supported (not in this patch). Using the same way, I store the imageClient/imageKey in WebRenderLayerManager so we could reuse them, just like what the imageLayer was doing. It makes the result of maze solver better, only 6~7 secs on my MBP.
Next step I will wire up APZ from display list and add fallback path. There is pretty much work needs to be done if we want to make try green. Maybe we should land it as prefs off if we want to try this method. :kats, could you suggest how to go next? What should be done before landing?
Attachment #8876674 - Attachment is obsolete: true
Attachment #8876675 - Attachment is obsolete: true
Attachment #8876676 - Attachment is obsolete: true
Flags: needinfo?(bugmail)
We talked about this during the daily meeting today to try and figure out what our path forward should be. The consensus is mostly that (a) it's great that you were able to get this working and cook up a prototype, (b) people should look at your patch to get an idea of what the WR glue will look like in the future, and avoid making changes that are going to be incompatible with that, (c) we should hold off on landing this more until more people have had a chance to look at it and discuss it at the all-hands in a couple of weeks.

So in particular that means we're not going to land it into m-c right now, but at the all-hands we'll do an architectural review of what you have so far, figure out if anything needs changing, and then once that's done we can clean it up and land it, either in a branch or in m-c. The main concern here is that if we put it into m-c right now it might interfere with our existing work because it's a lot of new code which shares codepaths with the stuff we have now. If you want to continue working on APZ from the display list and the fallback path to prototype what that's going to look like, that would be valuable so that we can discuss that also at the all-hands.
Flags: needinfo?(bugmail)
(In reply to Ethan Lin[:ethlin] from comment #6)
> Markus, if we skip ProcessDisplayItems and generate WR commands directy, do
> you know any possible problem we will have?

One other thing that came to mind was clips: You need to support both rounded rectangle clips and image clips. The former should be quite easy because you can just translate rounded rectangles directly (and don't need to go through an intermediate Layers API like in bug 1370682). The latter is a bit more complicated but should be doable as well.
Comment on attachment 8878430 [details] [diff] [review]
prototype of removing layer

Review of attachment 8878430 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/painting/nsDisplayList.h
@@ +1968,5 @@
> +   virtual bool CreateWebRenderCommands(mozilla::wr::DisplayListBuilder& aBuilder,
> +                                        const StackingContextHelper& aSc,
> +                                        nsTArray<WebRenderParentCommand>& aParentCommands,
> +                                        mozilla::layers::WebRenderLayerManager* aManager,
> +                                        nsDisplayListBuilder* aDisplayListBuilder) { return false; }

Can we use the same CreateWebRenderCommands interface for both situations where we have a DisplayItemLayer and don't? It looks like many of the current uses of aLayer could be dropped.
(Assignee)

Comment 16

2 years ago
(In reply to Markus Stange [:mstange] from comment #14)
> (In reply to Ethan Lin[:ethlin] from comment #6)
> > Markus, if we skip ProcessDisplayItems and generate WR commands directy, do
> > you know any possible problem we will have?
> 
> One other thing that came to mind was clips: You need to support both
> rounded rectangle clips and image clips. The former should be quite easy
> because you can just translate rounded rectangles directly (and don't need
> to go through an intermediate Layers API like in bug 1370682). The latter is
> a bit more complicated but should be doable as well.

Right, this is an issue. To my thinking, for the image clips, it should come from nsDisplayMask. We still paint the image on the content side and then push it as an image clip. The mask clip will apply to the child display list of the nsDisplayMask. So for this approach, we don't need something like the mask layer.
It occurs to me that if we couldn't convert a container item to WR commands, then we have to fall back for the whole child display list. Maybe we could use BasicLayerManager for the fallback? It seems an easier way.
(Assignee)

Comment 17

2 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> Comment on attachment 8878430 [details] [diff] [review]
> prototype of removing layer
> 
> Review of attachment 8878430 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/painting/nsDisplayList.h
> @@ +1968,5 @@
> > +   virtual bool CreateWebRenderCommands(mozilla::wr::DisplayListBuilder& aBuilder,
> > +                                        const StackingContextHelper& aSc,
> > +                                        nsTArray<WebRenderParentCommand>& aParentCommands,
> > +                                        mozilla::layers::WebRenderLayerManager* aManager,
> > +                                        nsDisplayListBuilder* aDisplayListBuilder) { return false; }
> 
> Can we use the same CreateWebRenderCommands interface for both situations
> where we have a DisplayItemLayer and don't? It looks like many of the
> current uses of aLayer could be dropped.

Many of the current 'aLayer' could be dropped and we probably could use LayerManager to replace the rest aLayer. Part of my patch are about unhooking the WR commands from 'aLayer'. Maybe we should do this right now as the first step.
But the DisplayItemLayer and WRXXXLayer don't have DisplayListBuilder. We can use the same interface but we may need to do some checks inside the function, checking the prefs or checking if the aDisplayListBuilder is nullptr. Should I use the same interface and bisect inside the function?
(In reply to Ethan Lin[:ethlin] from comment #17)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #15)
> > Comment on attachment 8878430 [details] [diff] [review]
> > prototype of removing layer
> > 
> > Review of attachment 8878430 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: layout/painting/nsDisplayList.h
> > @@ +1968,5 @@
> > > +   virtual bool CreateWebRenderCommands(mozilla::wr::DisplayListBuilder& aBuilder,
> > > +                                        const StackingContextHelper& aSc,
> > > +                                        nsTArray<WebRenderParentCommand>& aParentCommands,
> > > +                                        mozilla::layers::WebRenderLayerManager* aManager,
> > > +                                        nsDisplayListBuilder* aDisplayListBuilder) { return false; }
> > 
> > Can we use the same CreateWebRenderCommands interface for both situations
> > where we have a DisplayItemLayer and don't? It looks like many of the
> > current uses of aLayer could be dropped.
> 
> Many of the current 'aLayer' could be dropped and we probably could use
> LayerManager to replace the rest aLayer. Part of my patch are about
> unhooking the WR commands from 'aLayer'. Maybe we should do this right now
> as the first step.

Yeah. This seems like a good idea.

> But the DisplayItemLayer and WRXXXLayer don't have DisplayListBuilder. We
> can use the same interface but we may need to do some checks inside the
> function, checking the prefs or checking if the aDisplayListBuilder is
> nullptr. Should I use the same interface and bisect inside the function?

It looks like a least a WebRenderDisplayItemLayer has a nsDisplayListBuilder accessible:
  layers::WebRenderDisplayItemLayer* layer = static_cast<layers::WebRenderDisplayItemLayer*>(aLayer);
  nsDisplayListBuilder* builder = layer->GetDisplayListBuilder();

It also looks like  some refactoring could help here. For example:
- nsDisplayBackgroundImage::mImageFlags probably shouldn't exist because it's just duplicating the state on the display list builder. Perhaps we can move this into a LayerizationConfig state object that pulls the data from the nsDisplayListBuilder and that's passed around during painting and webrender displaylist building.
(Assignee)

Comment 19

2 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #18)
> > But the DisplayItemLayer and WRXXXLayer don't have DisplayListBuilder. We
> > can use the same interface but we may need to do some checks inside the
> > function, checking the prefs or checking if the aDisplayListBuilder is
> > nullptr. Should I use the same interface and bisect inside the function?
> 
> It looks like a least a WebRenderDisplayItemLayer has a nsDisplayListBuilder
> accessible:
>   layers::WebRenderDisplayItemLayer* layer =
> static_cast<layers::WebRenderDisplayItemLayer*>(aLayer);
>   nsDisplayListBuilder* builder = layer->GetDisplayListBuilder();
> 
> It also looks like  some refactoring could help here. For example:
> - nsDisplayBackgroundImage::mImageFlags probably shouldn't exist because
> it's just duplicating the state on the display list builder. Perhaps we can
> move this into a LayerizationConfig state object that pulls the data from
> the nsDisplayListBuilder and that's passed around during painting and
> webrender displaylist building.

Oh..right, I forgot DisplayItemLayer has nsDisplayListBuilder. I could start to do some refactoring and split patches to make them more readable.
(Assignee)

Updated

2 years ago
Attachment #8878430 - Attachment is obsolete: true
(Assignee)

Comment 22

2 years ago
Canvas 2D/3D seems to be something big since the binding of canvasLayer and canvas frame is so tight. So I reused the WebRenderCanvasLayer for the prototype and I didn't change too much to make it work. It's probably a way to make the conversion faster.
Depends on: 1375059
Priority: -- → P3
Whiteboard: gfx-noted
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 28

2 years ago
Rebase and split patches.
Summary: Prototype of not building layer tree → Prototype of layer free
(Assignee)

Updated

2 years ago
Attachment #8879484 - Flags: feedback?(howareyou322)

Comment 29

2 years ago
mozreview-review
Comment on attachment 8879485 [details]
Bug 1372118 - Part2. Use new interface of CreateWebRenderCommands for non-image display items.

https://reviewboard.mozilla.org/r/150788/#review157814

::: layout/forms/nsButtonFrameRenderer.cpp:151
(Diff revision 2)
>                                         WebRenderDisplayItemLayer* aLayer) override;
> +  virtual bool CreateWebRenderCommands(mozilla::wr::DisplayListBuilder& aBuilder,
> +                                       const StackingContextHelper& aSc,
> +                                       nsTArray<WebRenderParentCommand>& aParentCommands,
> +                                       mozilla::layers::WebRenderLayerManager* aManager,
> +                                       nsDisplayListBuilder* aDisplayListBuilder) override;

I'd still rather we'd not have both of these routines. It seems like the WebRenderDisplayItemLayer* aLayer is the big blocker here. Can we eliminate this dependency?

Comment 30

2 years ago
mozreview-review
Comment on attachment 8879485 [details]
Bug 1372118 - Part2. Use new interface of CreateWebRenderCommands for non-image display items.

https://reviewboard.mozilla.org/r/150788/#review157816
(Assignee)

Comment 31

2 years ago
mozreview-review-reply
Comment on attachment 8879485 [details]
Bug 1372118 - Part2. Use new interface of CreateWebRenderCommands for non-image display items.

https://reviewboard.mozilla.org/r/150788/#review157814

> I'd still rather we'd not have both of these routines. It seems like the WebRenderDisplayItemLayer* aLayer is the big blocker here. Can we eliminate this dependency?

Yes, I am refactoring this part. I will update my patches soon.

Comment 32

2 years ago
mozreview-review
Comment on attachment 8879484 [details]
Bug 1372118 - Part1. Have a path to create WebRender commands without layers.

https://reviewboard.mozilla.org/r/150786/#review157782

::: gfx/layers/wr/WebRenderLayerManager.cpp:226
(Diff revision 2)
> +  aDisplayList->AppendToTop(&savedItems);
> +}
> +
> +void
> +WebRenderLayerManager::EndTransactionWithoutLayer(nsDisplayList* aDisplayList,
> +                                                  nsDisplayListBuilder* aDisplayListBuilder)

I think we can share the same logic with WebRenderLayerManager::EndTransactionInternal for layers-free or without layers-free.

For layers-free, I think you need to add another LayerManager::EndTransaction with displaylist parameters and hit assertion for non-WR cases.

::: gfx/layers/wr/WebRenderLayerManager.cpp:253
(Diff revision 2)
> +  if (aDisplayList && aDisplayListBuilder) {
> +    StackingContextHelper sc;
> +    mParentCommands.Clear();
> +    CreateWebRenderCommandsFromDisplayList(this, aDisplayList, aDisplayListBuilder, sc, builder);
> +    builder.Finalize(contentSize, mBuiltDisplayList);
> +  } else {

Maybe add assertion for DisplayList and DisplayListBuilder.

::: gfx/thebes/gfxPrefs.h:464
(Diff revision 2)
>  
>    DECL_GFX_PREF(Live, "gfx.vsync.collect-scroll-transforms",   CollectScrollTransforms, bool, false);
>    DECL_GFX_PREF(Once, "gfx.vsync.compositor.unobserve-count",  CompositorUnobserveCount, int32_t, 10);
>    DECL_OVERRIDE_PREF(Live, "gfx.webrender.omta.enabled",       WebRenderOMTAEnabled, gfxPrefs::OverrideBase_WebRender());
>    DECL_GFX_PREF(Live, "gfx.webrender.profiler.enabled",        WebRenderProfilerEnabled, bool, false);
> +  DECL_GFX_PREF(Live, "gfx.webrender.layer-free",              WebRenderLayerFree, bool, false);

s/layer-free/layers-free

::: layout/painting/nsDisplayList.cpp:2092
(Diff revision 2)
>      layerManager = new BasicLayerManager(BasicLayerManager::BLM_OFFSCREEN);
>    }
>  
> +  if (gfxPrefs::WebRenderLayerFree() &&
> +      layerManager->GetBackendType() == layers::LayersBackend::LAYERS_WR &&
> +      XRE_IsContentProcess()) {

We should try IsChrome() instead of IsContentProcess() to work with non-e10s case.

::: modules/libpref/init/all.js:4830
(Diff revision 2)
>  // Never use gralloc surfaces, even when they're available on this
>  // platform and are the optimal surface type.
>  pref("layers.gralloc.disable", false);
>  
>  pref("webrender.highlight-painted-layers", false);
> +pref("gfx.webrender.layer-free", false);

s/layer-free/layers-free
(In reply to Jeff Muizelaar [:jrmuizel] from comment #29)
> 
> I'd still rather we'd not have both of these routines. It seems like the
> WebRenderDisplayItemLayer* aLayer is the big blocker here. Can we eliminate
> this dependency?

It seems like in both cases we need a place to retain some information for a display item (WMData and Layer) perhaps we can abstract over this.
(Assignee)

Comment 34

2 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #33)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #29)
> > 
> > I'd still rather we'd not have both of these routines. It seems like the
> > WebRenderDisplayItemLayer* aLayer is the big blocker here. Can we eliminate
> > this dependency?
> 
> It seems like in both cases we need a place to retain some information for a
> display item (WMData and Layer) perhaps we can abstract over this.

I think we could just leave the new interface. Originally we retain information in WebRenderDisplayItemLayer, now we could use WMData for either layer or non-layer paths.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8881402 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8881403 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8881404 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Attachment #8879484 - Flags: review?(bugmail)
Attachment #8879485 - Flags: review?(bugmail)
(Assignee)

Updated

2 years ago
Blocks: 1376855
(Assignee)

Comment 38

2 years ago
mozreview-review
Comment on attachment 8879485 [details]
Bug 1372118 - Part2. Use new interface of CreateWebRenderCommands for non-image display items.

https://reviewboard.mozilla.org/r/150788/#review158072

::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp:50
(Diff revision 4)
>      wr::DisplayListBuilder builder(WrBridge()->GetPipeline(), contentSize);
>      // We might have recycled this layer. Throw away the old commands.
>      mParentCommands.Clear();
> +
> +    // TODO: Remove the old interface onces we add support for image type display items.
> +    if (mItem->GetType() == nsDisplayItem::TYPE_BACKGROUND ||

For the first version, I only add support for some basic display items. So we may need this workaround for image type display items. I created Bug 1376855 for it and Morris is arranging the patches. The workaround will be removed soon in Bug 1376855.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8879484 - Flags: review?(matt.woodrow)

Comment 41

2 years ago
mozreview-review
Comment on attachment 8879484 [details]
Bug 1372118 - Part1. Have a path to create WebRender commands without layers.

https://reviewboard.mozilla.org/r/150786/#review158096

::: gfx/layers/wr/WebRenderLayerManager.h:55
(Diff revision 4)
> +  static void CreateWebRenderCommandsFromDisplayList(WebRenderLayerManager* aManager,
> +                                                     nsDisplayList* aDisplayList,
> +                                                     nsDisplayListBuilder* aDisplayListBuilder,
> +                                                     StackingContextHelper& aSc,
> +                                                     wr::DisplayListBuilder& aBuilder);
> +  void EndTransactionWithoutLayer(nsDisplayList* aDisplayList,

s/EndTransactionWithoutLayer/EndTransactionWithoutLayers/

::: gfx/layers/wr/WebRenderLayerManager.h:196
(Diff revision 4)
>    LayerRefArray mMutatedLayers;
>    bool mTransactionIncomplete;
>  
>    bool mNeedsComposite;
>    bool mIsFirstPaint;
> +  bool mEndTransactionWithoutLayer;

s/mEndTransactionWithoutLayer/mEndTransactionWithoutLayers/

::: layout/painting/nsDisplayList.cpp:2097
(Diff revision 4)
>    nsIPresShell* presShell = presContext->PresShell();
>    nsIDocument* document = presShell->GetDocument();
>  
> +  if (gfxPrefs::WebRenderLayersFree() &&
> +      layerManager->GetBackendType() == layers::LayersBackend::LAYERS_WR &&
> +      !presContext->IsChrome()) {

Why are we skipping this for chrome presContexts?

::: layout/painting/nsDisplayList.cpp:2100
(Diff revision 4)
> +  if (gfxPrefs::WebRenderLayersFree() &&
> +      layerManager->GetBackendType() == layers::LayersBackend::LAYERS_WR &&
> +      !presContext->IsChrome()) {
> +    MaybeSetupTransactionIdAllocator(layerManager, presContext);
> +    bool temp = aBuilder->SetIsCompositingCheap(layerManager->IsCompositingCheap());
> +    static_cast<WebRenderLayerManager*>(layerManager.get())->EndTransactionWithoutLayer(this, aBuilder);

We should probably call BeginTransaction here as well (or BeginTransactionWithTarget if there is a target).
(Assignee)

Comment 42

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #41)
> Comment on attachment 8879484 [details]
> Bug 1372118 - Part1. Have a path to create WR commands without layer.
> 
> https://reviewboard.mozilla.org/r/150786/#review158096
> 
> ::: gfx/layers/wr/WebRenderLayerManager.h:55
> (Diff revision 4)
> > +  static void CreateWebRenderCommandsFromDisplayList(WebRenderLayerManager* aManager,
> > +                                                     nsDisplayList* aDisplayList,
> > +                                                     nsDisplayListBuilder* aDisplayListBuilder,
> > +                                                     StackingContextHelper& aSc,
> > +                                                     wr::DisplayListBuilder& aBuilder);
> > +  void EndTransactionWithoutLayer(nsDisplayList* aDisplayList,
> 
> s/EndTransactionWithoutLayer/EndTransactionWithoutLayers/
> 
> ::: gfx/layers/wr/WebRenderLayerManager.h:196
> (Diff revision 4)
> >    LayerRefArray mMutatedLayers;
> >    bool mTransactionIncomplete;
> >  
> >    bool mNeedsComposite;
> >    bool mIsFirstPaint;
> > +  bool mEndTransactionWithoutLayer;
> 
> s/mEndTransactionWithoutLayer/mEndTransactionWithoutLayers/
> 
> ::: layout/painting/nsDisplayList.cpp:2097
> (Diff revision 4)
> >    nsIPresShell* presShell = presContext->PresShell();
> >    nsIDocument* document = presShell->GetDocument();
> >  
> > +  if (gfxPrefs::WebRenderLayersFree() &&
> > +      layerManager->GetBackendType() == layers::LayersBackend::LAYERS_WR &&
> > +      !presContext->IsChrome()) {
> 
> Why are we skipping this for chrome presContexts?

Many items will fall back on chrome ui and I haven't implemented fallback path. So I just let content to create wr commands without layers. 

> 
> ::: layout/painting/nsDisplayList.cpp:2100
> (Diff revision 4)
> > +  if (gfxPrefs::WebRenderLayersFree() &&
> > +      layerManager->GetBackendType() == layers::LayersBackend::LAYERS_WR &&
> > +      !presContext->IsChrome()) {
> > +    MaybeSetupTransactionIdAllocator(layerManager, presContext);
> > +    bool temp = aBuilder->SetIsCompositingCheap(layerManager->IsCompositingCheap());
> > +    static_cast<WebRenderLayerManager*>(layerManager.get())->EndTransactionWithoutLayer(this, aBuilder);
> 
> We should probably call BeginTransaction here as well (or
> BeginTransactionWithTarget if there is a target).

Okay, I will add BeginTransaction/BeginTransactionWithTarget here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 45

2 years ago
mozreview-review
Comment on attachment 8879484 [details]
Bug 1372118 - Part1. Have a path to create WebRender commands without layers.

https://reviewboard.mozilla.org/r/150786/#review158416

Looks good enough to get into the tree, just some minor stuff below

::: gfx/layers/wr/WebRenderLayerManager.h:14
(Diff revision 5)
>  #include "Layers.h"
>  #include "mozilla/MozPromise.h"
>  #include "mozilla/layers/APZTestData.h"
>  #include "mozilla/layers/TransactionIdAllocator.h"
>  #include "mozilla/webrender/WebRenderTypes.h"
> +#include "mozilla/webrender/WebRenderAPI.h"

Try to keep alphabetical sorting

::: gfx/layers/wr/WebRenderLayerManager.h:52
(Diff revision 5)
>    virtual int32_t GetMaxTextureSize() const override;
>  
>    virtual bool BeginTransactionWithTarget(gfxContext* aTarget) override;
>    virtual bool BeginTransaction() override;
>    virtual bool EndEmptyTransaction(EndTransactionFlags aFlags = END_DEFAULT) override;
> +  static void CreateWebRenderCommandsFromDisplayList(WebRenderLayerManager* aManager,

If you make this non-static you can drop the first argument. Unless you want to be able to call this with a nullptr WebRenderLayerManager there doesn't seem to be any value in making this static.

::: gfx/layers/wr/WebRenderLayerManager.h:185
(Diff revision 5)
>  
>    nsTArray<DidCompositeObserver*> mDidCompositeObservers;
>  
>    LayerRefArray mKeepAlive;
>  
> +  wr::BuiltDisplayList mBuiltDisplayList;

Add a comment here:
// These fields are used to save a copy of the display list for empty transactions in layers-free mode.

::: gfx/layers/wr/WebRenderLayerManager.cpp:281
(Diff revision 5)
> -  StackingContextHelper sc;
>    WrSize contentSize { (float)size.width, (float)size.height };
>    wr::DisplayListBuilder builder(WrBridge()->GetPipeline(), contentSize);
> +
> +  if (mEndTransactionWithoutLayers) {
> +    if (aDisplayList && aDisplayListBuilder) {

Add a comment here:
// aDisplayList being null here means this is an empty transaction following a layers-free transaction, so we reuse the previously built displaylist.

::: layout/painting/nsDisplayList.cpp:2097
(Diff revision 5)
>    nsIPresShell* presShell = presContext->PresShell();
>    nsIDocument* document = presShell->GetDocument();
>  
> +  if (gfxPrefs::WebRenderLayersFree() &&
> +      layerManager->GetBackendType() == layers::LayersBackend::LAYERS_WR &&
> +      !presContext->IsChrome()) {

Put a comment here clarifying that the !chrome check is just because we don't yet support many display items used in chrome, as opposed to being some fundamental limitation of the architecture.
Attachment #8879484 - Flags: review?(bugmail) → review+

Comment 46

2 years ago
mozreview-review
Comment on attachment 8879485 [details]
Bug 1372118 - Part2. Use new interface of CreateWebRenderCommands for non-image display items.

https://reviewboard.mozilla.org/r/150788/#review158418

It seems to me like this patch should be split in two. There's two things you're doing here:
- One is to taking the existing CreateWebRenderCommands functions and moving their implementation from the old signature to the new signature. The change to WebRenderDisplayItemLayer where you start using the new signature should go with this change in one patch.
- The other thing you're doing is adding layer state checks and implementing CreateWebRenderCommands on new display items (such as nsDisplayTransform). This should go into another patch.

Although to be honest I'm not really sure why you need the layer state checks at all. If we're in "layers" mode, and the display item has an inactive layer state, then we shouldn't be creating a WebRenderDisplayItemLayer for it at all, so the CreateWebRenderCommands function shouldn't get called. And if we're in "layers-free" mode, then we need to build the WR display list regardless of whether the layer state is active or inactive. Can you clarify why layer state needs checking?

::: gfx/layers/wr/StackingContextHelper.h:27
(Diff revision 6)
>   * some of the coordinate space transformations needed.
>   */
>  class MOZ_RAII StackingContextHelper
>  {
>  public:
> +  StackingContextHelper(const StackingContextHelper& aParentSC,

Move this down below the next two constructors and add a comment on it that specifies it is the constructor used in layers-free mode.

::: layout/forms/nsButtonFrameRenderer.cpp:228
(Diff revision 6)
>    // This is really a combination of paint box shadow inner +
>    // paint border.
>    nsRect buttonRect = nsRect(ToReferenceFrame(), mFrame->GetSize());
> +  nsRegion visible;
> +  bool snap;
> +  visible.OrWith(GetBounds(aDisplayListBuilder, &snap));

You can do visible = ... Instead of .OrWith, since the region starts out empty.

::: layout/forms/nsButtonFrameRenderer.cpp:231
(Diff revision 6)
> +  nsRegion visible;
> +  bool snap;
> +  visible.OrWith(GetBounds(aDisplayListBuilder, &snap));
>    nsDisplayBoxShadowInner::CreateInsetBoxShadowWebRenderCommands(aBuilder,
>                                                                   aSc,
> -                                                                 aLayer,
> +                                                                 visible,

Matt should probably weigh in on whether the visible region we are using here is equivalent to what would have been on the layer.

::: layout/painting/nsDisplayList.cpp:7572
(Diff revision 6)
> +                                            WebRenderLayerManager* aManager,
> +                                            nsDisplayListBuilder* aDisplayListBuilder)
> +{
> +  Matrix4x4 newTransformMatrix = GetTransformForRendering();
> +  gfx::Matrix4x4* transformForSC = &newTransformMatrix;
> +  if (transformForSC && newTransformMatrix.IsIdentity()) {

transformForSC is guaranteed to be non-null here so you don't need to check it in the condition.
Attachment #8879485 - Flags: review?(bugmail)
(Assignee)

Comment 47

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #46)
> Comment on attachment 8879485 [details]
> Bug 1372118 - Part2. Make basic display items support layer-free.
> 
> https://reviewboard.mozilla.org/r/150788/#review158418
> 
> It seems to me like this patch should be split in two. There's two things
> you're doing here:
> - One is to taking the existing CreateWebRenderCommands functions and moving
> their implementation from the old signature to the new signature. The change
> to WebRenderDisplayItemLayer where you start using the new signature should
> go with this change in one patch.
> - The other thing you're doing is adding layer state checks and implementing
> CreateWebRenderCommands on new display items (such as nsDisplayTransform).
> This should go into another patch.
> 
Okay, I will split the patch in two.

> Although to be honest I'm not really sure why you need the layer state
> checks at all. If we're in "layers" mode, and the display item has an
> inactive layer state, then we shouldn't be creating a
> WebRenderDisplayItemLayer for it at all, so the CreateWebRenderCommands
> function shouldn't get called. And if we're in "layers-free" mode, then we
> need to build the WR display list regardless of whether the layer state is
> active or inactive. Can you clarify why layer state needs checking?

Currently either layers mode or layers-free mode need GetLayerState. GetLayerState will prepare some necessary data for generating WR commands and the return value shows if the item could be converted to WR commands.
For layers mode, the parameter 'aDisplayListBuilder' in CreateWebRenderCommands will be nullptr, so we will skip the layer state checks in CreateWebRenderCommands since it's already done in ProcessDisplayItems.
For layers-free mode, we still call GetLayerState in CreateWebRenderCommands to generate necessary data. If the return value is not active, the item should go to the fallback path (return false).
I will add some comments for the function 'CreateWebRenderCommands'.
Ok, thanks for the explanation. Putting comments in the code would be good.
Assignee: nobody → ethlin
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 52

2 years ago
mozreview-review
Comment on attachment 8879485 [details]
Bug 1372118 - Part2. Use new interface of CreateWebRenderCommands for non-image display items.

https://reviewboard.mozilla.org/r/150788/#review158520

This is better, thanks.
Attachment #8879485 - Flags: review?(bugmail) → review+

Comment 53

2 years ago
mozreview-review
Comment on attachment 8882196 [details]
Bug 1372118 - Part3. Implement CreateWebRenderCommands for text, transform and background color.

https://reviewboard.mozilla.org/r/153302/#review158536

This looks reasonable enough. I'm not sure about the correctness of all the things but presumably we can fix that in a follow-up if there are problems.

::: layout/painting/nsDisplayList.cpp:7586
(Diff revision 1)
> +  LayerRect bounds = LayerRect::FromUnknownRect(
> +                      LayoutDeviceRect::FromAppUnits(visibleRect, appUnitsPerDevPixel).ToUnknownRect());

You can use ViewAs<LayerPixel>(LayoutDeviceRect::FromAppUnits(visibleRect, appUnitsPerDevPixel), PixelCastJustification::WebRenderHasUnitResolution); instead of the ToUnknown/FromUnknown business.
Attachment #8882196 - Flags: review?(bugmail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 57

2 years ago
There are some test failures on try server. One problem is that I set wrong region for nsDisplayBoxShadowInner, and it looks like I should call nsDisplayItem::GetBounds(nsDisplayListBuilder* aBuilder, bool* aSnap) to get correct region. So both layers and layers-free mode need the DisplayListBuilder. My fix way is passing DisplayListBuilder for layers mode too and adding another parameter to show if we already called GetLayerState.
Another problem seems ccache bug[1]. I got build break on linux[2] even my change is just overloading the "CreateWebRenderCommands". Currently my solution is to change the original function name to CreateWebRenderCommand (remove 's'). Because the original function will be removed soon and I need to see the try result to make sure the correctness. 

After those changes, the try result is green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd0790c68303696a5e359a656c33c32ab3b24f42.
:kats, please have a look and please let me know if the changes are fine with you.


[1] https://bugzilla.redhat.com/show_bug.cgi?id=1373295
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=867692eecdc95c4c248cad84f0ca701e462722af&selectedJob=110850979
Flags: needinfo?(bugmail)
(In reply to Ethan Lin[:ethlin] from comment #57)
> Another problem seems ccache bug[1]. I got build break on linux[2] even my
> change is just overloading the "CreateWebRenderCommands".

I filed bug 1377211 for this the other day, and traced it to a problem in sccache, so I also filed https://github.com/mozilla/sccache/issues/146 for it. In this particular case the error the compiler is generating is like so:

/home/worker/workspace/build/src/obj-firefox/dist/include/nsDisplayList.h:1964:17: error: 'virtual bool nsDisplayItem::CreateWebRender
Commands(const StackingContextHelper&, mozilla::wr::DisplayListBuilder&, nsTArray<mozilla::layers::WebRenderParentCommand>&, mozilla::
layers::WebRenderLayerManager*, nsDisplayListBuilder*)' was hidden [-Werror=overloaded-virtual]
    virtual bool CreateWebRenderCommands(const StackingContextHelper& aSc,
                 ^
/home/worker/workspace/build/src/obj-firefox/dist/include/nsDisplayList.h:3601:16: error:   by 'virtual void nsDisplayOutline::CreateW
ebRenderCommands(mozilla::wr::DisplayListBuilder&, const StackingContextHelper&, nsTArray<mozilla::layers::WebRenderParentCommand>&, m
ozilla::layers::WebRenderDisplayItemLayer*)' [-Werror=overloaded-virtual]
   virtual void CreateWebRenderCommands(mozilla::wr::DisplayListBuilder& aBuilder,
                ^

It seems to me the compiler is just being dumb, although Botond can probably explain why it's doing this :)

> Currently my
> solution is to change the original function name to CreateWebRenderCommand
> (remove 's'). Because the original function will be removed soon and I need
> to see the try result to make sure the correctness. 

Yeah that's ok fine for now.

> One problem is that I set wrong
> region for nsDisplayBoxShadowInner, and it looks like I should call
> nsDisplayItem::GetBounds(nsDisplayListBuilder* aBuilder, bool* aSnap) to get
> correct region. So both layers and layers-free mode need the
> DisplayListBuilder.

ok.

> My fix way is passing DisplayListBuilder for layers mode
> too and adding another parameter to show if we already called GetLayerState.

The way you did this makes it seem like there's some restriction against calling GetLayerState multiple times when in fact it should be ok to do so. I think it might be better to add a method on WebRenderLayerManager:

public:
bool IsLayersFreeTransaction() { return mEndTransactionWithoutLayers; }

and then when you're calling GetLayerState you can do something like this:

if (aManager->IsLayersFreeTransaction()) {
  ContainerLayerParameters parameter;
  if (GetLayerState(aDisplayListBuilder, aManager, parameter) != LAYER_ACTIVE) {
    return false;
  }
}

Just to confirm my understanding: in the "layers-free" transaction, we are walking through the raw display list and only want to process display items for which GetLayerState() returns LAYER_ACTIVE, so we want this check. For the "layers" transaction we already do the GetLayerState() filtering beforehand and only ever create the WebRenderDisplayItemLayer for display items that have returned LAYER_ACTIVE, and so we don't need to do this check again. However - it seems to me that we should be *able* to do the check again and it should return the same result. That is, if we did this:

ContainerLayerParameters parameter;
LayerState state = GetLayerState(aDisplayListBuilder, aManager, parameter);
if (aManager->IsLayersFreeTransaction()) {
  if (state != LAYER_ACTIVE) {
    return false;
  }
} else {
  MOZ_ASSERT(state == LAYER_ACTIVE);
}

the assertion should never fail. Is that correct? Please let me know if I'm misunderstanding.
Flags: needinfo?(bugmail)
(Assignee)

Comment 59

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #58) 
> Just to confirm my understanding: in the "layers-free" transaction, we are
> walking through the raw display list and only want to process display items
> for which GetLayerState() returns LAYER_ACTIVE, so we want this check. For
> the "layers" transaction we already do the GetLayerState() filtering
> beforehand and only ever create the WebRenderDisplayItemLayer for display
> items that have returned LAYER_ACTIVE, and so we don't need to do this check
> again. However - it seems to me that we should be *able* to do the check
> again and it should return the same result. That is, if we did this:
> 
> ContainerLayerParameters parameter;
> LayerState state = GetLayerState(aDisplayListBuilder, aManager, parameter);
> if (aManager->IsLayersFreeTransaction()) {
>   if (state != LAYER_ACTIVE) {
>     return false;
>   }
> } else {
>   MOZ_ASSERT(state == LAYER_ACTIVE);
> }
> 
> the assertion should never fail. Is that correct? Please let me know if I'm
> misunderstanding.

Your understanding is correct. So far as I know, for a display item, FrameLayerBuilder may call GetLayerState() multiple times while building the layer, though the return value may vary if we pass different layerManager, like BasicLayerManager. This should be why we couldn't cache the layer state for the display item.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #58)
> In this particular case the error the compiler is generating is like so:
> 
> /home/worker/workspace/build/src/obj-firefox/dist/include/nsDisplayList.h:
> 1964:17: error: 'virtual bool nsDisplayItem::CreateWebRender
> Commands(const StackingContextHelper&, mozilla::wr::DisplayListBuilder&,
> nsTArray<mozilla::layers::WebRenderParentCommand>&, mozilla::
> layers::WebRenderLayerManager*, nsDisplayListBuilder*)' was hidden
> [-Werror=overloaded-virtual]
>     virtual bool CreateWebRenderCommands(const StackingContextHelper& aSc,
>                  ^
> /home/worker/workspace/build/src/obj-firefox/dist/include/nsDisplayList.h:
> 3601:16: error:   by 'virtual void nsDisplayOutline::CreateW
> ebRenderCommands(mozilla::wr::DisplayListBuilder&, const
> StackingContextHelper&, nsTArray<mozilla::layers::WebRenderParentCommand>&, m
> ozilla::layers::WebRenderDisplayItemLayer*)' [-Werror=overloaded-virtual]
>    virtual void CreateWebRenderCommands(mozilla::wr::DisplayListBuilder&
> aBuilder,
>                 ^
> 
> It seems to me the compiler is just being dumb, although Botond can probably
> explain why it's doing this :)

There are two different signatures here.

In the base class (nsDisplayItem), we have:
  CreateWebRenderCommands(const StackingContextHelper&, 
                          mozilla::wr::DisplayListBuilder&,
                          nsTArray<mozilla::layers::WebRenderParentCommand>&, 
                          mozilla::layers::WebRenderLayerManager*, 
                          nsDisplayListBuilder*)

and in the derived class (nsDisplayOutline), we have:
  CreateWebRenderCommands(mozilla::wr::DisplayListBuilder&,
                          const StackingContextHelper&, 
                          nsTArray<mozilla::layers::WebRenderParentCommand>&, 
                          mozilla::layers::WebRenderDisplayItemLayer*)

The second signature in the derived class hides the first one, which would otherwise be accessible (since it's inherited), and the compiler warns about that.

The usual way to fix the warning is to add a statement of the form:

  using nsDisplayItem::CreateWebRenderCommands;

in the class scope of nsDisplayOutline. This keeps the first signature accessible, overloading it with the second.
(In reply to Ethan Lin[:ethlin] from comment #59)
> Your understanding is correct. So far as I know, for a display item,
> FrameLayerBuilder may call GetLayerState() multiple times while building the
> layer, though the return value may vary if we pass different layerManager,
> like BasicLayerManager. This should be why we couldn't cache the layer state
> for the display item.

Ok, sounds good. So yeah I'd prefer if you did the IsLayersFreeTransaction() approach but if you can't get that to work then what you have now is fine too.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 65

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #61)
> (In reply to Ethan Lin[:ethlin] from comment #59)
> > Your understanding is correct. So far as I know, for a display item,
> > FrameLayerBuilder may call GetLayerState() multiple times while building the
> > layer, though the return value may vary if we pass different layerManager,
> > like BasicLayerManager. This should be why we couldn't cache the layer state
> > for the display item.
> 
> Ok, sounds good. So yeah I'd prefer if you did the IsLayersFreeTransaction()
> approach but if you can't get that to work then what you have now is fine
> too.

Using IsLayersFreeTransaction() is better. I just updated my patches.
(Assignee)

Updated

2 years ago
Blocks: layers-free

Comment 66

2 years ago
mozreview-review
Comment on attachment 8879484 [details]
Bug 1372118 - Part1. Have a path to create WebRender commands without layers.

https://reviewboard.mozilla.org/r/150786/#review158884
Attachment #8879484 - Flags: review?(jmuizelaar) → review+

Comment 67

2 years ago
mozreview-review
Comment on attachment 8879485 [details]
Bug 1372118 - Part2. Use new interface of CreateWebRenderCommands for non-image display items.

https://reviewboard.mozilla.org/r/150788/#review158886
Attachment #8879485 - Flags: review?(jmuizelaar) → review+

Comment 68

2 years ago
mozreview-review
Comment on attachment 8882196 [details]
Bug 1372118 - Part3. Implement CreateWebRenderCommands for text, transform and background color.

https://reviewboard.mozilla.org/r/153302/#review158888
Attachment #8882196 - Flags: review?(jmuizelaar) → review+
We had a chat about this the other day, and I have a few concerns.

The main one, is that skipping FrameLayerBuilder entirely means that we're forced to support rendering everything with WebRender. I'm particularly worried about complex SVG pages that produce 10's of thousands of display items, and we're going to have to ship over a separate BlobImage for each of them.

It's not obvious to me that we can guarantee that we can handle that well, without reimplementing FrameLayerBuilder on the WR side (and I wouldn't wish that on anyone).

Unfortunately, it's hard to do a mixed approach, since as soon as we output some Layers, we need to preserve the ordering between blocks of WR commands and the Layers.

Some options:

* Just go for this approach, do basic invalidation per-item, and hope that WR can handle this volume of BlobImages without breaking. I think we'd want to know sooner rather than later for this, so we can start planning an alternative ASAP.

* Do some analysis of the display list (probably while we build it?), determine if it's suitable for layer-free, and then decide which path to take from there. That's nice, since it gives us these big wins for simple pages, and doesn't add complexity to FLB (\o/), but it just feels sad, and we risk having weird performance cliffs that kick in as you scroll an SVG graphic into view.

* Optimize building WebRenderDisplayItemLayers within the existing FrameLayerBuilder. I think we'd basically want to duplicate the handling of PaintedLayers, so that FLB combines multiple WR-compatible items into a single layer. This is pretty unappealing, since it adds more complexity to FLB.

* Try run FLB on just the SVG subsets. If we could guarantee we get a ContainerLayer display item around SVG, then we could try building layers just for that container.
I'm not entirely sure if SVG creates a stacking context, but if it does it seems like this might work.
> * Try run FLB on just the SVG subsets. If we could guarantee we get a ContainerLayer display item around SVG, then we could try building layers just for that container.

I like this idea.

> I'm not entirely sure if SVG creates a stacking context, but if it does it seems like this might work.

I believe per the spec it does. The "outermost svg element" is defined to create a stacking context. See https://www.w3.org/TR/2015/WD-SVG2-20150915/render.html#ZIndexProperty

Another option, probably the ideal option if we can make it work in time, is just to add SVG support to WebRender. This is my current area of focus.
(Assignee)

Updated

2 years ago
Blocks: 1377571
(Assignee)

Comment 71

2 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #69) 
> Some options:
> 
> * Just go for this approach, do basic invalidation per-item, and hope that
> WR can handle this volume of BlobImages without breaking. I think we'd want
> to know sooner rather than later for this, so we can start planning an
> alternative ASAP.
> 
> * Do some analysis of the display list (probably while we build it?),
> determine if it's suitable for layer-free, and then decide which path to
> take from there. That's nice, since it gives us these big wins for simple
> pages, and doesn't add complexity to FLB (\o/), but it just feels sad, and
> we risk having weird performance cliffs that kick in as you scroll an SVG
> graphic into view.
> 
> * Optimize building WebRenderDisplayItemLayers within the existing
> FrameLayerBuilder. I think we'd basically want to duplicate the handling of
> PaintedLayers, so that FLB combines multiple WR-compatible items into a
> single layer. This is pretty unappealing, since it adds more complexity to
> FLB.
> 
> * Try run FLB on just the SVG subsets. If we could guarantee we get a
> ContainerLayer display item around SVG, then we could try building layers
> just for that container.
> I'm not entirely sure if SVG creates a stacking context, but if it does it
> seems like this might work.

Thanks for the comments. I just created bug 1377571 for fallback path, and I think I could do some profiling for the fallback path soon. The option 1 is the first thing I may try. Option 4 is also interesting. We could have more discussion in bug 1377571.
(Assignee)

Comment 72

2 years ago
As Matt and I discussed on IRC, he’s fine with r+.

try result is good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e3a95d85d918696b29a2688d3d439d19b4817aa

Comment 73

2 years ago
Pushed by ethlin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3280d069c650
Part1. Have a path to create WebRender commands without layers. r=jrmuizel, r=kats, r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4000907558
Part2. Use new interface of CreateWebRenderCommands for non-image display items. r=jrmuizel, r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f8309863362
Part3. Implement CreateWebRenderCommands for text, transform and background color. r=jrmuizel, r=kats

Comment 74

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3280d069c650
https://hg.mozilla.org/mozilla-central/rev/2b4000907558
https://hg.mozilla.org/mozilla-central/rev/0f8309863362
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8879484 [details]
Bug 1372118 - Part1. Have a path to create WebRender commands without layers.

https://reviewboard.mozilla.org/r/150786/#review175676
Attachment #8879484 - Flags: review?(matt.woodrow)
Depends on: 1452703
You need to log in before you can comment on or make changes to this bug.