Given our recent performance push, I thought I'd bring this topic up again.

When writing loops using async functions in browser code, it's still
very easy to cause jank. For example, a trivial function:

 > async function janky() {
 >   for (let i = 0; i < 100000; i++) {
 >     await Promise.resolve();
 >   }
 > }
 >
 > janky().then(() => console.log("done"));

will cause the browser to hang. While that's not considered a bug in the
implementation of the promise scheduler, and might not be particularly
surprising in that trivial example, lots of real-world code can still
hit this case - which both isn't obvious from casual inspection, and
even if it was, doesn't have an obvious solution.

Concretely, we struck this a couple of years ago in bug 1186714 -
creating a backup of all bookmarks ends up looking alot like the loop
above. In addition, the Sync team is moving away from nested event loops
towards promises, and our work to date is hitting this problem.

In bug 1186714, we solved the problem by inserting code into the loop
that looks like:

 >    if (i % 50 == 0) {
 >      await new Promise(resolve => {
 >        Services.tm.dispatchToMainThread(resolve);
 >      });
 >    }

http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/toolkit/components/places/PlacesUtils.jsm#2022

so we explicitly yield to the event loop every 50 iterations. However,
this isn't optimal as the 50 is obviously a magic number, determined by
experimentation on a couple of machines - when running on low spec
hardware, this loop is almost certainly still janky. If we try and err
on the side of caution (eg, yielding every iteration) we see the wall
time of the loop take a massive hit (around twice as long in that bug).

I'm wondering if there are any ideas about how to solve this optimally?
Naively, it seems that the (broadest sense of the term) "platform" might
be able to help here using it's knowledge of the event-loop state - eg,
a function that indicates "are we about to starve the event loop and
become janky?", or possibly even the whole hog (ie, "please yield to the
event loop if you think now is a good time, otherwise just hand back a
pre-resolved promise").

Or maybe there are simpler options I've overlooked?

Thanks,

Mark

On 5/17/17 9:22 PM, Mark Hammond wrote:
> I'm wondering if there are any ideas about how to solve this optimally?

I assume
https://w3c.github.io/requestidlecallback/#the-requestidlecallback-method
doesn't have quite the right semantics here?  That would let you run
when the browser is idle, and give you some idea of how long you can run
for before you should yield.

-Boris

On Thursday 2017-05-18 11:22 +1000, Mark Hammond wrote:
> I'm wondering if there are any ideas about how to solve this optimally?
> Naively, it seems that the (broadest sense of the term) "platform" might be
> able to help here using it's knowledge of the event-loop state - eg, a
> function that indicates "are we about to starve the event loop and become
> janky?", or possibly even the whole hog (ie, "please yield to the event loop
> if you think now is a good time, otherwise just hand back a pre-resolved
> promise").

One other option would be to use time rather than iterations as a
measure of when to return.

A platform API that's somewhat like this is requestIdleCallback,
whose spec is at https://w3c.github.io/requestidlecallback/ -- in
particular, the timeRemaining() method on the IdleDeadline object
passed to the callback.  See also:
https://developer.mozilla.org/en-US/docs/Web/API/Window/requestIdleCallback
https://developer.mozilla.org/en-US/docs/Web/API/Background_Tasks_API

I'm not sure whether it works for privileged (chrome) JS, but it
seems like it ought to...

-David

--
𝄞   L. David Baron                         http://dbaron.org/   𝄂
𝄢   Mozilla                          https://www.mozilla.org/   𝄂
             Before I built a wall I'd ask to know
             What I was walling in or walling out,
             And to whom I was like to give offense.
               - Robert Frost, Mending Wall (1914)

If rIC is not the right semantics, we could also set a time budget instead
of a magic flat limit.  Every N operations call performance.now() and check
to see if the configured time exceeds the limit.

FWIW, we have a similar problem in the native TimeoutManager::RunTImeout()
method.  I'm using a time budget approach to make it adapt to different
hardware better.

On Wed, May 17, 2017 at 10:19 PM, Ben Kelly <bke...@mozilla.com> wrote:

> FWIW, we have a similar problem in the native TimeoutManager::RunTImeout()
> method.  I'm using a time budget approach to make it adapt to different
> hardware better.
>

I meant to include the bug number here:

https://bugzilla.mozilla.org/show_bug.cgi?id=1343912

