Closed Bug 1441308 (document-splitting) Opened 6 years ago Closed 4 years ago

WebRender: Document splitting

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: jrmuizel, Assigned: alexical)

References

Details

Attachments

(14 files, 12 obsolete files)

Bug 1441308 - Add the render root splitting pref r?sotaro
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Make WR caches document-aware r?bholley
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Split out document pieces of WebRenderLayerManager r?jrmuizel
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Make GpuCache document-aware r?kvark
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Add pref to disable texture cache clear r?bholley
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Remove unnecessary StackingContextHelper params from clips r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Fix picture-caching interaction with doc spitting r?gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Ensure lazy init of debug renderer in draw_frame_debug_items r?gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Core renderroot splitting changes r?kats
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Rename ForEach trait to AsyncForEach r?lsalzman
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Correct Renderer issues with multiple documents r?gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Map document layers to z ranges r?gw
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Support moving Maybe's in IPC serialization r?mccr8
47 bytes, text/x-phabricator-request
Details | Review
Bug 1441308 - Always send parent commands when sending mDestroyedActors r?kats
47 bytes, text/x-phabricator-request
Details | Review 
We have multiple proposals for this:

A:
 - Split the Gecko display list into documents when remote iframe's are encountered.
 - This will require some trickier code to do.

B:
 - Only remote iframe can be new documents
 - This means the status bar must be moved into a new remote iframe.
 - This means a DOM document can't have content above and below a WebRender Document.

We don't yet know which is best.
Depends on: 1191971
Blocks: 1476801
Some more spit balling:

C: Sort of an iteration on B. Add a some kind of new frame type (WRDoc) that corresponds to a webrender document. During paint we build separate wr display lists under each WRDoc frame. We also gather information about the position and order of the WRDoc frames and communicate that to WebRender so that the documents can go in the right locations.
Also something to keep in mind is that with the fission project the number of remote iframes is going to go up dramatically, so if we have one webrender document per remote iframe then we'll end up with a lot of webrender documents. That might be desirable, or it might not be.
I talked option C over with mstange and he thought it would be reasonable. In the tagged frame we would add a new wrapping display item that would note the document. As we're building the WebRender display list we would just keep track of our current document as we traverse and switch it as appropriate.
Blocks: 1479786
Flags: needinfo?(mconley)
I've pointed dthayer at this - let's see where it goes!
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Flags: needinfo?(mconley)
Summing up a conversation with Markus and Jeff, everything above the content-deck is going to get its own document, and everything inside content-deck and below will get the same document. I.e., the status bar / side bar / devtools / etc. will all share a document with the content. At present we'll just render both documents into the default framebuffer with different offsets.

