Use idleDispatch for Places expiration

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mak, Assigned: agashlin)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [fxsearch][reserve-photon-performance][qf:p1])

Attachments

(1 attachment)

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.
See Also: → 1376535
Depends on: 1275878
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.
Whiteboard: [fxsearch] → [fxsearch][photon-performance][qf]
Whiteboard: [fxsearch][photon-performance][qf] → [fxsearch][photon-performance][qf] [triage]
Flags: qe-verify?
Priority: P2 → P3
Whiteboard: [fxsearch][photon-performance][qf] [triage] → [fxsearch][reserve-photon-performance][qf]
(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.
Whiteboard: [fxsearch][reserve-photon-performance][qf] → [fxsearch][reserve-photon-performance][qf:p1]
Flags: qe-verify? → qe-verify-
(Assignee)

Comment 6

2 years ago
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?
Flags: needinfo?(florian)
(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.
Flags: needinfo?(florian)
(Assignee)

Updated

2 years ago
Assignee: nobody → agashlin
Status: NEW → ASSIGNED
Priority: P3 → P1
(Assignee)

Updated

2 years ago
Attachment #8896003 - Flags: review?(adw)
(Assignee)

Comment 9

2 years ago
Drew, can you suggest a reviewer for this small change?
Flags: needinfo?(adw)

Comment 10

2 years ago
mozreview-review
Comment on attachment 8896003 [details]
Bug 1376533 - Use low priority timer for places expiration

https://reviewboard.mozilla.org/r/167276/#review173674

Drive-by r+. :-)
Attachment #8896003 - Flags: review+
Flags: needinfo?(adw)
Attachment #8896003 - Flags: review?(adw)

Comment 11

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

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e201e729246
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Iteration: --- → 57.2 - Aug 29
(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.
You need to log in before you can comment on or make changes to this bug.