The only way I could see that work would be if the code was explicitly
written as a consumer of a queue of promises (along the lines of that
"Cooperative Scheduling of Background Tasks API" example). However, I
can't see how code written "naturally" (eg, that loop in PlacesUtils.jsm
I linked to) could leverage that.

IdleDeadline.timeRemaining() does appear to have the semantics I want,
but just seems "inverted" in terms of how it is used. A function that,
basically, returns this value synchronously sounds like what I am after
though.

Thanks,

Mark

That sounds interesting and is almost certainly better than a hard-coded
50, but doesn't it have the same basic problem? That depending on what
other things are going on, the magic time-limit chosen will either be
too conservative (ie, sacrificing wall time) or too liberal (ie, still
causing jank)?

Thanks,

Mark.

On Thu, May 18, 2017, at 10:10 AM, L. David Baron wrote:

> On Thursday 2017-05-18 11:22 +1000, Mark Hammond wrote:
> > I'm wondering if there are any ideas about how to solve this optimally?

> > Naively, it seems that the (broadest sense of the term) "platform" might be
> > able to help here using it's knowledge of the event-loop state - eg, a
> > function that indicates "are we about to starve the event loop and become
> > janky?", or possibly even the whole hog (ie, "please yield to the event loop
> > if you think now is a good time, otherwise just hand back a pre-resolved
> > promise").
>
> One other option would be to use time rather than iterations as a
> measure of when to return.
>
> A platform API that's somewhat like this is requestIdleCallback,
> whose spec is at https://w3c.github.io/requestidlecallback/ -- in
> particular, the timeRemaining() method on the IdleDeadline object
> passed to the callback.  See also:
> https://developer.mozilla.org/en-US/docs/Web/API/Window/requestIdleCallback
> https://developer.mozilla.org/en-US/docs/Web/API/Background_Tasks_API
>
> I'm not sure whether it works for privileged (chrome) JS, but it
> seems like it ought to...

The non-DOM version of rIC is being tracked by
https://bugzilla.mozilla.org/show_bug.cgi?id=1353206 and
https://bugzilla.mozilla.org/show_bug.cgi?id=1358476

Kanru

FWIW, I just yesterday suggested in #whatwg that the platform should have something like IdlePromise or AsyncPromise.
And there is the related spec bug https://github.com/whatwg/html/issues/512#issuecomment-171498578

On Thursday, May 18, 2017 at 4:34:37 AM UTC-4, smaug wrote:
> FWIW, I just yesterday suggested in #whatwg that the platform should have something like IdlePromise or AsyncPromise.
> And there is the related spec bug https://github.com/whatwg/html/issues/512#issuecomment-171498578

In my opinion there's no need for a whole new promise subclass, just make requestIdleCallback() return a promise when there's no argument passed.

On 5/18/17 12:03 PM, Boris Zbarsky wrote:

I didn't quite expect this to work, but by abusing rIC I can almost make
something work - I just have the callback stash the IdleDeadline object
and return immediately, but continue to refer to it anyway. eg:

let idleChecker = {
   resolved: Promise.resolve(),
   deadline: null,
   promiseIdle() {
     if (this.deadline && this.deadline.timeRemaining() > 0) {
       return this.resolved;
     }
     this.deadline = null
     return new Promise(resolve => {
       window.requestIdleCallback(deadline => {
         this.deadline = deadline;
         resolve();
       });
     })
   }
}

async function janky() {
   let start = Date.now();
   for (let i = 0; i < 1000; i++) {
     await Promise.resolve();
     await idleChecker.promiseIdle();
   }
   console.log("took", Date.now() - start)

}
janky().then(() => console.log("done"));

I 1/2 expect this to defeat the intent/implementation of rIC, but it
does work, causing ~2x-5x slowdown of the loop. I wonder if this is
worth experimenting with some more?

Mark

So if you have a look at how the idle callback algorithm is defined[1]
and what timeRemaining is supposed to return[2] you see that
timeRemaining doesn't update its sense of idleness, it only concerns
itself with the deadline. So if you save the IdleDeadline object and
resolve early, then timeRemaining won't know that the idle period
entered upon calling the idle callback might have ended.

