1019856 - Avoid double buffering in BasicCompositor when nsWindow says its ok

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[David Parks [:handyman]]

Splitting bug 994554 into three separate tasks.  From that bug:

> Don't double buffer unless the OS actually needs us to. The widget code 
> figures this out as a value of BufferMode, see http://mxr.mozilla.org/mozilla-
> central/ident?i=BUFFERED
> 
> We pass this into BasicLayerManager, we could do something very similar, 
> probably via the nsIWidget::StartRemoteDrawing call in 
> BasicCompositor::BeginFrame.
> 
> This should be really easy, but it won't actually have any effect on windows 
> or linux which is probably all we care about.

Presently, BufferMode::BUFFERED is used for all layers on Windows and Linux, so this is irrelevant on those OSs.

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: avoid double-buffering in BasicCompositor when window allows it

Now that we've disabled xrender and are relying on the BasicCompositor with XShm as our workhorse on X11 platforms, the extra copying in BasicCompositor hurts us.

This lets nsWindow::StartRemoteDrawingInRegion report back whether it wants to be buffered or not, so that the BasicCompositor can then avoid the copy when it is not needed.

Comment 2

[Lee Salzman [:lsalzman]]

Attachment: avoid double-buffering in BasicCompositor when window allows it

Incorporated feedback from Matt regarding handling of the RT origin.

Comment 3

[Lee Salzman [:lsalzman]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96cd3da5e661

Comment 4

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae59fd411ba

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6ae59fd411ba

Comment 6

[William Lachance (:wlach)]

This change seems to have improved perf on the talos scrolling tests with e10s enabled, and makes it worse when not. I assume this is expected?

https://treeherder.allizom.org/perf.html#/alerts?id=189

Comment 7

[Lee Salzman [:lsalzman]]

(In reply to William Lachance (:wlach) from comment #6)
> This change seems to have improved perf on the talos scrolling tests with
> e10s enabled, and makes it worse when not. I assume this is expected?
> 
> https://treeherder.allizom.org/perf.html#/alerts?id=189

I tried to address the issues that I saw, as bug 1249813, and in which I managed to improve performance on a few other talos tests. But other than that I could not reproduce the strange tscrollx results locally and it would take far more time than is conscionable to get to the root of why only try manifests the problem. So we're just going to accept it, since it still puts us on par with what try was reporting for tscrollx when we had xrender enabled.

Comment 8

[Marco Castelluccio [:marco]]

(In reply to Lee Salzman [:lsalzman] from comment #7)
> So we're just going to accept it, since it still puts
> us on par with what try was reporting for tscrollx when we had xrender
> enabled.

Isn't tscrollx still worse?

From bug 1249813:
tscrollx summary linux64 opt e10s  (acknowledged)   graph · subtests 	2.61 	< 	6.7

This bug:
tscrollx summary linux64 opt e10s  (reassigned from alert #194 )   graph · subtests 	6.2 	> 	4.9

Comment 9

[Lee Salzman [:lsalzman]]

(In reply to Marco Castelluccio [:marco] from comment #8)
> (In reply to Lee Salzman [:lsalzman] from comment #7)
> > So we're just going to accept it, since it still puts
> > us on par with what try was reporting for tscrollx when we had xrender
> > enabled.
> 
> Isn't tscrollx still worse?
> 
> From bug 1249813:
> tscrollx summary linux64 opt e10s  (acknowledged)   graph · subtests 	2.61 
> < 	6.7
> 
> This bug:
> tscrollx summary linux64 opt e10s  (reassigned from alert #194 )   graph ·
> subtests 	6.2 	> 	4.9

Those numbers are from after we disabled xrender. Before we disabled xrender, they were more or less similar to the current numbers. But as stated, I already spent too much time investigating it, and in performance measured on my local machine (not try), we're up. So, just considering the try regression WONTFIX.

Comment 10

[Marco Castelluccio [:marco]]

(In reply to Lee Salzman [:lsalzman] from comment #9)
> (In reply to Marco Castelluccio [:marco] from comment #8)
> > (In reply to Lee Salzman [:lsalzman] from comment #7)
> > > So we're just going to accept it, since it still puts
> > > us on par with what try was reporting for tscrollx when we had xrender
> > > enabled.
> > 
> > Isn't tscrollx still worse?
> > 
> > From bug 1249813:
> > tscrollx summary linux64 opt e10s  (acknowledged)   graph · subtests 	2.61 
> > < 	6.7
> > 
> > This bug:
> > tscrollx summary linux64 opt e10s  (reassigned from alert #194 )   graph ·
> > subtests 	6.2 	> 	4.9
> 
> Those numbers are from after we disabled xrender. Before we disabled
> xrender, they were more or less similar to the current numbers. But as
> stated, I already spent too much time investigating it, and in performance
> measured on my local machine (not try), we're up. So, just considering the
> try regression WONTFIX.

Sorry, I mentioned the wrong bug, the first numbers are from bug 1241832 comment 4 (where we disabled xrender).

My experience from bug 1247935 might be related, since it's related to scrolling.