Open Bug 1404477 Opened 3 years ago Updated 5 months ago

Chrome repaints cause rebuilds of the entire browser scene

Categories

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

enhancement

Tracking

()

Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected

People

(Reporter: jrmuizel, Unassigned)

References

(Blocks 2 open bugs)

Details

(Keywords: leave-open, Whiteboard: [wr-reserve])

Attachments

(2 files, 1 obsolete file)

For example: Today I get 22ms of cpu time per frame when a throbber on another tab is spinning with gmail open in the foreground.
Whiteboard: [wr-mvp] [triage]
Priority: -- → P2
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
If the throbber is an OMT animation then bug 1419851 should help here. There is also some discussion in servo/webrender#1781 about how to split up the display list into separate documents so that the chrome stuff is in a separate document from the content stuff. That seems to be the suggested way of dealing with this problem in the general case.
Priority: P2 → P3
Whiteboard: [wr-mvp] → [wr-reserve]
Assignee: nobody → nical.bugzilla
So this is sort of fixed now, we at least don't rebuild the scene. However we still need to do the document api work. So it may be worth pivoting this to that.
Attached patch Part 1 - Bindings (obsolete) — Splinter Review
Starting to separate out parts of the work in progress that are likely to stay.
Attachment #8944678 - Attachment is obsolete: true
I am making some progress here, but there are more unresolved than resolved questions:

- The document API doesn't allow synchronizing transactions between documents, which is probably not a bad thing, but resets the Austin discussion about synchronizing the transactions for content and chrome.

- We probably need to rethink the way we synchronize/throttle frames. Currently we have a per-widget counter that is incremented when scheduling a frame and decremented when the frame is rendered. because we have to call generate_frame for each  document, this system breaks as soon as we have more than one document. I am not sure what's the best thing to do about this yet.

- Right now the chrome renders things above *and* below the content, and that isn't going to work with only one document for the chrome and one for the content. The main reason for the chrome to draw under the content seem to be the widget background. I think that the best thing to do is to render all of the chrome on top of the content and not have the widget background rendered by the chrome on the content area. If for whatever reason we don't have any content document to fill in the content area we can always have a placeholder document that draws the widget background there.

