1553705 - avoid creating the Forms and FormControls content lists when we can

Status: RESOLVED FIXED

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

Description

[Cameron McCormack (:heycam)]

Looking at a profile of the HTML spec loading, we spend 150ms (or about 2% of the load time) under nsINode::CompareDocumentPosition in response to elements being inserted into the document from the parser, so that we can potentially update the document's Forms and FormControls nsContentLists eagerly rather than just marking them as dirty. These nsContentLists exist due to GenerateStateKey being called for form controls just after they are created. I see that bug 314117 made us avoid calling GenerateStateKey unless we knew we had state to restore, but perhaps we regressed that.

Comment 1

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #0)

I see that bug 314117 made us avoid calling GenerateStateKey unless we knew we had state to restore, but perhaps we regressed that.

I mean bug 388387.

Comment 2

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfc78974975830e2531216eda3bccb0af3a7b16f

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1553705 - Make GenerateStateKey() infallible.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1553705 - Use a cheaper to compute state key for parser inserted form controls.

Comment 5

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dfd286dab76b8d5955c9c60d5dc8d534259eb44

Comment 6

[Cameron McCormack (:heycam)]

So I think the approach in this patch isn't going to work. By delaying the call to GenerateStateKey from the point the element is first inserted into the document to some later point when we want to capture its state, we can end up generating a different state key. (The element may have moved around, or other elements inserted before it, causing the part of the key that comes from its index in the FormControls content list to be different.)

We only do this on insertion for parser generated elements. Given that, I wonder if we could change the key so that instead of using its index in FormControls, we instead use some counter that is incremented on the document. That should preserve the same order for those parser generated elements. For non-parser generated elements inserted into the document later, we could continue to use the index in FormControls (although that is still not great for perf with very large documents like this), as long as the state keys for the parser generated elements were distinct from the non-parser generated ones.

Comment 7

[Cameron McCormack (:heycam)]

Updated patches to do that:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=541009241fa634373b4b2c44598a9eb636406340

Comment 8

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=659eded418f15e061531f43fd94ecae9ce4221d7

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dd6e7c0970d5
Make GenerateStateKey() infallible. r=smaug
https://hg.mozilla.org/integration/autoland/rev/fbb26a04ec1f
Use a cheaper to compute state key for parser inserted form controls. r=smaug

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/dd6e7c0970d5
https://hg.mozilla.org/mozilla-central/rev/fbb26a04ec1f

Comment 11

[Narcis Beleuzu [:NarcisB]]

Backed out for causing Bug1562142

Backout link: https://hg.mozilla.org/integration/autoland/rev/1ddc47f7370187b3a07d61a65f9f0fd19e7e20a5

Comment 12

[Cameron McCormack (:heycam)]

The regression flagged in bug 1562142 showed a lot of time spent under GenerateStateKey when running Speedometer. There, the form control elements are inserted into the document not by the parser coming from the network, which means we do the slow "find the position of the form control in the document using an nsContentList" thing. But my patch makes us call GenerateStateKey more often (specifically, the calls is moved out of the if (!mInhibitRestoration) checks). So I need to remember why I did that...

Comment 13

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=755b6b067337e12b06964e552028d22e3349b8ae

And a raptor-speedometer run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90d637ce3359f77ff323c7f81457d07b7b07a0df

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02312da6f1ca
Make GenerateStateKey() infallible. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d65cbaac955b
Use a cheaper to compute state key for parser inserted form controls. r=smaug

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/02312da6f1ca
https://hg.mozilla.org/mozilla-central/rev/d65cbaac955b