Too much time spent by osfile_async_front.jsm calling restartTimer / setTimeout (from Timer.jsm)

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: florian, Assigned: kanru)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
See this profile https://perf-html.io/public/ebdfb772af3c32bd7fb592ce8d0020a6b90de887/calltree/?invertCallstack&jsOnly&thread=0 where 500+ms are spent calling the Timer.jsm version of setTimeout from osfile_async_front.jsm's restartTimer method.

This profile was taken soon after startup on a low-end laptop with an i3 CPU.

Could we:
- call this significantly less often? (needinfo Yoric for this question)
- replace Timer.jsm by something much more efficient provided by the platform? (needinfo ehsan)
Flags: needinfo?(ehsan)
Flags: needinfo?(dteller)

Comment 1

2 years ago
This is the code, right? <http://searchfox.org/mozilla-central/source/toolkit/modules/Timer.jsm>

It can just be replaced with nsITimer.  :-)  Or even better, in contexts where a Window is available (such as browser.xul) it can be replaced with Window.setTimeout and friends.

Your profile lacks native symbols, so unfortunately it's hard to say exactly where all of the overhead exactly comes from.  But we know enough about the overhead caused by JSMs due to the usage of cross-compartment wrappers that it's probably prudent to get rid of Timer.jsm as it seems like it's not really doing much besides add needless overhead.
Flags: needinfo?(ehsan)
Whiteboard: [photon][qf] → [photon][qf:p1]
(Reporter)

Comment 2

2 years ago
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #1)

> It can just be replaced with nsITimer.  :-)   Or even better, in contexts
> where a Window is available (such as browser.xul) it can be replaced with
> Window.setTimeout and friends.

Timer.jsm is specifically for use in contexts where we don't have a window, because the nsITimer API is a pain to use in JS.

I was mostly asking if it's possible to provide a better native API exposed to all our JS modules and components, so that Timer.jsm could be removed from the tree.

> Your profile lacks native symbols, so unfortunately it's hard to say exactly
> where all of the overhead exactly comes from.  But we know enough about the
> overhead caused by JSMs due to the usage of cross-compartment wrappers that
> it's probably prudent to get rid of Timer.jsm as it seems like it's not
> really doing much besides add needless overhead.

For this specific bug, I think it'll be easy to just reset the timer less often.
Whiteboard: [photon][qf:p1] → [qf:p1]
Sorry for the delay, for some reason, this didn't show up on my Bugzilla backlog until today.

> I was mostly asking if it's possible to provide a better native API exposed to all our JS modules and components, so that Timer.jsm could be removed from the tree.

Yeah, that would be nice.

> For this specific bug, I think it'll be easy to just reset the timer less often.

This was most likely caused by bug 1279389, so forwarding to bkelly.
Flags: needinfo?(dteller) → needinfo?(bkelly)
If its causing problems feel free to backout bug 1279389.  I still think we should pursue shutting down stuff when we're not using instead of keeping everything in memory all the time, but I don't have time to pursue it right now.  Also, bug 1279389 only enabled the OS.file auto-restart stuff on nightly/aurora.  It should have no impact on release channels.
Flags: needinfo?(bkelly)
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> comment #1)
> 
> > It can just be replaced with nsITimer.  :-)   Or even better, in contexts
> > where a Window is available (such as browser.xul) it can be replaced with
> > Window.setTimeout and friends.
> 
> Timer.jsm is specifically for use in contexts where we don't have a window,
> because the nsITimer API is a pain to use in JS.
> 
> I was mostly asking if it's possible to provide a better native API exposed
> to all our JS modules and components, so that Timer.jsm could be removed
> from the tree.

Sure, we could do that, but what pain is Timer.jsm removing?  There's a tiny amount of code there...
Flags: needinfo?(florian)
See Also: → bug 1357731
(Reporter)

Comment 6

