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

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.

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.

Fix build failure.

Remove unnecessary part.

(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?

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.

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

(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.

(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().

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

(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.

: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?

(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 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?

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

Rebased.

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