- I am not sure whether various elements of the chrome (like the little link target url overlay) need to be broken up into separate documents, or if we should have a big chrome document that covers the whole window and contain everything. Long term I suspect they should be separate documents and have their own composition surfaces.
(In reply to Nicolas Silva [:nical] from comment #6)
> - I am not sure whether various elements of the chrome (like the little link
> target url overlay) need to be broken up into separate documents, or if we
> should have a big chrome document that covers the whole window and contain
> everything. Long term I suspect they should be separate documents and have
> their own composition surfaces.

I thought the link URL tooltip was a popup widget, is that not the case?
> I thought the link URL tooltip was a popup widget, is that not the case?

I believe the tooltip you see near the cursor when hovering over a link has its own widget but not the one on the lower left corner of the browser window.
(In reply to Nicolas Silva [:nical] from comment #6)
> - We probably need to rethink the way we synchronize/throttle frames.
> Currently we have a per-widget counter that is incremented when scheduling a
> frame and decremented when the frame is rendered. because we have to call
> generate_frame for each  document, this system breaks as soon as we have
> more than one document. I am not sure what's the best thing to do about this
> yet.

If we want to draw 2 documents(chrome and content) in one window, don't we need to generate all 2 document's frames even when we updated only one document?

For me, per-widget counter and throttling are for throttling a task of GLContext that represents a window surface. The task of GLContext needs to be based on Vsync. Then if we continue to use one GLContext(one window surfae) for one window, it seems that we could continue to use current per-widget counter and throttling.
(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> 
> For me, per-widget counter and throttling are for throttling a task of
> GLContext that represents a window surface. The task of GLContext needs to
> be based on Vsync. Then if we continue to use one GLContext(one window
> surfae) for one window, it seems that we could continue to use current
> per-widget counter and throttling.

But it seems necessary to add per document frame counter to manage async image pipeline and WebRenderTextureHost.
Anther thing to resolve: We need to add some way to resize documents atomically. The plan could be to update each document on their own transaction without rendering them and then have a specific generate frame on the api object to render all updated documents at once.
This is something that will happen in webrender soon-ish so we might as well do the equivalent in gecko now. It should facilitate the webrender update (we'll only have to tweak the bindings a bit) and also this simplifies s subsequent patch for this bug.
Attachment #8945514 - Flags: review?(bugmail)
Comment on attachment 8945063 [details] [diff] [review]
Part 1 - Bindings

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

Missing bindings for the document API.
Attachment #8945063 - Flags: review?(bugmail)
Even if I put this bug on hold for a month, we are better off landing the first two patches earlier rather than later.
Comment on attachment 8945514 [details] [diff] [review]
Merge ResourceUpdates and TransactionBuilder

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

r+ with a commit message

::: gfx/webrender_bindings/WebRenderAPI.h
@@ +143,5 @@
>  protected:
>    Transaction* mTxn;
> +  wr::ResourceUpdates* mResourceUpdates;
> +
> +  friend class WebRenderAPI;

Rather than make WebRenderAPI a friend I'd prefer to add a RawUpdates() function that returns the mResourceUpdates pointer and just the WebRenderAPI code accordingly.
Attachment #8945514 - Flags: review?(bugmail) → review+
Comment on attachment 8945063 [details] [diff] [review]
Part 1 - Bindings

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

r+ with comments addressed. Also for future reference this review would have been much easier in MozReview because I wouldn't have to keep switching away to find more context.

::: gfx/webrender_bindings/WebRenderAPI.h
@@ +231,2 @@
>    RefPtr<wr::WebRenderAPI> mRootApi;
> +  RefPtr<wr::WebRenderAPI> mRootDocument;

I'd like to rename mRootDocument to mRootDocumentApi. Also fix the typos "alaive" and "teh" in the comment, and add a sentence clarifying that mRootDocumentApi is null for the WebRenderAPI holding the root doc, and non-null for other APIs that point to some other API's doc.
Attachment #8945063 - Flags: review?(bugmail) → review+
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/593402ddfee5
WebRender document API bindings. r=kats
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8da37aaabfc
Merge ResourceUpdateQueue and TransactionBuilder. r=kats
Whiteboard: [wr-reserve] → [wr-reserve][leave-open]
Keywords: leave-open
Whiteboard: [wr-reserve][leave-open] → [wr-reserve]
Blocks: wr-intel
Depends on: document-splitting
Priority: P1 → P3
Assignee: nical.bugzilla → nobody
No longer regressions: 1563622

The leave-open keyword is there and there is no activity for 6 months.
:jbonisteel, maybe it's time to close this bug?

Flags: needinfo?(jbonisteel)

We will leave it open for now still

Flags: needinfo?(jbonisteel)
Blocks: wr-perf
No longer blocks: wr-intel
Flags: needinfo?(gwatson)

This is a bit hand-wavy, and needs more detail, but I think we can roughly say:

  • Start traversal of the scene from the root pipeline id (as we do now).
  • When we encounter an iframe element, we know that we create separate picture cache tiles for that pipeline.
  • If we see that there is no new display list for that child pipeline (we know this already), we should be able to say that the picture cache tiles for that pipeline are valid, and skip recursing into the display list for that pipeline.

Even if the clips and/or visible part of the child pipeline change, just drawing with the existing frame builder should correctly determine which of those tiles need to be allocated / rasterized. Equally, if the animated transforms, opacity etc have changed, they will still be valid as we know the animation property IDs haven't changed.

There's probably some complexity related to how we retain certain parts of the frame builder while replacing other bits, which we don't currently handle. This might be a bit tedious to refactor, but doesn't sound too complex.

We'd probably also refactor how picture cache tiles are stored and retained, so that they hang off the data structures stored for each pipeline.

For the case where the content pipeline has changed, and not the parent pipeline, we could probably just pay the cost of traversing the parent scene, since it's typically very small relative to the content.

There's also a bit of complexity needed to ensure that child iframes of the content don't have new display lists. But we should be able to handle this by tracking iframe dependencies for each pipeline.

Flags: needinfo?(gwatson)
You need to log in before you can comment on or make changes to this bug.