Form AutoFill uses the super inefficient getElementsByTagName() API

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: lchang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 unaffected, firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [qf:p2] [form autofill])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Matt, do you mind helping to find an owner for this please (assuming that form autofill is still targeted to ship in 57)?  Thanks!
Flags: needinfo?(MattN+bmo)
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.
Assignee: nobody → lchang
Blocks: 1387988, 1375568
Status: NEW → ASSIGNED
Flags: needinfo?(MattN+bmo)
Priority: -- → P1
Whiteboard: [qf] → [qf] [form autofill]
(Assignee)

Comment 3

2 years ago
I didn't know that "getElementsByTagName" is that slow. I'll make a patch using "querySelectorAll" instead. Thanks.
Comment hidden (mozreview-request)
Comment on attachment 8900550 [details]
Bug 1392888 - [Form Autofill] Avoid using getElementsByTagName.

https://reviewboard.mozilla.org/r/171944/#review177614
Attachment #8900550 - Flags: review+
(Reporter)

Comment 6

2 years ago
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!
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
Thank Matt for the review and thank Ehsan for pointing it out. I've filed bug 1393664 for following up an eslint rule.
(Assignee)

Updated

2 years ago
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox57: --- → affected
(Assignee)

Comment 9

2 years ago
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.
Flags: needinfo?(ehsan)
Flags: needinfo?(MattN+bmo)
(Assignee)

Comment 10

2 years ago
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.

Comment 11

2 years ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0865d305aa6b
[Form Autofill] Avoid using getElementsByTagName. r=MattN
(Assignee)

Comment 12

2 years ago
Aryx, Could you help me backout this patch? It seems not to fix the issue. Thanks a lot.
Flags: needinfo?(aryx.bugmail)
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]
(Reporter)

Comment 15

2 years ago
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.
Flags: needinfo?(bugs)
(Assignee)

Comment 17

2 years ago
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.
(Assignee)

Comment 19

2 years ago
I see. It sounds worth switching to "querySelectorAll" in our case. If so, I'll proceed with my patch. Thanks.

Comment 20

2 years ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/14960c3fbf80
[Form Autofill] Avoid using getElementsByTagName. r=MattN
https://hg.mozilla.org/mozilla-central/rev/14960c3fbf80
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Comment 22

2 years ago
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?
(Assignee)

Updated

2 years ago
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(lchang)
(Assignee)

Comment 25

2 years ago
You're right. It must have been forgotten. Will request to uplift it first. Thanks.
Flags: needinfo?(lchang)

Comment 26

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d1940f385423
status-firefox56: affected → fixed
You need to log in before you can comment on or make changes to this bug.