Recycle back buffer in BeginFrame

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jrmuizel, Assigned: sotaro, NeedInfo)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
We spend a noticeable amount faulting in the new pages for this allocation.
(Reporter)

Updated

3 years ago
Blocks: 1253062
Whiteboard: [gfx-noted]
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → sotaro.ikeda.g
(Reporter)

Updated

3 years ago
Blocks: 1255703
(Reporter)

Comment 2

3 years ago
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.
(Assignee)

Updated

3 years ago
Depends on: 1255303
(Assignee)

Comment 3

3 years ago
Created attachment 8731973 [details] [diff] [review]
patch part 1- Recycle back buffer in BasicCompositor
(Assignee)

Updated

3 years ago
Attachment #8731973 - Attachment description: Bug 1254897 - Recycle back buffer in BasicCompositor → patch part 1- Recycle back buffer in BasicCompositor
(Assignee)

Comment 4

3 years ago
Created attachment 8732075 [details] [diff] [review]
patch part 2 - Do not use back buffer on mac
(Assignee)

Comment 6

3 years ago
Created attachment 8732109 [details] [diff] [review]
patch part 1- Recycle back buffer in BasicCompositor

Fix build failure.
Attachment #8731973 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8732075 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8732109 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
Attachment #8732109 - Flags: review?(jmuizelaar)
(Assignee)

Comment 8

3 years ago
Created attachment 8732150 [details] [diff] [review]
patch part 1- Recycle back buffer in BasicCompositor

Remove unnecessary part.
Attachment #8732109 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8732150 - Flags: review?(jmuizelaar)
(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?
(Reporter)

Updated

3 years ago
Attachment #8732075 - Flags: review?(jmuizelaar) → review+
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.
Flags: needinfo?(jmuizelaar)
I filed bug 994554 for this a while back, adding that (along with bug 994556) to the unaccel-video meta bug.
(Assignee)

Comment 12

3 years ago
(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.
(Assignee)

Comment 13

3 years ago
(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().
(Assignee)

Comment 14

3 years ago
:mattwoodrow, can't we handle bug 994554 as a different problem?
Flags: needinfo?(matt.woodrow)
(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.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 16

3 years ago
:mattwoodrow, thanks for the quick reply.
(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.
(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?
(Assignee)

Comment 19

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

Comment 20

3 years ago
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?
Attachment #8732150 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 21

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

Comment 22

3 years ago
Created attachment 8734211 [details] [diff] [review]
patch part 1- Recycle back buffer in BasicCompositor

Rebased.
Attachment #8732150 - Attachment is obsolete: true
Attachment #8734211 - Flags: review+

Comment 24

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0662b0935257
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
this looks to have some perf wins!
https://treeherder.mozilla.org/perf.html#/alerts?id=604
Improvements also reported in the microbenchmark suite:
https://treeherder.allizom.org/perf.html#/alerts?id=733
(Assignee)

Updated

3 years ago
No longer depends on: 1255303

Updated

2 years ago
Depends on: 1275680
(Assignee)

Updated

2 years ago
Depends on: 1276403
(Assignee)

Updated

2 years ago
No longer depends on: 1276403
You need to log in before you can comment on or make changes to this bug.