Closed Bug 1379900 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Session Restore, defect)

54 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: beekill, Assigned: beekill)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170616092053
Blocks: 536910
Component: Untriaged → XSLT
Product: Firefox → Core
Profiling result:
+ Before the patch: https://perfht.ml/2uLJyYO
+ After the patch: https://perfht.ml/2ucGwQe
Component: XSLT → Session Restore
Product: Core → Firefox
Assignee: nobody → nnn_bikiu0707
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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?
Attachment #8885143 - Flags: review?(mdeboer) → review-
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 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!
Attachment #8885143 - Flags: review?(mdeboer) → review+
(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?
Flags: needinfo?(mdeboer)
(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?
Flags: needinfo?(mdeboer) → needinfo?(mconley)
Redirecting to jandem who definitely knows way more than I do about the fine details of SpiderMonkey.
Flags: needinfo?(mconley) → needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
The try result looks good. Dão, can you land this? Thanks!!
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
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
I just found out I can land the commit myself :).
(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 :).
Flags: needinfo?(dao+bmo)
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0a41228220b
Doing elements filtering in JS to simplify XPath query in FormData::collect. r=mikedeboer
Flags: needinfo?(dao+bmo)
https://hg.mozilla.org/mozilla-central/rev/a0a41228220b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.