Closed Bug 1553705 Opened 6 months ago Closed 5 months ago

avoid creating the Forms and FormControls content lists when we can

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

(Keywords: perf)

Attachments

(2 files)

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.

(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.

Assignee: nobody → cam
Status: NEW → ASSIGNED

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.

Attachment #9066924 - Attachment description: Bug 1553705 - Don't call GenerateStateKey() unless we have state to restore. → Bug 1553705 - Use a cheaper to compute state key for parser inserted form controls.
Priority: -- → P2
Pushed by cmccormack@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla69 → ---

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...

Flags: needinfo?(cam)
Pushed by cmccormack@mozilla.com:
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
Status: REOPENED → RESOLVED
Closed: 5 months ago5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
You need to log in before you can comment on or make changes to this bug.