We are currently expiring on a timer and on idle, we could probably replace both by expiring with idleDispatch. We should check whether idle dispatch keeps dispatching during a longer idle though, we don't want to run for a whole idle period, to save on battery.

Adding to photon-perf and qf. While this doesn't directly cause main-thread IO, it's I/O that happens on a timer rather than on idle timers, so it may be worth changing it. Currently the timer fires in a variable time (depending on profile status) between 1 to 9 minutes.

(In reply to Marco Bonardo [::mak] from comment #0) > We are currently expiring on a timer and on idle, we could probably replace > both by expiring with idleDispatch. Could we just add an idleDispatch callback inside the existing timer callback to avoid accidental jank if the user starts interacting right at the same time? > We should check whether idle dispatch keeps dispatching during a longer idle > though, we don't want to run for a whole idle period, to save on battery. idle dispatch will run whatever callbacks it has whenever the event queue is empty.

(In reply to Florian Quèze [:florian] [:flo] from comment #2) > Could we just add an idleDispatch callback inside the existing timer > callback to avoid accidental jank if the user starts interacting right at > the same time? Probably? It's very new and I'm not 100% sure how it is supposed to work, I thought it was notifying every time there was usable idle time, but looks more like it's a one-shot thing, thus we register a callback, and it will be executed at the first idle. And every time we need a new callback. If I got it right as a one-shot thing, I think we should indeed keep the timers, and just idleDispatch inside them. > > We should check whether idle dispatch keeps dispatching during a longer idle > > though, we don't want to run for a whole idle period, to save on battery. > > idle dispatch will run whatever callbacks it has whenever the event queue is > empty. Makes sense now, so we must keep the timers logic that interrupts work during long idles.

(In reply to Marco Bonardo [::mak] from comment #3) > Probably? It's very new and I'm not 100% sure how it is supposed to work, I > thought it was notifying every time there was usable idle time, but looks > more like it's a one-shot thing, thus we register a callback, and it will be > executed at the first idle. Right, it's a one time thing. > > > We should check whether idle dispatch keeps dispatching during a longer idle > > > though, we don't want to run for a whole idle period, to save on battery. > > > > idle dispatch will run whatever callbacks it has whenever the event queue is > > empty. > > Makes sense now, so we must keep the timers logic that interrupts work > during long idles. Are you using the idle service for this? I guess my comments would make more sense if I had seen the code this is about. But it's likely the only change to do is to add an idleDispatchToMainThread in the existing timer callback.

Yes, expiration uses idle, when 5 minutes idle starts it stops the timers and does a large expiration, it restarts the timer when idle finishes. When the user is not idle, we expire on a timer.

Assuming I'm looking in the right place ( http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/toolkit/components/places/nsPlacesExpiration.js#967-987 ), does it make sense for this to just use a timer with TYPE_REPEATING_SLACK_LOW_PRIORITY instead of TYPE_REPEATING_SLACK? Or is low priority too low (comment suggests it will allow idle events to run first)? Is the existing timer callback already nice enough, or should this use idleDispatchToMainThread as suggested?

(In reply to Adam Gashlin [:agashlin] from comment #6) > Assuming I'm looking in the right place ( > http://searchfox.org/mozilla-central/rev/ > bd39b6170f04afeefc751a23bb04e18bbd10352b/toolkit/components/places/ > nsPlacesExpiration.js#967-987 ), does it make sense for this to just use a > timer with TYPE_REPEATING_SLACK_LOW_PRIORITY instead of > TYPE_REPEATING_SLACK? Yes. I didn't know about TYPE_REPEATING_SLACK_LOW_PRIORITY, it seems pretty new. And this seems to be as good (or even better) than using idleDispatchToMainThread.

Drew, can you suggest a reviewer for this small change?

Comment on attachment 8896003 [details] Bug 1376533 - Use low priority timer for places expiration https://reviewboard.mozilla.org/r/167276/#review173674 Drive-by r+. :-)

Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9e201e729246 Use low priority timer for places expiration r=kitcambridge

(In reply to Florian Quèze [:florian] [:flo] from comment #7) > Yes. I didn't know about TYPE_REPEATING_SLACK_LOW_PRIORITY, it seems pretty > new. And this seems to be as good (or even better) than using > idleDispatchToMainThread. Well, it is different, not really better. _LOW_PRIORITY timers just don't prevent idle callbacks to be called, otherwise it is just like a normal timeout.

My doubt is indeed that we didn't address the original concern. The original intent was to run expiration when the user is idle, while the patch runs it a little bit more lazier than before but still on a timer that can fire at any time. That's why idleDispatch was suggested originally. So, while this is a positive change, it doesn't really address the issue of not causing jank while the user is actively browsing, for which we still need a separate bug. I still don't know what idleDispatchToMainThread considers idle though, for example, if the user is watching a video, is that considered idle?

(In reply to Marco Bonardo [::mak] from comment #14) > My doubt is indeed that we didn't address the original concern. Right, my answer in comment 7 was wrong, sorry about that :-(. (In reply to Olli Pettay [:smaug] from comment #13) > (In reply to Florian Quèze [:florian] [:flo] from comment #7) > > Yes. I didn't know about TYPE_REPEATING_SLACK_LOW_PRIORITY, it seems pretty > > new. And this seems to be as good (or even better) than using > > idleDispatchToMainThread. > > Well, it is different, not really better. _LOW_PRIORITY timers just don't > prevent idle callbacks to be called, otherwise it is just like a normal > timeout. So I was confused when looking at these _LOW_PRIORITY types of timers. I assumed that when firing the low priority timer just pushed the callback to the idle queue... and now I'm wondering if we should have a type of timer that would do just that, instead of having several timer callbacks that just do an idleDispatchToMainThread call.

Sync would also be improved significantly with this kind of facility; I started the thread at https://groups.google.com/forum/#!topic/mozilla.dev.platform/A6aRmSgExb0 but never found a reasonable solution :(

For C++ there is http://searchfox.org/mozilla-central/source/xpcom/threads/IdleTaskRunner.h JS could easily use setTimeout(function() {Services.tm.idleDispatchToMainThread(callback)}, <sometimeout>), if such initial timeout is needed. Main thread has idle period when there are no normal/input/high(vsync) priority pending runnables in the event queue, and there isn't a refresh driver tick coming soon and there are no timers (setTimeout or nsITimer) about to fire soon.