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)

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.

(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.

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.

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.

(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...

(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)

(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.)

importGlobalProperties is the safest option here.

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.

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`.

(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.

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

(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™.

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