Closed Bug 1391707 Opened 2 years ago Closed 2 years ago

Use idle dispatch in JSONFile/DeferredSave/DeferredTask

Categories

(Toolkit :: Async Tooling, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p3])

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.
DeferredTask uses a timer, so it could just use TYPE_ONE_SHOT_LOW_PRIORITY instead of TYPE_ONE_SHOT.
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.
Whiteboard: [qf] → [qf:p3]
Blocks: 1395213
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 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 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?
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.
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.
(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 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 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/827ea05cc1cbe8cee8d7c3059bc8d5786c72ee90
Bug 1391707: Follow-up: Update DeferredTask tests not to use generator functions. r=me
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
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
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
Depends on: 1524786
You need to log in before you can comment on or make changes to this bug.