Closed
Bug 882120
Opened 12 years ago
Closed 11 years ago
Regression: Mozilla-Inbound - Robocop Pan Benchmark - Android 2.2 (Native) - 171% increase
Categories
(Firefox for Android Graveyard :: Toolbar, defect)
Tracking
(firefox23 unaffected, firefox24+ fixed, fennec24+)
VERIFIED
FIXED
Firefox 24
Tracking | Status | |
---|---|---|
firefox23 | --- | unaffected |
firefox24 | + | fixed |
fennec | 24+ | --- |
People
(Reporter: kats, Assigned: kats)
References
Details
Attachments
(1 file, 2 obsolete files)
2.95 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
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
Updated•12 years ago
|
tracking-fennec: --- → ?
status-firefox23:
--- → unaffected
status-firefox24:
--- → affected
tracking-firefox24:
--- → ?
Assignee | ||
Comment 2•12 years ago
|
||
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
Updated•12 years ago
|
tracking-fennec: ? → 24+
Assignee | ||
Comment 3•12 years ago
|
||
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
Assignee | ||
Comment 4•12 years ago
|
||
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
Assignee | ||
Comment 5•12 years ago
|
||
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
Attachment #763251 -
Flags: review?(ajones)
Assignee | ||
Comment 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•11 years ago
|
||
Comment on attachment 763251 [details] [diff] [review]
Patch
Uhh whoops. That's not the right patch at all.
Attachment #763251 -
Flags: review?(ajones)
Assignee | ||
Comment 8•11 years ago
|
||
Here's the right one. Sorry about that...
Attachment #763251 -
Attachment is obsolete: true
Attachment #763532 -
Flags: review?(ajones)
Updated•11 years ago
|
Attachment #763532 -
Flags: review?(ajones) → review+
Updated•11 years ago
|
Assignee | ||
Comment 9•11 years ago
|
||
Comment 10•11 years ago
|
||
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.
Assignee | ||
Comment 11•11 years ago
|
||
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.
Attachment #763532 -
Attachment is obsolete: true
Attachment #765444 -
Flags: review+
Assignee | ||
Comment 12•11 years ago
|
||
And landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/63c694785050
Comment 13•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Assignee | ||
Comment 14•11 years ago
|
||
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
Status: RESOLVED → VERIFIED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•