1386206 - Breaking FormData's collect loop into chunks to be executed in idle dispatch

Status: NEW

(Firefox :: Session Restore, enhancement, P3)

Description

[Bao Quan [:beekill]]

The loop [1] can be very expensive when there are a huge amount of input nodes to iterate through, therefore, will lead to janking UI thread. We should break the loop into chunks and put those chunks inside a requestIdleCallback. That way, the UI will have a chance to process users' interactions and thus, prevent janking.

[1]: http://searchfox.org/mozilla-central/source/toolkit/modules/sessionstore/FormData.jsm#186

Comment 1

[Bao Quan [:beekill]]

Attachment: Bug 1386206 - Part 1: Make createLazy function promise-aware so that other collect functions can return a promise instead of value.

Review commit: https://reviewboard.mozilla.org/r/164296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164296/

Comment 2

[Bao Quan [:beekill]]

Attachment: Bug 1386206 - Part 2: Make form data collect function to execute in async fashion.

Review commit: https://reviewboard.mozilla.org/r/164298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164298/

Comment 3

[Bao Quan [:beekill]]

The patches failed at these test browser_454908 / 456342 / formdata / formdata_cc / formdata_xpath. I looked at those tests and found out that they were waiting at [1]. Not sure what happened.

The patches also timed out at the test browser_590563 even though they passed all test inside [2].

It also complains about:

> TelemetryStopwatch: key "FX_SESSION_RESTORE_CONTENT_COLLECT_DATA_MS" was already initialized

or:

> TelemetryStopwatch: requesting elapsed time for nonexisting stopwatch. Histogram:
> "FX_SESSION_RESTORE_CONTENT_COLLECT_DATA_MS", key: "formdata"

[1]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/head.js#482
[2]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_590563.js#34-51

Comment 4

[Dão Gottwald [:dao]]

