1254897 - Recycle back buffer in BeginFrame

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

We spend a noticeable amount faulting in the new pages for this allocation.

Comment 1

[Lee Salzman [:lsalzman]]

There are two related optimizations that might help here that I was working on previously:

bug 1019856 allows the widget backend to report from StartRemoteDrawingInRegion that the resulting DrawTarget should not be double-buffered inside BasicCompositor, so that BasicCompositor will no longer allocate a new similar DrawTarget each frame. This requires the widget backend to make sure it does the actual double-buffering or syncing itself. That was so far only utilized by nsShmImage + GTK. Other platforms might benefit from this depending on how they work.

bug 1249813 tells the compositor in BeginFrame if it actually needs to clear the backbuffer for this frame, or if it will be completely overwritten with opaque content and can skip the clear. Only the BasicCompositor currently takes advantage of this mainly by changing from INIT_MODE_CLEAR to INIT_MODE_NONE. This is mainly of benefit in combination with the changes for bug 1019856. Making other compositors utilize it may or may not help, depending on whether this bug is intended to address just BasicCompositor or all compositors, as page faults would still happen, but at least some memory churn could be avoided.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Sotaro and I talked about this and decided that we should remove the double buffering from BasicCompositor and move that responsibility into widget. OS X is double buffering by mistake, X only uses the double buffering when not using XShm and we'd rather handle the backbuffer ourselves on Windows.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1- Recycle back buffer in BasicCompositor

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Do not use back buffer on mac

Comment 5

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=358f6609d4f4

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1- Recycle back buffer in BasicCompositor

Fix build failure.

Comment 7

[Sotaro Ikeda [:sotaro]]

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

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1- Recycle back buffer in BasicCompositor

Remove unnecessary part.

Comment 9

