1392975 - [Form Autofill] Improve the performance before calling identifyAutofillFields

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P3)

Description

[Luke Chang [:lchang] (inactive)]

In this bug, I'd like to improve the following parts:

1. Duplicate "isFieldEligibleForAutofill" in FormAutofillFrameScript.js so we can get rid of cross-JSM calls of "FormAutofillUtils.jsm".

2. Avoid registering multiple listeners of DOMContentLoaded.

3. Cache the pref value.

Comment 1

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1392975 - [Form Autofill] Avoid registering multiple listeners of DOMContentLoaded.

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

Comment 2

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8900180 [details]
Bug 1392975 - [Form Autofill] Avoid registering multiple listeners of DOMContentLoaded.

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

Comment 3

[Sean Lee [:seanlee][:weilonge]]

(In reply to Luke Chang [:lchang] from comment #0)
> In this bug, I'd like to improve the following parts:
> 
> 1. Duplicate "isFieldEligibleForAutofill" in FormAutofillFrameScript.js so
> we can get rid of cross-JSM calls of "FormAutofillUtils.jsm".
May I know how much performance is improved by this way?
That's a trade-off between code duplication and performance improvement.

> 
> 2. Avoid registering multiple listeners of DOMContentLoaded.
> 
> 3. Cache the pref value.
I am fine with the part 2 and 3.

Comment 4

[Luke Chang [:lchang] (inactive)]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #3)
> May I know how much performance is improved by this way?
> That's a trade-off between code duplication and performance improvement.

Runs "handleEvent" 100,000 times:

Applies part 2 & 3
219ms
236ms
213ms
221ms
213ms

Applies part 1, 2 & 3
182ms
193ms
188ms
189ms
186ms

It reduces only 0.0003ms per each cross-JSM call.

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8900180 [details]
Bug 1392975 - [Form Autofill] Avoid registering multiple listeners of DOMContentLoaded.

https://reviewboard.mozilla.org/r/171566/#review176758

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:134
(Diff revision 2)
> +Services.prefs.addObserver(PREF_ADDRESSES_ENABLED, () => {
> +  delete FormAutofillFrameScript.__prefEnabled;
> +});

If you're going to add an observer then defineLazyPreferenceGetter is less lines than the observer+getter

Comment 6

[Luke Chang [:lchang] (inactive)]

In my benchmark, "defineLazyPreferenceGetter" actually causes additional costs so I intended avoiding that.

Comment 7

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8900180 [details]
Bug 1392975 - [Form Autofill] Avoid registering multiple listeners of DOMContentLoaded.

https://reviewboard.mozilla.org/r/171566/#review176788

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:305
(Diff revision 2)
> +   * @param   {HTMLElement} element
> +   *          The element to be determined.
> +   * @returns {boolean}
> +   *          Whether the element is eligible.
> +   */
> +  _isFieldEligibleForAutofill(element) {

Moving `isFieldEligibleForAutofill` to `FormAutofillHeuristics.jsm` is quite worth to do since it does uses a lot in `getFormInfo`(`getInfo` actually).

If `isFieldEligibleForAutofill` is a public function, `FormAutofillFrameScript.js` can use it as well. This only costs one extra cross-JSM function call.

My suggestion is to make `isFieldEligibleForAutofill` public, so we don't have to duplicate `isFieldEligibleForAutofill` in two files.

Comment 8

[Luke Chang [:lchang] (inactive)]

(In reply to Sean Lee [:seanlee][:weilonge] from comment #7)
> My suggestion is to make `isFieldEligibleForAutofill` public, so we don't
> have to duplicate `isFieldEligibleForAutofill` in two files.

It'll cause "FormAutofillHeuristics.jsm" to be loaded unnecessarily when the focused element isn't eligible. I don't think it's worth.

Comment 9

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8900180 [details]
Bug 1392975 - [Form Autofill] Avoid registering multiple listeners of DOMContentLoaded.

https://reviewboard.mozilla.org/r/171566/#review177810

r=me if we think the performance improvement is worth to duplicate `isFieldEligibleForAutofill` function.

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:18
(Diff revision 2)
>  Cu.import("resource://gre/modules/Services.jsm");
> -Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://formautofill/FormAutofillContent.jsm");
> -Cu.import("resource://formautofill/FormAutofillUtils.jsm");
>  
> +const PREF_ADDRESSES_ENABLED = "extensions.formautofill.addresses.enabled";

Add `const PREF_CREDIT_CARDS_ENABLED = "extensions.formautofill.creditCards.enabled";` for credit card pref.

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:30
(Diff revision 2)
>   *
>   * NOTE: Declares it by "var" to make it accessible in unit tests.
>   */
>  var FormAutofillFrameScript = {
> +  _nextHandleElement: null,
> +  _alreadyDCL: false,

Is DLC a common abbreviation for `DOMContentLoaded`?
If not, add a comment for it:
`// DLC: DOMContentLoaded`

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:32
(Diff revision 2)
>   */
>  var FormAutofillFrameScript = {
> +  _nextHandleElement: null,
> +  _alreadyDCL: false,
> +  _hasDCLhandler: false,
> +  _hasPendingTask: false,

nit: Move these variables right before the functions use them if the function is the only place to use them.

e.g. Move `_hasPendingTask` right before `_doIdentifyAutofillFields`.

::: browser/extensions/formautofill/content/FormAutofillFrameScript.js:36
(Diff revision 2)
> +  _hasDCLhandler: false,
> +  _hasPendingTask: false,
> +
> +  get _prefEnabled() {
> +    if (this.__prefEnabled === undefined) {
> +      this.__prefEnabled = Services.prefs.getBoolPref(PREF_ADDRESSES_ENABLED);

Do you think we shoudld consider the credit card case in this bug?

If so...
```JS
this.__prefEnabled = Services.prefs.getBoolPref(PREF_ADDRESSES_ENABLED) ||
  Services.prefs.getBoolPref(PREF_CREDIT_CARDS_ENABLED);
```

Comment 10

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8900180 [details]
Bug 1392975 - [Form Autofill] Avoid registering multiple listeners of DOMContentLoaded.

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

Comment 11

[Luke Chang [:lchang] (inactive)]

According to the benchmark in comment 4, I don't really think the impact of duplicating code is significant so I reverted part 1. Also, bug 1390757 has already addressed part 3, which cached the pref in FormAutofillUtils. Thus, this bug will mainly aim to address part 2 and some small refactoring. Thanks for the review.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02ca408ae2ca
[Form Autofill] Avoid registering multiple listeners of DOMContentLoaded. r=seanlee

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/02ca408ae2ca

Comment 14

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8900180 [details]
Bug 1392975 - [Form Autofill] Avoid registering multiple listeners of DOMContentLoaded.

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

[User impact if declined]:
Not user-facing but it can improve performance in certain situation.

[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.

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

[Is the change risky?]:
No.

[Why is the change risky/not risky?]:
It affects Form Autofill system add-on only and only changed the loading process.

[String changes made/needed]:
N/A

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8900180 [details]
Bug 1392975 - [Form Autofill] Avoid registering multiple listeners of DOMContentLoaded.

Performance improvement. Beta56+.

Comment 16

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

I'm hitting conflicts in browser/extensions/formautofill/content/FormAutofillFrameScript.js trying to uplift this. Can someone take a look at this and see what needs changed?

Comment 17

[Luke Chang [:lchang] (inactive)]

Attachment: for_beta_uplift.patch

I've resolved the conflicts. Thanks.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/12809a8ce21a

Comment 19

[Luke Chang [:lchang] (inactive)]

Thanks.