I do think that you need to invert this somehow, actually doing the
work inside a rIC callback. Something like[3]:

  let idleTask = {
    total: 100000,
    progress: 0,
    doWork: async function(deadline) {
      for (; this.progress < this.total && deadline.timeRemaining() >
0; ++this.progress) {
        await Promise.resolve();
      }
      console.log(this.total - this.progress)
    },
    schedule(deadline) {
      this.doWork(deadline).then(() => {
        if (this.progress < this.total) {
          requestIdleCallback(this.schedule.bind(this));
        } else {
          this.resolve();
        }
      })
    },
    run() {
      this.resolve = null;
      return new Promise(resolve => {
        this.resolve = resolve;
        requestIdleCallback(this.schedule.bind(this));
      })
    }
  }

  async function janky() {
    var start = Date.now();
    await idleTask.run();

    console.log("took", Date.now() - start)
  }

  var handle = setInterval(function (){ console.log("I shouldn't be
blocked!")}, 500)
  janky().then(() => console.log("janky done")).then(() =>
clearTimeout(handle));

perhaps? The doWork function is indeed async, and executed within a
loop that both handles how much work that should be done as well as
using IdleDeadline.timeRemaining as expected.

[1] https://w3c.github.io/requestidlecallback/#invoke-idle-callbacks-algorithm
[2] https://w3c.github.io/requestidlecallback/#the-timeremaining-method

Cheers,
Andreas

That wouldn't really catch my idea. I was hoping to have a whole Promise chain using async or idle scheduling. Running tons of Promises even at rIC
time is still running tons of promises and possibly causing jank.

On 5/19/17 8:00 PM, Andreas Farre wrote:
> So if you have a look at how the idle callback algorithm is defined[1]
> and what timeRemaining is supposed to return[2] you see that
> timeRemaining doesn't update its sense of idleness, it only concerns
> itself with the deadline. So if you save the IdleDeadline object and
> resolve early, then timeRemaining won't know that the idle period
> entered upon calling the idle callback might have ended.

Right - I was guessing something like that to be the case.

> I do think that you need to invert this somehow, actually doing the
> work inside a rIC callback. Something like[3]:
>
>    let idleTask = {
>      total: 100000,
>      progress: 0,
>      doWork: async function(deadline) {

The problem is that this is typically an unnatural way to express what
is being done - particularly when attempting to address jank after the
fact - and even more-so now that we have async functions, which making
writing async code extremely natural and expressive.

As I mentioned at the start of the thread, in one concrete example we
had code already written that we identified being janky -
http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/toolkit/components/places/PlacesUtils.jsm#2022

Re-writing this code to work as you describe is certainly possible, but
unlikely. So I'm still hoping there is something we can do to avoid jank
without losing expressiveness and rewriting code identified as janky
after the fact.

Thanks,

Mark

On Sun, May 21, 2017, at 09:29 PM, Mark Hammond wrote:
> As I mentioned at the start of the thread, in one concrete example we
> had code already written that we identified being janky -
> http://searchfox.org/mozilla-central/rev/f55349994fdac101d121b11dac769f3f17fbec4b/toolkit/components/places/PlacesUtils.jsm#2022

Is this a scenario where we could move this intensive logic off the
(parent process) main thread by fulfilling the dream of the "SQLite
interface for Workers" bug[1] by using WebIDL instead of js-ctypes to
let the Sqlite.jsm abstraction operate on ChromeWorkers?  The only
XPConnect leakage on the Sqlite.jsm API surface is
mozIStorageStatementRow[2], and although it's a bit unwieldy in terms of
method count, we never exposed any of the nsIXPCScriptable magic on it
that we did on statements.  (And thankfully SQLite.jsm neither uses or
otherwise exposes the API.)

It wouldn't be a trivial undertaking, but it's also not impossible
either.  And if Sync is chewing up a lot of main thread time both
directly (processing) and indirectly (generating lots of of garbage that
lengthens parent-process main-thread GC's), it may be worth considering
rather than trying to optimize the time-slicing of Sync.  This does, of
course, assume that Sync can do meaningful work without access to
XPConnect and that there aren't major gotchas in coordinating with
Places' normal operation.

Note: I'm talking exclusively about using the existing asynchronous
Sqlite.jsm API on top of the existing async mozStorage API usage.

Andrew

1: https://bugzilla.mozilla.org/show_bug.cgi?id=853438
2:
https://searchfox.org/mozilla-central/source/storage/mozIStorageRow.idl
subclasses
https://searchfox.org/mozilla-central/source/storage/mozIStorageValueArray.idl