792037 - idle-daily delay wait may be too long for Metro tablets

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

Attachment: patch

I'd like to tighten this delay up a bit. This relates to telemetry reporting. 

Recap: The idle service fires an idle-daily once a day, assuming the system is idle. Once the daily timeout expires, idle service waits DAILY_SIGNIFICANT_IDLE_SERVICE_SEC (5 min.) to fire idle-daily. On the telemetry side, we receive idle-daily, trigger an accumulate and wait IDLE_TIMEOUT_SECONDS (5 min.) before sending pings. So we have to reach the daily timeout and then stay idle for ten minutes. For comparison, my tablet is designed to sleep (default setting) after seven minutes when idle.

So I'd like to change DAILY_SIGNIFICANT_IDLE_SERVICE_SEC from 5 minutes to 1 so we have a better chance of hitting idle-daily. I think a tablet or desktop sitting idle for one minute is a pretty good indication the system isn't active.

Another option would be to make this value dynamic through a call into platform specific idle impls. We already do this for polling for activity. From this we could detect what type of device we are running on and adjust DAILY_SIGNIFICANT_IDLE_SERVICE_SEC accordingly. This might be overkill though.

Comment 1

[Jim Mathies [:jimm]]

Attachment: tel ping patch

Another option would be to leave idle-daily delay at five minutes, and instead tweak telemetry code. I think telemetry needs time to collect data though, so I'd rather force the start of that vs. expedite sending pings data.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 662150 [details] [diff] [review]
patch

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

::: widget/xpwidgets/nsIdleService.cpp
@@ +29,5 @@
>  // Time used by the daily idle serivce to determine a significant idle time.
> +#ifdef MOZ_METRO
> +// five minutes is a bit long for a tablet sitting idle on a table, so we
> +// shortened this to one minute.
> +#define DAILY_SIGNIFICANT_IDLE_SERVICE_SEC 60 /* 60 seconds */

Why don't we just change this across the board?

Comment 3

[Marco Bonardo [:mak]]

note that idle-daily is also used for some heavy tasks, starting an heavy task after 1 minute of idle doesn't sound like a good idea imo, I may just leave my computer while answering the phone or to open the door to the cat, and when I'm back everything stutters badly...

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That could be 5 minutes too. We can't be completely foolproof.

Comment 5

[Jim Mathies [:jimm]]

> Why don't we just change this across the board?

I'm ok with this. I don't see how a few minutes makes any difference. Trying to predict the typical use pattern of 200 million users is next to impossible.

Plus this is an idle-daily ping, it can only happen once every 24-hour period.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> That could be 5 minutes too. We can't be completely foolproof.

yes, though we are going far worse direction by reducing that time, 1 minute is really really short to even thinking it may be a valid idle.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe we could be a little bit smarter: after the daily timer has expired, try to find a 5-minute idle interval. If there isn't one until the *next* daily timer expires, then try to only find a 30-second idle interval.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Maybe we could be a little bit smarter: after the daily timer has expired,
> try to find a 5-minute idle interval. If there isn't one until the *next*
> daily timer expires, then try to only find a 30-second idle interval.

I'll put this together.

I also found a bug that is interfering with our daily pings. On init we check to see if it's time to do an idle-daily ping -

http://mxr.mozilla.org/mozilla-central/source/widget/xpwidgets/nsIdleService.cpp#135

and call DailyCallback to set up the idle-daily event. In there we check for an early time out on the timer that calls DailyCallback. The math there uses mDailyTimerStart, which has never been initialized, so the result of the (now < launchTime) check is completely undetermined.

Thankfully this code hasn't been in the tree for long.

Comment 9

[Jim Mathies [:jimm]]

Attachment: branch patch

This fixes the bug introduced in bug 762620.

Comment 10

[Jim Mathies [:jimm]]

Attachment: idle svc patch v.1

Comment 11

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #10)
> Created attachment 662969 [details] [diff] [review]
> idle svc patch v.1

Ugh, sorry, I left DAILY_SIGNIFICANT_IDLE_SERVICE_SEC at 60 from debugging. will update in a bit after some testing. I think I should also get someone on android to test this to be sure I haven't reintroduced bug 762620.

Comment 12

[Jim Mathies [:jimm]]

Attachment: idle svc patch v.2

Taras, mind taking the review for this one?

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 662979 [details] [diff] [review]
idle svc patch v.2

