Open Bug 1404477 Opened 7 years ago Updated 2 years ago

Chrome repaints cause rebuilds of the entire browser scene

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Tracking Flags:
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)

Part 1 - Bindings
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
Merge ResourceUpdates and TransactionBuilder
(deleted), patch
kats
: review+
Details | Diff | Splinter Review
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.
Depends on: 1419851
See Also: → https://github.com/servo/webrender/issues/1781
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) (deleted) — — Splinter Review
Starting to separate out parts of the work in progress that are likely to stay.
Attached patch Part 1 - Bindings (deleted) — — Splinter Review
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]
No longer blocks: stage-wr-nightly
Keywords: leave-open
Whiteboard: [wr-reserve][leave-open] → [wr-reserve]
Blocks: wr-intel
Depends on: document-splitting
Priority: P1 → P3
Assignee: nical.bugzilla → nobody
Blocks: wr-intel-mvp
No longer blocks: wr-intel
No longer blocks: wr-intel-mvp
Blocks: wr-intel
Regressions: 1563622
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)
No longer depends on: document-splitting
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)

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

Flags: needinfo?(ktaeleman)

@Nical: does this need to be left open?

Flags: needinfo?(ktaeleman) → needinfo?(nical.bugzilla)

Yeah. Glenn hopes to be able to make scene building cheaper via more interning magic, It might become cheap enough that we can close this. Otherwise the solution is to do a lot of plumbing work so that the UI and page can be in separate "sub-scenes" that can built independently.

Flags: needinfo?(nical.bugzilla)
Blocks: wr-perf-p1
No longer blocks: stage-wr-next
No longer blocks: wr-perf

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

Flags: needinfo?(jmathies)

This is still in our backlog.

Flags: needinfo?(jmathies)

The leave-open keyword is there and there is no activity for 6 months.
:gw, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

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

Attachment

General

Creator:
Created:
Updated:
Size: