1375568 - [meta] Form AutoFill overhead in Speedometer

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[(no longer active)]

Please see this profile which is captured from a partial run of Speedometer (from about the first 100 or so of the tests): http://bit.ly/2sZXHn3

It seems the majority of the overhead is coming form this timer: <https://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/extensions/formautofill/content/FormAutofillFrameScript.js#52>.  This is probably a good candidate to be moved to idle dispatch + timeout.

Also, this event handler could probably be more efficient: <https://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/extensions/formautofill/content/FormAutofillFrameScript.js#33>  Firstly: the preference access there should probably use a cached variable instead.  Also, IINM this <https://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/extensions/formautofill/content/FormAutofillFrameScript.js#55> could end up installing many event listeners potentially?  The other issue is that isFieldEligibleForAutofill() uses Xray wrappers which are known to be slow, it would be nice to rewrite this whole event handler in C++ to make it not go through Xray wrapper machinery at all.  (But please note that from this profile, it seems that the handleEvent part of the cost is the small part of this.)

If Form AutoFill is expected to be shipped on or before 57, this should probably be addressed with a [qf:p1] priority.  Matt, do you know about the expected shipping schedule?

Comment 1

[Matthew N. [:MattN]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #0)

Thanks, we'll look at this.

> If Form AutoFill is expected to be shipped on or before 57, this should
> probably be addressed with a [qf:p1] priority.  Matt, do you know about the
> expected shipping schedule?

It's expected to ship in 56 as a system add-on.

Comment 2

[(no longer active)]

Thanks, [qf:p1] it is then.  :-)

Comment 3

[Matthew N. [:MattN]]

Now that bug 1373195 landed, maybe this is reduced?

Is there a good way to measure the impact? Do I just compare a score before and after and also look for autofill in profiles after? What sampling interval do you recommend?

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #0)
> Please see this profile which is captured from a partial run of Speedometer
> (from about the first 100 or so of the tests): http://bit.ly/2sZXHn3
> 
> It seems the majority of the overhead is coming form this timer:
> <https://searchfox.org/mozilla-central/rev/
> ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/extensions/formautofill/
> content/FormAutofillFrameScript.js#52>.  This is probably a good candidate
> to be moved to idle dispatch + timeout.

I'm not sure this will work since we need this be done before the user opens the autocomplete popup.

> Also, this event handler could probably be more efficient:
> <https://searchfox.org/mozilla-central/rev/
> ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/extensions/formautofill/
> content/FormAutofillFrameScript.js#33>  Firstly: the preference access there
> should probably use a cached variable instead.

I agree. We should use defineLazyPreferenceGetter in FormAutofillUtils.jsm to share a cached pref value.

>  Also, IINM this
> <https://searchfox.org/mozilla-central/rev/
> ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/extensions/formautofill/
> content/FormAutofillFrameScript.js#55> could end up installing many event
> listeners potentially?

I also seem to recall that. IIRC adding it to the docShell instead would avoid that, right?

>  The other issue is that isFieldEligibleForAutofill()
> uses Xray wrappers which are known to be slow, it would be nice to rewrite
> this whole event handler in C++ to make it not go through Xray wrapper
> machinery at all.  (But please note that from this profile, it seems that
> the handleEvent part of the cost is the small part of this.)

If it's small it's not clear whether you think it's worth it. The problem with moving this to C++ is that formautofill is a system add-on. It wouldn't be great to have to bake C++ parts into the browser.

Comment 4

[(no longer active)]

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #3)
> Now that bug 1373195 landed, maybe this is reduced?

I doubt it, since that bug was only showing up initialy in the beginning of the Speedometer run...  See bug 1374014.

> Is there a good way to measure the impact? Do I just compare a score before
> and after and also look for autofill in profiles after? What sampling
> interval do you recommend?

See bug 1376594 comment 6 for my recommended way of profiling this.  If this bug gets fixed, when you profile Speedometer like that (with a sampling frequency of 0.1ms) and search the profile for FormAutofillFrameScript.js, there should ideally be no results.  I doubt that the impact of this bug alone will be easily measurable in the total score due to the noise.

> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #0)
> > Please see this profile which is captured from a partial run of Speedometer
> > (from about the first 100 or so of the tests): http://bit.ly/2sZXHn3
> > 
> > It seems the majority of the overhead is coming form this timer:
> > <https://searchfox.org/mozilla-central/rev/
> > ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/extensions/formautofill/
> > content/FormAutofillFrameScript.js#52>.  This is probably a good candidate
> > to be moved to idle dispatch + timeout.
> 
> I'm not sure this will work since we need this be done before the user opens
> the autocomplete popup.

We can consider using a short timeout perhaps, like 100ms.  I suggested using a timeout because the user needs to see the result of this.

> > Also, this event handler could probably be more efficient:
> > <https://searchfox.org/mozilla-central/rev/
> > ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/extensions/formautofill/
> > content/FormAutofillFrameScript.js#33>  Firstly: the preference access there
> > should probably use a cached variable instead.
> 
> I agree. We should use defineLazyPreferenceGetter in FormAutofillUtils.jsm
> to share a cached pref value.

Is that the most efficient thing we have in JS?  It seems valuable for to have a proper API for exposing pref var caches to JS.  Probably not worth it here though.

> >  Also, IINM this
> > <https://searchfox.org/mozilla-central/rev/
> > ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/extensions/formautofill/
> > content/FormAutofillFrameScript.js#55> could end up installing many event
> > listeners potentially?
> 
> I also seem to recall that. IIRC adding it to the docShell instead would
> avoid that, right?

Hmm, I didn't know that.  Perhaps Olli knows if we have some kind of specific optimization for it?  I don't see why docshell would be different.

> >  The other issue is that isFieldEligibleForAutofill()
> > uses Xray wrappers which are known to be slow, it would be nice to rewrite
> > this whole event handler in C++ to make it not go through Xray wrapper
> > machinery at all.  (But please note that from this profile, it seems that
> > the handleEvent part of the cost is the small part of this.)
> 
> If it's small it's not clear whether you think it's worth it.

It is small on the HTML page for Speedometer (TodoMVC) which isn't an interesting page at all.  It will probably be much worse for pages with many input elements.  We have had to do specific optimizations in the past for such pages (such as lazily initializing input elements) so we know they exist, and IIRC you mostly find them in intranet applications etc typically on the release channel and rarely on nightly/beta population a lot.  We also have many years of bad experience with similar code in sessionrestore touching content objects from chrome JS suffering from similar perf issues which we have to now port to C++.  See for example bug 1362330.  The fundamental problem is that part of the performance aspect (the number of times this code is called/iterated) is controlled by the page, so there is no way for us to ensure a reasonable performance with an implementation in chrome JS unless it systematically ensures that accesses going through Xray wrappers on hot code paths are minimized and will be kept as such in the future.  :-(

> The problem
> with moving this to C++ is that formautofill is a system add-on. It wouldn't
> be great to have to bake C++ parts into the browser.

It seems to me that we should prioritize the quality of the implementation over the delivery vehicle if those two come at odds, but hopefully system add-ons have thought of a way to deal with code that requires C++ components?  (I honestly don't know, since none of the system add-ons planning was done on firefox-dev or dev-platform so I still don't have any knowledge of its details.)  It seems like we'd need to figure out a good boundary point between Gecko and the system add-on code and land the Gecko bits sooner and allow the add-on control the behavior of the Gecko bits where you would like to be able to change things without shipping binary updates?  Even if there is no generic solution, perhaps we can achieve a custom one.  For example Gecko could listen to events, so some filtering and then raise a UI invocation event that the UI code would be listening on.

Comment 5

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

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #4)
> > >  Also, IINM this
> > > <https://searchfox.org/mozilla-central/rev/
> > > ae94cfb36d804727dfb38f2750bfcaab4621df6e/browser/extensions/formautofill/
> > > content/FormAutofillFrameScript.js#55> could end up installing many event
> > > listeners potentially?
> > 
> > I also seem to recall that. IIRC adding it to the docShell instead would
> > avoid that, right?
> 
> Hmm, I didn't know that.  Perhaps Olli knows if we have some kind of
> specific optimization for it?  I don't see why docshell would be different.
> 
I have no idea what this means. DocShell isn't an EventTarget so talking about event listeners and docshells doesn't make sense.

Comment 6

[Luke Chang [:lchang] (inactive)]

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #3)
> I agree. We should use defineLazyPreferenceGetter in FormAutofillUtils.jsm
> to share a cached pref value.

Just did some tests and found that using "defineLazyPreferenceGetter" in "FormAutofillUtils" actually increases the overhead. I haven't figured out what causes this. I even removed the pref check in "handleEvent" but didn't get notable improvements.

BTW, not surprisingly, the profiling result dramatically improved if I put an early-return in "identifyAutofillFields". Maybe "handleEvent" itself is not a bottleneck?

Comment 7

[(no longer active)]

