Closed
Bug 1391707
Opened 7 years ago
Closed 7 years ago
Use idle dispatch in JSONFile/DeferredSave/DeferredTask
Categories
(Toolkit :: Async Tooling, enhancement)
Toolkit
Async Tooling
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
JSONFile and DeferredSave do potentially expensive work in their async save operations. DeferredTask has loose guarantees on when deferred tasks will be executed, and only guarantees that a minimal time will pass. All three should attempt to use idle dispatch when possible.
Comment 1•7 years ago
|
||
DeferredTask uses a timer, so it could just use TYPE_ONE_SHOT_LOW_PRIORITY instead of TYPE_ONE_SHOT.
Assignee | ||
Comment 2•7 years ago
|
||
As far as I understand it, TYPE_ONE_SHOT_LOW_PRIORITY only affects the scheduling of idle tasks relative to timer callbacks. It shouldn't do what we need in terms of scheduling the task for the next available idle slice.
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p3]
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8903841 [details] Bug 1391707: Part 2 - Use idle dispatch in DeferredSave. https://reviewboard.mozilla.org/r/175604/#review181286 Looks good to me.
Attachment #8903841 -
Flags: review?(florian) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8903840 [details] Bug 1391707: Part 1 - Use idle dispatch in DeferredTask. https://reviewboard.mozilla.org/r/175602/#review181280 ::: toolkit/modules/DeferredTask.jsm:124 (Diff revision 1) > -this.DeferredTask = function(aTaskFn, aDelayMs) { > +this.DeferredTask = function(aTaskFn, aDelayMs, aIdleTimeoutMs) { > this._taskFn = aTaskFn; > this._delayMs = aDelayMs; > + this._timeoutMs = aIdleTimeoutMs; > + > + this._wrapTask = aTaskFn.isGenerator(); I'm not confident that this will work. I think the edge case we were working around with: let result = this._taskFn(); if (Object.prototype.toString.call(result) == "[object Generator]") { is that aTaskFn can be a normal function returning a generator. Some context in bug 1371895. But... it may be time to just remove the Task.jsm hacks, now that we are in the 57 cycle. ::: toolkit/modules/DeferredTask.jsm:315 (Diff revision 1) > - * Executes the associated task and catches exceptions. > + * Executes the associated task in an idle callback and catches exceptions. > */ > async _runTask() { > try { > - let result = this._taskFn(); > - if (Object.prototype.toString.call(result) == "[object Generator]") { > + let result; > + // If we're being finalized, execute the task immediately, so we don't Is there still a risk of blocking shutdown if _finalized is false when the timer fires, but .finalize() is called before we get an idle callback? Or a risk of _taskFn being called more than once in that case? If I'm reading the code correctly, .finalize() will cause _runTask to be triggered again synchronously, so I think we should add a this._finalized test and a return between getting an idle callback and actually executing _taskFn.
Attachment #8903840 -
Flags: review?(florian) → review-
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8903841 [details] Bug 1391707: Part 2 - Use idle dispatch in DeferredSave. https://reviewboard.mozilla.org/r/175604/#review181292 ::: toolkit/mozapps/extensions/DeferredSave.jsm:192 (Diff revision 1) > > this.logger.debug("Starting timer"); > if (!this._timer) > this._timer = MakeTimer(); > - this._timer.initWithCallback(() => this._deferredSave(), > + this._timer.initWithCallback(() => this._timerCallback(), > this._delay, Ci.nsITimer.TYPE_ONE_SHOT); Would this benefit from using TYPE_ONE_SHOT_LOW_PRIORITY instead of TYPE_ONE_SHOT?
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8903840 [details] Bug 1391707: Part 1 - Use idle dispatch in DeferredTask. https://reviewboard.mozilla.org/r/175602/#review181280 > I'm not confident that this will work. I think the edge case we were working around with: > let result = this._taskFn(); > if (Object.prototype.toString.call(result) == "[object Generator]") { > is that aTaskFn can be a normal function returning a generator. > Some context in bug 1371895. > > But... it may be time to just remove the Task.jsm hacks, now that we are in the 57 cycle. I know. I just meant this to be a halfway point between removing Task.jsm support entirely, so we could avoid the more expensive type checks in every task callback. I'd be happy to just remove the Task.jsm support entirely in one go if you'd prefer, though. > Is there still a risk of blocking shutdown if _finalized is false when the timer fires, but .finalize() is called before we get an idle callback? Or a risk of _taskFn being called more than once in that case? > > If I'm reading the code correctly, .finalize() will cause _runTask to be triggered again synchronously, so I think we should add a this._finalized test and a return between getting an idle callback and actually executing _taskFn. It won't cause `_runTask` to be triggered again, because we check for an existing promise and just wait for it if it hasn't resolved yet. There is some risk of it still blocking shutdown, but to be clear, not of it blocking shutdown *indefinitely*. Idle tasks will still run when we spin the event loop during async shutdown. They just may add a bit of unnecessary delay that I'd rather avoid.
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8903841 [details] Bug 1391707: Part 2 - Use idle dispatch in DeferredSave. https://reviewboard.mozilla.org/r/175604/#review181292 > Would this benefit from using TYPE_ONE_SHOT_LOW_PRIORITY instead of TYPE_ONE_SHOT? I don't think so. That would prevent us from even scheduling the idle callback if there were other idle callbacks that wanted to run, which might lead to some weird event ordering.
Comment 10•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #8) > > I'm not confident that this will work. I think the edge case we were working around with: > > let result = this._taskFn(); > > if (Object.prototype.toString.call(result) == "[object Generator]") { > > is that aTaskFn can be a normal function returning a generator. > > Some context in bug 1371895. > > > > But... it may be time to just remove the Task.jsm hacks, now that we are in the 57 cycle. > > I know. I just meant this to be a halfway point between removing Task.jsm > support entirely, so we could avoid the more expensive type checks in every > task callback. I'd be happy to just remove the Task.jsm support entirely in > one go if you'd prefer, though. I don't think we should break Task.jsm edge cases. I'm ok with r+'ing a patch that either keeps it working in all cases, or drops support. If dropping support, the "@param aTaskFn" comment needs to be updated. > > Is there still a risk of blocking shutdown if _finalized is false when the timer fires, but .finalize() is called before we get an idle callback? Or a risk of _taskFn being called more than once in that case? > > > > If I'm reading the code correctly, .finalize() will cause _runTask to be triggered again synchronously, so I think we should add a this._finalized test and a return between getting an idle callback and actually executing _taskFn. > > It won't cause `_runTask` to be triggered again, because we check for an > existing promise and just wait for it if it hasn't resolved yet. Ah, I had missed that this._timer will always be null in that case, sorry.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•7 years ago
|
||
mozreview-review |
Comment on attachment 8906405 [details] Bug 1391707: Part 0 - Remove Task.jsm support from DeferredTask. https://reviewboard.mozilla.org/r/178110/#review183360 ::: toolkit/modules/DeferredTask.jsm:118 (Diff revision 1) > this.DeferredTask = function(aTaskFn, aDelayMs) { > this._taskFn = aTaskFn; > this._delayMs = aDelayMs; > + > + if (aTaskFn.isGenerator()) { > + Cu.reportError(new Error(`Unexpected generator function passed to DeferredTask`)); nit: Normal double quotes please. I'm not sure we need this error reporting and/or how long we should keep it before removing it, but I guess it doesn't hurt to have it.
Attachment #8906405 -
Flags: review?(florian) → review+
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8903840 [details] Bug 1391707: Part 1 - Use idle dispatch in DeferredTask. https://reviewboard.mozilla.org/r/175602/#review183366 Thanks!
Attachment #8903840 -
Flags: review?(florian) → review+
Assignee | ||
Comment 16•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8154fcbcb707191b3526ff10d78a1ea36550213b Bug 1391707: Part 0 - Remove Task.jsm support from DeferredTask. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/df31bae194f894d29e0f91ef79d0aa34709eafb1 Bug 1391707: Part 1 - Use idle dispatch in DeferredTask. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/abf475c970af34d92acf0e29d0db394678d09156 Bug 1391707: Part 2 - Use idle dispatch in DeferredSave. r=florian
Assignee | ||
Comment 17•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/827ea05cc1cbe8cee8d7c3059bc8d5786c72ee90 Bug 1391707: Follow-up: Update DeferredTask tests not to use generator functions. r=me
Assignee | ||
Comment 18•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/041b28ae05dc6e6e12130ceb05908361281a07b6 Bug 1391707: Follow-up: Skip idle dispatch in passwordManager.js for increasing the failure rate of an intermittent. r=me
Assignee | ||
Comment 19•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/132beb0e0cc25b98ac51470b7c4efe773e2100e3 Bug 1391707: Follow-up: Skip idle dispatch during tests in TelemetrySession.jsm for increasing the failure rate of an intermittent. r=me
Assignee | ||
Comment 20•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f332ddce365e7eead25573f7d49e7523ea4ff07 Bug 1391707: Follow-up: Skip idle in more places that incorrectly expect strict timing. r=me CLOSED TREE
Comment 21•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8154fcbcb707 https://hg.mozilla.org/mozilla-central/rev/df31bae194f8 https://hg.mozilla.org/mozilla-central/rev/abf475c970af https://hg.mozilla.org/mozilla-central/rev/827ea05cc1cb https://hg.mozilla.org/mozilla-central/rev/041b28ae05dc https://hg.mozilla.org/mozilla-central/rev/132beb0e0cc2 https://hg.mozilla.org/mozilla-central/rev/0f332ddce365
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•2 years ago
|
Performance Impact: --- → P3
Whiteboard: [qf:p3]
You need to log in
before you can comment on or make changes to this bug.
Description
•