Do not needlessly clear the compositor background before drawing

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: bas, Assigned: bas)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

4 months ago
Right now we always clear the invalid region of the backbuffer in CompositorD3D11::BeginFrame, there's not a real good reason to do that when we're going to overwrite that with new content. We pass an opaque region to BeginFrame already and can use that to restrict the area that we clear.
Comment hidden (mozreview-request)

Comment 2

4 months ago
mozreview-review
Comment on attachment 8853446 [details]
Bug 1352442: Only clear transparent regions of the back buffer on BeginFrame.

https://reviewboard.mozilla.org/r/125536/#review128122
Attachment #8853446 - Flags: review?(matt.woodrow) → review+

Comment 3

4 months ago
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58181c34aa51
Only clear transparent regions of the back buffer on BeginFrame. r=mattwoodrow
https://hg.mozilla.org/mozilla-central/rev/58181c34aa51
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
it looks like this patch had a huge win on performance according to talos:
== Change summary for alert #5801 (as of March 31 2017 18:51 UTC) ==

Improvements:

  9%  tscrollx summary windows8-64 pgo      2.73 -> 2.49
  8%  tscrollx summary windows7-32 pgo      2.76 -> 2.53
  7%  tp5o_scroll summary windows8-64 pgo e10s1.99 -> 1.85
  7%  tp5o_scroll summary windows7-32 opt e10s2.41 -> 2.23
  7%  tp5o_scroll summary windows8-64 opt e10s2.02 -> 1.88
  6%  tscrollx summary windows8-64 pgo e10s 2.46 -> 2.3
  6%  tscrollx summary windows8-64 opt e10s 2.43 -> 2.29
  6%  glterrain summary windows7-32 opt e10s5.18 -> 4.87
  5%  tpaint linux64 pgo                    239.09 -> 226.9
  3%  tp5o_scroll summary windows7-32 pgo   2.54 -> 2.46
  3%  tpaint linux64 opt                    286.64 -> 277.97
  2%  damp summary linux64 pgo              283.87 -> 277.88

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5801
Blocks: 1330814
Is this something we'd consider backporting to 54 given that it fixed at least one known correctness issue, or should we let it ride the 55 train?
Flags: needinfo?(bas)
(Assignee)

Comment 7

3 months ago
(In reply to Joel Maher ( :jmaher) from comment #5)
> it looks like this patch had a huge win on performance according to talos:
> == Change summary for alert #5801 (as of March 31 2017 18:51 UTC) ==
> 
> Improvements:
> 
>   9%  tscrollx summary windows8-64 pgo      2.73 -> 2.49
>   8%  tscrollx summary windows7-32 pgo      2.76 -> 2.53
>   7%  tp5o_scroll summary windows8-64 pgo e10s1.99 -> 1.85
>   7%  tp5o_scroll summary windows7-32 opt e10s2.41 -> 2.23
>   7%  tp5o_scroll summary windows8-64 opt e10s2.02 -> 1.88
>   6%  tscrollx summary windows8-64 pgo e10s 2.46 -> 2.3
>   6%  tscrollx summary windows8-64 opt e10s 2.43 -> 2.29
>   6%  glterrain summary windows7-32 opt e10s5.18 -> 4.87
>   5%  tpaint linux64 pgo                    239.09 -> 226.9
>   3%  tp5o_scroll summary windows7-32 pgo   2.54 -> 2.46
>   3%  tpaint linux64 opt                    286.64 -> 277.97
>   2%  damp summary linux64 pgo              283.87 -> 277.88
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=5801

The linux ones must be flukes. This patch has no affect on Linux. The Windows improvements are believeable.
(Assignee)

Comment 8

3 months ago
(In reply to [Out of Office Until 24-April] Ryan VanderMeulen [:RyanVM] from comment #6)
> Is this something we'd consider backporting to 54 given that it fixed at
> least one known correctness issue, or should we let it ride the 55 train?

This should be a low-risk patch to backport. I'll leave it up to release folks to decide if the risk and disruption of any backport is justified at this point in the cycle.
Flags: needinfo?(bas)
Whiteboard: [qf:p1]
You need to log in before you can comment on or make changes to this bug.