Also, "document" is kind of confusing since it's an overloaded word. We're thinking of going with "stage" instead?
(In reply to Doug Thayer [:dthayer] from comment #5)
> Summing up a conversation with Markus and Jeff, everything above the
> content-deck is going to get its own document, and everything inside
> content-deck and below will get the same document. I.e., the status bar /
> side bar / devtools / etc. will all share a document with the content. At
> present we'll just render both documents into the default framebuffer with
> different offsets.
> 
> Also, "document" is kind of confusing since it's an overloaded word. We're
> thinking of going with "stage" instead?

"stage" feels kinda vague too. What about something like RenderRoot?
Summary: Document splitting → WebRender: Document splitting
The bulk of the work can be found here: https://hg.mozilla.org/try/rev/02c1618e5bed

To simplify the initial implementation, I hard-coded it to two documents - the chrome document, for everything in the chrome area above the content, and the content document, for everything underneath. The content document includes the content but also the devtools, the sidebar if it exists, and anything else that occupies that space. To delineate this I added a "webrendercontent" attribute which gets added to the "content-deck" here[1]. For the most part the document that we're intending to use for a particular thing is just described by a boolean like "aIsForContent"[2]. In the future, we can expand this to support N documents rather than just 2, but it was simplest to first implement it this way and expand if necessary.

The overall strategy was to start building the chrome WR display list and, if we encounter a special display item (nsDisplayRenderRoot), we switch to building the content display list. This is implemented by housing a pointer to the content DisplayListBuilder inside the chrome DisplayListBuilder, and switching them out inside nsDisplayRenderRoot's CreateWebRenderCommands implementation[3]. I also split the IPCResourceUpdateQueue and StackingContextHelper in a similar way for similar reasons. Additionally, from the content process we only build for the content document.

We send these split display lists off to the WebRenderBridgeParent through duplicated parameters - one set of parameters for the chrome document, and one for the content document. We could try to implement this by adding a boolean and sending them as separate messages, but there seem to be ordering problems with doing so and there's bookkeeping that the WebRenderBridgeParent does which becomes complicated when it's getting, say, two RecvSetDisplayList calls per logical update.

Unfortunately not all updates are done from the scope of CreateWebRenderCommands, so in some cases additional state needs to be stored on objects so they can appropriately add their updates to either the chrome document or the content document's command buffer[4].

Additionally, we want to avoid as much unnecessary work as possible, so we check HasAnyStateBits(NS_FRAME_NEEDS_PAINT | NS_FRAME_DESCENDANT_NEEDS_PAINT) when creating the nsDisplayRenderRoot display item, and we avoid sending the display list accordingly. Unfortunately, in the linked patch I'm still building the display lists, and only avoiding sending them. This is because we create the APZ WebRenderScrollData in the course of creating the WR display lists, and the APZUpdater clobbers the previous WebRenderScrollData in the process of updating, and I haven't been able to split it out by document yet, so if the content document doesn't build this and send it up then we run into trouble.

Lastly, I am using WebRenderBridgeParent's mPipelineId right now as the root pipeline id for both the chrome and the content documents. This is because this ID is derived from CompositorSession::mRootLayerTreeId, which holds meaning outside of WebRender. It's mostly fine to have the same mPipelineId for both documents, as WR doesn't impose any requirement that they be unique, except for the fact that WR epochs are tracked in a hash map keyed on pipeline ID, and so the content and chrome documents can clobber each other's epochs. I'm investigating solutions to this right now, but suggestions / questions are welcome.

[1] https://hg.mozilla.org/try/rev/02c1618e5bed#l1.13
[2] https://hg.mozilla.org/try/rev/02c1618e5bed#l9.14
[3] https://hg.mozilla.org/try/rev/02c1618e5bed#l52.125
[4] https://hg.mozilla.org/try/rev/02c1618e5bed#l9.17, https://hg.mozilla.org/try/rev/02c1618e5bed#l27.13, etc.
Sotaro, since you work in some of this space, would you mind reading over the summary and note if anything sounds like it's going in the wrong direction? The overall goal is just to be able to split out the work we do for the chrome area from the content area, so that we don't have to build or send display lists for things in the content area when only the chrome area changes and vice versa. Additionally, as a follow-up we would like to be able to draw these into separate framebuffers and composite them with the OS compositor.

Please let me know if you have any questions!
Flags: needinfo?(sotaro.ikeda.g)
Thanks for the detailed summary! I am looking into https://hg.mozilla.org/try/rev/02c1618e5bed.
Just chatted with Matt Woodrow about the HasAnyStateBits(NS_FRAME_NEEDS_PAINT | NS_FRAME_DESCENDANT_NEEDS_PAINT) check to determine if we can skip building the content area WR display list. Unfortunately this doesn't cover what we need and the only thing that does is behind retained-dl, which is disabled in the chrome right now.

So my current plan is to update this to use the retained-dl information, and gate doc splitting behind retained-dl. Then I can try to prove out the perf gains of doc splitting to justify finalizing chrome retained-dl. Unfortunately I suspect really proving out the perf gains of doc splitting will require painting the documents to separate framebuffers and compositing them with the OS compositor.
I built https://hg.mozilla.org/try/rev/02c1618e5bed and it worked well except the following build failure. It is great!
  https://hg.mozilla.org/try/rev/02c1618e5bed#l50.36

We need to keep current WebRenderLayerManager for layout(Refresh Driver, etc), but needs to support multiple documents.Then current approach looks reasonable. But the change is very big than I expected. We need to make the change as cleaner and simpler as possible.

Some comments.

-[1] I do not know well about StackingContextHelper. You might want to ask :kats about it.

-[2] There are a lot of bool IsForContentRect flag. It might be better to use "Document type enum" instead of bool.
     It make an intent of the code clearer.

-[3] The IsForContentRect flag is used for ImageKeys handling like SharedSurfacesChild, but it seems not necessary.
   WebRender resources does not belong to WebRender document. The Update reource belongs to enum ApiMsg.
   https://dxr.mozilla.org/mozilla-central/source/gfx/webrender_api/src/api.rs#672

-[4] Some WebRenderBridgeParent functions like RecvUpdateResources() uses aIsForContentRect flag to choose api.
  like [3], WebRender resources does not belong to WebRender document. Any wr api should be OK.

-[5] Some function calls in WebRenderBridgeParent looks redundant.
  For example, WebRenderBridgeParent::FlushSceneBuilds() calls FlushSceneBuilder() twice like the following.
  The function triggers task that does not belong to document. Then we do not need to call twice.

 >  mApi->FlushSceneBuilder();
 >  mContentRectApi->FlushSceneBuilder();

-[6] aSkipFrameNotification of ContentFrame in WebRenderBridgeParent::MaybeGenerateFrame() seems not work well with current WebRender.
  The MaybeGenerateFrame() does not know in advance if new_frame_ready() will request rendering.
  Current WebRender might skip frame rendering since Bug 1461239 fix.
Flags: needinfo?(sotaro.ikeda.g)
Still working on cleaning this up, but I did have some responses to a few of these.

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> -[1] I do not know well about StackingContextHelper. You might want to ask
> :kats about it.
> 
> -[2] There are a lot of bool IsForContentRect flag. It might be better to
> use "Document type enum" instead of bool.
>      It make an intent of the code clearer.
> 
> -[3] The IsForContentRect flag is used for ImageKeys handling like
> SharedSurfacesChild, but it seems not necessary.
>    WebRender resources does not belong to WebRender document. The Update
> reource belongs to enum ApiMsg.

The resources do not belong to WebRender documents, but because we want to be able to generate frames for chrome and content documents independently, we still need this. Right now we use whether there were any resource updated to determine if we want to generate a frame or not, and in these cases we would have to generate both chrome and content frames if we didn't differentiate ownership between resources.

> https://dxr.mozilla.org/mozilla-central/source/gfx/webrender_api/src/api.
> rs#672
> 
> -[4] Some WebRenderBridgeParent functions like RecvUpdateResources() uses
> aIsForContentRect flag to choose api.
>   like [3], WebRender resources does not belong to WebRender document. Any
> wr api should be OK.

See above.

> 
> -[5] Some function calls in WebRenderBridgeParent looks redundant.
>   For example, WebRenderBridgeParent::FlushSceneBuilds() calls
> FlushSceneBuilder() twice like the following.
>   The function triggers task that does not belong to document. Then we do
> not need to call twice.

Yeah I think you're right. I'm still going through all of this and cleaning up. Many things were somewhat coarsely duplicated.

> 
>  >  mApi->FlushSceneBuilder();
>  >  mContentRectApi->FlushSceneBuilder();
> 
> -[6] aSkipFrameNotification of ContentFrame in
> WebRenderBridgeParent::MaybeGenerateFrame() seems not work well with current
> WebRender.
>   The MaybeGenerateFrame() does not know in advance if new_frame_ready()
> will request rendering.
>   Current WebRender might skip frame rendering since Bug 1461239 fix.

Good call! I'll work on a more robust solution.
Jeff, given the dependency mentioned in comment 11. My current plan is to get this reviewed and landed behind a pref before I go on paternity leave (for another 4 weeks - beginning after mozlando). After I come back, or if anyone else wants to take it over, I can look into exploring the possibilities of integrating this more deeply with OS compositors to prove out the performance wins, along with helping to hammer out any of the remaining kinks with getting retailed-dl on in the chrome.

Thoughts?
Flags: needinfo?(jmuizelaar)
In order for multiple documents to share caches that use
FrameIds as a clock, FrameIds need to be document-aware,
otherwise documents which are not updating can have their
cached resources expired out from underneath them.

Hash table lookups were the simplest way I could drum up
to accomplish this, but I'm certainly open to suggestions.
I couldn't profile any difference in the single-document
case, so I didn't put much more into it.
Fairly straightforward. Now that caches are document-aware,
we need to clean up document-specific bits when documents
are removed. We might want to avoid this when we're removing
documents as part of removing a whole WebRenderAPI.

Depends on D12840
Since we're now clearing the interal frame_id on caches at
the end of frames, we need to push the end_frame back a bit
in order for all uses of frame_id to still be valid.

Depends on D12841
With multiple documents sharing the same root pipeline ID
and updating independently, epochs can get out of sync.
In the cases where we use these epochs though we seem to
be able to just check the latest epoch, since we just want
to know if something that we've requested has been completed.

This does carry an assumption with it that anything using
documents will update epochs using a global counter, rather
than a per-document counter, which we may want to make
explicit, but I'm not certain of the best way to do that.

Depends on D12842
The changes are a bit coarse, but it is difficult to split them
out logically. This covers the parent and ipdl changes for splitting
a window into two documents, one for the chrome area and one
for the content area.

Depends on D12844
This is the core of the changes on the child side of things. It
handles looking for the webrendercontent attribute and splitting
off a nsDisplayRenderRoot if necessary, which will split off a
separate WR display list.

Depends on D12845
I wasn't altogether sure around how far this is necessary. I
know we need a stacking context and trial and error suggested
that I needed to create it at the top level and pass it all
the way down, but I'm not certain to what extent we need to
be duplicating the tree all the way down. Feedback is welcome.

Depends on D12846
These changes largely revolve around ensuring that we know which
document a pipeline tracked by the AsyncImagePipelineManager
belongs to, so we can know what WR document needs to be updated.

Depends on D12847
This ensures that frame generation from the scene builder is
sent to the appropriate document.

Depends on D12848
A few of these changes are self-explanatory, but the big
one that still has a TODO on it is what to do with the
aScrollUpdates parameter that normally gets passed through
RecvSetDisplayList. The problem is that we overwrite the
WebRenderScrollData for both the content and the chrome
documents when the root pipeline is updated, even though
the content could have been left out of that update since
it was not changed.

My idea is to merge the old content document parts of the
WebRenderScrollData into the new one if we know that we
skipped updating content, but I wanted to get feedback
on this approach before diving too deep.

Depends on D12849
This allows us to generate frames for multiple documents, but
only actually render once.

Depends on D12850
Just to confirm, the problem listed in bug 1506435 is resolved by these patches with layout.display-list.retain.chrome on. Pages that jank the chrome by being complex no longer jank the chrome with these patches (yay!). Except, they still jank the tab bar. This is due to a problem on the retained-dl side that I believe is unnecessary and thus fixable.
This change makes the various WR caches segment their cached data by
document, so that documents' data are not evicted out from underneath them.
Attachment #9027371 - Attachment is obsolete: true
Attachment #9027372 - Attachment is obsolete: true
Attachment #9027373 - Attachment is obsolete: true
I'm not certain what to do with this regarding landing it. I will be on paternity leave after the all hands in Orlando, but my goal was to simply land it preffed off right at the beginning of my leave to avoid any severe merge conflicts, and to be available to aid in rolling it back should the need arise.

An alternative to this is that someone else takes it over. If anyone else is willing, I would be happy to offload any knowledge I have of the subject.

There seem to be three remaining issues with the patch. All of these only arise when the change is preffed ON, I can't observe any issues with doc splitting preffed off.

1. On Linux (can't observe on Windows or OSX), if I add ~10 new tab pages, some of the items that should be on the page disappear, beginning with pieces of the search input, then as I add more new tab pages the top site icons start disappearing. I've tried messing with cache eviction to simply never remove any cache items, and that doesn't seem to affect it. Still diagnosing.

2. Also only on Linux, when scrolling down a page, the scrolling occasionally jerks back and flashes content that should be scrolled off page. Previously I encountered similar issues that were due to epochs clashing due to the root pipeline ID being shared across the two documents. There might be remaining issues with epochs, or it could be something else. Any insights would be appreciated.

3. There's a TODO waiting on feedback from kats in the APZUpdater, regarding what to do with the mScrollData tree when we update the chrome area without updating the content area document, since the two share the tree of WebRenderScrollData. I think I have a reasonable plan for this but I'm not 100% certain.

In any case it's only worth preffing this on if we can also turn on layout.display-list.retain.chrome. So my hope was to land it preffed off and then assist in any of the remaining work to get chrome retained display lists into a good state. It already demonstrates big wins in the responsiveness of the chrome area on complex pages (see bug 1506435), and it could have additional wins if we move documents into separate framebuffers that we pass to the OS compositor.
Sorry for the delay reviewing, it's a big change and I need to carve out some time to page it all in. I'll try to get it done before Orlando so that if there are any major issues we can discuss during the week, or if not you can land the changes and we can monitor for fallout.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #32)
> Sorry for the delay reviewing, it's a big change and I need to carve out
> some time to page it all in. I'll try to get it done before Orlando so that
> if there are any major issues we can discuss during the week, or if not you
> can land the changes and we can monitor for fallout.

No worries! Thanks for the update :)

Also, jrmuizel requested one big patch to see all the changes - if anyone else needs that, you can see it here:
https://hg.mozilla.org/try/rev/7dd0d6a0ed41604b607865fbd5be15d2d89e8697
--- a/gfx/layers/wr/WebRenderLayerManager.h
+++ b/gfx/layers/wr/WebRenderLayerManager.h
@@ -128,20 +128,21 @@ public:
 
   wr::IpcResourceUpdateQueue& AsyncResourceUpdates();
   void FlushAsyncResourceUpdates();
 
   void RegisterAsyncAnimation(const wr::ImageKey& aKey, SharedSurfacesAnimation* aAnimation);
   void DeregisterAsyncAnimation(const wr::ImageKey& aKey);
   void ClearAsyncAnimations();
@@ -186,17 +187,19 @@ private:
    * Take a snapshot of the parent context, and copy
    * it into mTarget.
    */
   void MakeSnapshotIfRequired(LayoutDeviceIntSize aSize);
 
 private:
   nsIWidget* MOZ_NON_OWNING_REF mWidget;
   nsTArray<wr::ImageKey> mImageKeysToDelete;
+  nsTArray<wr::ImageKey> mContentRectImageKeysToDelete;
   nsTArray<wr::BlobImageKey> mBlobImageKeysToDelete;
+  nsTArray<wr::BlobImageKey> mContentRectBlobImageKeysToDelete;
 
   // Set of compositor animation ids for which there are active animations (as
   // of the last transaction) on the compositor side.
   std::unordered_set<uint64_t> mActiveCompositorAnimationIds;
   // Compositor animation ids for animations that are done now and that we want
   // the compositor to discard information for.
   nsTArray<uint64_t> mDiscardedCompositorAnimationsIds;
 
@@ -230,16 +233,17 @@ private:
   // See equivalent field in ClientLayerManager
   APZTestData mApzTestData;
 
   TimeStamp mTransactionStart;
   nsCString mURL;
   WebRenderCommandBuilder mWebRenderCommandBuilder;
 
   size_t mLastDisplayListSize;
+  size_t mLastContentDisplayListSize;
 
   Maybe<wr::IpcResourceUpdateQueue> mAsyncResourceUpdates;
   std::unordered_map<uint64_t, RefPtr<SharedSurfacesAnimation>> mAsyncAnimations;
 };