(In reply to Luke Chang [:lchang] from comment #6)
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #3)
> > I agree. We should use defineLazyPreferenceGetter in FormAutofillUtils.jsm
> > to share a cached pref value.

Thanks for looking into this!

> Just did some tests and found that using "defineLazyPreferenceGetter" in
> "FormAutofillUtils" actually increases the overhead. I haven't figured out
> what causes this. I even removed the pref check in "handleEvent" but didn't
> get notable improvements.

I suspect before addressing the rest of the performance issues here, the impact of the preference checking won't be very easy to measure as that is a tiny part of the overhead.

> BTW, not surprisingly, the profiling result dramatically improved if I put
> an early-return in "identifyAutofillFields". Maybe "handleEvent" itself is
> not a bottleneck?

Yeah it isn't.  Please see the second paragraph of comment 0 again.  :-)  If you search the profile I have posted there for "identifyAutofillFields" you will see that function is almost responsible for all of the overhead here.

Comment 8

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1375568 - [Form Autofill] Reduce cross-JSM function calls between FormAutofillUtils and FormAutofillHeuristics.

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

Comment 9

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1375568 - [Form Autofill] Merge FormAutofillHeuristics into FormAutofillHandler.jsm.

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

Comment 10

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1375568 - [Form Autofill] Improve the performance of the identifyAutofillFields process.

- reduce many cross-JSM function calls
- reduce access to DOM element's properties
- cache the preferences
- avoid installing redundant event listeners
- remove logs

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

Comment 11

[Luke Chang [:lchang] (inactive)]

In the above patches, I got at least 25% improvement in calling "collectFormFields" 1000 times by reducing cross-JSM function calls and removing debugging logs. I also rewrote the "handleEvent" in "FormAutofillFrameScript.js" to avoid installing redundant event listeners and accessing to preferences unnecessarily.

BTW, I did try to replace "setTimeout" with "requestIdleCallback" in the "handleEvent". However, the running time of "doIdentifyAutofillFields" appeared to increase sharply in Gecko Profiler no matter what "timeout" I set. I haven't figured out what causes it so I left "setTimeout" as-is for now.

Comment 12

[Sean Lee [:seanlee][:weilonge]]

(In reply to Luke Chang [:lchang] from comment #9)
> Created attachment 8889298 [details]
> Bug 1375568 - [Form Autofill] Merge FormAutofillHeuristics into
> FormAutofillHandler.jsm.
> 
> Review commit: https://reviewboard.mozilla.org/r/160354/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/160354/

Hey Luke,

As we discussed offline, you think FormAutofillHandler use FormAutofillHeuristics many times so you would like to do this patch to avoid cross JSM function invoke.
However, [1] is the only FormAutofillHeuristics usage in FormAutofillHandler.
Do you think it's still worth to do?

I wish we can leave FormAutofillHeuristics in a single file for tracking the history of the heuristics algorithm easily.
Thanks.

[1] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/browser/extensions/formautofill/FormAutofillHandler.jsm#115

Comment 13

[Luke Chang [:lchang] (inactive)]

Followings are my experiments given calling "collectFormFields" 1000 times in each run:

Baseline (https://hg.mozilla.org/mozilla-central/rev/2b0f859bd071)
183ms
184ms
183ms
186ms
180ms

Attached patches applied (https://github.com/luke-chang/gecko/tree/1375568_formautofill_speedometer)
151ms
142ms
149ms
146ms
146ms

Restore Heuristics in a separated JSM (https://github.com/luke-chang/gecko/tree/1375568_formautofill_speedometer_test)
168ms
165ms
162ms
153ms
167ms

Looks like it does affect the performance. Though I haven't figure out what causes it.

Comment 14

[Luke Chang [:lchang] (inactive)]

If I triple the amount of inputs:

Baseline
412ms
426ms
417ms
419ms
428ms

Attached patches applied
324ms
338ms
330ms
330ms
331ms

Restore Heuristics in a separated JSM
348ms
354ms
355ms
352ms
343ms

Comment 15

[Luke Chang [:lchang] (inactive)]

Attachment: Bug 1375568 - [Form Autofill] Optimize "findLabelElements".

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

Comment 16

[Luke Chang [:lchang] (inactive)]

Due to the performance issue mentioned in bug 1374724, we might not switch the implementation of finding labels to use the native API soon. Therefore, I tried to improve our JS implementation.

I modified my test page to contain more label elements. Followings are the benchmark:

Baseline
684ms
688ms
683ms
687ms
684ms

Attached patches applied (without optimized findLabelElements)
514ms
519ms
512ms
517ms
502ms
506ms

Attached patches applied (with optimized findLabelElements)
397ms
406ms
408ms
395ms
412ms

Comment 17

[Matthew N. [:MattN]]

Clearing reviews after our discussion yesterday that we'll tackle individual fixes in dependencies as there won't be one fix that will remove all of autofill from speedometer profiles. I'll leave Luke as an assignee to make it clear he is owning this performance investigation.

Comment 18

[Matthew N. [:MattN]]

The patches in this bug got split into their own bugs so marking this as a meta bug.

There aren't any open dependencies remaining so I'll close this and new issues can be handled separately.