Closed Bug 1392888 Opened 5 years ago Closed 5 years ago
Fill uses the super inefficient get Elements By Tag Name() API
59 bytes, text/x-review-board-request
This API is called from here: <https://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/browser/extensions/formautofill/FormAutofillHeuristics.jsm#242> This function returns a *live* node list, which registers a mutation observer which slows down all of the DOM operations happening inside the document from that point on. This can slow down things like the DOM manipulations in Speedometer. This code needs to stop calling this function and use an alternative implementation strategy if possible.
Matt, do you mind helping to find an owner for this please (assuming that form autofill is still targeted to ship in 57)? Thanks!
This was actually added to try to improve speedometer perf in bug 1387988. I guess switching back to querySelectorAll will solve the problem since it returns a non-live NodeList. Luke can probably look at this since he is looking at other deps of bug 1375568 at the moment.
I didn't know that "getElementsByTagName" is that slow. I'll make a patch using "querySelectorAll" instead. Thanks.
Comment on attachment 8900550 [details] Bug 1392888 - [Form Autofill] Avoid using getElementsByTagName. https://reviewboard.mozilla.org/r/171944/#review177614
Attachment #8900550 - Flags: review+
Thanks for jumping so quickly on this! :-) I'm not super familiar with our eslint facilities these days, but if it's possible please file a follow-up bug in the right component to get a linting rule to catch people when they use functions returning live node lists, it's hard to catch these things under a profiler and it's easy for the cost that they add to creep in without us noticing. I'm not sure if it's feasible to ban these things and what the right component for the bug would be... Thanks again!
Thank Matt for the review and thank Ehsan for pointing it out. I've filed bug 1393664 for following up an eslint rule.
I should have tested it before patching it. However, I noticed that "querySelectorAll" might be actually slower than "getElementsByTagName". I tried to call `getElementsByTagName("div")` and `querySelectorAll("div")` 100,000 times in a random complex page with 324 DIVs and they take 58ms and 1,210ms respectively. I'm afraid my patch will make it worse. Matt and Ehsan, do you have any idea? The tree is closed so the patch hasn't landed, but unfortunately I don't know how to abort the autoland.
BTW, maybe DOM operations won't be affected a lot in our case because the live node list we used will be freed immediately within a sync function, where no DOM operation is supposed to happen.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/0865d305aa6b [Form Autofill] Avoid using getElementsByTagName. r=MattN
Aryx, Could you help me backout this patch? It seems not to fix the issue. Thanks a lot.
Calling this [qf:p2] - high priority because speedometer, but also unclear if/how to fix at this point (do we need a new/faster DOM api or something? or can/should we optimize one of these APIs?), so probably too risky/unclear to be p1 given that 57 is happening now.
Whiteboard: [qf] [form autofill] → [qf:p2] [form autofill]
Olli, can you think of a good API to use here from JS?
Flags: needinfo?(ehsan) → needinfo?(bugs)
I don't. getElementsByTagName access is highly optimized. We cache results and when one calls the method again, then next call is very fast, so a test using a loop ends up measuring something quite different what we're interested in here. The test should be more like lots of different DOM subtrees, and then call getElementsByTagName or querySelectorAll on the roots of the subtrees, And do that call only once per subtree root. But I still expect that getElementsByTagName could be quite a bit faster. I guess what we would need here is getElementsByTagName which returns non-live list. How often is the list of elements needed? If only rarely, I would probably use querySelectorAll.
Depend on the amount of forms, "getElementsByTagName" might be called frequently. However, we call it on the same root element every time no matter which form we are handling. The cached results of "getElementsByTagName" might benefit us in most cases.
The issue is the liveness. Until the ::Release has been called last time on the nodelist/htmlcollection, it observers all the DOM mutations under the root node, and ::Release won't be called the last time before GC runs.
I see. It sounds worth switching to "querySelectorAll" in our case. If so, I'll proceed with my patch. Thanks.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/14960c3fbf80 [Form Autofill] Avoid using getElementsByTagName. r=MattN
Comment on attachment 8900550 [details] Bug 1392888 - [Form Autofill] Avoid using getElementsByTagName. Approval Request Comment [Feature/Bug causing the regression]: Performance improvement. [User impact if declined]: DOM operations might be slow. [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?]: only API changed. [String changes made/needed]: N/A
Attachment #8900550 - Flags: approval-mozilla-beta?
Comment on attachment 8900550 [details] Bug 1392888 - [Form Autofill] Avoid using getElementsByTagName. Performance improvement. Beta56+.
Attachment #8900550 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Looks like this depends on the second patch from bug 1387988, which never got uplifted to Beta.
You're right. It must have been forgotten. Will request to uplift it first. Thanks.
Performance: --- → P2
Whiteboard: [qf:p2] [form autofill] → [form autofill]
You need to log in before you can comment on or make changes to this bug.