I think it makes sense to group the per document state and have it owned by the layer manager. This will let other parts of the pipeline refer to it directly instead of the pair of (LayerManager, RenderRoot). For example WebRenderUserData's could be changed to just point to the DocumentState instead of the LayerManager.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #34)
> I think it makes sense to group the per document state and have it owned by
> the layer manager. This will let other parts of the pipeline refer to it
> directly instead of the pair of (LayerManager, RenderRoot). For example
> WebRenderUserData's could be changed to just point to the DocumentState
> instead of the LayerManager.

Quick update on this. To avoid just replacing the (LayerManager, RenderRoot) pair with a (LayerManager, DocumentState) pair, DocumentState has to either take over more of LayerManager's managerie of responsibilities, or proxy to it for those responsibilities. I've taken the route of having it take over those responsibilities rather than proxy them for right now, but it's unclear to me how far the infection will spread, and whether we'll just end up replacing most of the references to WebRenderLayerManager with references to DocumentState. That might not be so bad, but it's a lot of function signatures to change.
(In reply to Doug Thayer [:dthayer] from comment #35)
> 
> Quick update on this. To avoid just replacing the (LayerManager, RenderRoot)
> pair with a (LayerManager, DocumentState) pair, DocumentState has to either
> take over more of LayerManager's managerie of responsibilities, or proxy to
> it for those responsibilities. I've taken the route of having it take over
> those responsibilities rather than proxy them for right now, but it's
> unclear to me how far the infection will spread, and whether we'll just end
> up replacing most of the references to WebRenderLayerManager with references
> to DocumentState. That might not be so bad, but it's a lot of function
> signatures to change.

FWIW, I imagined it taking over those responsibilities. I hope it works out :)
Flags: needinfo?(jmuizelaar)
Ok, so I looked over the patchset to get an idea of how things work generally. I think in order to make this work well with APZ we will probably want to treat render roots similarly to how we treat layer ids. e.g. instead of just using the LayersId as the key at [1] and [2], we should use the (LayersId, RenderRoot) tuple as the key.

