1372974 - Form controller overhead in Speedometer

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[(no longer active)]

nsFormFillController incurs some amount of overhead which is observable when running Speedometer.  For example, you can see this profile of running the react-reduct test case (http://speedometer2.benj.me/InteractiveRunner.html?suite=React-Redux-TodoMVC) in Nightly: https://perfht.ml/2rhE1qH

For example:

  * <https://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/autocomplete/nsAutoCompleteController.cpp#1183> is a JS call which is quite expensive.
  * <https://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/autocomplete/nsAutoCompleteController.cpp#265> attempts to grab the value of the input control which can be expensive.
  * It registers mutation observers that need to be called.
  * Smaller costs like bug 1372794 (there are a few more small fixes like this which I'll file with patches later.)

I've thought about what to do with these.  I think instead of fixing these individually one by one, it may be better to think of a more systematic fix.  Looking at what these tests are doing which is triggering this code in the first place, it's just calling methods like .focus() and .click() on an input element.  The form fill controller code shouldn't really kick in for programmatic calls to input controls like this at all!  So my ideal fix would be to modify <https://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/satchel/nsFormFillController.cpp#1020> which is the place where this component is "hooked up" to an input and only do this if EventStateManager::IsHandlingUserInput() returns true (IOW, only do this if the user is really interacting with the page, not if the script is programmatically focusing the control.)


With this patch, plus another one (which I will post in a separate bug after cleaning it up) to make nsFormFillController::HandleEvent() a bit more efficient, nsFormFillController now doesn't show up any more in my local profiles of Speedometer, and the overall benchmark gains 2-3 points on my fast Linux machine.


Of course, our tests don't like what I'm proposing here one bit! <https://treeherder.mozilla.org/#/jobs?repo=try&revision=606cad4d74804e532b8ac3b0bbf1e9ab357e3dc3>  As a fix for that, I think we can add a "testing mode" pref which would do what this code does today (i.e., always hook up the input element to this code when the pref is set to true) when we're running tests.

Matt, what do you think about this plan?

Comment 1

[Matthew N. [:MattN]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #0)
>   *
> <https://searchfox.org/mozilla-central/rev/
> d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/autocomplete/
> nsAutoCompleteController.cpp#1183> is a JS call which is quite expensive.

Hmm… I see that this is indeed called without user interaction and without a popup already being open…

>   *
> <https://searchfox.org/mozilla-central/rev/
> d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/autocomplete/
> nsAutoCompleteController.cpp#265> attempts to grab the value of the input
> control which can be expensive.

OK, I can see this getting called from nsFormFillController from some of the events you mention later.

>   * It registers mutation observers that need to be called.
>   * Smaller costs like bug 1372794 (there are a few more small fixes like
> this which I'll file with patches later.)
> 
> I've thought about what to do with these.  I think instead of fixing these
> individually one by one, it may be better to think of a more systematic fix.
> Looking at what these tests are doing which is triggering this code in the
> first place, it's just calling methods like .focus() and .click() on an
> input element.  The form fill controller code shouldn't really kick in for
> programmatic calls to input controls like this at all!  So my ideal fix
> would be to modify
> <https://searchfox.org/mozilla-central/rev/
> d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/satchel/
> nsFormFillController.cpp#1020> which is the place where this component is
> "hooked up" to an input and only do this if
> EventStateManager::IsHandlingUserInput() returns true (IOW, only do this if
> the user is really interacting with the page, not if the script is
> programmatically focusing the control.)

You're talking about only checking IsHandlingUserInput for the "focus" event, correct? From what I can tell, that'll cause two problems:
1) it's going to break all autocomplete on a field where a site focuses the field via `element.focus()` on page load.

That may be okay if you then call MaybeStartControllingInput for other events such as "input", "keypress", "mousedown", "compositionend" so that autocomplete will get attached for any action where autocomplete may need to appear. This may be hard to do since all the current nsFormFillController code assumes that the fields starts being controlled from the focus event. That is basically just delaying the cost to a slightly later time and possibly ends up doing much more work overall if you sum up all the MaybeStartControllingInput calls that will have to happen in many more events.

I think it may be possible to make this work but it's not trivial and it's unclear if it will be an overall net win with real users unless we also optimize MaybeStartControllingInput. I think a simple optimization may be to check if aInput == mFocusedInput and return in MaybeStartControllingInput.

2) It will break the automatic popup showing for type=password upon element.focus(): https://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/satchel/nsFormFillController.cpp#1039,1051 This can probably be easily fixed by bypassing the IsHandlingUserInput check for type=password.

> Of course, our tests don't like what I'm proposing here one bit!
> <https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=606cad4d74804e532b8ac3b0bbf1e9ab357e3dc3>  As a fix
> for that, I think we can add a "testing mode" pref which would do what this
> code does today (i.e., always hook up the input element to this code when
> the pref is set to true) when we're running tests.

We have tests specifically testing whether the autocomplete popup appears for input.focus() and I wouldn't want to be testing a different path than what we ship so I would rather we have a test focus helper to use `E10SUtils.wrapHandlingUserInput` for any .focus() call.

> Matt, what do you think about this plan?

Overall I think it's fine as long as it doesn't regress current behaviour but may just be shifting the work to later events. I suspect it's possible to achieve some real wins here though, it will just require thinking carefully about each of the events after "focus" and handling them appropriately and changing assumptions about "focus" always happening first.

Comment 2

[(no longer active)]

(In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking you) from comment #1)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #0)
> >   *
> > <https://searchfox.org/mozilla-central/rev/
> > d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/autocomplete/
> > nsAutoCompleteController.cpp#1183> is a JS call which is quite expensive.
> 
> Hmm… I see that this is indeed called without user interaction and without a
> popup already being open…

Yeah in this case it's just returning false, but by going through the JS implemented call.  :/

> >   * It registers mutation observers that need to be called.
> >   * Smaller costs like bug 1372794 (there are a few more small fixes like
> > this which I'll file with patches later.)
> > 
> > I've thought about what to do with these.  I think instead of fixing these
> > individually one by one, it may be better to think of a more systematic fix.
> > Looking at what these tests are doing which is triggering this code in the
> > first place, it's just calling methods like .focus() and .click() on an
> > input element.  The form fill controller code shouldn't really kick in for
> > programmatic calls to input controls like this at all!  So my ideal fix
> > would be to modify
> > <https://searchfox.org/mozilla-central/rev/
> > d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/satchel/
> > nsFormFillController.cpp#1020> which is the place where this component is
> > "hooked up" to an input and only do this if
> > EventStateManager::IsHandlingUserInput() returns true (IOW, only do this if
> > the user is really interacting with the page, not if the script is
> > programmatically focusing the control.)
> 
> You're talking about only checking IsHandlingUserInput for the "focus"
> event, correct? From what I can tell, that'll cause two problems:
> 1) it's going to break all autocomplete on a field where a site focuses the
> field via `element.focus()` on page load.

That is, the site calling .focus() and Firefox opening the autocomplete list box?  Yes, that's what I'm suggesting.  But I was under the impression that this is indeed undesirable, I didn't know that we actually want to show this UI before the user has interacted with the form control.

> That may be okay if you then call MaybeStartControllingInput for other
> events such as "input", "keypress", "mousedown", "compositionend" so that
> autocomplete will get attached for any action where autocomplete may need to
> appear. This may be hard to do since all the current nsFormFillController
> code assumes that the fields starts being controlled from the focus event.
> That is basically just delaying the cost to a slightly later time and
> possibly ends up doing much more work overall if you sum up all the
> MaybeStartControllingInput calls that will have to happen in many more
> events.

I think it's OK to pay this cost for the cases where the user is really interacting with the browser, I just prefer not to pay it in cases where a script is running some code where there is potentially no chance of us ever showing this UI in the first place (for example the script may call .focus() in a loop 10,000 times and then remove the node from the DOM tree before yielding in which case all of the cost we're paying would have been for naught!)

> I think it may be possible to make this work but it's not trivial and it's
> unclear if it will be an overall net win with real users unless we also
> optimize MaybeStartControllingInput.

Fair enough.  The other approach that I haven't tried yet is just good old optimization of everything that shows up from nsFormFillContainer in the profiles without the proposal in comment 0.  I'll do that a bit today to see how far that gets me.  It could be that it turns out we won't need to do something that drastic after all...

> I think a simple optimization may be to
> check if aInput == mFocusedInput and return in MaybeStartControllingInput.

That seems to be a super low hanging fruit and a great idea, I'll do that in a separate bug (not sure if that will help with Speedometer, but who cares!)

> 2) It will break the automatic popup showing for type=password upon
> element.focus():
> https://searchfox.org/mozilla-central/rev/
> d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/satchel/
> nsFormFillController.cpp#1039,1051 This can probably be easily fixed by
> bypassing the IsHandlingUserInput check for type=password.

