1064677 - Autophone - s1s2 - Regression in "time to throbber start" on 2014-09-05

Status: RESOLVED FIXED

(Android Background Services Graveyard :: Geolocation, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

See:
http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstart/local-blank/norejected/2014-09-01/2014-09-08/notcached/noerrorbars/standarderror

The slower phones are more affected than the faster phones. I think the "nexus-7-jss15q-2" line has a very nice "jump" right around "2014-09-05 08:23:39" which is:
http://hg.mozilla.org/integration/fx-team/rev/25c524c5af2f

Which doesn't have much of interest in the range.

the previous push is:
http://hg.mozilla.org/integration/fx-team/rev/60fbfb079b69

Which enables MozStumbler and may be a more likely candidate.

I think we should backout 60fbfb079b69 and see if it has any affect on the lines after a few cycles. If it doesn't we can reland it.

Comment 1

[:garvan]

Agreed, thanks Mark. I'll have to learn about this test case. The stumbler starts when Gecko starts (unless the service has been started previously). Any further work beyond instantiating listeners is scheduled with an additional few seconds of delay so as not to interfere with other post-gecko startup ops. 

On my Nexus 5, it is 4 seconds for Gecko to start, but these test results are around the 1000 ms range, so they seem to be measuring a time period prior to the stumbler starting.

I'll profile the startup code on a slow device and see if anything isn't as I expect.

Comment 2

[Nick Alexander :nalexander [he/him]]

Backed out:

https://hg.mozilla.org/integration/fx-team/rev/c14cb28dcc09

Sheriffs, if y'all can leave this one open until we get some autophone numbers, that would be appreciated.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c14cb28dcc09

Comment 4

[:garvan]

No luck yet, here is some investigation.

Full Profile:
https://www.dropbox.com/s/ssqu18dro2e7gf2/Screenshot%202014-09-10%2010.21.08.png?dl=0

The above shows 6 ms of main thread time and 7 ms of the service thread, which are affected by the profiler, both are double from my real-world time measurements.

Heaviest call to stumbler code (onHandleIntent, which is basically the only function that is run):
https://www.dropbox.com/s/s1zb6tymn5h2jzw/Screenshot%202014-09-10%2010.21.40.png?dl=0

Within the above, the heaviest call is StumblerService.init()
https://www.dropbox.com/s/7p38nhwjshh2cao/Screenshot%202014-09-10%2010.23.34.png?dl=0

Within that, the heaviest call is getStorageDir() which calls Context.getExternalFilesDir(), which is <1ms.


More accurate to real-world time is just inserting getElapsedTime into the code, from entry point to exit on the main thread, I measure 2 ms. The call on the main thread sends an Intent to the service thread, this code runs for 3 ms.

The StumblerService.init() is deliberately initializing core objects (ex. the DataStorageManager). I could init these objects on-demand, and shave off 2 ms. Not going to do that, as I need to find what is taking >50ms.

Comment 5

[:garvan]

Comment 4 assumes that the test case has the stumbler pref off. I'll profile the 'on' case now.

Comment 6

[:garvan]

With the pref 'on' to start the stumbler service, the main thread is the same <3ms, and the service thread takes an additional 1ms, most of that 1ms is spent in android's GPS listener registration. Which makes sense, the 'on' case just sets some state flags, and registers the passive GPS listener.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Based on the output of the Java traces, we don't have a good way to figure out why the stumbler code is affecting startup for phones and tablets. I am not surprised the Nexus S is affected, but I am surprised the Nexus 7 is affected.

So let's switch gears a bit:
1. Can Garvan reproduce the 100ms regression using a scrip that simulates autophone?
2. What would happen if the autophone devices were doing some GPS? Would that activate a code path that is different?
3. Can Bob run a different APK through the test with some logging and a startup trace patch so we can do more investigating?

If we can't do any of the first 3 items, then I'd suggest re-enabling and landing speculative fixes to see what happens. But I'd like to avoid that because I think Bob can do #3.

Comment 8

[Richard Newman [:rnewman]]

Bear in mind:

* Thread creation.
* Context-switching.
* While your intent thread is running, it's tying up a core that otherwise would be running the Gecko thread, the Java background thread, or starting up another service thread.

That is: my hypothesis is that this is a contention problem, not total workload. It doesn't matter what you're doing, only when you're doing it.

Comment 9

[Richard Newman [:rnewman]]

Note from IRC: garvank's profile is from a Nexus 5 with 4.4.4, profiling MozStumbler.

Comment 10

[Richard Newman [:rnewman]]

A full startup profile from Fennec (not mozstumbler) with everything turned on should yield smoking guns… did we already try that?

Comment 11

[Bob Clary [:bc] (inactive)]

1. Instructions on how to run the basic test are in bug 1062097.

You can see how the measurements are grabbed from logcat at:
https://github.com/mozilla/autophone/blob/master/tests/perftest.py#L75
https://github.com/mozilla/autophone/blob/master/tests/webappstartup.py#L289

We are working on making Autophone more developer friendly but aren't there yet.

2. mcote: Since you are my remote hands in MV this week, can you check the GPS settings on the Autophone devices?

3. Sure.

Comment 12

[:garvan]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> 2. What would happen if the autophone devices were doing some GPS? Would
> that activate a code path that is different?

It shouldn't make any difference, unless the devices had another app running that was actively listening to the GPS, in which case, yes, the picture changes completely, and stumbler actually starts doing a ton of stuff.

(In reply to Richard Newman [:rnewman] from comment #10)
> A full startup profile from Fennec (not mozstumbler) with everything turned
> on should yield smoking guns… did we already try that?

Trying now. Less noisy and time-consuming to profile standalone, but that lead nowhere.

I agree with trying to find out what is happening before patching. Fortunately the jump in startup time of 100ms should be something we can pinpoint.

Comment 13

[Richard Newman [:rnewman]]

A nice trick would be to add the code-based profiling start/stop lines (<http://developer.android.com/reference/android/os/Debug.html#startMethodTracing%28%29>), and do so between t0 and "wherever autophone thinks we're done". The profile duration should actually be longer in the regression case.

Comment 14

[:garvan]

This is the startup profiles with the stumbler pref broadcast commented out, and on, as html reports:
https://drive.google.com/file/d/0B1sYPwjczMsQQXpDemRqY2RNVWM/edit?usp=sharing

This is the same data as raw trace files:
https://drive.google.com/file/d/0B1sYPwjczMsQQXpDemRqY2RNVWM/edit?usp=sharing

What do I see? No difference, except some calls to to stumbler taking a few millis.

Comment 15

[Richard Newman [:rnewman]]

And with the normal code flow, including broadcast?

Comment 16

[:garvan]

(In reply to Richard Newman [:rnewman] from comment #15)
> And with the normal code flow, including broadcast?

Best I describe the test I did. Used startMethodTracing in application onCreate, and stopMethodTracing 100ms after Gecko:DelayedStartup. As I mentioned I ran with the unmodified code, and with stumbler pref broadcast commented out in Fennec. Those 2 test runs are zipped up in each file.

Some screenshots, which if not informative, are at least colourful:
Off (commented out):
https://www.dropbox.com/s/c3zsvaw2pek3z55/Screenshot%202014-09-10%2015.09.24.png?dl=0
On:
https://www.dropbox.com/s/qccl6vnn803cmtr/Screenshot%202014-09-10%2015.09.54.png?dl=0

Comment 17

[Bob Clary [:bc] (inactive)]

GPS was on but has now been turned off.

Comment 18

[:garvan]

Here is the entirety of code that runs in stumbler in Fennec with the pref set to off:

public void onReceive(Context context, Intent intent) {
        if (intent == null) {
            return;
        }
        final String action = intent.getAction();
        final boolean isIntentFromHostApp = (action != null) && action.contains(".STUMBLER_PREF");
        if (!isIntentFromHostApp) {
<snip irrelevant code>
        }

        if (intent.hasExtra("is_debug")) {
            AppGlobals.isDebug = intent.getBooleanExtra("is_debug", false);
        }
        StumblerService.sFirefoxStumblingEnabled.set(intent.getBooleanExtra("enabled", false));

        if (!StumblerService.sFirefoxStumblingEnabled.get()) {
            context.stopService(new Intent(context, StumblerService.class));
            return; 
        }
<end of code>

Comment 19

[:garvan]

Following the instructions in bug 1062097.

broadcastStumblerPref commented out
Start | Stop
761272599 | 1410379524744, diff	1409618252145	

broadcastStumblerPref on
762223145 | 1410380475222, diff	1409618252077

Comment 20

[:garvan]

> broadcastStumblerPref on
> 762223145 | 1410380475222, diff	1409618252077

I mean on as in not commented out (the value of the preference would be that stumbling is off).

Comment 21

[:garvan]

In comment 19, what are the time units?

I got these from the 'zerdatime \d+' in the log.

Comment 22

[Bob Clary [:bc] (inactive)]

If you get them from I/GeckoToolbarDisplayLayout.*zerdatime the units are milliseconds. If you get it from D/GeckoBrowser.*zerdatime it is a timestamp in microseconds.

09-10 03:14:26.838 I/GeckoToolbarDisplayLayout( 4002): zerdatime 621677 - Throbber start
09-10 03:14:27.229 W/GeckoThread( 4002): zerdatime 622065 - runGecko
09-10 03:13:35.930 W/GeckoThread(11836): zerdatime 646006 - runGecko
09-10 03:13:38.343 D/GeckoBrowser(11836): zerdatime 1410344018346 - browser chrome startup finished.

>>> datetime.datetime.fromtimestamp(1410344018346/1000)
datetime.datetime(2014, 9, 10, 3, 13, 38)

I use logcat with format time and filter spec *:V and parse the time from the leading date rather than the zerdatime.

Comment 23

[:garvan]

Attachment: adb logcat from app start test

Does this look correct? I get a start time at 02.73 and a "Startup finished" at 09.48.

That is 6.75 seconds. I am running on a Nexus 5, seems too long to be correct.

The command I ran was
adb shell 'am start  -n com.firefox.cli.apk.webappstartupperformancetestbclary.pc91282b8e6a542101851c47a670b9c8c/org.mozilla.android.synthapk.LauncherActivity -a android.intent.action.MAIN --es env0 MOZ_CRASHREPORTER_NO_REPORT=1'

Comment 24

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8487511 [details]
adb logcat from app start test

It looks like you're running a debug version of libxul, which will slow everything down.  Also, be aware that logging and sending |adb logcat| to a host machine is very slow on many devices.

Comment 25

[Bob Clary [:bc] (inactive)]

Garvan, you can check out the live versions results for opt builds of fennec at:
http://phonedash.mozilla.org/#/org.mozilla.fennec/throbberstart/webappstartup/norejected/2014-09-10/2014-09-10/notcached/noerrorbars/standarderror

They show about 3 seconds to start for Nexus 5's, so 7 seconds might not be too far off for a debug build.

From your log:

09-10 17:39:02.580 I/ActivityManager(15970): START u0 {act=android.intent.action.MAIN flg=0x10000000 cmp=com.firefox.cli.apk.webappstartupperformancetestbclary.pc91282b8e6a542101851c47a670b9c8c/org.mozilla.android.synthapk.LauncherActivity (has extras)} from pid 27376

09-10 17:39:09.480 D/GeckoBrowser(27407): zerdatime 1410385149484 - browser chrome startup finished.

09-10 17:39:03.280 W/GeckoAppShell(27407): STARTUP PERFORMANCE WARNING: un-official build: purging the startup (JavaScript) caches.

*** This may be an issue ***

09-10 17:39:03.280 D/GeckoAppShell(27407): GeckoLoader.nativeRun /data/app/org.mozilla.fennec_gkeeley-1.apk -greomni /data/app/org.mozilla.fennec_gkeeley-1.apk  -P webapp0 -url https://webappstartupperformancetestbclary.apk.cli.firefox.com/manifest.webapp -purgecaches 

But I don't see the WEBAPP STARTUP COMPLETE message.

You may need to change the command line to point to your version of the webapp or use the one I use. https://bugzilla.mozilla.org/attachment.cgi?id=8483275 or https://github.com/mozilla/autophone/blob/master/tests/webapp-startup-test.apk

Comment 26

[Richard Newman [:rnewman]]

Attachment: Profile.

Here are my findings.

A picture tells a thousand words, so see attached.

I see 138 milliseconds real-time spent on BinderProxy handling.

Of that, much is a big chunk in the StumblerService thread. That's because of the prefs broadcast, which itself only takes about 11msec (most of it in the guts of Android).

That StumblerService work occurs *before* the GeckoThread work.

(There are a lot of threads in this system.)

My conclusion:

* In onResume we fetch the stumbler pref. That's cheap. Then we broadcast it. That's cheap *at point of call*.

* The *handling* of that broadcast is expensive, because it involves instantiating a thread and running 50-100msec of code just to set it up as a service, before the stumbler code actually runs.

My recommendation:

* Everything unnecessary that happens before that block of GeckoThread work should happen later.

Comment 27

[Richard Newman [:rnewman]]

See also that SharedPreferences loader, which happens on a background thread (thanks, Android!), and appears to have been triggered by StumblerService or the broadcast.

Comment 28

[:garvan]

Thanks Richard, moving this to a later init point seems like the best approach.

My concern though:
The screenshot shows [18]IntentService[StumblerService]. Why? Did you toggle the pref for this test? That should not be there.

Comment 29

[Richard Newman [:rnewman]]

(In reply to Garvan Keeley [:garvank] from comment #28)

> The screenshot shows [18]IntentService[StumblerService]. Why? Did you toggle
> the pref for this test? That should not be there.

This is the default setup. No prefs touched.

Indeed, I just killed the process, cleared data, launched, and connected `monitor`.

I'm looking at Settings, where stumbling is unchecked, and IntentService[StumblerService] is present.

Why do you think the service thread shouldn't be running? If you have a service declared in your manifest, and you send it a broadcast, a thread will be started for it, no?

Comment 30

[:garvan]

> If you have a
> service declared in your manifest, and you send it a broadcast, a thread
> will be started for it, no?

If startService is called, yes, but this code only calls stopService (when the stumbling pref is set to false) on a non-running service. 

Fennec calls broadcastPref() -> sends to PassiveServiceReceiver (a BroadcastReceiver, not part of the stumbler service) on main thread, which if stumbling is enabled (pref is 'true') calls startService() -> stumbler IntentService is started on its own thread

Comment 31

[Richard Newman [:rnewman]]

So then it looks like we have two good avenues to investigate.

Firstly, if stumbling is enabled, it shouldn't affect startup. So let's address that by delaying.

Secondly -- and this is slightly academic -- we can figure out why that service is starting.

I can think of a few hypotheses:

* We're wrong: the pref being broadcast and the checkbox being displayed are different, and we're really broadcasting "yes, please stumble". I understood this to be the default, so maybe that's what's happening.

* Android is starting the service as soon as its static initializer is run (the "StumblerService" mention in the code in Comment 18).

* Android is starting the service in order to handle the stopService call, which runs counter to the docs.

* Android is starting the service for the hell of it.

I think the first is most likely. Debugging (either with Eclipse or print statements) will explain. I can do that if you like. Let me know, Garvan.

Comment 32

[:garvan]

> Firstly, if stumbling is enabled, it shouldn't affect startup. So let's
> address that by delaying.

agreed.
 
> Secondly -- and this is slightly academic -- we can figure out why that
> service is starting.
> 
> I can think of a few hypotheses:
> 
> * We're wrong: the pref being broadcast and the checkbox being displayed are
> different, and we're really broadcasting "yes, please stumble". I understood
> this to be the default, so maybe that's what's happening.

99% sure this is the case.
 
> * Android is starting the service in order to handle the stopService call,
> which runs counter to the docs.

I profiled the crap out of this code, unless my eyes are failing me, I would have seen this.
 
> I think the first is most likely. Debugging (either with Eclipse or print
> statements) will explain. I can do that if you like. Let me know, Garvan.

Please and thank you, I can't seem to get back on this task.

Comment 33

[Richard Newman [:rnewman]]

(In reply to Garvan Keeley [:garvank] from comment #32)

> > * We're wrong: the pref being broadcast and the checkbox being displayed are
> > different, and we're really broadcasting "yes, please stumble". I understood
> > this to be the default, so maybe that's what's happening.

Confirmed: broadcastStumblerPref's second argument is `true`.

This is the value of 

        final boolean value = getBooleanPref(context, PREFS_GEO_REPORTING, true);

which -- as you can see -- defaults to `true`.

I imagine that you're expecting this to reflect the value of the Gecko pref app.geo.reportdata (the value of that constant).

That will not be the case until after GeckoPreferences.setupPreferences is called, which will be when the appropriate settings pane is displayed. And of course, Gecko must be running in order to do that.

In short: if you want to use a Gecko value here, you need to wait until Gecko is started to fetch the pref (and do a little dance to retrieve it).

If you don't want to use a Gecko value here -- and I don't see why you would -- then you need to make some small changes to GeckoPreferences to ensure that the checkbox shows the right value.

We shouldn't pref this on until this division is resolved.

Comment 34

[:garvan]

> Confirmed: broadcastStumblerPref's second argument is `true`.
> 
> This is the value of 
> 
>         final boolean value = getBooleanPref(context, PREFS_GEO_REPORTING,
> true);
> 
> which -- as you can see -- defaults to `true`.

Thanks Richard. Headsmack. Definitely not wanted. 

The #2 question, where is a good delay point to start the stumbler, could be a few seconds after startup complete. 
It could be as simple as doing:
new Handler(<on main loop>).postDelayed(new Runnable() { void run() { <broadcastStumblerPref> }}, FOUR_SECOND_DELAY)
But my hunch tells me you have a better way... because you usually have a better way :).

Comment 35

[Richard Newman [:rnewman]]

(In reply to Garvan Keeley [:garvank] from comment #34)

> But my hunch tells me you have a better way... because you usually have a
> better way :).

That depends. If you do want this pref to be managed by Gecko, then you'll want to trigger a pref fetch some time after Gecko:Ready. That would be via PrefsHelper.getPref. See where we trigger the display of the data reporting notification:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java#1500

And you'd need to fix anywhere else it's broadcast, too -- none of them can rely on the shared preference being set.

If you want the pref to not be managed by Gecko, then I'd simply add the broadcast line late in BrowserApp's delayedstartup handler. Which we already do! So the only work would be to 'fix' GeckoPreferences to treat the pref as a non-Gecko pref (and remove mention of it from prefs.js).

Comment 36

[:garvan]

Attachment: Part 1: stumbler pref should default to off

Comment 37

[:garvan]

Attachment: Part 2: stumbler service startup delay

To avoid cpu/thread contention, delay message from host app the to stumbler service. 
In consideration that other Fennec components may be delaying their load slightly to avoid contention, ensure that the delay is sufficiently long.

Comment 38

[Richard Newman [:rnewman]]

Comment on attachment 8489491 [details] [diff] [review]
Part 1: stumbler pref should default to off

Review of attachment 8489491 [details] [diff] [review]:
-----------------------------------------------------------------

I'll take your word for it, but this isn't a complete fix -- needs to address the question of who owns this pref.

Please clean up the commit message:

  Bug 1064677 - Part 1: default stumbler pref to 'off'. r=rnewman

Comment 39

[Richard Newman [:rnewman]]

Comment on attachment 8489501 [details] [diff] [review]
Part 2: stumbler service startup delay

Review of attachment 8489501 [details] [diff] [review]:
-----------------------------------------------------------------

Why not just broadcast the pref later from Fennec? See my earlier comments in this bug.

Comment 40

[:garvan]

 > Why not just broadcast the pref later from Fennec? See my earlier comments
> in this bug.

I can't imagine a lower priority startup item than stumbler, and this adds ample delay. 
If I decide to delay it another few hundred ms by putting in another event during startup, I assume it could still contend with other services that Fennec would start.

In my mind, this is a problem coupled with stumbler (like the upload delay, and aggregating/coalescing messages in general to it).

Comment 41

[Richard Newman [:rnewman]]

(In reply to Garvan Keeley [:garvank] from comment #40)

> In my mind, this is a problem coupled with stumbler (like the upload delay,
> and aggregating/coalescing messages in general to it).

I understand that perspective. On the other hand, you're not doing anything especially evil here -- services simply aren't zero-cost to start up, and it's the caller's responsibility to make sure it doesn't trigger a service earlier than its performance envelope prefers.

With that in mind, I'd rather go for something simpler/cheaper than adding another listener and an alarm. For example, let's just remove the onResume broadcaster: broadcast only in delayed start.

Delayed start is already designed to be out of the startup path. The issue we see in this bug is that we're also doing work in onResume, but we strictly speaking don't need to. (Verify that, of course!)

(And perhaps even in delayed start, queue a new background runnable to make sure we're not blocking anything else that's doing work in the mean time.)

Comment 42

[:garvan]

> With that in mind, I'd rather go for something simpler/cheaper than adding
> another listener and an alarm. For example, let's just remove the onResume
> broadcaster: broadcast only in delayed start.

Will do. I should mention, my design choice will invariably be to add code to stumbler over Fennec, let me know when that seems like an odd choice to the reader. 
It is a difference of 10 lines of code in Stumbler (for an alarm), or 5 lines of code in Fennec (for a posted runnable), further difference is unclear (isn't it just posting events to the main thread in both cases?).
 
> Delayed start is already designed to be out of the startup path. The issue
> we see in this bug is that we're also doing work in onResume, but we
> strictly speaking don't need to. (Verify that, of course!)

Will also verify that.

> (And perhaps even in delayed start, queue a new background runnable to make
> sure we're not blocking anything else that's doing work in the mean time.)

Will do this instead. Patches to follow in a few mins.

Comment 43

[:garvan]

Attachment: Part 1: stumbler pref should default to off

carryover r+ from prev review, updated commit message

Comment 44

[:garvan]

Attachment: Part 2: start service: remove from onResume(), also added 1sec delay

Removed starting stumbler from onResume(), broadcasting the stumbler startup
pref happens only in DelayedStartup. To further minimize the startup impact,
and so as not to compete for CPU/thread-space with more important services that also have a delayed startup, delay by an additional second sending the start message to stumbler.

Comment 45

[:garvan]

Comment on attachment 8489683 [details] [diff] [review]
Part 2: start service: remove from onResume(), also added 1sec delay

Review of attachment 8489683 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ -1491,5 @@
>                          // pageload is finished.
>                          ensureTabsPanelExists();
>                      }
>                  });
> -

oops, removed CR, will add back

@@ +1493,5 @@
>                  if (AppConstants.MOZ_STUMBLER_BUILD_TIME_ENABLED) {
>                      // Start (this acts as ping if started already) the stumbler lib; if the stumbler has queued data it will upload it.
>                      // Stumbler operates on its own thread, and startup impact is further minimized by delaying work (such as upload) a few seconds.
> +                    // Avoid any potential startup CPU/thread contention by delaying the pref broadcast.
> +                    final int oneSecondInMillis = 1000;

this should be a long.

Comment 46

[Richard Newman [:rnewman]]

Comment on attachment 8489683 [details] [diff] [review]
Part 2: start service: remove from onResume(), also added 1sec delay

Review of attachment 8489683 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/BrowserApp.java
@@ +1494,5 @@
>                      // Start (this acts as ping if started already) the stumbler lib; if the stumbler has queued data it will upload it.
>                      // Stumbler operates on its own thread, and startup impact is further minimized by delaying work (such as upload) a few seconds.
> +                    // Avoid any potential startup CPU/thread contention by delaying the pref broadcast.
> +                    final int oneSecondInMillis = 1000;
> +                    new Handler(Looper.getMainLooper()).postDelayed(new Runnable() {

You don't want to use getMainLooper here. You'll end up doing the broadcast on the UI thread.

Instead:

  ThreadUtils.getBackgroundHandler().postDelayed(...);

Comment 47

[:garvan]

Attachment: Part 2: start service: remove from onResume(), also added 1sec delay

Addressed comments from rnewman, carry-over of r+

Comment 48

[Richard Newman [:rnewman]]

Garvan: don't forget to un-Geckoify this pref in SharedPreferences.

Comment 49

[:garvan]

(In reply to Richard Newman [:rnewman] from comment #48)
> Garvan: don't forget to un-Geckoify this pref in SharedPreferences.

I'm not sure what this issue is, if isn't related to the performance problem in this bug can you file a bug and explain? I believe this code used the pre-existing stumbler pref (granted the rest of that code has been removed).

Comment 50

[Richard Newman [:rnewman]]

What you broadcast is read from SharedPreferences.

The pref that's shown in Settings is read from Gecko.

See the use of not_a_pref in GeckoPreferences to indicate "nope, this isn't a Gecko pref".

See also how "locale" isn't prefixed that way, and so is specially handled elsewhere.


Right now we assume false on both sides for stumbling, but that's just coincidence!

Comment 51

[:garvan]

Thanks Richard, added Bug 1068388 for comment 50.

Whether the pref is on or off, stumbler should now be out of the startup path of Fennec.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Can we please run this through Try first? :)

Comment 53

[:garvan]

Thanks for the reminder Ryan, try green:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a5445ea36124
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-a5445ea36124
Summary:
 * try: -b o -p android -u none -t none
 * Bug 1064677 - Part 1: default stumbler pref to 'off'. r=rnewman
 * Bug 1064677 - Part 2: Delay start of stumbler. r=rnewman

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2e599cc67f39
https://hg.mozilla.org/integration/fx-team/rev/0e894cd657e7

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2e599cc67f39
https://hg.mozilla.org/mozilla-central/rev/0e894cd657e7