1357114 - WindowsJumpLists.jsm refreshes itself every 2 minutes or so via a timer, kicking off a Places query and a list building activity that can cause us to skip a frame

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P1)

Description

[Mike Conley (:mconley) (:⚙️)]

Here's a profile I captured:

https://perf-html.io/public/20232669f468e741f89f237bc9612ec0c4792596/calltree/?jsOnly&range=92.9771_107.7333~101.5390_101.9975&search=WindowsJump&thread=0

We schedule this thing off of a timer based on the browser.taskbar.lists.refreshInSeconds pref, which defaults to 2 minutes.

Also, it _looks_ like we might do main thread IO here for things like favicons: http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/widget/windows/JumpListBuilder.cpp#186-188

We should probably be smarter here - perhaps waiting for a better idle time to do the query (assuming it has to occur on the main thread), and using the IO thread for doing the favicons stuff.

Comment 1

[Florian Quèze [:florian]]

See bug 1354143 for some other main thread I/O done by JumpListBuilder.cpp.

Comment 2

[(no longer active)]

Another candidate for idle dispatch.

Comment 3

[Will Wang [:WillWang]]

This need the requestIdleCallback API implementation for non-DOM JS execution contexts.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

If we could avoid polling it might help improve battery as well.  If someone leaves their browser open over night it seems kind of wasteful to run this every two minutes with nothing happening that would result in a change.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #4)
> If we could avoid polling it might help improve battery as well.

likely could use the existing idle service to stop refreshing when the system is idle.
Potentially, this could also register a weak history observer, and after the first update schedule the following update only if onVisit was invoked.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Marco Bonardo [::mak] from comment #5)
> Potentially, this could also register a weak history observer, and after the
> first update schedule the following update only if onVisit was invoked.

on the negative side, this would make new visits require to notify an additional observer, so it depends on the likely for new visits to be added when the user is not idle (probably high)

Comment 7

[(no longer active)]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #2)
> Another candidate for idle dispatch.