2 years ago
(In reply to Nathan Froyd [:froydnj] from comment #5)
> (In reply to Florian Quèze [:florian] [:flo] from comment #2)
> > (In reply to :Ehsan Akhgari (super long backlog, slow to respond) from
> > comment #1)
> > 
> > > It can just be replaced with nsITimer.  :-)   Or even better, in contexts
> > > where a Window is available (such as browser.xul) it can be replaced with
> > > Window.setTimeout and friends.
> > 
> > Timer.jsm is specifically for use in contexts where we don't have a window,
> > because the nsITimer API is a pain to use in JS.
> > 
> > I was mostly asking if it's possible to provide a better native API exposed
> > to all our JS modules and components, so that Timer.jsm could be removed
> > from the tree.
> 
> Sure, we could do that, but what pain is Timer.jsm removing?  There's a tiny
> amount of code there...

setTimeout(runnable, delay);

is significantly nicer to type (and remember!) than:

 let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
 timer.initWithCallback(runnable, delay, timer.TYPE_ONE_SHOT);
 (+ keep a JS reference to 'timer' somewhere or it may be GC'ed and never run; but don't forget to cleanup that reference once the timer did run)

Also, it's much more convenient for JS developers to use the same familiar API in all files, so exposing the familiar setTimeout/setInterval API in JS modules and JS xpcom components is... well, exactly what Timer.jsm does.

Could we expose it in all JS contexts? If not, could we make it available through Cu.importGlobalProperties? (feel free to redirect the needinfo if you aren't the best person to answer this)
Flags: needinfo?(florian) → needinfo?(nfroyd)
(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> Also, it's much more convenient for JS developers to use the same familiar
> API in all files, so exposing the familiar setTimeout/setInterval API in JS
> modules and JS xpcom components is... well, exactly what Timer.jsm does.
> 
> Could we expose it in all JS contexts? If not, could we make it available
> through Cu.importGlobalProperties? (feel free to redirect the needinfo if
> you aren't the best person to answer this)

I'm sure we could do the importGlobalProperties thing; exposing something in all JS context sounds a bit sketchy to me...whether or not it would be a good idea is another question.  I think answering that bit would require some XPConnect experience, so bholley might know?  (I would ask Gabor, but he's on PTO.)
Flags: needinfo?(nfroyd) → needinfo?(bobbyholley)
importGlobalProperties is the safest option here.
Flags: needinfo?(bobbyholley)
(Reporter)

Comment 9

2 years ago
Let's move the discussion about replacing Timer.jsm to bug 1357731, and keep this bug to make osfile_async_front.jsm reset its timer less frequently.
(Reporter)

Updated

2 years ago
Blocks: 1363771
(Reporter)

Updated

2 years ago
No longer blocks: 1348289
(Assignee)

Updated

2 years ago
Blocks: 1364015
Comment hidden (mozreview-request)
(Assignee)

Comment 11

2 years ago
This should make the impact to Nightly much less. After this is fixed maybe we should try to enable this for release as well?
Comment on attachment 8881587 [details]
Bug 1353731 - Only attempt to reset timer every per osfile.reset_worker_delay.

https://reviewboard.mozilla.org/r/152746/#review157924

I'm not a huge fan. I believe that the algorithm is already pretty complicated and prone to race conditions and this makes it even more so.

Would the following be acceptable?

- Maintain a flag `hasRecentActivity`, set to `true` whenever we send a message to the worker.
- Run a `setInterval` every `osfile.reset_worker_delay`.
- Whenever the callback fires, if `hasRecentActivity` is `false`, stop the `setInterval`, kill the worker. Otherwise, reset `hasRecentActivity` to `false`.
- Whenever we restart the worker, restart the `setInterval`.
Assignee: nobody → kchen
Flags: needinfo?(kchen)
(Assignee)

Comment 13

2 years ago
(In reply to David Teller [:Yoric] (hard to reach until July 17th - please use "needinfo") from comment #12)
> Comment on attachment 8881587 [details]
> Bug 1353731 - Only attempt to reset timer every per
> osfile.reset_worker_delay.
> 
> https://reviewboard.mozilla.org/r/152746/#review157924
> 
> I'm not a huge fan. I believe that the algorithm is already pretty
> complicated and prone to race conditions and this makes it even more so.
> 
> Would the following be acceptable?
> 
> - Maintain a flag `hasRecentActivity`, set to `true` whenever we send a
> message to the worker.
> - Run a `setInterval` every `osfile.reset_worker_delay`.
> - Whenever the callback fires, if `hasRecentActivity` is `false`, stop the
> `setInterval`, kill the worker. Otherwise, reset `hasRecentActivity` to
> `false`.
> - Whenever we restart the worker, restart the `setInterval`.

Sounds good.
Flags: needinfo?(kchen)
Comment hidden (mozreview-request)
Comment on attachment 8881587 [details]
Bug 1353731 - Only attempt to reset timer every per osfile.reset_worker_delay.

https://reviewboard.mozilla.org/r/152746/#review158020

Looks good to me. Can you think of a way to write a regression test that we do not enter in an infinite loop when killing the worker?
r=me regardless
Attachment #8881587 - Flags: review?(dteller) → review+
(Assignee)

Comment 16

2 years ago
(In reply to David Teller [:Yoric] (hard to reach until July 17th - please use "needinfo") from comment #15)
> Comment on attachment 8881587 [details]
> Bug 1353731 - Only attempt to reset timer every per
> osfile.reset_worker_delay.
> 
> https://reviewboard.mozilla.org/r/152746/#review158020
> 
> Looks good to me. Can you think of a way to write a regression test that we
> do not enter in an infinite loop when killing the worker?
> r=me regardless

The kill logic looks complicated. Since in theory this patch should not change the frequency we call kill, I assume it would just do the Right Thing™.

Comment 17

2 years ago
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22017d54ba07
Only attempt to reset timer every per osfile.reset_worker_delay. r=Yoric

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22017d54ba07
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.