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)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: lchang, Assigned: lchang)
References
Details
(Whiteboard: [form autofill:MVP])
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
selee
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
7.56 KB,
patch
|
Details | Diff | Splinter Review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
(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.
Assignee | ||
Comment 4•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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
Assignee | ||
Comment 6•7 years ago
|
||
In my benchmark, "defineLazyPreferenceGetter" actually causes additional costs so I intended avoiding that.
Comment 7•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 8•7 years ago
|
||
(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•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
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•7 years ago
|
||
Pushed by lchang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/02ca408ae2ca [Form Autofill] Avoid registering multiple listeners of DOMContentLoaded. r=seanlee
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/02ca408ae2ca
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Assignee | ||
Updated•7 years ago
|
status-firefox56:
--- → affected
Assignee | ||
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
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)
Assignee | ||
Comment 17•7 years ago
|
||
I've resolved the conflicts. Thanks.
Flags: needinfo?(lchang) → needinfo?(wkocher)
Comment 18•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/12809a8ce21a
You need to log in
before you can comment on or make changes to this bug.
Description
•