I'm not 100% sure if I fully understand this part but that's OK, I'll ping you if I end up pursuing this solution further.

> > Of course, our tests don't like what I'm proposing here one bit!
> > <https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=606cad4d74804e532b8ac3b0bbf1e9ab357e3dc3>  As a fix
> > for that, I think we can add a "testing mode" pref which would do what this
> > code does today (i.e., always hook up the input element to this code when
> > the pref is set to true) when we're running tests.
> 
> We have tests specifically testing whether the autocomplete popup appears
> for input.focus() and I wouldn't want to be testing a different path than
> what we ship so I would rather we have a test focus helper to use
> `E10SUtils.wrapHandlingUserInput` for any .focus() call.

Makes sense, thanks for the suggestion.

Comment 3

[Matthew N. [:MattN]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #2)
> (In reply to Matthew N. [:MattN] (huge backlog; PM if requests are blocking
> you) from comment #1)
> > (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> > comment #0)
> > >   * It registers mutation observers that need to be called.
> > >   * Smaller costs like bug 1372794 (there are a few more small fixes like
> > > this which I'll file with patches later.)
> > > 
> > > I've thought about what to do with these.  I think instead of fixing these
> > > individually one by one, it may be better to think of a more systematic fix.
> > > Looking at what these tests are doing which is triggering this code in the
> > > first place, it's just calling methods like .focus() and .click() on an
> > > input element.  The form fill controller code shouldn't really kick in for
> > > programmatic calls to input controls like this at all!  So my ideal fix
> > > would be to modify
> > > <https://searchfox.org/mozilla-central/rev/
> > > d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/satchel/
> > > nsFormFillController.cpp#1020> which is the place where this component is
> > > "hooked up" to an input and only do this if
> > > EventStateManager::IsHandlingUserInput() returns true (IOW, only do this if
> > > the user is really interacting with the page, not if the script is
> > > programmatically focusing the control.)
> > 
> > You're talking about only checking IsHandlingUserInput for the "focus"
> > event, correct? From what I can tell, that'll cause two problems:
> > 1) it's going to break all autocomplete on a field where a site focuses the
> > field via `element.focus()` on page load.
> 
> That is, the site calling .focus() and Firefox opening the autocomplete list
> box?  Yes, that's what I'm suggesting.  But I was under the impression that
> this is indeed undesirable, I didn't know that we actually want to show this
> UI before the user has interacted with the form control.

We only show the popup upon a focus of the element for login fields (case 2) but what I'm talking about in case 1 is if the site calls element.focus() and then the user types a character, we need to start controlling the input to do a search for that typed character. The popup would only need to open once the user types the character (if there are relevant results). It sounded like you were maybe considering just adding the IsHandlingUserInput check for focus but then that field would remain uncontrolled until the user manually re-focused it which would be bad.

> > That may be okay if you then call MaybeStartControllingInput for other
> > events such as "input", "keypress", "mousedown", "compositionend" so that
> > autocomplete will get attached for any action where autocomplete may need to
> > appear. This may be hard to do since all the current nsFormFillController
> > code assumes that the fields starts being controlled from the focus event.
> > That is basically just delaying the cost to a slightly later time and
> > possibly ends up doing much more work overall if you sum up all the
> > MaybeStartControllingInput calls that will have to happen in many more
> > events.
> 
> I think it's OK to pay this cost for the cases where the user is really
> interacting with the browser, I just prefer not to pay it in cases where a
> script is running some code where there is potentially no chance of us ever
> showing this UI in the first place (for example the script may call .focus()
> in a loop 10,000 times and then remove the node from the DOM tree before
> yielding in which case all of the cost we're paying would have been for
> naught!)

Yeah, sure, though I'm not sure how often we would actually be wasting work in practice.

> > I think it may be possible to make this work but it's not trivial and it's
> > unclear if it will be an overall net win with real users unless we also
> > optimize MaybeStartControllingInput.
> 
> Fair enough.  The other approach that I haven't tried yet is just good old
> optimization of everything that shows up from nsFormFillContainer in the
> profiles without the proposal in comment 0.  I'll do that a bit today to see
> how far that gets me.  It could be that it turns out we won't need to do
> something that drastic after all...
> 
> > I think a simple optimization may be to
> > check if aInput == mFocusedInput and return in MaybeStartControllingInput.
> 
> That seems to be a super low hanging fruit and a great idea, I'll do that in
> a separate bug (not sure if that will help with Speedometer, but who cares!)
> 
> > 2) It will break the automatic popup showing for type=password upon
> > element.focus():
> > https://searchfox.org/mozilla-central/rev/
> > d67ef71097da4d1aa344c9d9c672e49a7228e765/toolkit/components/satchel/
> > nsFormFillController.cpp#1039,1051 This can probably be easily fixed by
> > bypassing the IsHandlingUserInput check for type=password.
> 
> I'm not 100% sure if I fully understand this part but that's OK, I'll ping
> you if I end up pursuing this solution further.

We auto-open the popup for username and password field suggestions upon focus (regardless if it's by the user or not). This is to help with visibility of login suggestions but also to immediately should the insecure login UI before the user starts typing. Test on http://http-login.badssl.com/

Comment 4

[(no longer active)]

So I spent almost all of my time today implementing what we discussed last week and then I realized that I hadn't fully thought this though...  Since if we defer all of this work until those secondary events, then one of those would be the input event, which is one of those events that would show up in the profile in comment 0 and of course I would realize this after having done all of the work.  ;-)

But to throw salt into my wound today, at the very end I discovered that the input element that the input event is fired on there is a *checkbox*, not a textbox!  So we can easily just ignore it.  :-)  And that takes care of the HandleText() part of the cost that shows up here...

Comment 5

[(no longer active)]

Compare a perf profile before and after the patches that I posted in the dependencies of this bug, searching for nsFormFillController:

Before:

+    0.05%     0.00%  Web Content  libxul.so  [.] nsFormFillController::HandleEvent                                                 ▒
+    0.02%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::GetPopupOpen(bool*)                    ▒
+    0.01%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::GetTextValue(nsAString&)               ▒
+    0.01%     0.00%  Web Content  libxul.so  [.] nsFormFillController::StopControllingInput                                        ▒
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::ContentRemoved(nsIDocument*, nsIContent▒
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::AddRef()                               ▒
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::HandleEvent(nsIDOMEvent*)              ▒
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::ContentAppended(nsIDocument*, nsIConten▒
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::GetDisableAutoComplete(bool*)          ▒
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::AttributeWillChange(nsIDocument*, mozil▒
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::AddRef()                               ▒
+    0.00%     0.00%  Web Content  libxul.so  [.] nsFormFillController::cycleCollection::TraverseNative                             ▒
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::Release()                              ▒
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::Release()                              ▒
+    0.00%     0.00%  Web Content  libxul.so  [.] nsFormFillController::StartControllingInput                                       ▒
+    0.00%     0.00%  Web Content  libxul.so  [.] nsFormFillController::MaybeStartControllingInput                                  ▒
+    0.00%     0.00%  Web Content  libxul.so  [.] nsFormFillController::Focus                                                       ▒

After:

+    0.00%     0.00%  Web Content  libxul.so  [.] nsFormFillController::HandleEvent
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::Release()
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::AddRef()
+    0.00%     0.00%  Web Content  libxul.so  [.] non-virtual thunk to nsFormFillController::HandleEvent(nsIDOMEvent*)
+    0.00%     0.00%  Web Content  libxul.so  [.] nsFormFillController::QueryInterface

The remaining nsFormFillController::HandleEvent() part is the QI() that I added in bug 1374893.  I don't really care all that much about it.  The rest of the cost is basically about nsFormFillController existing in the first place, so I'm going to call this done after the dependencies land.  \o/

Comment 6

[(no longer active)]

Fixed by dependencies.