All users were logged out of Bugzilla on October 13th, 2018

[Form Autofill] Optimize "findLabelElements" function

RESOLVED FIXED in Firefox 56

Status

()

P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: lchang, Assigned: lchang)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [form autofill:MVP])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
We currently use a JS-based "findLabelElements" function to find the corresponding label elements from an input element. We was planning to switch to an identical native API ".labels" after its implementation finished so "findLabelElements" function was treated as a temporary solution and wasn't optimized.

However, we noticed that ".labels" has a performance concern when we tried to switch to it (see bug 1374724) and we might not fix it on schedule. Therefore, we should optimize "findLabelElements" function for production in case we need to ship with it.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 3

a year ago
My experiments on calling "collectFormFields" 1000 times in each run:

Baseline
2310ms
2336ms
2353ms
2381ms
2282ms

Patches applied
1677ms
1689ms
1633ms
1625ms
1664ms
Comment on attachment 8894842 [details]
Bug 1387988 - [Form Autofill] Move "findLabelElements" function to FormAutofillHeuristics.jsm.

https://reviewboard.mozilla.org/r/166004/#review174214
Attachment #8894842 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8894842 [details]
Bug 1387988 - [Form Autofill] Move "findLabelElements" function to FormAutofillHeuristics.jsm.

https://reviewboard.mozilla.org/r/166004/#review174224

::: browser/extensions/formautofill/test/unit/test_extractLabelStrings.js:1
(Diff revision 1)
> +/* global LabelUtils */

Instead of using `global`, update https://dxr.mozilla.org/mozilla-central/source/tools/lint/eslint/modules.json
Comment on attachment 8894843 [details]
Bug 1387988 - [Form Autofill] Optimize "findLabelElements" function.

https://reviewboard.mozilla.org/r/166006/#review174220

Clearing review until there are more comments

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:187
(Diff revision 1)
> +  _mappedLabels: null,
> +  _unmappedLabels: null,

This would be easier to review and easier for future readers to understand if it was documented what the key and value are in these maps and what they are for.

::: browser/extensions/formautofill/test/unit/test_getInfo.js:1
(Diff revision 1)
> +/* global LabelUtils */

Remove this after the part 1 fix.
Attachment #8894843 - Flags: review?(MattN+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
Thanks. They are fixed.
Comment on attachment 8894843 [details]
Bug 1387988 - [Form Autofill] Optimize "findLabelElements" function.

https://reviewboard.mozilla.org/r/166006/#review174578

Thanks!

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:187
(Diff revision 2)
> +  // A map object, whose keys are the id's of input elements and each value is
> +  // an array consisting of label elements correponding to the id.
> +  _mappedLabels: null,
> +
> +  // An array consisting of label elements whose correponding input element
> +  // doesn't have an id attribute.
> +  _unmappedLabels: null,

These should use JSDoc syntax with @type

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:187
(Diff revision 2)
>    // The tag name list is from Chromium except for "STYLE":
>    // eslint-disable-next-line max-len
>    // https://cs.chromium.org/chromium/src/components/autofill/content/renderer/form_autofill_util.cc?l=216&rcl=d33a171b7c308a64dc3372fac3da2179c63b419e
>    EXCLUDED_TAGS: ["SCRIPT", "NOSCRIPT", "OPTION", "STYLE"],
> +
> +  // A map object, whose keys are the id's of input elements and each value is

s/input elements/form fields/ ?

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:247
(Diff revision 2)
> +        if (!labels) {
> +          mappedLabels.set(id, [label]);
> +        } else {
> -        labels.push(label);
> +          labels.push(label);
> -      }
> +        }

Nit: Invert the condition to avoid the unnecessary negative to make it easier to comprehend:
```js
if (labels) {
  labels.push(label);
} else {
  mappedLabels.set(id, [label]);
}
```
Attachment #8894843 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

a year ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea2000b5721e
[Form Autofill] Move "findLabelElements" function to FormAutofillHeuristics.jsm. r=MattN
https://hg.mozilla.org/integration/autoland/rev/8290e527cf40
[Form Autofill] Optimize "findLabelElements" function. r=MattN

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea2000b5721e
https://hg.mozilla.org/mozilla-central/rev/8290e527cf40
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment hidden (mozreview-request)
Attachment #8898656 - Attachment is obsolete: true
Attachment #8898656 - Flags: review?(MattN+bmo)
https://hg.mozilla.org/releases/mozilla-beta/rev/fd79fbc2cce8
status-firefox56: --- → fixed
Flags: in-testsuite+
(Assignee)

Comment 17

a year ago
Comment on attachment 8894843 [details]
Bug 1387988 - [Form Autofill] Optimize "findLabelElements" function.

Approval Request Comment
[Feature/Bug causing the regression]:
Performance improvement.

[User impact if declined]:
The performance will decrease if there are lots of labels in the same page.

[Is this code covered by automated tests?]:
Yes.

[Has the fix been verified in Nightly?]:
Yes.

[Needs manual test from QE? If yes, steps to reproduce]: 
No. It's about performance.

[List of other uplifts needed for the feature/fix]:
N/A

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It should affect Form Autofill system add-on only.

[String changes made/needed]:
N/A
Attachment #8894843 - Flags: approval-mozilla-beta?
Comment on attachment 8894843 [details]
Bug 1387988 - [Form Autofill] Optimize "findLabelElements" function.

Performance improvement. Beta56+.
Attachment #8894843 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.