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

RESOLVED FIXED in Firefox 57

Status

()

Firefox
General
P1
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: mconley, Assigned: WillWang)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 57
Unspecified
Windows
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox57 fixed)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

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.
Flags: qe-verify?
Priority: -- → P2
See bug 1354143 for some other main thread I/O done by JumpListBuilder.cpp.
See Also: → bug 1354143
Whiteboard: [photon-performance][qf] → [photon-performance][qf:p1]

Comment 2

a year ago
Another candidate for idle dispatch.
Depends on: 1355746
Depends on: 1358476
No longer depends on: 1355746
(Assignee)

Comment 3

11 months ago
This need the requestIdleCallback API implementation for non-DOM JS execution contexts.
Depends on: 1353206

Comment 4

11 months ago
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.
(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.
(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

11 months ago
(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!
No longer depends on: 1353206, 1358476

Updated

11 months ago
Flags: qe-verify? → qe-verify-
(Assignee)

Comment 8

11 months ago
(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.
Assignee: nobody → wiwang

Updated

11 months ago
Status: NEW → ASSIGNED
Priority: P2 → P1

Updated

11 months ago
Whiteboard: [photon-performance][qf:p1] → [reserve-photon-performance][qf:p1]
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.
Blocks: 1363771
(Assignee)

Comment 10

9 months ago
Created attachment 8894792 [details] [diff] [review]
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!
Attachment #8894792 - Flags: review?(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 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();	
  });
Attachment #8894792 - Flags: review?(florian) → feedback+
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.
(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
(Assignee)

Comment 16

8 months ago
Created attachment 8904498 [details] [diff] [review]
(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!!
Attachment #8894792 - Attachment is obsolete: true
Attachment #8904498 - Flags: review?(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
Attachment #8904498 - Flags: review?(florian) → review-
(Assignee)

Comment 18

8 months ago
Created attachment 8905384 [details] [diff] [review]
(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! :)
Attachment #8904498 - Attachment is obsolete: true
Attachment #8905384 - Flags: review?(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!
Attachment #8905384 - Flags: review?(florian) → review+
(Assignee)

Comment 20

8 months ago
Thank you for your assistance, Florian!

====

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

Comment 21

8 months ago
Please help to check-in, thanks! :)
Keywords: checkin-needed

Comment 22

8 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5fc64fe2ae
Move the taskbar refresh timer to idle dispatch. r=florian
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca5fc64fe2ae
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

8 months ago
Iteration: --- → 57.3 - Sep 19
You need to log in before you can comment on or make changes to this bug.