Closed Bug 1019856 Opened 11 years ago Closed 9 years ago

Avoid double buffering in BasicCompositor when nsWindow says its ok

Categories

(Firefox :: General, enhancement)

x86
All
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: handyman, Assigned: lsalzman)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8721051 - Flags: review?(matt.woodrow)
Incorporated feedback from Matt regarding handling of the RT origin.
Attachment #8721051 - Attachment is obsolete: true
Attachment #8721051 - Flags: review?(matt.woodrow)
Attachment #8721065 - Flags: review?(matt.woodrow)
Attachment #8721065 - Flags: review?(matt.woodrow) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
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
Flags: needinfo?(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.
Flags: needinfo?(lsalzman)
(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
(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.
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: