Closed
Bug 898416
Opened 11 years ago
Closed 11 years ago
Reflow Timer Test does not run on my phone
Categories
(Firefox for Android Graveyard :: Toolbar, defect)
Tracking
(fennec+)
RESOLVED
FIXED
Firefox 28
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: marcia, Assigned: jrmuizel)
References
()
Details
(Keywords: regression, reproducible)
Attachments
(2 files, 2 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Samsung Galaxy S3 (SPH-L170) running Firefox 22
Android Version 4.1.2
STR:
1. Load site in the URL field
2. Press blue button to begin test
3. Tests begin to run but I can never get past the Display Test (Test 1 of 5/Pass 3 of 3). White screen shows in the background while the test is running. Test runs very slowly compared to Desktop and FF OS.
Test runs fine in the desktop browser as well as on Firefox OS.
Logcat attached. While running the test I turned power management off and increased the screen timeout to 10 minutes.
Comment 1•11 years ago
|
||
It progresses painfully slow on my Samsung Galaxy SIV (4.3), Nightly (07/26).
Keywords: reproducible
Comment 2•11 years ago
|
||
The test completed on Fennec running on a Nexus 4. It tool almost 40 minutes. I also ran Chrome on the same Nexus 4. It completed in less than 10 seconds.
The results are interesting though:
Fennec on Nexus 4
testDisplay: 1529
testVisibility: 152
testFourClassReflows: 1087
testFourScriptReflows: 1083
testFontSizeEm: 1104
Chrome on Nexus 4
testDisplay: 888
testVisibility: 490
testFourClassReflows: 990
testFourScriptReflows: 983
testFontSizeEm: 1027
Results for Fennec are in the ballpark with Chrome. How is that possible given the amount of time it takes to complete the test? Somehow whatever is being benchmarked is not dramatically affected by the slow test completion.
Comment 3•11 years ago
|
||
A good candidate to profile using the Gecko Profiler.
Comment 4•11 years ago
|
||
ask and you shall receive.
http://people.mozilla.com/~bgirard/cleopatra/#report=b02cb82629fe70d7ef0d8bddd000de24ffc59c06
93.2% of our time was spent in ContainerState:ProcessDisplayItems:
Comment 5•11 years ago
|
||
recreating the Cleopatra display in ASCII art:
100.0% Startup::XRE_Main
99.8% |_nsRefreshDriver::Tick
98.6% |_Paint::PresShell::Paint
98.6% |_nsLayoutUtils::PaintFrame
98.5% |_nsDisplayList::PaintRoot
93.2% |_ContainerState::ProcessDisplayItems
93.2% |_ContainerState::ProcessDisplayItems
93.2% |_ContainerState::ProcessDisplayItems
Comment 6•11 years ago
|
||
I had looked at this earlier. One thing I tried was to disable font inflation by setting the prefs to 0, this did not result in any speedup in the results.
The first result is the run without font inflation.
* 1453 974 954 1064 166 Mozilla/5.0 (Android; Mobile; rv:25.0) Gecko/25.0
* 1427 986 990 1026 184 Mozilla/5.0 (Android; Mobile; rv:25.0) Gecko/25.0
* 1346 1051 1046 1027 141 Mozilla/5.0 (Android; Mobile; rv:25.0) Gecko/25.0
Comment 7•11 years ago
|
||
Note that this test runs fine for me, it just takes way too long. ~40 minutes on Galaxy Nexus, a couple hours on an HTC Status.
Comment 8•11 years ago
|
||
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #5)
> recreating the Cleopatra display in ASCII art:
>
> 100.0% Startup::XRE_Main
> 99.8% |_nsRefreshDriver::Tick
> 98.6% |_Paint::PresShell::Paint
> 98.6% |_nsLayoutUtils::PaintFrame
> 98.5% |_nsDisplayList::PaintRoot
> 93.2% |_ContainerState::ProcessDisplayItems
> 93.2% |_ContainerState::ProcessDisplayItems
> 93.2% |_ContainerState::ProcessDisplayItems
jchen got a few more symbols in his profile, so the next level down is:
33.6% |-nsRegion::InsertInPlace()
19.3% |_nsRegion::Optimize()
http://people.mozilla.com/~bgirard/cleopatra/#report=df28724691d1c75a2bb7bc88c7677f0b8bee06bd
CC'ing milan for ideas on who can look into this
Comment 9•11 years ago
|
||
There's a huge regression from 2013-03-09 Nightly [1] to 2013-03-10 Nightly [2]. The former completes the test in 8 minutes on Galaxy Nexus; still not as fast as it should be, but significantly faster than now.
[1] http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2013/03/2013-03-09-03-08-41-mozilla-central-android/
[2] http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2013/03/2013-03-10-03-09-06-mozilla-central-android/
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e6215e0357fa&tochange=9e6232e86000
Updated•11 years ago
|
Keywords: regression
Updated•11 years ago
|
Flags: needinfo?(milan)
Comment 10•11 years ago
|
||
Jim, thanks for the regression range. I'm not sure this is graphics, but we are looking at some region issues right now. Matt?
Flags: needinfo?(milan) → needinfo?(matt.woodrow)
Comment 11•11 years ago
|
||
I think it's bug 725981. Not sure why those region operations are particularly expensive in Fennec.
Comment 12•11 years ago
|
||
Region operations are fairly expensive everywhere.
On most platforms we simplify the region outward into a simpler one if it gets too complex. On fennec we're not doing that.
Jeff Muizelaar is currently looking into the opposite of this bug for b2g, where simplifying the region means that we repaint more than we need to and end up with equally awful performance.
Currently the plan is to both try reduce the complexity of the regions formed (rather than simplifying them afterwards), and improve the performance of the region code (possibly by replacing it).
I'm not sure if there's a bug filed yet.
Flags: needinfo?(matt.woodrow)
Updated•11 years ago
|
tracking-fennec: --- → ?
Comment 13•11 years ago
|
||
(In reply to Jim Chen [:jchen :nchen] from comment #11)
> I think it's bug 725981. Not sure why those region operations are
> particularly expensive in Fennec.
The fix for bug 725981 does cause a large regression (8 minutes vs 40 minutes). I measured by backing out rev d10b1ac51ece on top of rev 2da17db2a304.
(In reply to Matt Woodrow (:mattwoodrow) from comment #12)
> Region operations are fairly expensive everywhere.
>
> On most platforms we simplify the region outward into a simpler one if it
> gets too complex. On fennec we're not doing that.
Is there a reason we don't do that on Fennec? We are looking at ways to mitigate this bug in Fennec trunk; I think it would make sense to start by simplifying regions (since profiling indicates most of the time running the test is spent on region operations).
Flags: needinfo?(matt.woodrow)
Comment 14•11 years ago
|
||
Matt, this goes from bad to awful with your patch, can we back it out?
Assignee: nobody → matt.woodrow
Comment 15•11 years ago
|
||
I'd really rather not, since that's just trading one performance regression for another.
Jeff and I are actively working on a fix for this, can we wait a bit before resorting to backing things out?
The problem with simplifying outward is that our current implementation isn't smart enough for the regions we tend to get. Our standard scroll invalidations usually have a bunch of rects in the area we just scrolled out of view, and a bunch in the area just scrolled into view.
The simplify outward code does a single pass attempting to reduce the number of rects to at most the specified number. If it fails, then it gives up and simplifies to a single rect, which will cover the entire display port.
Switching to using this would probably be a big win for this page, and a big regression for some scrolling cases (b2g is repainting the entire screen while scrolling timecube because of this).
I don't think either of these solutions are good ones, since we're just going to regress something else.
The plan I outlined in comment 12 should let us get wins everywhere.
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 16•11 years ago
|
||
It would be interesting to hear if this patch helps.
Assignee | ||
Comment 17•11 years ago
|
||
This time with the added files
Attachment #784024 -
Attachment is obsolete: true
Updated•11 years ago
|
Attachment #784026 -
Attachment is patch: true
Attachment #784026 -
Attachment mime type: text/x-patch → text/plain
Comment 18•11 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #15)
> I'd really rather not, since that's just trading one performance regression
> for another.
>
> Jeff and I are actively working on a fix for this, can we wait a bit before
> resorting to backing things out?
Yes certainly
Updated•11 years ago
|
tracking-fennec: ? → +
Assignee | ||
Comment 19•11 years ago
|
||
A working version.
Attachment #784026 -
Attachment is obsolete: true
Assignee | ||
Comment 20•11 years ago
|
||
Before:
I gave up running the test after only getting half way though testVisibilty and having 13 minutes pass.
After:
Total Time: 2 min
testDisplay:2374
testVisibilty:398
testFourClassReflow:1774
testFourScriptReflows:1809
testFontSizeEm:1816
Comment 21•11 years ago
|
||
I got some errors when trying to build with this patch. I'll find out the NDK and gcc versions.
from /home/mfinkle/source/mozilla-inbound/mozilla/xpcom/base/nsMemoryInfoDumper.cpp:14:
../../dist/include/mozilla/CanonicalRegion.h:180:3: error: a class-key must be used when declaring a friend
../../dist/include/mozilla/CanonicalRegion.h:180:3: error: friend declaration does not name a class or function
../../dist/include/mozilla/CanonicalRegion.h: In constructor 'nsIntRegionRectIterator::nsIntRegionRectIterator(const nsIntRegion&)':
../../dist/include/mozilla/CanonicalRegion.h:182:21: error: 'pixman_region32_t nsIntRegion::mImpl' is private
../../dist/include/mozilla/CanonicalRegion.h:213:85: error: within this context
Comment 22•11 years ago
|
||
Is it expected that this patch doesn't make a difference on the desktop (at least OS X where it takes ~10 seconds before and after the change)?
Comment 24•11 years ago
|
||
Since bug 845874 landed, I ran the test on my Galaxy Nexus running Android x
As expected, the test ran much faster in Firefox than previously. It completed in 1 minute 50 seconds. Same device, using Chrome the test completed in 1 minute 5 seconds.
Since the test certainly completes, we could close this bug. We could open a different bug if we want to explore making the test even faster.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Assignee: matt.woodrow → jmuizelaar
Target Milestone: --- → Firefox 28
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
•