[Lee Salzman [:lsalzman]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> Created attachment 8732150 [details] [diff] [review]
> patch part 1- Recycle back buffer in BasicCompositor
> 
> Remove unnecessary part.

I don't understand why these changes are necessary. You are indicating no double-buffering in StartRemoteDrawingInRegion in the part 2 patch with BUFFER_NONE, so this new CreateBackBuffer interface would not actually be used. The reuse could/should be handled in StartRemoteDrawingInRegion since using BUFFER_NONE will cause the BasicCompositor to directly use that and not allocate its own draw target.

Did we really need to add a second way to basically do the same thing to BasicCompositor?

Comment 10

[Matt Woodrow (:mattwoodrow)]

This was done within the compositor to match BasicLayerManager.

We have an optimization there which skips the double buffering if the layer tree consists of only layers that don't overlap each other. We even have a test (test_leaf_layers_partition_browser_window.xul) that checks that this is the common state for a simple webpage.

If the overlapping layers exist entirely within a ContainerLayer descendant, then we only only double buffer that container, rather than the entirety of the window/invalid region.

Moving the double buffering logic into widget will make it hard to implement the same optimizations for BasicCompositor.

Comment 11

[Matt Woodrow (:mattwoodrow)]

I filed bug 994554 for this a while back, adding that (along with bug 994556) to the unaccel-video meta bug.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Lee Salzman [:lsalzman] from comment #9)
> (In reply to Sotaro Ikeda [:sotaro] from comment #8)
> > Created attachment 8732150 [details] [diff] [review]
> > patch part 1- Recycle back buffer in BasicCompositor
> > 
> > Remove unnecessary part.
> 
> I don't understand why these changes are necessary. You are indicating no
> double-buffering in StartRemoteDrawingInRegion in the part 2 patch with
> BUFFER_NONE, so this new CreateBackBuffer interface would not actually be
> used.

Part 2 disable creating back buffer by BasicCompositor on mac, because nsChildView already does double buffering within nsChildView. Then current code does triple buffering for screen. BasicCompositor does not need to allocate back buffer for the widget on mac.

Part 1 care about back buffer allocation by BasicCompositor. "Part 1" re-usees back baffer that is allocated by BasicCompositor to repress the buffer allocation overhead.

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> Moving the double buffering logic into widget will make it hard to implement
> the same optimizations for BasicCompositor.

The patches does not move double buffering logic. bug 994554 seems orthogonal problem.

attachment 8732075 [details] [diff] [review] disable double buffering of BasicCompsoitor, because nsChildView already does double buffering within nsChildView. Additional double buffering in BasicCompositor is redundant.

attachment 8732150 [details] [diff] [review] just adds back buffer recycling that is allocated for BasicCompsoitor. The patch allocates a back buffer in widget code. It is a preparation for Bug 1255703 to handle DIBSection thing as in Bug 1255303  Comment 7. The back buffer allocation function calling is still in BasicCompositor::CreateRenderTargetForWindow().

Comment 14

[Sotaro Ikeda [:sotaro]]

:mattwoodrow, can't we handle bug 994554 as a different problem?

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> :mattwoodrow, can't we handle bug 994554 as a different problem?

Sorry Sotaro, my comment was replying to Comment 2.

The current patches look great.

Comment 16

[Sotaro Ikeda [:sotaro]]

:mattwoodrow, thanks for the quick reply.

Comment 17

[Lee Salzman [:lsalzman]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)
> (In reply to Lee Salzman [:lsalzman] from comment #9)
> > (In reply to Sotaro Ikeda [:sotaro] from comment #8)
> > > Created attachment 8732150 [details] [diff] [review]
> > > patch part 1- Recycle back buffer in BasicCompositor
> > > 
> > > Remove unnecessary part.
> > 
> > I don't understand why these changes are necessary. You are indicating no
> > double-buffering in StartRemoteDrawingInRegion in the part 2 patch with
> > BUFFER_NONE, so this new CreateBackBuffer interface would not actually be
> > used.
> 
> Part 2 disable creating back buffer by BasicCompositor on mac, because
> nsChildView already does double buffering within nsChildView. Then current
> code does triple buffering for screen. BasicCompositor does not need to
> allocate back buffer for the widget on mac.
> 
> Part 1 care about back buffer allocation by BasicCompositor. "Part 1"
> re-usees back baffer that is allocated by BasicCompositor to repress the
> buffer allocation overhead.

Part 1 does not work. When you use BUFFER_NONE (as in part 2), this means the CreateBackBufferDrawTarget interface you added does not get called, so it does nothing. So as it stands, these patches do not work at all.

Comment 18

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #17)
> (In reply to Sotaro Ikeda [:sotaro] from comment #12)
> > (In reply to Lee Salzman [:lsalzman] from comment #9)
> > > (In reply to Sotaro Ikeda [:sotaro] from comment #8)
> > > > Created attachment 8732150 [details] [diff] [review]
> > > > patch part 1- Recycle back buffer in BasicCompositor
> > > > 
> > > > Remove unnecessary part.
> > > 
> > > I don't understand why these changes are necessary. You are indicating no
> > > double-buffering in StartRemoteDrawingInRegion in the part 2 patch with
> > > BUFFER_NONE, so this new CreateBackBuffer interface would not actually be
> > > used.
> > 
> > Part 2 disable creating back buffer by BasicCompositor on mac, because
> > nsChildView already does double buffering within nsChildView. Then current
> > code does triple buffering for screen. BasicCompositor does not need to
> > allocate back buffer for the widget on mac.
> > 
> > Part 1 care about back buffer allocation by BasicCompositor. "Part 1"
> > re-usees back baffer that is allocated by BasicCompositor to repress the
> > buffer allocation overhead.
> 
> Part 1 does not work. When you use BUFFER_NONE (as in part 2), this means
> the CreateBackBufferDrawTarget interface you added does not get called, so
> it does nothing. So as it stands, these patches do not work at all.

Maybe I am misunderstanding the intent of part 2. Was it your intent to NOT use the code in part 1 on Mac, and the code in part 1 is intended rather for everything else?

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Lee Salzman [:lsalzman] from comment #18)
> 
> Maybe I am misunderstanding the intent of part 2. Was it your intent to NOT
> use the code in part 1 on Mac, and the code in part 1 is intended rather for
> everything else?

Yes, part 1 is back buffer allocation for BasicCompositor. part 2 disable the allocation on mac because widget on mac does double buffering by itself.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8732150 [details] [diff] [review]
patch part 1- Recycle back buffer in BasicCompositor

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

::: widget/nsBaseWidget.h
@@ +527,5 @@
>    RefPtr<CompositorParent> mCompositorParent;
>    RefPtr<mozilla::CompositorVsyncDispatcher> mCompositorVsyncDispatcher;
>    RefPtr<APZCTreeManager> mAPZC;
>    RefPtr<APZEventState> mAPZEventState;
> +  RefPtr<DrawTarget> mLastBackBuffer;

Do you have plans to reuse this member in the other backends that do recycling?

Comment 21

[Sotaro Ikeda [:sotaro]]

I do not have a clear plan yet, but it seems better to use the member if backends do double buffering by software.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1- Recycle back buffer in BasicCompositor

Rebased.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0662b0935257

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0662b0935257

Comment 25

[Joel Maher ( :jmaher ) (UTC -8)]

this looks to have some perf wins!
https://treeherder.mozilla.org/perf.html#/alerts?id=604

Comment 26

[Benoit Girard (:BenWa)]

Improvements also reported in the microbenchmark suite:
https://treeherder.allizom.org/perf.html#/alerts?id=733