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)

22 Branch
ARM
Android
defect
Not set
normal

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)

Attached file logcatS3.txt (deleted) —
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.
It progresses painfully slow on my Samsung Galaxy SIV (4.3), Nightly (07/26).
Keywords: reproducible
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.
A good candidate to profile using the Gecko Profiler.
ask and you shall receive. http://people.mozilla.com/~bgirard/cleopatra/#report=b02cb82629fe70d7ef0d8bddd000de24ffc59c06 93.2% of our time was spent in ContainerState:ProcessDisplayItems:
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
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
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.
(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
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
Flags: needinfo?(milan)
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)
I think it's bug 725981. Not sure why those region operations are particularly expensive in Fennec.
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)
tracking-fennec: --- → ?
(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)
Matt, this goes from bad to awful with your patch, can we back it out?
Assignee: nobody → matt.woodrow
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)
Blocks: 900165
Attached patch Some new regions (obsolete) (deleted) — Splinter Review
It would be interesting to hear if this patch helps.
Attached patch Some new regions v2 (obsolete) (deleted) — Splinter Review
This time with the added files
Attachment #784024 - Attachment is obsolete: true
Attachment #784026 - Attachment is patch: true
Attachment #784026 - Attachment mime type: text/x-patch → text/plain
(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
tracking-fennec: ? → +
Attached patch Some new regions v3 (deleted) — Splinter Review
A working version.
Attachment #784026 - Attachment is obsolete: true
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
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
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)?
The region rewrite will happen in bug 845874
Depends on: 845874
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
Assignee: matt.woodrow → jmuizelaar
Target Milestone: --- → Firefox 28
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: