Chrome repaints cause rebuilds of the entire browser scene

NEW
Unassigned

Status

()

enhancement
P3
normal
2 years ago
2 months ago

People

(Reporter: jrmuizel, Unassigned)

Tracking

(Depends on 1 bug, Blocks 2 bugs, {leave-open})

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 unaffected)

Details

(Whiteboard: [wr-reserve])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 3

a year ago
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.
Posted 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+

Comment 17

a year ago
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]
(Reporter)

Updated

11 months ago
No longer blocks: stage-wr-nightly
Keywords: leave-open
Whiteboard: [wr-reserve][leave-open] → [wr-reserve]
(Reporter)

Updated

3 months ago
Blocks: wr-intel
Depends on: document-splitting
Priority: P1 → P3
Assignee: nical.bugzilla → nobody
You need to log in before you can comment on or make changes to this bug.