Closed Bug 1392975 Opened 7 years ago Closed 7 years ago

[Form Autofill] Improve the performance before calling identifyAutofillFields

Categories

(Toolkit :: Form Manager, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

Details

(Whiteboard: [form autofill:MVP])

Attachments

(2 files)

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.
(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.
(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 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
In my benchmark, "defineLazyPreferenceGetter" actually causes additional costs so I intended avoiding that.
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.
(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 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);
```
Attachment #8900180 - Flags: review?(selee) → review+
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.
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/02ca408ae2ca
[Form Autofill] Avoid registering multiple listeners of DOMContentLoaded. r=seanlee
https://hg.mozilla.org/mozilla-central/rev/02ca408ae2ca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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
Attachment #8900180 - Flags: approval-mozilla-beta?
Comment on attachment 8900180 [details]
Bug 1392975 - [Form Autofill] Avoid registering multiple listeners of DOMContentLoaded.

Performance improvement. Beta56+.
Attachment #8900180 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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?
Flags: needinfo?(lchang)
I've resolved the conflicts. Thanks.
Flags: needinfo?(lchang) → needinfo?(wkocher)
Thanks.
Flags: needinfo?(wkocher)
You need to log in before you can comment on or make changes to this bug.