(In reply to Bao Quan [:beekill] from comment #0)
> The loop [1] can be very expensive when there are a huge amount of input
> nodes to iterate through, therefore, will lead to janking UI thread.

Wait, this runs in the content process. How can it jank the UI thread?

Comment 5

[Bao Quan [:beekill]]

(In reply to Dão Gottwald [::dao] from comment #4)
> (In reply to Bao Quan [:beekill] from comment #0)
> > The loop [1] can be very expensive when there are a huge amount of input
> > nodes to iterate through, therefore, will lead to janking UI thread.
> 
> Wait, this runs in the content process. How can it jank the UI thread?

Oops! I thought it is called UI thread. Anyway, you can check this with test in bug 536910. When users click multiple checkboxes continuously, the thread pauses for a second or so to do the loop [1]. During that time, the scrolling pauses and checkboxes aren't shown as checked.

[1]: http://searchfox.org/mozilla-central/source/toolkit/modules/sessionstore/FormData.jsm#186

Comment 6

[Bao Quan [:beekill]]

Here is the discussion between me and Dão about the tests failing:

<beekill> I want to check what makes the patch in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1386206 causes time out in some tests.
<beekill> What I found are:
<beekill> + they're all waiting for: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/head.js#482 to finish
<beekill> + The function is blocked because the function collectInIdle (FormData.jsm, line 299: https://reviewboard.mozilla.org/r/164298/diff/1#index_header) is never called.
<beekill> However, if I added a simple wait function: https://pastebin.mozilla.org/9028824 in http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_454908.js#20. The test passes.
<beekill> So I want to know what prevents the collectInIdle from being called
<beekill> I mean: call the wait function here: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_454908.js#25
<beekill> I didn't test if it is going to work for all the timed out tests. But they're all waiting for promiseRemoveTab, so I think they should work too.
<dao> does collectInIdle get called if you replace requestIdleCallback with setTimeout(..., 0)?
<beekill> let me check that
<beekill> it doesn't get called
<dao> hm, hm, does the function passed to new Promise ever get called?
<beekill> yeah, it does :)
<dao> and if you call collectInIdle directly without requestIdleCallback?
<beekill> then everything is fine

Comment 7

[Bao Quan [:beekill]]

Comment on attachment 8893260 [details]
Bug 1386206 - Part 1: Make createLazy function promise-aware so that other collect functions can return a promise instead of value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164296/diff/1-2/

Comment 8

[Bao Quan [:beekill]]

Comment on attachment 8893261 [details]
Bug 1386206 - Part 2: Make form data collect function to execute in async fashion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164298/diff/1-2/

Comment 9

[Bao Quan [:beekill]]

Updated to the latest build.

Comment 10

[Mike de Boer [:mikedeboer]]

Comment on attachment 8893260 [details]
Bug 1386206 - Part 1: Make createLazy function promise-aware so that other collect functions can return a promise instead of value.

https://reviewboard.mozilla.org/r/164296/#review171718

I see how you tried your best to divide the patches up into to standalone parts, but perhaps in this case a monolithic changeset would've been easier for me to grasp.

::: browser/components/sessionstore/content/content-sessionStore.js:63
(Diff revision 2)
>   * Returns a lazy function that will evaluate the given
>   * function |fn| only once and cache its return value.
> + * The function will return a promise that is resolved
> + * with a cached value.
>   */
>  function createLazy(fn) {

This confused me quite a bit. Doesn't the following do the same thing?

```js
function createLazy(fn) {
  let cached = false;
  let cachedValue = null;

  return function lazy() {
    let resolve;
    let promise = new Promise(res => resolve = res);

    if (!cached) {
      cachedValue = fn();
      cached = true;
      resolve(value);
    }

    return promise;
  };
}
```

BTW, this does this exact same thing as before, but just turns this into promise-returning function with the same value. Was that your intention? Why did you feel you needed to touch `createLazy` at all?

::: browser/components/sessionstore/content/content-sessionStore.js:850
(Diff revision 2)
> -        TelemetryStopwatch.finishKeyed(histID, key);
> +          TelemetryStopwatch.finishKeyed(histID, key);
> -      }
> +        }
>  
> -      if (value || (key != "storagechange" && key != "historychange")) {
> +        if (value || (key != "storagechange" && key != "historychange")) {
> -        data[key] = value;
> +          data[key] = value;
> +        }  

nit: trailing space.

::: browser/components/sessionstore/content/content-sessionStore.js:861
(Diff revision 2)
> +        TelemetryStopwatch.startKeyed(histID, key);
> +      }
> +
> +      let promise = func();
> +      promise.then(dataCollected(key));
> +      promises.push(promise);

nit: you could've shortened this to:

```js
// This will really need an explainer, regardless!
promises.push(func().then(dataCollected(key)));
```

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8893261 [details]
Bug 1386206 - Part 2: Make form data collect function to execute in async fashion.

https://reviewboard.mozilla.org/r/164298/#review171722

::: browser/components/sessionstore/content/content-sessionStore.js:556
(Diff revision 2)
>      MessageQueue.push("formdata", () => null);
>    },
>  
>    collect() {
> -    return mapFrameTree(FormData.collect);
> +    let promises = [];
> +    let resolve;

Also use the `let defer = {};` pattern here, please.

::: browser/components/sessionstore/content/content-sessionStore.js:559
(Diff revision 2)
>    collect() {
> -    return mapFrameTree(FormData.collect);
> +    let promises = [];
> +    let resolve;
> +    let formData = mapFrameTree((frame) => {
> +      // We'll return a placeholder object. The |isCollecting|
> +      // is needed if we don't want |map| returns null.

But that's the whole idea... returning `null` explicitly means that the consumers of this data can skip potentially expensive operations. Also not for sub-frames, because you'll always return an object where `FormData.collect` would sometimes return `null`.

You'll have to find a solution where you can make the operation async, while not affecting the return values.

::: toolkit/modules/sessionstore/FormData.jsm:272
(Diff revision 2)
> +      this.resolveNS.bind(this),
> +      Ci.nsIDOMXPathResult.UNORDERED_NODE_ITERATOR_TYPE, null
> +    );
> +
> +    let resolve;
> +    let ret = {};

If you want to use the callbacks of a Promise in another place too, please consider using a pattern that more developers already know:

```js
let defer = {};
defer.promise = new Promise((resolve, reject) => {
  defer.resolve = resolve;
  defer.reject = reject;
});

function doingSomethingLazilyHere() {
  defer.resolve(cachedResultValue);
}

return defer.promise;
```

There's also a module called 'PromiseUtils.jsm' that exposes this pattern, but it can only be used in the main (chrome) process, because of performance reasons (Cross Component Wrappers).

::: toolkit/modules/sessionstore/FormData.jsm:303
(Diff revision 2)
>  
> -    return ret;
> +      // Resolve the promise.
> +      resolve(ret);
> +    }
> +
> +    function collectInIdle(deadline) {

nit: please rename this to `collectWhileIdle`

::: toolkit/modules/sessionstore/FormData.jsm:308
(Diff revision 2)
> +    function collectInIdle(deadline) {
> +      while (deadline.timeRemaining()) {
> +        let node = formNodes.iterateNext();
> +
> +        if (node) {
> +          xpathGenerated = FormDataInternal.collectNode(node, ret, xpathGenerated);

```js
if (FormDataInternal.collectFromNode(node, ret)) {
  ++generatedCount;
}
```

Comment 12

[Bao Quan [:beekill]]

Comment on attachment 8893260 [details]
Bug 1386206 - Part 1: Make createLazy function promise-aware so that other collect functions can return a promise instead of value.

https://reviewboard.mozilla.org/r/164296/#review171718

> This confused me quite a bit. Doesn't the following do the same thing?
> 
> ```js
> function createLazy(fn) {
>   let cached = false;
>   let cachedValue = null;
> 
>   return function lazy() {
>     let resolve;
>     let promise = new Promise(res => resolve = res);
> 
>     if (!cached) {
>       cachedValue = fn();
>       cached = true;
>       resolve(value);
>     }
> 
>     return promise;
>   };
> }
> ```
> 
> BTW, this does this exact same thing as before, but just turns this into promise-returning function with the same value. Was that your intention? Why did you feel you needed to touch `createLazy` at all?

Your version is definitely bettern than mine :)).

And it was my mistake. I should include `Promise.resolve(cachedValue)`. That way, |fn| can returns a value or a promise and we're still ok.

Comment 13

[Bao Quan [:beekill]]

Comment on attachment 8893261 [details]
Bug 1386206 - Part 2: Make form data collect function to execute in async fashion.

https://reviewboard.mozilla.org/r/164298/#review171722

> But that's the whole idea... returning `null` explicitly means that the consumers of this data can skip potentially expensive operations. Also not for sub-frames, because you'll always return an object where `FormData.collect` would sometimes return `null`.
> 
> You'll have to find a solution where you can make the operation async, while not affecting the return values.

I can only think of this solution to make the collect function execute in async and get the correct result. I can add another filter operation to filter out empty object `{}` after all the promises are resolved. However, we already did that in `PrivacyFilter.filterFormData` [1].

Mike, do you have an alternative approach to this?

[1]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/PrivacyFilter.jsm#74

> ```js
> if (FormDataInternal.collectFromNode(node, ret)) {
>   ++generatedCount;
> }
> ```

Instead of using your version, I think this is much nicer:

```js
  collectNode(node, ret, xpath) {
    ...
    if (!node.id && xpath.count > MAX_TRAVERSED_XPATHS) {
      return;
    }
    
    ...
    
    if (node.id) {
    
    } else {
      xpath.count++;
    }
  }
  ...

  let xpath = {count: 0};
  ...
  collectNode(node, ret, xpath);
  
```

What do you think?

Comment 14

[Bao Quan [:beekill]]

Attachment: Bug 1386206 - Part 3: Flush tab state and ensure we received the message before we close a tab.

Also, the valid CC tests are changed to pass when form data is null and form data doesn't present in data collected.

Review commit: https://reviewboard.mozilla.org/r/168124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/168124/

Comment 15

[Bao Quan [:beekill]]

Comment on attachment 8893260 [details]
Bug 1386206 - Part 1: Make createLazy function promise-aware so that other collect functions can return a promise instead of value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164296/diff/2-3/

Comment 16

[Bao Quan [:beekill]]

Comment on attachment 8893261 [details]
Bug 1386206 - Part 2: Make form data collect function to execute in async fashion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164298/diff/2-3/

Comment 17

[Bao Quan [:beekill]]

In the 3rd part of the patch, I added the `TabStateFlusher.flush` to ensure that we have the latest form data from each tab. This will make the form data collect promise is resolved before we hit `promiseRemoveTab`. One drawback of this solution is the slow execution of the browser_formdata_cc.js test. On my machine, it takes 129 seconds to finish.

The only failed test remained is the test browser_590563.js, which test middle click on tab. I'm not sure what is wrong. The test passed all the assertions [1] and hit `finish()` [2]. However, it just freezes and causes the whole test suite to time out.

Mike, do you know what may cause this?

[1]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_590563.js#48-51
[2]: http://searchfox.org/mozilla-central/source/browser/components/sessionstore/test/browser_590563.js#28

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8893260 [details]
Bug 1386206 - Part 1: Make createLazy function promise-aware so that other collect functions can return a promise instead of value.

https://reviewboard.mozilla.org/r/164296/#review174060

Comment 19

[Mike de Boer [:mikedeboer]]

Comment on attachment 8893261 [details]
Bug 1386206 - Part 2: Make form data collect function to execute in async fashion.

https://reviewboard.mozilla.org/r/164298/#review174062

During our 1:1 we discussed a different approach to solve this issue, which should result a simpler implementation:

Instead of making each and every collectNode call asynchronous by virtue of wrapping them inside Promises, we'd:
* Start collecting form nodes from within an idle callback
* After each `iterateNext` we check if we stil have enough time to continue according to `deadline.timeRemaining()`
* If there's enough time, continue on, if not we should request a new idle callback.
* Continue until we're out of form nodes.

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8896829 [details]
Bug 1386206 - Part 3: Flush tab state and ensure we received the message before we close a tab.

https://reviewboard.mozilla.org/r/168124/#review174068

Comment 21

[Bao Quan [:beekill]]

Comment on attachment 8893261 [details]
Bug 1386206 - Part 2: Make form data collect function to execute in async fashion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164298/diff/3-4/

Comment 22

[Bao Quan [:beekill]]

Comment on attachment 8896829 [details]
Bug 1386206 - Part 3: Flush tab state and ensure we received the message before we close a tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168124/diff/1-2/

Comment 23

[Mike de Boer [:mikedeboer]]

Comment on attachment 8893261 [details]
Bug 1386206 - Part 2: Make form data collect function to execute in async fashion.

https://reviewboard.mozilla.org/r/164298/#review176804

As per discussion on IRC: Quan is writing an improved patch.

Comment 24

[Mike de Boer [:mikedeboer]]

Comment on attachment 8896829 [details]
Bug 1386206 - Part 3: Flush tab state and ensure we received the message before we close a tab.

https://reviewboard.mozilla.org/r/168124/#review176812

Comment 25

[Bao Quan [:beekill]]

Comment on attachment 8893260 [details]
Bug 1386206 - Part 1: Make createLazy function promise-aware so that other collect functions can return a promise instead of value.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164296/diff/3-4/

Comment 26

[Bao Quan [:beekill]]

Comment on attachment 8893261 [details]
Bug 1386206 - Part 2: Make form data collect function to execute in async fashion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164298/diff/4-5/

Comment 27

[Bao Quan [:beekill]]

Comment on attachment 8896829 [details]
Bug 1386206 - Part 3: Flush tab state and ensure we received the message before we close a tab.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/168124/diff/2-3/

Comment 28

[Mike de Boer [:mikedeboer]]

Comment on attachment 8893260 [details]
Bug 1386206 - Part 1: Make createLazy function promise-aware so that other collect functions can return a promise instead of value.

https://reviewboard.mozilla.org/r/164296/#review178530

I apologise for not being clear enough the first time around.
What you're doing here is making the entire code flow here async, solely by virtue of sprinkling the code with Promises. Whilst that is certainly achieving that goal, it shoots past the real one, which is making this code run inside an idle callback. This you achieve in Part 2.
Even though the main idea is to make things - including sessionstore - perform better, this in fact does the opposite; there's a cost to using Promises (or any scheduling-backed primitive, for that matter) and its use here will certainly make data collection run slower than it does now. Async != fast.
So please discard this patch and start with Part 2 instead.

Comment 29

[Mike de Boer [:mikedeboer]]

Comment on attachment 8893261 [details]
Bug 1386206 - Part 2: Make form data collect function to execute in async fashion.

https://reviewboard.mozilla.org/r/164298/#review178534

::: browser/components/sessionstore/content/content-sessionStore.js:151
(Diff revision 5)
> +    if (children.length) {
> +      obj.children = children;
> +    }
> +
> +    return Object.keys(obj).length ? obj : null;
> +  })(content, callback);

With part 1 gone, you can keep this the way it was - so remove this new function and the defer helper above.
What you do need to do is make `function mapFrameTree(callback)` `async function mapFrameTree(callback)`, because `callback` will be an async function in this case.
Don't worry, it will still work for regular function callbacks!
You only need to change `let obj = cb(frame) || {};` to `let obj = await cb(frame) || {};`.

::: toolkit/modules/sessionstore/FormData.jsm:100
(Diff revision 5)
>  /**
>   * The public API exported by this module that allows to collect
>   * and restore form data for a document and its subframes.
>   */
>  this.FormData = Object.freeze({
> -  collect(frame) {
> +  collect(frame, window) {

You can get the window object from the `frame` argument, so please remove it.

Comment 30

[Mike de Boer [:mikedeboer]]

Comment on attachment 8896829 [details]
Bug 1386206 - Part 3: Flush tab state and ensure we received the message before we close a tab.

https://reviewboard.mozilla.org/r/168124/#review178538

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:26
(Diff revision 3)
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
>  Cu.import("resource://gre/modules/Timer.jsm");
>  Cu.import("resource://testing-common/TestUtils.jsm");
>  Cu.import("resource://testing-common/ContentTask.jsm");
> +const {TabStateFlusher} = Cu.import("resource:///modules/sessionstore/TabStateFlusher.jsm", {});

Please add this as a lazy module getter.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:978
(Diff revision 3)
> +    let tabFlushed = TabStateFlusher.flush(tab.linkedBrowser).then(() => {
> -    if (!tab.closing) {
> +      if (!tab.closing) {
> -      tab.ownerGlobal.gBrowser.removeTab(tab, options);
> +        tab.ownerGlobal.gBrowser.removeTab(tab, options);
> -    }
> +      }
> -    return tabRemoved;
> +    });
> +    return Promise.all([tabFlushed, tabRemoved]);

This can potentially impact _a lot_ of tests! Please run this through try extensively!

Comment 31

[Mike de Boer [:mikedeboer]]

Quan, will you be finishing up the work here?

Comment 32

[Bao Quan [:beekill]]

Sure!! I'm getting a lot of failure tests with this patch!! I'm trying to fix them all.

Comment 33

[Mike de Boer [:mikedeboer]]

Quan hasn't been able to work on Firefox bugs for a while now. Unassigning to allow others to finish this up.

Comment 34

[:Gijs (he/him)]

P3 in the absence of profiles.

Comment 35

[Mayank Bansal]

Is this still relevant?

Comment 36

[Sam Foster [:sfoster] (he/him)]

I'm sorry I don't actually know. The FormData.jsm no longer exists, nor are there any hits for FormDataInternal. However, TabStateFlusher.sys.mjs is still a thing and we still have need to collect up form data and serialize it into the session store. So it might be that our current implementation has the same problem we were trying to address here originally, but perhaps it would be better to start fresh with a new bug, new observable problem and new STR?