Gian-Carlo, looking for someone to test on android. You were the last person to touch this code so that makes you an easy target. :)

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 662979 [details] [diff] [review]
idle svc patch v.2

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

Looks good to me. There isn't much I can do to check this on Android as we don't know what devices had the bad timers (you didn't break the logic as far as I can tell, so that's good). There's already a patch for the uninitialized value on m-i (bug 792065).

I guess this might also slightly improve our behavior on Android devices that have little memory and where we're at high risk of being pushed out of RAM before we can get our idle timer to fire.

::: widget/xpwidgets/nsIdleService.cpp
@@ +27,5 @@
>  #define MIN_IDLE_POLL_INTERVAL_MSEC (5 * PR_MSEC_PER_SEC) /* 5 sec */
>  
> +// After the twenty four hour period expires for an idle daily, this is the
> +// amount of idle time we wait for before actually firing the idle-daily
> +// event. 

nit: spurious whitespace

@@ +188,1 @@
>  

You can convert directly into USEC here and avoid the extra one below.

@@ +252,5 @@
>  #endif
>  
> +    // Reset the timer to the time remaining
> +    (void)self->mTimer->InitWithFuncCallback(DailyCallback,
> +                                              self,

nit: broken alignment

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 662979 [details] [diff] [review]
idle svc patch v.2

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

::: widget/xpwidgets/nsIdleService.cpp
@@ +104,5 @@
>  
>    // Stop observing idle for today.
>    (void)mIdleService->RemoveIdleObserver(this,
>                                           DAILY_SIGNIFICANT_IDLE_SERVICE_SEC);
> +  (void)mIdleService->RemoveIdleObserver(this,

If I'm not misreading, you're always going to attempt to remove both timers, even though you'll only add one. This will work correctly in RemoveIdleObserver but will generate a spurious warning. I think you want to turn the log level of that one down now that the failure is expected.

Comment 16

[(dormant account)]

Comment on attachment 662979 [details] [diff] [review]
idle svc patch v.2

mak or MikeK should review this. I don't know this code.

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 662979 [details] [diff] [review]
idle svc patch v.2

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

There'a a bunch of seconds/milliseconds/microseconds conversions that complicate the code, being able to somehow reduce those would be good, but I'm not sure if it can be improved further, just pointing out a thought.

It looks good, I like the adapting idea, I used a similar approach for history expiration indeed.
Though, since idle daily had already many regressions (up to the point of not being fired at all for a whole release), and idle is hard to test automatically, I'd ask you to test this manually with a paranoid aim.

::: widget/xpwidgets/nsIdleService.cpp
@@ +147,5 @@
>  
>  void
>  nsIdleServiceDaily::Init()
>  {
> +  // When we initialize first check the time of the last idle-daily event

s/when we initialize// since the function is called Init already :)

@@ +182,4 @@
>  #endif
>  
> +    // There's still time to wait. Set a timer to the amount of time we have
> +    // until we should fire idle-daily.

I'm not sure what "there's still time to wait" means, I think it should be clarified, something like "The last idle-daily was invoked less then 24 hours ago, thus wait for the remaining time before the next one"?

@@ +216,5 @@
>  }
>  
> +
> +void
> +nsIdleServiceDaily::PrepareToSendIdleDaily(bool aHasBeenLongWait)

The name is a bit misleading, unless I'm not misunderstanding it, since it doesn't send idle-daily, it may even never send it if there's no idle, maybe WaitSignificantIdle?

@@ +252,3 @@
>  #endif
>  
> +    // Reset the timer to the time remaining

I think this comment is superfluous, we already said above "timed returned early, reschedule", you may just expand better that comment above and remove this one that is just paraphrasing the code.

::: widget/xpwidgets/nsIdleService.h
@@ +61,5 @@
> +   * PrepareToSendIdleDaily is the interim call made when an idle-daily
> +   * event is due. However we don't want to fire idle-daily until the user
> +   * is idle for this session, so this sets up a short interim wait for an
> +   * idle event which triggers the actual idle-daily event.
> +   */

please add documentation for the param.

Comment 18

[Jim Mathies [:jimm]]

Attachment: mc compat patch

Comment 19

[Jim Mathies [:jimm]]

(In reply to Marco Bonardo [:mak] from comment #17)
> There'a a bunch of seconds/milliseconds/microseconds conversions that
> complicate the code, being able to somehow reduce those would be good, but
> I'm not sure if it can be improved further, just pointing out a thought.

I didn't address this here but will file a follow up.

> > +void
> > +nsIdleServiceDaily::PrepareToSendIdleDaily(bool aHasBeenLongWait)
> 
> The name is a bit misleading, unless I'm not misunderstanding it, since it
> doesn't send idle-daily, it may even never send it if there's no idle, maybe
> WaitSignificantIdle?

I ended up using 'StageIdleDaily'.

Other nits address.

Comment 20

[Jim Mathies [:jimm]]

(In reply to Gian-Carlo Pascutto (:gcp) from comment #15)
> >    // Stop observing idle for today.
> >    (void)mIdleService->RemoveIdleObserver(this,
> >                                           DAILY_SIGNIFICANT_IDLE_SERVICE_SEC);
> > +  (void)mIdleService->RemoveIdleObserver(this,
> 
> If I'm not misreading, you're always going to attempt to remove both timers,
> even though you'll only add one. This will work correctly in
> RemoveIdleObserver but will generate a spurious warning. I think you want to
> turn the log level of that one down now that the failure is expected.

I ended placing the wait time in a member variable so I could kill the right one.

Comment 21

[Jim Mathies [:jimm]]

Running this through try and I'll do some local testing. Then I think I'll use Elm first to test, since we have so little telemetry data coming in from metrofx, it should make for a good testing ground.

After that and assuming there are no issues I'll land on mc.

Comment 22

[Jim Mathies [:jimm]]

Added some additional logging. I will seek a final sr on the changes.

Note in testing on Windows I'm finding mShutdownInProgress is never initialized properly. :/ The profile-after-change event occurs before the idle daily service initializes. I think we should change this to final-ui-startup, or better yet, initialize mShutdownInProgress to false, and take profile-after-change observer reliance out completely. I don't see a need for it.

Comment 23

[Jim Mathies [:jimm]]

Hmm, I wouldn't expect the idle daily ping to work at all on Windows post the landing in bug 732368 this spring.

Anyone know if we experienced a big drop off reporting triggered by idle-daily around that time for win builds?

Comment 24

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #23)
> Hmm, I wouldn't expect the idle daily ping to work at all on Windows post
> the landing in bug 732368 this spring.
> 
> Anyone know if we experienced a big drop off reporting triggered by
> idle-daily around that time for win builds?

ah, nm, was looking at the wrong constructor. This is initialized to false.

Comment 25

[Jim Mathies [:jimm]]

Attachment: mc compat patch

Added debug logging for idle-daily.

Comment 26

[Jim Mathies [:jimm]]

Final try run:
https://tbpl.mozilla.org/?tree=Try&rev=d3f6eb2cf1f3

Push to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56bc99190c79

This has worked out well for Elm over the last couple weeks.

Comment 27

[(no longer active)]

Hmm, this seemingly broke all of the AndroidNoIon opt tests.  Not sure why, but I backed it out for now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e0c49bbdd81

Comment 28

[Jim Mathies [:jimm]]

(In reply to Ehsan Akhgari [:ehsan] from comment #27)
> Hmm, this seemingly broke all of the AndroidNoIon opt tests.  Not sure why,
> but I backed it out for now:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0e0c49bbdd81

Ehsan, those failures appear to have stopped two runs prior to your back out. Looks like the culprit wasn't this landing?

Comment 29

[(no longer active)]

(In reply to comment #28)
> (In reply to Ehsan Akhgari [:ehsan] from comment #27)
> > Hmm, this seemingly broke all of the AndroidNoIon opt tests.  Not sure why,
> > but I backed it out for now:
> > 
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/0e0c49bbdd81
> 
> Ehsan, those failures appear to have stopped two runs prior to your back out.
> Looks like the culprit wasn't this landing?

Probably.  Please reland your patch, and sorry for the wrong backout.

Comment 30

[Jim Mathies [:jimm]]

Some try pushes:
https://tbpl.mozilla.org/?tree=Try&rev=13526890d1be
https://tbpl.mozilla.org/?tree=Try&rev=16e26df9f6da

We'll see if the nonion problem shows up again on inbound -
https://hg.mozilla.org/integration/mozilla-inbound/rev/31bebe4cdeed

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31bebe4cdeed