1392888 - Form AutoFill uses the super inefficient getElementsByTagName() API

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P1)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

Matt, do you mind helping to find an owner for this please (assuming that form autofill is still targeted to ship in 57)?  Thanks!

Comment 2

[Matthew N. [:MattN]]

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.

Comment 3

[Luke Chang [:lchang] (inactive)]

I didn't know that "getElementsByTagName" is that slow. I'll make a patch using "querySelectorAll" instead. Thanks.

Comment 4

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1392888 - [Form Autofill] Avoid using getElementsByTagName.

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

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8900550 [details]
Bug 1392888 - [Form Autofill] Avoid using getElementsByTagName.

https://reviewboard.mozilla.org/r/171944/#review177614

Comment 6

[(no longer active)]

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 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8900550 [details]
Bug 1392888 - [Form Autofill] Avoid using getElementsByTagName.

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

Comment 8

[Luke Chang [:lchang] (inactive)]

Thank Matt for the review and thank Ehsan for pointing it out. I've filed bug 1393664 for following up an eslint rule.

Comment 9

[Luke Chang [:lchang] (inactive)]

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.

Comment 10

[Luke Chang [:lchang] (inactive)]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0865d305aa6b
[Form Autofill] Avoid using getElementsByTagName. r=MattN

Comment 12

[Luke Chang [:lchang] (inactive)]

Aryx, Could you help me backout this patch? It seems not to fix the issue. Thanks a lot.

Comment 13

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

Done: https://hg.mozilla.org/integration/autoland/rev/556d5b15e3567cc72bc5c6ce2eeac3b335870db2

Comment 14

[Daniel Holbert [:dholbert]]

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.

Comment 15

[(no longer active)]

Olli, can you think of a good API to use here from JS?

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 17

[Luke Chang [:lchang] (inactive)]

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.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 19

[Luke Chang [:lchang] (inactive)]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14960c3fbf80
[Form Autofill] Avoid using getElementsByTagName. r=MattN

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/14960c3fbf80

Comment 22

[Luke Chang [:lchang] (inactive)]

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

Comment 23

[Gerry Chang [:gchang]]

Comment on attachment 8900550 [details]
Bug 1392888 - [Form Autofill] Avoid using getElementsByTagName.

Performance improvement. Beta56+.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Looks like this depends on the second patch from bug 1387988, which never got uplifted to Beta.

Comment 25

[Luke Chang [:lchang] (inactive)]

You're right. It must have been forgotten. Will request to uplift it first. Thanks.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d1940f385423