29730 - nsGenericHTMLElement::SetAttribute is slow because GetPrimaryFrameFor can be slow

Status: VERIFIED WONTFIX

(Core :: Layout, defect, P3)

Description

[Simon Fraser [no longer active]]

We're finding in editor that a single call to 
nsIDOMElement::SetAttribute("_moz_dirty", "") is taking a significant proportion 
of the time taken to insert a <br> into the content model:

Trace Name              Count  Total Time (µs)  Min Time    Avg Time    Max Time
InsertBreak             1           3333        3333        3333        3333
SetAttribute            1           1789        1789        1789        1789
AttributeChanged        1           1720        1720        1720        1720
GetPrimaryFrameFor      1           1482        1482        1482        1482

So nsFrameManager::GetPrimaryFrameFor is taking about 40% of the time spent
in SetAttribute().

The test case here was pasting several lines of text into the editor, which 
causes the _moz_dirty attribute to get set on the new nodes. GetPrimaryFrameFor() 
is actually called twice for each line of text that gets pasted in, and accounts 
for around 34% of the total time spent inserting text into the editor.

Comment 1

[Joe Francis]

troy, would it be possible to have reflow batching also batch the style 
resolution?  Or have a parallel style bacthing call we could make?

Comment 2

[troy]

As far as GetPrimaryFrameFor() goes, some frames are added to the map when they 
are constructed and some are not. It depends on the type of frame, and it is a 
space/time tradeoff between a large map and quick lookup time

It looks like you're talking about BR frames, and those don't get added to the 
hash table by default, because no one typically calls GetPrimaryFrameFor() for a 
BR content object

Why are you setting an attribute on the frame for a BR object? Can't you set it 
_before_ adding it to the content model so we don't go through all the work of 
reresolving style and finding the frame and such?

If it's not in the map by default, then we search the frame tree for it. That's 
reasonably fast as well, because it doesn't start at the root it starts at the 
nearest ancestor frame that is in the map

Once we do find the frame, then it is added to the map so any subsequent lookups 
are fast

Comment 3

[Joe Francis]

will adding the attribute before adding the node actually circumvent the style 
resolution?  I don't understand why that would be.  You mean if I actually had a 
style sheet that cared about that attribute, we would get different behavior 
depending on when I added the attribute??

Adding the attribute first is completely doable, btw.  I just had no idea that it 
mattered.

Comment 4

[troy]

It won't circumvent style resolution, but it should mean we only do it once 
instead of twice.

What I think is happening now is that you insert the content object into the 
content model and then we build a frame for it. When we build the frame we 
resolve style for it. We have to resolve style before creating a frame because 
otherwise we don't know what kind of frame to create

Then you set the attribute and the content code checks to see whether the 
content object has an associated document object. If it does, then it calls over 
to the style system and notifies it that the attribute changed.

That causes us to resolve the style for a second time. It also causes us to find 
the frame for the content object and hence the GetPrimaryFrameFor() call

That's what I think is happening anyway, and so yes you can get a substantial 
gain by setting attributes before inserting the content object into the content 
model

Comment 5

[troy]

Let me know what you find when you change it to set the attribute first.

I'm inclined to mark this bug WONTFIX, because GetPrimaryFrameFor() performs as 
designed, and it's because the BR frame is lazily added to the map that the 
call takes longer than you like

Comment 6

[troy]

Per my conversation with Simon marking this WONTFIX. Setting the attribute 
before adding the BR content to the content model should fix the performance 
issue. If necessary, BR frames could be added to the map by default when editing 
a document. However, this will increase the size of the map considerably

Comment 7

[Chris Petersen]

Marking verified won't fix per last comments.