Sorry I had confused myself, this needs idle service callbacks (which means, running it when the user isn't interacting with the browser.)  This double-meaning "idle" terminology we have right now is very confusing!

Comment 8

[Will Wang [:WillWang]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #7)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #2)
> > Another candidate for idle dispatch.
> 
> Sorry I had confused myself, this needs idle service callbacks (which means,
> running it when the user isn't interacting with the browser.)  This
> double-meaning "idle" terminology we have right now is very confusing!

I believe you are right about the usage of idle dispatch, Ehsan :)

Two aspects here:
1. Use idle dispatch(requestIdleCallback) to do the refresh at better time (comment 0)
2. Use idle service(nsIIdleService) to avoid unnecessary polling (comment 4/5/6)

If possible, I'd like to take this bug.

Comment 9

[Florian Quèze [:florian]]

Here is profile: http://bit.ly/2u7e6E6 where various WindowsJumpLists.jsm methods do main thread IO or call Windows API that do main thread IO.

Comment 10

[Will Wang [:WillWang]]

Attachment: Bug 1357114 - Move the taskbar refresh timer to idle dispatch.

Here is the patch which moves the taskbar refresh timer to idle dispatch.

Thanks to :wcpan's effort in bug 1354143, which already moved most works(`_commitBuild`) off the main thread;
So now we move it(`_commitBuild`) and other operations which show up in the profile to idle dispatch, such as `_startBuild`, `_buildTasks`.

I did some experiments then found a way of making a minimum patch as attached.

Florian, would it be possible for you to review this patch?
Thanks a lot!

Comment 11

[Will Wang [:WillWang]]

It's the try which tests all windows platform:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9b7c35cf0b2107eb0b1bd090bdf5e2cb9dac98d

Comment 12

[Florian Quèze [:florian]]

(In reply to Will Wang [:WillWang] from comment #10)

> So now we move it(`_commitBuild`) and other operations which show up in the
> profile to idle dispatch, such as `_startBuild`, `_buildTasks`.

Moving this to idle dispatch is a step in the right direction, but both _startBuild and _buildTasks seem to call Windows APIs that do main thread IO, so while moving this to idle dispatch is an improvement, it won't fully fix the jank here. I think we need follow-ups:

- It looks like JumpListBuilder::InitListBuild should call CDestinationList::BeginList off main thread (so WTBJL__startBuild should become asynchronous).

- Each WTBJL__getHandlerAppItem call causes a nsLocalFile::Exists call (main thread IO) when calling the .app setter: http://searchfox.org/mozilla-central/rev/bd39b6170f04afeefc751a23bb04e18bbd10352b/widget/windows/JumpListItem.cpp#200
I think this is useless as we always set this to Services.dirsvc.get("XREExeF", Ci.nsIFile) which should always exist. We could probably just remove this JumpListShortcut::ExecutableExists code, or make it debug-builds only.

- JumpListBuilder::AddListToBuild calls JumpListShortcut::GetShellLink which calls CShellLink::SetPath, which triggers a load of cscapi.dll. We may want to preload this library using the mechanism that's being added in bug 1367416.

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8894792 [details] [diff] [review]
Bug 1357114 - Move the taskbar refresh timer to idle dispatch.

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

this.update(); is also called when a preference is changed. Would this benefit from an idle dispatch too?

::: browser/modules/WindowsJumpLists.jsm
@@ +514,5 @@
>     */
>  
>    notify: function WTBJL_notify(aTimer) {
> +    var idleCallbackRefreshTaskbar = this.refreshTaskbar.bind(this);
> +    Services.tm.idleDispatchToMainThread(idleCallbackRefreshTaskbar);

You can simplify using an arrow function:
  Services.tm.idleDispatchToMainThread(() => {
    // Add idle observer on the first notification so it doesn't hit startup.
    this._updateIdleObserver();
    this.update();	
  });

Comment 14

[Florian Quèze [:florian]]

Actually, even simpler, change the type of the timer from TYPE_REPEATING_SLACK to TYPE_REPEATING_SLACK_LOW_PRIORITY. This is the equivalent of using an idle callback within the notify method.

Comment 15

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] [:flo] from comment #14)
> Actually, even simpler, change the type of the timer from
> TYPE_REPEATING_SLACK to TYPE_REPEATING_SLACK_LOW_PRIORITY. This is the
> equivalent of using an idle callback within the notify method.

probably not, https://bugzilla.mozilla.org/show_bug.cgi?id=1376533#c13

Comment 16

[Will Wang [:WillWang]]

Attachment: (v2) Bug 1357114 - Move the taskbar refresh timer to idle dispatch.

(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> this.update(); is also called when a preference is changed. Would this
> benefit from an idle dispatch too?
You are right.


> ::: browser/modules/WindowsJumpLists.jsm
> @@ +514,5 @@
> >     */
> >  
> >    notify: function WTBJL_notify(aTimer) {
> > +    var idleCallbackRefreshTaskbar = this.refreshTaskbar.bind(this);
> > +    Services.tm.idleDispatchToMainThread(idleCallbackRefreshTaskbar);
> 
> You can simplify using an arrow function:
>   Services.tm.idleDispatchToMainThread(() => {
>     // Add idle observer on the first notification so it doesn't hit startup.
>     this._updateIdleObserver();
>     this.update();	
>   });
At first, I tend to use the .bind way instead of the arrow funtion to avoid the abuse of 'lexical scoping' (or 'enclosing scoping'), as some books suggested; but in this case I realized the arrow function is more clear in readability, thanks for your remind!


(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> - Each WTBJL__getHandlerAppItem call causes a nsLocalFile::Exists call (main
> thread IO) when calling the .app setter:
> http://searchfox.org/mozilla-central/rev/
> bd39b6170f04afeefc751a23bb04e18bbd10352b/widget/windows/JumpListItem.cpp#200
> I think this is useless as we always set this to
> Services.dirsvc.get("XREExeF", Ci.nsIFile) which should always exist. We
> could probably just remove this JumpListShortcut::ExecutableExists code, or
> make it debug-builds only.
Thanks for your rich suggestions! I think we can remove it for better readability.
As for two other suggestions, I'd like to file another bug to deal with them, if possible.


Therefore, here is the patch(v2):
1. Move the idle dispatch to deeper level
2. Use arrow function
3. Remove the unused JumpListShortcut::ExecutableExists

Besides, I decide not to use the timeout of idle dispatch since I believe we should not do low priority tasks like jumplist update during the main thread is still busy; so we wait. (and the default update interval is quite long: browser.taskbar.lists.refreshInSeconds = 120 sec.)


Could you help to review this patch?
Thanks a lot!!

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8904498 [details] [diff] [review]
(v2) Bug 1357114 - Move the taskbar refresh timer to idle dispatch.

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

Thanks for the updated patch. In the future, please include 8 lines of context in your patches.

(In reply to Will Wang [:WillWang] from comment #16)

> As for two other suggestions, I'd like to file another bug to deal with
> them, if possible.

Yes, opening a new bug (or maybe even two) is the right path forward here, let's land the improvements we have here without waiting for a perfect solution.

::: browser/modules/WindowsJumpLists.jsm
@@ +152,4 @@
>        return;
>  
>      // do what we came here to do, update the taskbar jumplist
> +    Services.tm.idleDispatchToMainThread(() => { this._buildList(); });

Unfortunately, some callers of the update method need the update to happen immediately. This really matters when purging the history on shutdown (http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/browser/modules/WindowsJumpLists.jsm#165) and matters slightly less when purging the history during a session (http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/browser/modules/WindowsJumpLists.jsm#538).

So I think we should simply do Services.tm.idleDispatchToMainThread(() => { this.update(); }); at http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/browser/modules/WindowsJumpLists.jsm#519 and http://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/browser/modules/WindowsJumpLists.jsm#530

Comment 18

[Will Wang [:WillWang]]

Attachment: (v3) Bug 1357114 - Move the taskbar refresh timer to idle dispatch.

(In reply to Florian Quèze [:florian] [:flo] from comment #17)
> Thanks for the updated patch. In the future, please include 8 lines of
> context in your patches.
Thanks! I didn't notice this side effect of using Cygwin which doesn't apply all settings in hgrc of the MinGW :/


> Unfortunately, some callers of the update method need the update to happen
> immediately. This really matters when purging the history on shutdown
> (http://searchfox.org/mozilla-central/rev/
> 4d8e389498a08668cce9ebf6232cc96be178c3e4/browser/modules/WindowsJumpLists.
> jsm#165) and matters slightly less when purging the history during a session
> (http://searchfox.org/mozilla-central/rev/
> 4d8e389498a08668cce9ebf6232cc96be178c3e4/browser/modules/WindowsJumpLists.
> jsm#538).
Thanks for your suggestion! 

Could you review again, thanks! :)

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 8905384 [details] [diff] [review]
(v3) Bug 1357114 - Move the taskbar refresh timer to idle dispatch.

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

Looks good to me, thanks!

Comment 20

[Will Wang [:WillWang]]

Thank you for your assistance, Florian!

====

Here is the try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=598199866efc778c3987f9bd1165146a89aaccff

Comment 21

[Will Wang [:WillWang]]

Please help to check-in, thanks! :)

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5fc64fe2ae
Move the taskbar refresh timer to idle dispatch. r=florian

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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