Basically, the problem as I see it is when the content process sends display lists for both render roots in a transaction. These are assigned the same epoch on the parent side, but are processed in separate documents, and therefore the scene builds in WR can happen in either order. So, we might get the post-scene-build notification to APZUpdater with the next epoch after the chrome display list is processed, but at that point the contentrect display list has not yet been processed. If we allow APZUpdater to go ahead and process all the messages for that LayersId, that will result in the APZ state getting updated to something that is out of sync with what WR has internally (because it will have processed the DL from one render root but not the other). So subsequent hit tests could fail because WR will be doing hit-tests against the old content scene and APZ has its internal state reflecting the new content scene.

This is basically the same problem we had to solve for display lists from different layers IDs, and I think it would be relatively straightforward to extend that solution to cover separate render roots as well. The hardest part will be making sure that the WebRenderScrollData items for the contentrect portion can be attached properly, but I think it's doable. The main problem here is the chain of FrameMetrics that needs to be maintained, which is actually very similar in nature to the chain of StackingContexts, and can probably be dealt with in a similar way.

With respect to the stacking context helper stuff, the general idea you have makes sense, I think. I think your current implementation will also generate spurious stacking contexts in the contentrect display list, because it will do so for stacking contexts that are not on the path to the content render root, but that should be relatively easy to fix - we can prevent the sub-context-helper from actually pushing anything to the builder until we hit the render root, at which point it can flush all the sub-context-helpers up the chain (would need to add parent links, but that's not hard). This modification would also allow us to squash together the stacking contexts if possible.

Overall I think much of the "visible" complexity (in terms of reading the code) in the patch actually comes from hard-coding the two render roots in a lot of places. If we made it more generic to take an arbitrary number of render roots I suspect it might actually make the code easier to read and follow. But at least from my look at the patch the overall ideas seem sound. Doing the DocumentState thing from the previous comments makes sense and will also help with this I think.

[1] https://searchfox.org/mozilla-central/rev/8f0db72fb6e35414fb9a6fc88af19c69f332425f/gfx/layers/apz/public/APZUpdater.h#165
[2] https://searchfox.org/mozilla-central/rev/8f0db72fb6e35414fb9a6fc88af19c69f332425f/gfx/layers/apz/public/APZUpdater.h#196
Per our discussion, this patch splits out the state management bits of
WebRenderLayerManager, allowing for them to be maintained per-document.
This was split into its own patch since *most* of the changes are
mechanical and have little to do with doc splitting itself.

Depends on D13343
GpuCache can currently evict things out from underneath docs which
are not updating this frame. This makes its roots document-specific,
so that we only evict items for currently updating documents.

Depends on D13343
To facilitate testing of document splitting before it is preffed on,
I'm adding a pref to disable clearing the texture cache, since this
will currently crash the browser with doc splitting on.

Depends on D13840
Update on this now that I'm back from parental leave:

Unfortunately I didn't manage to get the mostly landable patches here landed before I left, so I suspect I'm going to have to slog through some rebasing to get them back into shape. Accordingly, my plan looks like this:

- Get all the r+ patches which don't have r- dependencies rebased and landed
- Rebase and fix all of the r- patches and get them reviewed
- Profit?

Somewhere in there I think I'm supposed to add a patch to rename all the things to whatever it is we decided to call documents. Last I heard was we're calling them pipelines, and we're renaming the existing PipelinId to DisplayListId? I think?

And since I'm alive again please feel free to ping me about anything and everything to do with document(?) splitting!
Keywords: leave-open
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ee870d4a4308
Split out document pieces of WebRenderLayerManager r=jrmuizel

The WebRenderLayerManager cleanup landed, and the major WR side pieces are in a PR.

As best I can tell, the remaining work here is:

  • Implement a way of running a maintenance step before all frame generation in order to do document-independent texture_cache cleanup.
    • I think the best way of doing this is to add a send_transactions method to RenderApi with a list of DocumentId, Transaction pairs.
  • Clean up the code to accept an arbitrary number of RenderRoots rather than hard-coding the content and chrome area RenderRoots
  • Pair our usage of PipelineIds with DocumentIds (a lot of plumbing but should be fairly straightforward overall) for the APZUpdater to use as its hash map keys.
  • Figure out the WebRenderScrollData situation once the previous step has been properly sorted out
  • Rename all the RenderRoots and Documents to Pipeline or whatever it is we settle on

Based on what I was seeing a month or so ago, there's a good chance there will be a few minor talos regressions after all this, so those will need to be sorted out as well.

After all this, I'll probably want to focus on helping to get retained display lists enabled in the parent process.

Hi Doug, can I ask a (probably naive) question

Does there need to be a separate document for the sidebar?

IIUC one aim of splitting is to reduce power consumption by reducing the number of vibrant (transparent/blurred) pixels being sent to the OS compositor per second. To do this you'll have a vibrant but small and infrequently updating document (the toolbar) and a large and frequently updating but opaque document (the content) & each document will be sent separately to the OS compositor.

Where does the sidebar fit? It's vibrant & I think it's part of the content document. If I open a vibrant sidebar, won't the whole content document become vibrant leading to a large, frequently updating and vibrant document thus negating the savings from splitting?

Since multiple documents are presently still being rendered to the same framebuffer, we're still giving the OS compositor the same number of pixels every time. Once we're sending different documents to different framebuffers I imagine this will open up different potential wins for us, but we first need to get multiple documents working with the same framebuffer.

Right now the chief goal for doc splitting is to reduce the amount of work we have to do when the chrome changes but the content does not. Content can be very simple or can be very complex - if it is very complex but relatively static, it can be very expensive right now to handle changes in the chrome area, and this can be bad because it feels like the browser itself is slow, rather than just the page you're on.

That being said, the sidebar is in a similar situation as the main toolbar, and I could certainly see it being worth it to give it its own document, but right now my only goal is to get two documents functioning properly. Once we have that, it should be much simpler to get an arbitrary number and arrangement of documents working.

Thanks very much! It sounds like we Mac users will have to wait a while longer for a battery-friendly opaque Firefox (equivalent to setting gfx.compositor.glcontext.opaque true). Cheers, Mark.

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/95537e83071a
Make WR caches document-aware r=bholley
https://hg.mozilla.org/integration/autoland/rev/da3629e101b9
Make GpuCache document-aware r=kvark
https://hg.mozilla.org/integration/autoland/rev/afb8c84c9677
Add pref to disable texture cache clear r=bholley

We are trying a similar approach with Servo. Questions:

  • If I understand correctly, Chrome and content use 2 different WR documents, is that right?
  • Is the content document origin moved below the chrome document or the display list are shifted down? In other words, do the documents overlap?

(In reply to Paul Rouget [:paul] from comment #52)

We are trying a similar approach with Servo. Questions:

  • If I understand correctly, Chrome and content use 2 different WR documents, is that right?
  • Is the content document origin moved below the chrome document or the display list are shifted down? In other words, do the documents overlap?
  • Correct(ish). The top bar chrome gets its own document, and anything below that fold (sidebar, devtools, etc.) uses the content document. This means that content processes have no idea that there is any document splitting going on, but the parent process has to juggle two documents' display lists and other things.
  • The documents do not overlap in the default setup. set_window_parameters is called with two disjoint rects for the chrome area and content area. However this is not strictly enforced, and if a user customizes their setup with weird css that keeps their URL bar at the top of the window and their tabs at the bottom or something, then we do not currently account for that which means that the content totally overlaps the chrome area.
Alias: document-splitting
Blocks: 1404477

I was hoping to get patches cleaned up enough for review today, but I've run into a few things I need to debug after rebasing. However, if anyone is interested in doing a drive-by on the patches, here is the condensed form with all the patches rolled into one:

https://hg.mozilla.org/try/rev/9b372d014146ba1d4573c4600153ba246b6cc546

Blocks: stage-wr-trains
No longer blocks: stage-wr-next
Priority: P1 → P4

Current status:

  • It's all been split out into N render roots rather than hard-coded to two

  • I've got patches functioning that make our APZ code document-aware by ensuring we have RenderRoots keying all of our hashtables and whatnot

  • Ran into some difficulties in the latest rebase due to not having the clip chain ids set up on the chain of stacking context that we clone for the content render root. Kats realized though that we should be able to just accumulate the offsets of the default stacking contexts and apply that when we create our content stacking context. I just finished out the patches for this, it resolved the issue and seems to be working well

  • Currently combing over all of the code that handles multiple render roots to clean it up. I originally implemented it with arrays of length wr::kRenderRootCount for all of the things we needed to split. This was fine but it's a bit ugly and difficult to read since we key them with RenderRoots which I don't want to be implicitly convertible to integers. So I added a RenderRootArray type which takes in RenderRoots as indices and I'm just bringing everything over to that

  • After this I'm going to make sure I've gone through all of the review comments and then get the new patches up for review

These aren't used, so I'm just getting rid of them as cleanup.

Fairly self-explanatory. With multiple documents this can be a problem.

Depends on D20693

This is a stab at what the correct approach to this should be. It
seems that we should be using the window size here and not the
screen_rect, as the screen_rect is not used to offset what we
normally draw, but instead generally for scissoring(?). The end
result is if we use an offset screen_rect, we end up applying
the offset of the chrome area twice, once because the document's
screen rect is offset, and once because of the tile.world_rect
offset.

Depends on D20696

Document splitting is crashing with early initialization of the debug
renderer. Not sure why, and this is just a temporary workaround, but
one that I think we want anyway, as we don't want to be unnecessarily
lazy-initting the debug renderer.

Depends on D20698

This just adds a few types that are commonly used by the rest of
the render root splitting patches.

Depends on D12844

Attachment #9027374 - Attachment is obsolete: true
Blocks: wr-67
No longer blocks: stage-wr-trains
Priority: P4 → P2
Priority: P2 → P3
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/deb682690025
Ensure lazy init of debug renderer in draw_frame_debug_items r=gw
Attachment #9045707 - Attachment is obsolete: true
Attachment #9027377 - Attachment is obsolete: true
Attachment #9027378 - Attachment is obsolete: true
Attachment #9027379 - Attachment is obsolete: true
Attachment #9027380 - Attachment is obsolete: true
Attachment #9027381 - Attachment is obsolete: true
Attachment #9027382 - Attachment is obsolete: true
Attachment #9027383 - Attachment is obsolete: true
Attachment #9045714 - Attachment description: Bug 1441308 - Add Render Root splitting types r?kats → Bug 1441308 - Core renderroot splitting changes r?kats

Just a little rename for clarity - I ended up scratching my head at
something for a little more time than I should have because I assumed
this was synchronous without looking at the implementation.

Depends on D20701

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/48bf7d6e2ea7
Fix picture-caching interaction with doc spitting r=gw
Attachment #9027376 - Attachment description: Bug 1441308 - Add the render root splitting pref r=sotaro → Bug 1441308 - Add the render root splitting pref r?sotaro

This corrects A) An issue encountered with our strategy for skipping
the end_pass call for all but an offscreen render target. See the
comment above the end_pass call for details, and B) An issue with
depth clearing where we do not clear the whole rect if there are
multiple non-intersecting documents.

