1379900 - Simplify the XPath query in FormData.collect by doing elements filtering in JS

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Bao Quan [:beekill]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170616092053

Comment 1

[Bao Quan [:beekill]]

Attachment: Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

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

Comment 2

[Bao Quan [:beekill]]

Profiling result:
+ Before the patch: https://perfht.ml/2uLJyYO
+ After the patch: https://perfht.ml/2ucGwQe

Comment 3

[Mike de Boer [:mikedeboer]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

https://reviewboard.mozilla.org/r/156006/#review162528

This is a good start, but I think you can do even more!

::: toolkit/modules/sessionstore/FormData.jsm:134
(Diff revision 1)
>    get restorableFormNodesXPath() {
>      // for a comprehensive list of all available <INPUT> types see
>      // https://dxr.mozilla.org/mozilla-central/search?q=kInputTypeTable&redirect=false
>      let ignoreInputs = new Map([
> -      ["type", ["password", "hidden", "button", "image", "submit", "reset"]],
>        ["autocomplete", ["off"]]

What I'm not sure about is _why_ you needed inputs with 'autocomplete=off' to continue to be filtered using XPath instead of JS. Any particular reason?
I'd like to simplify the code here as well by just removing the `ignoreInputs` array altogether.

I'd recommend the following structure:
```js
const IGNORE_INPUTS = new Map([
  ["type", new Set(["password", "hidden"/*, etc*/])],
  ["autocomplete", new Set(["off"])]
]);

function shouldIgnoreNode(node) {
  for (let [attrName, attrValues] of IGNORE_INPUTS) {
    if (node.hasAttribute(attrName) &&
        attrValues.has(node.getAttribute(attrName)) {
      return true;
    }
  }
  return false;
}
```

What do you think?

Comment 4

[Bao Quan [:beekill]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

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

Comment 5

[Bao Quan [:beekill]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

https://reviewboard.mozilla.org/r/156006/#review162746

::: toolkit/modules/sessionstore/FormData.jsm:134
(Diff revision 1)
>    get restorableFormNodesXPath() {
>      // for a comprehensive list of all available <INPUT> types see
>      // https://dxr.mozilla.org/mozilla-central/search?q=kInputTypeTable&redirect=false
>      let ignoreInputs = new Map([
> -      ["type", ["password", "hidden", "button", "image", "submit", "reset"]],
>        ["autocomplete", ["off"]]

I don't know how to write a function that is simple but still get the job done. So I kept the 'autocomplete=off' to be filtered with using XPath.

Thanks for your suggestion :).

I changed the Map with array since `for (let ... of ...) {}` is expensive in the loop with many elements (and it does show in the profiler). Array seems to be less expensive.

Comment 6

[Bao Quan [:beekill]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

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

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

https://reviewboard.mozilla.org/r/156006/#review162828

r=me with the comment addressed. Did you push this patch to try?

::: toolkit/modules/sessionstore/FormData.jsm:86
(Diff revision 3)
> +const IGNORE_ATTRIBUTES = [
> +  ["type", new Set(["password", "hidden", "button", "image", "submit", "reset"])],
> +  ["autocomplete", new Set(["off"])]
> +];
> +function shouldIgnoreNode(node) {
> +  for (let i = 0; i < IGNORE_ATTRIBUTES.length; ++i) {

Please use a `for...of` loop here. I'm really surprised to hear that using Maps are way less performant than an Array here, so please file a bug in the 'Core :: Javascript Engine' component so that the JS team can take a look!

Comment 8

[Bao Quan [:beekill]]

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> Please use a `for...of` loop here. I'm really surprised to hear that using
> Maps are way less performant than an Array here, so please file a bug in the
> 'Core :: Javascript Engine' component so that the JS team can take a look!

I want to share you some profiles I did with different ways of looping:
+ `for ... of ...`: https://perfht.ml/2vh4E1u . The shouldIgnoreMethod takes ~440 ms.
+ `for ... in ...`: https://perfht.ml/2vgbnZF . The shouldIgnoreMethod takes ~268 ms.
+ (*): https://perfht.ml/2vgM9tQ . The shouldIgnoreMethod takes ~87 ms.

(*) is something similar to current patch:
```js
let i = IGNORE_ATTRIBUTES.length;
while (i--) {
  let [attrName, attrValues] = IGNORE_ATTRIBUTES[i];
  ...
}
```

The reason I said Map is slower than Array is because of the profiles like above, I guess it's because of different ways of looping?

So based on the profiles, we should use (*)? However, I don't think it is the case because the time of the UI janks are the same, ~1.1 to ~1.2 seconds.

Why is the result profiles like that? I only change the looping method and they show different results. Is this normal in profiler?

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Bao Quan [:beekill] from comment #8)
> Why is the result profiles like that? I only change the looping method and
> they show different results. Is this normal in profiler?

No, not that I know of! If you can see that _not_ using for...of is noticeably faster, than please keep the patch as it is right now.

Mike, when you read command 8, does it ring a bell? Could it be that `for...of` is in fact (much) slower than other iteration methods?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Redirecting to jandem who definitely knows way more than I do about the fine details of SpiderMonkey.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Mike de Boer [:mikedeboer] from comment #9)
> Could it be that `for...of` is in fact (much) slower than other iteration methods?

Yeah for-of is slower than for-in or a C-style loop. This is mostly because ES6 added this heavy-weight iteration protocol: for-of uses iterators that have to allocate/return a {done, value} object each time iterator.next() is called.

The JITs can optimize this sometimes but it's not trivial. Please file bugs if this is actually blocking you, ideally with a standalone .htm or .js testcase.

Comment 12

[Bao Quan [:beekill]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

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

Comment 13

[Bao Quan [:beekill]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

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

Comment 14

[Bao Quan [:beekill]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

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

Comment 15

[Bao Quan [:beekill]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156006/diff/6-7/

Comment 16

[Bao Quan [:beekill]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/156006/diff/7-8/

Comment 17

[Bao Quan [:beekill]]

The try result looks good. Dão, can you land this? Thanks!!

Comment 18

[Dão Gottwald [:dao]]

(In reply to Bao Quan [:beekill] from comment #17)
> The try result looks good. Dão, can you land this? Thanks!!

Please close this issue first: https://reviewboard.mozilla.org/r/156006/#issue-summary

I can't do this for you because Reviewboard is stupid.

Comment 19

[Bao Quan [:beekill]]

Comment on attachment 8885143 [details]
Bug 1379900 - Doing elements filtering in JS to simplify XPath query in FormData::collect.

https://reviewboard.mozilla.org/r/156006/#review167612

Comment 20

[Bao Quan [:beekill]]

I just found out I can land the commit myself :).

Comment 21

[Bao Quan [:beekill]]

(In reply to Bao Quan [:beekill] from comment #20)
> I just found out I can land the commit myself :).

Oops! Something is wrong with ReviewBoard that made me think I can land commit :P.

Dão, can you land this for me? Thanks :).

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0a41228220b
Doing elements filtering in JS to simplify XPath query in FormData::collect. r=mikedeboer

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a0a41228220b