882120 - Regression: Mozilla-Inbound - Robocop Pan Benchmark - Android 2.2 (Native) - 171% increase

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

There appears to be a robopan regression caused by bug 785929.

Previous: avg 11164.550 stddev 1398.303 of 12 runs up to revision c8dc1b737045
New     : avg 30289.458 stddev 3954.638 of 12 runs since revision 57c8642772b5
Change  : +19124.908 (171% / z=13.677)
Graph   : http://mzl.la/12EvNeH

The graph is pretty useless but the inbound view at https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=436c88ed1e5e&jobname=robopan and the try build from that change at https://tbpl.mozilla.org/?tree=Try&rev=37616b14a0f5 do show an increase.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Investigation step the first:

I pushed two try builds, one without my patch [1] and one with my patch [2], both of which had additional logging for the frame times and a few other metrics.

I took all the frame times > 25ms (which is what the test looks at) from all 5 runs of the test and got stats on them.

Before my change:
Samples: 11
Average: 37.3636
Stddev: 12.5273
Max: 48
Min: 29

After my change:
Samples: 67
Average: 33.4776
Stddev: 7.11059
Max: 47
Min: 26

So the number of frames which took longer than 25ms did go up from 11 to 67 (totalled across 5 runs of the test), but with a lower average and standard deviation. This means the frame timing distribution has shifted from a few large janks here and there to a (much) larger number of (slightly) smaller janks.

So I agree that this regression appears to be a legitimate one, particularly given that the code in question isn't supposed to affect this at all.

In the try builds [1] and [2] I also logged the number of calls to GeckoLayerClient.setPageRect and GeckoLayerClient.setFirstPaintViewport to see if perhaps rounding issues at [3] were causing more calls to those functions. Both before and after reported that setPageRect was called 0 times and setFirstPaintViewport was called 2 times (which seems reasonable).

The rest of the patch is really just changes in linear code, so the most likely problem left is that the code being run is more expensive than before, causing slowness. I'll bisect the patch to see if I can verify and isolate the expensive bits.

[1] https://tbpl.mozilla.org/?tree=Try&rev=da6110fa5ccd
[2] https://tbpl.mozilla.org/?tree=Try&rev=b6737c9839f5
[3] https://hg.mozilla.org/integration/mozilla-inbound/rev/a1ebe92839fa#l2.23

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Investigation step the second:

I pushed a try build with what I thought was the trivial half of the patch only at [1], and it also reproduced the regression. I will bisect further.

[1] https://tbpl.mozilla.org/?tree=Try&rev=e6250d5e1c1d

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Investigation step the third:

Another try build at [1] with just a tiny bit of my patch still shows the regression. I'll continue investigating, probably by seeing how different those numbers are and where they end up getting used.

[1] https://tbpl.mozilla.org/?tree=Try&rev=6aaf014d3a1b

Comment 4

[Kartikaya Gupta (email:[e-mail])]

So from investigation step the fourth [1] and fifth [2] it looks like the regression stems from the layer pixel sizes not being rounded. I'm not yet sure why this makes so much of a difference, but I can at least put up a patch to fix it.

[1] https://tbpl.mozilla.org/?tree=Try&rev=e5b0495e962f
[2] https://tbpl.mozilla.org/?tree=Try&rev=55b2778f7f10

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

In bug 785929 I switched from using mContentRect (LayerIntRect) to mScrollableRect (CSSRect) and moving some scaling into Java. Apparently the rounding that was in the old code but not the new code was important, because it causes a panning jank regression. I'm not entirely sure why, but this patch restores the effective behaviour to the same as what it was before and fixes the regression. At some point we'll probably need to do an audit of all the code to figure out where rounding should actually be happening and where it should not, but for now I'd like to just apply this patch and keep moving. This code will be obsoleted by the switch to APZC anyway.

Try build with this applied that shows the fixed robopan scores:
https://tbpl.mozilla.org/?tree=Try&rev=63449495ef2a

Comment 6

[Kartikaya Gupta (email:[e-mail])]

For my own future reference, I did some digging on this code and found the following relevant bugs:

https://bugzilla.mozilla.org/show_bug.cgi?id=744901
https://bugzilla.mozilla.org/show_bug.cgi?id=744916
https://bugzilla.mozilla.org/show_bug.cgi?id=749953

Just leaving these links here so I don't have to dig for them again in case other regressions turn up.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 763251 [details] [diff] [review]
Patch

Uhh whoops. That's not the right patch at all.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Here's the right one. Sorry about that...

Comment 9

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35bf7e6dbf90

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/86ae36fa3ea2 for failing one of the two reftests hiding in crashtests, and also in at least one armv6 reftest run 994 reftests.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch v2

Trimmed the original patch to remove the changes to setPageRect, which I realized wasn't actually affected by the original regressing bug. The try run came back clean and still fixes the regression: https://tbpl.mozilla.org/?tree=Try&rev=f66fa2831132

Carrying the r+ since it's a fairly minor change.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

And landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/63c694785050

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/63c694785050

Comment 14

[Kartikaya Gupta (email:[e-mail])]

The robopan numbers are back down to previous levels.

https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/GiIeW6VoSTM
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/6FRBbe8MahY
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/q_l1PsMeZ7Q