1814310 - nsGenericHTMLFormElement::UpdateFormOwner shows up in the performance profiles

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

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

https://share.firefox.dev/3JvUYc4

https://browserbench.org/Speedometer2.1/InteractiveRunner.html jQuery-TodoMVC

Comment 1

[Vincent Hilla [:vhilla]]

Here is a profile running the test 10 times with higher sampling. Three methods seem relevant: FindAncestorForm, GetAttr and GetFormInternal.

FindAncestorForm has a high self time and I don't think there is much to be done, as we have to walk the tree to find possible forms. I guess NodeInfo::Equals shows up in the call tree as it's called often?

GetAttr has some overhead, as we call GetAttr(..., nsAString) which calls GetAttr(..., DOMString), which again retrieves the value from a nsAttrValue object. So there is an intermediate DOMString object that we could eliminate by duplicating code between GetAttr(..., nsAString) and GetAttr(..., DOMString). Truncating an empty nsAString also shows up, maybe that can be optimized? Using nsString instead of nsAutoString did not change that.

I'm unsure why GetFormInternal shows up, maybe because of virtual overhead or HTMLElements implementation here. We can eliminate some calls to it by updating a local variable whenever ClearForm or SetFormInternal is called, though one might fail to update the variable correctly in the future causing bugs. Given this reduced the number of samples by 4 of 93 total, this might not be worth it.

Here is a profile with GetAttr directly using nsAttrValue and GetFormInternal being called only once.

Comment 2

[Vincent Hilla [:vhilla]]

Attachment: Bug 1814310 - Improve runtime of nsGenericHTMLFormElement::UpdateFormOwner. r=smaug

Comment 3

[Vincent Hilla [:vhilla]]

:smaug what is your assessment of this? Can we do more about FindAncestorForm or Truncate and is the change around GetFormInternal worth the complexity?

Comment 4

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

I don't think the changes to GetFormInternal handling look too complicated. Since it is virtual, better to optimize the calls a bit.
Hard to see much to optimize in Truncate. It should get inlined rather well in pgo builds.
Possibly overkill optimization for FindAncestorForm would be to use a flag in nsINode to indicate that the node is a form, and then use that for the check and not IsHTMLElement(nsGkAtoms::form)

Comment 5

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

But we could land the patch I think, and profile then some more and file new bugs if needed.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

FWIW, I don't think Chrome has anything like UpdateFormOwner. Instead, to implement <form>.elements they'll just build a standard dynamic HTMLCollection for form controls:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/forms/html_form_controls_collection.cc;l=39;drc=9d817b1cfebcb123bab29651e532081c3a900c5c

Comment 7

[Markus Stange [:mstange]]

It looks like they collection gets the ListedElements() from the form element, which builds the list lazily in HTMLFormElement::ListedElements(): https://source.chromium.org/chromium/chromium/src/+/refs/heads/main:third_party/blink/renderer/core/html/forms/html_form_element.cc;l=767-795;drc=fdfd85f836e0e59c79ed9bf6d527a2b8f7fdeb6e

Comment 8

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

Chrome does have something very similar to UpdateFormOwner,called ResetFormOwner(). It does a bit different things, sure, but the concept is quite similar.

Chrome seems to have some optimization for parser created elements which we don't have.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #8)

Chrome does have something very similar to UpdateFormOwner,called ResetFormOwner(). It does a bit different things, sure, but the concept is quite similar.

Indeed, it looks quite similar.

Comment 10

[Vincent Hilla [:vhilla]]

We can eliminate Truncate by using HasForm at the cost of an additional mAttrs.GetAttr. Here is a profile with that change. There aren't too many samples for Truncate and GetFormInternal, so there is some uncertainty about them. Though Truncate should have previously been called less than mAttrs.GetAttr and still consistently showed up more.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b80c658a6c19
Improve runtime of nsGenericHTMLFormElement::UpdateFormOwner. r=smaug

Comment 12

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/b80c658a6c19