Depends on D20701

Hey Neil, I have a question that mstange said you might be able to answer. As part of the core patch for this bug (link), I'm trying to add an attribute ("renderroot" - see the usage inside browser.xul in the patch) which when present will cause us to insert an nsDisplayRenderRoot into the display list. This is roughly similar to how nsDisplayOwnLayer works, so I cargo culted its approach and just added similar code into nsBoxFrame.cpp. However, this feels fairly hacky, and I wanted to get your take on what might be a better approach.

Ideally, to avoid future headaches, it should work on whatever XUL element we put it on. Do you have any thoughts on this? And perhaps whether we could refactor the "layer" attribute to use a similar approach?

(Apologies for the large patch - the changes in browser.xul, nsBoxFrame.cpp, and the addition of nsDisplayRenderRoot in nsDisplayList.cpp should be enough to show what the change is.)

Flags: needinfo?(enndeakin)
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a64d942b966d
Remove unnecessary StackingContextHelper params from clips r=kats

(In reply to Doug Thayer [:dthayer] from comment #68)

Hey Neil, I have a question that mstange said you might be able to answer. As part of the core patch for this bug (link), I'm trying to add an attribute ("renderroot" - see the usage inside browser.xul in the patch) which when present will cause us to insert an nsDisplayRenderRoot into the display list. This is roughly similar to how nsDisplayOwnLayer works, so I cargo culted its approach and just added similar code into nsBoxFrame.cpp. However, this feels fairly hacky, and I wanted to get your take on what might be a better approach.

That's probably ok for now. Note that nsBoxFrame is used for anything that has CSS display: -moz-box (and related types that inherit it) and isn't specifically a XUL element. Some XUL elements that don't have children such as images and labels use nsLeafBoxFrame that does not inherit from nsBoxFrame. The patch here also adds renderroot="content" to some html:div elements in browser.xul but I don't see where that is handled.

If you want it to be supported on all elements, you might need to place the support somewhere in the display list handling (I'm not familiar enough with display lists to know what this would mean). I'm guessing that you probably want to only support this on specific container type elements, so in the long run you might want to create or require a specific element that does this, and wrap existing content inside it.

Flags: needinfo?(enndeakin)

As discussed in IRC. AFAICT the ORTHO_NEAR|FAR_PLANE should match
up with the ranges of valid ZBufferIds, but please double-check
me.

Depends on D23056

Try runs:

Pref off: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41e9fb24c1e82c73517d7ab724f53e98dcc35cfe
Pref on: https://treeherder.mozilla.org/#/jobs?repo=try&revision=485638ad90544da7c6c5f324378b4d524370c2c5

(They're still building at the time of me posting this. They were green before rebasing and adding the new changesets.)

Try did not seem to like that. Investigating shortly.

I'd like to move an ipc::ByteBuf member of a struct into a Maybe,
and in order for that to work IPDLParamTraits<Maybe> needs to support
the rvalue Write.

Depends on D23599

If we try to send them separately as we were before, we can run into
cases where we try to destroy the actors and then send the OpRemoveTexture,
which crashes.

Depends on D23986

(In reply to Doug Thayer [:dthayer] from comment #77)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b2342bfab4151003085eb871d10bc1102e45bed

Looks green to me? Let me know if it looks like I mis-classified anything.

This looks ok to me.

Try with pref off is running here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=557dda58ad01f526ef29619f8578438952d11e32

This is with pref on and it's not looking so good, but I'm ok with landing it with the pref off and then add additional patches to clean this up.

Blocks: 1536661
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/737807563dd5
Add the render root splitting pref r=sotaro
https://hg.mozilla.org/integration/autoland/rev/c09a51622e98
Core renderroot splitting changes r=kats,sotaro
https://hg.mozilla.org/integration/autoland/rev/baee8ada680f
Rename ForEach trait to AsyncForEach r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/e2f83e4816dd
Correct Renderer issues with multiple documents r=gw
https://hg.mozilla.org/integration/autoland/rev/d8b4d6ec9b40
Map document layers to z ranges r=gw
https://hg.mozilla.org/integration/autoland/rev/32f7793dfd1a
Support moving Maybe's in IPC serialization r=mccr8
https://hg.mozilla.org/integration/autoland/rev/1764701d11d1
Always send parent commands when sending mDestroyedActors r=kats,sotaro
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8cefa694f811
Add the render root splitting pref r=sotaro
https://hg.mozilla.org/integration/autoland/rev/96da9d241051
Core renderroot splitting changes r=kats,sotaro
https://hg.mozilla.org/integration/autoland/rev/e36670b15dd6
Rename ForEach trait to AsyncForEach r=lsalzman
https://hg.mozilla.org/integration/autoland/rev/31eb0acf3a1d
Correct Renderer issues with multiple documents r=gw
https://hg.mozilla.org/integration/autoland/rev/1d405e872db4
Map document layers to z ranges r=gw
https://hg.mozilla.org/integration/autoland/rev/8761ce294d2c
Support moving Maybe's in IPC serialization r=mccr8
https://hg.mozilla.org/integration/autoland/rev/9bfd4e60ec4e
Always send parent commands when sending mDestroyedActors r=kats,sotaro

I run MotionMark test with gfx.webrender.split-render-roots:true. Sometimes rendering became all white or all black.

Depends on: 1538538
Depends on: 1538540
Depends on: 1538572
Blocks: 1538710
Depends on: 1538944
No longer blocks: 1538710
Depends on: 1538710
Blocks: 1539629
Blocks: wr-68
No longer blocks: wr-67
Depends on: 1541078
Depends on: 1538798
No longer depends on: 1538768
Regressions: 1538768
No longer depends on: 1538572
Regressions: 1538572
Depends on: 1413546
Whiteboard: [wr-q2][wr-april]
No longer depends on: 1538798
Regressions: 1538798
Depends on: 1545767
Depends on: 1548247
Depends on: 1548688
Depends on: 1549976
Blocks: 1550029

Summary of the current state of document splitting (with a view to my involvement with it and where that involvement should end):

Bug 1538540 - Crash in [@ core::option::expect_failed | webrender::renderer::TextureResolver::bind]

This seems to have died down close to nothing. I suspect that the remaining crashes fall into two categories. 1) Crashes that would be happening without the patches from this bug. 2) Crashes that only happen with the document splitting pref on. Regarding 1), I think that's clearly outside the scope of my involvement. Regarding 2), I don't have any specific leads as to what might be causing this. I've been looking through the source and I think it's going to take a deeper dive which someone more familiar with WebRender itself might be able to do more quickly. However I'd be happy to be a sounding board for their ideas, and if absolutely necessary I can tackle this, I just suspect I wouldn't be the most efficient choice.

Bug 1547351 - Contents on popup of Addons are shown as white when gfx.webrender.split-render-roots is true

I fixed this on OSX it seems, but the patch appears to not fix things for Windows (uncertain about Linux - will investigate). I will continue working on the remaining bits of this for non-OSX.

Bug 1548247 - document splitting: Sluggish autoscrolling

I have a patch which fixes the exact issue mentioned in the bug, but it seems there are still scrolling problems on about:support and other parent process pages which I haven't been able to sort out. I've been scratching my head against them but I think it will be best if kats takes the rest. I will put up my patches for fixing the autoscrolling issue for review by kats and then I think it will be most efficient if he can take it over.

Bug 1548688 - document splitting: Picture-in-Picture player buttons are visible, but the video itself is just white

I will take this one (unless it unearths some bigger problem that I don't know about. I think this is unlikely).

Bug 1549976 - document splitting: "minecraft.net has control of your mouse pointer" slips behind website content

I think this is going to be rather large. I have a sketch in mind of what we'll need to do, but I think it's going to be a fair bit of code. My proposal for this is that I write up patches to get this mostly working, and that someone more familiar with the gfx pipeline takes that over and hammers out the kinks. I am open to pushback on this, though. I think it's anyone's guess as far as whether I would be the most efficient person for this work after identifying the major pieces.

jbonisteel / pslawless, what are your thoughts? I think these bugs can be in good shape for 59. However, it's not certain that this encompasses all of the work needed to turn on document splitting on Nightly, and it's quite likely that there will be more work which will only surface when we do turn this on for Nightly, so we should probably articulate a plan for my involvement with those unknowns, yes?

Flags: needinfo?(plawless)
Flags: needinfo?(jbonisteel)
Blocks: wr-69
Flags: needinfo?(jbonisteel)

Thanks Doug - that sounds good. For 1538540 we will leave this for now as it has died down and if it picks back up when we re-enable, we will figure out what to do. For 1549976 can you ping me when you have the patches up for the mostly working stuff? That will help determine who on GFX should help with the rest.

Regressions: 1554426
Depends on: 1554985
Flags: needinfo?(plawless)
Depends on: 1555977
Depends on: 1557761
Depends on: 1559389
Depends on: 1562813
No longer depends on: 1562813
Depends on: 1563178
No longer blocks: wr-69
No longer blocks: wr-68
Whiteboard: [wr-q2][wr-april] → [wr-q3][wr-july]
Depends on: 1568887
Depends on: 1569051
Depends on: 1569131
Depends on: 1570981
Whiteboard: [wr-q3][wr-july] → [wr-q3][wr-sept]
Blocks: wr-71
No longer blocks: wr-70
Depends on: 1577225

With latest m-c, enabling document split with pref gfx.webrender.split-render-roots:true caused almost all test failures :(
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8360c69097cd7fa5e1588cb9131e9d38c3aa2e62

Depends on: 1579308
Whiteboard: [wr-q3][wr-sept]
No longer blocks: wr-71
No longer blocks: 1404477
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: