985477 - windows 8 paint regression on inbound related to bug 979853

Status: RESOLVED INCOMPLETE

(Core :: Graphics, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

Looking at some graphs, I see that the talos paint test has regressed a bit:
http://graphs.mozilla.org/graph.html#tests=[[82,132,31],[82,203,31],[82,131,31]]&sel=1393948987868.9211,1394406045873.5615&displayrange=30&datatype=running

and the datazilla graph yields the same results: 
https://datazilla.mozilla.org/?start=1393871795&stop=1394571987&product=Firefox&repository=Mozilla-Inbound-Non-PGO&os=win&os_version=6.2.9200&test=tpaint&graph_search=5de7483b01d2&tr_id=4736168&graph=tpaint&project=talos

both graphs indicate that the patch from bug 979973 are the cause.

:cwiiis, can you take a look at this and verify it is your patch and propose a fix?

Comment 1

[Chris Lord [:cwiiis]]

Does these tests use OMTC and APZC? Because that's the only way that bug would have an effect. I don't think bug 979973 is the cause here.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Are these used when opening a new firefox chrome window?  do we need to set a preference or do something to use OMTC or APZC?

Comment 3

[Chris Lord [:cwiiis]]

(In reply to Joel Maher (:jmaher) from comment #2)
> Are these used when opening a new firefox chrome window?  do we need to set
> a preference or do something to use OMTC or APZC?

APZC would be required to hit this code (I said OMTC because OMTC is required for APZC). APZC is only enabled on fennec, b2g and Metro Firefox.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

got it.  this is only seen on windows 8, I know that isn't metro firefox- just double checking.  Let me do some retriggers and look at other related pushes.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

it seems there was a push in the middle that didn't run this test, now I have done some retriggers and it points at:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=b850bc9ae306&tochange=efbea5e8c766&rev=2ac59d28974c

:jwatt, you landed 3 patches from 3 different bugs, can you take a brief look at this?

here is a link to the test:
https://wiki.mozilla.org/Buildbot/Talos/Tests#tpaint

Comment 6

[Jonathan Watt [:jwatt]]

That could only be bug 979853. There's an issue with the "Convert format to SurfaceFormat::B8G8R8A8" block that I've become aware of since that push (blindly using a data wrapping DrawTarget can result in extra upload to the GPU and then readback). Bug 985545 adds a helper that is intended to avoid this issue, so let's first fix that and see if that solves this regression.

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

thanks :jwatt.

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] from comment #6)
> That could only be bug 979853. There's an issue with the "Convert format to
> SurfaceFormat::B8G8R8A8" block that I've become aware of since that push
> (blindly using a data wrapping DrawTarget can result in extra upload to the
> GPU and then readback). Bug 985545 adds a helper that is intended to avoid
> this issue, so let's first fix that and see if that solves this regression.

Actually, I've just got a Windows 8 build up and running, and loaded:

http://hg.mozilla.org/build/talos/raw-file/tip/talos/startup_test/tpaint.html

after setting the prefs mentioned in that file's source.

Of the code added in bug 979853 only nsWindowGfx::CreateIcon is hit, and when it's hit we always end up in the |else| statements that simply set |dataSurface = surface->GetDataSurface();|. So I'm not sure how my changes could have caused this regression (since we don't hit the "Convert format to SurfaceFormat::B8G8R8A8" code).

I'll take a closer look tomorrow.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

thanks, ping me on IRC if you need a second set of eyes to review data or ensure you have the same prefs as talos.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

checking in here, <2 weeks until the uplift.

Comment 11

[Jonathan Watt [:jwatt]]

I wasn't able to get much from a profile in my Win 8 VM. That said, the only thing that seems capable of creating a perf regression is the GetSourceSurfaceForSurface() call in nsWindowGfx::CreateIcon (we don't hit the SurfaceToPackedBGRA() call there and, as noted in comment 8, we only end up in the |else| statement in that function).

There's not much we can do about the GetSourceSurfaceForSurface() call untill it goes away in bug 950372. I'm hoping the patch there not only regains any lost ground but actually does more than that since it removes a bunch of conversion code.

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

thanks!  I look forward to bug 950372 landing, I see a r+ there- a good sign that we can fix this prior to the uplift after next week.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

some confusion on my part here, but I think this graph answers the questions:
http://graphs.mozilla.org/graph.html#tests=[[82,53,31],[82,52,31],[82,63,31],[82,1,31]]&sel=1391172255010,1398948255011&displayrange=90&datatype=running

This specific regression from March 6th invalidates a win we had ~Feb14th (probably http://hg.mozilla.org/integration/mozilla-inbound/rev/c7802c9d6eec).  So the net result when this was uplifted to Aurora on March 16th, was no change!  Likewise as this is now on beta, we have no concerns.

While this is a true regression, we could do some work to back it out or fix it, looking at the big picture and asking "are we regression firefox performance for release of v30" the answer is no.

Unfortunately the firefox 31 story is scarier, but that is unrelated to this bug :)

Comment 14

[Jonathan Watt [:jwatt]]

(In reply to Joel Maher (:jmaher) from comment #13)
> This specific regression from March 6th invalidates a win we had ~Feb14th

That sucks.

> (probably
> http://hg.mozilla.org/integration/mozilla-inbound/rev/c7802c9d6eec).

That changeset doesn't seem right, since it only changes the text of a couple of menu items.

> So the
> net result when this was uplifted to Aurora on March 16th, was no change! 
> Likewise as this is now on beta, we have no concerns.

Although it would still be nice to avoid cancelling out the previous win.

> While this is a true regression, we could do some work to back it out or fix
> it

Backing it out is now probably impractical since a bunch of other work has built on this change (had already done so before I became aware of this bug). I think I need to get my hands on a Win8 machine to debug what happened.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

after chatting in IRC with jwatt, we don't see value in tracking this for firefox31, but we will leave this open so jwatt has visibility into this for possible future perf wins in this general code.