166450 - Lots of Clone()ing in nsMappedAttributes

Status: RESOLVED INACTIVE

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

Description

[Boris Zbarsky [:bzbarsky]]

nsHTMLAttributes::EnsureSingleMappedFor ensures uniqueness by calling Clone() on
the mMapped object.  In the case when mMapped is already unique (has an
mUseCount of "1"), this is a waste of cycles and memory (memory because it
bloats the ruletree, see bug 117316).

It's simple enough to fix things not to clone unless needed here, but I need to
investigate whether we then need to clear out style data the way inline style
rules do.....

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: something to play with

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 97683 [details] [diff] [review]
something to play with

Well, this makes us crash in the nsHTMLStyleSheet destructor when leaving
pages.. so something is amiss.	;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Something else to play with

This no longer crashes because now we take ourselves out of the hash before
changing out hashkey.  But the code in question is disabled because, as the
comments say, we have to massage the ruletree if we do this.  I'm starting to
think that this change may not be worth it...

Comment 4

[Boris Zbarsky [:bzbarsky]]

With the move toward nsIStyleRule immutability, we really do need the cloning, I
think.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We can at least avoid Clone()-ing the maps during parsing since they won't be
used as nsIStyleRules until the element is fully parsed.

Reopening this to investigate that possiblity. My plan is to not give the
attribute-map a stylesheet until after the element is fully parsed. I'm not sure
how to do that yet though.

Another thing that would be good to do is to try to get an existing map with the
correct values if one already exists. Especially the senario of having a large
number of elements with only one mapped attribute might be common (say 100 <font
size="1"> elements). In that case we should be able to just grab the existing
map right away rather then creating our own and then throw it away.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ooh.  That last idea is a good one.

sicking, wanna take this one?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

sure

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ideally i would like to use the fact that mDocument is not set as a qualifier to
when we don't need to worry about cloning the attribute-map. However that
doesn't work during parsing since mDocument is in fact set even though the
element isn't in the document yet.

So my idea was to use the fact that mParent isn't set during parsing as
qualifier. I.e. test for (!mDocument || !mParent). However that will fubar the
root element since that never gets it's parent set.

However we could possibly make an exception for the root element by testing
(!mDocument || (!mParent && mDocument->GetRootContent() != this)). This would
circumvent the optimization entierly on the root element during parsing.


Hmm.. i think.. or isn't the root content set while we parse the root contents
attributes? If so we could maybe change that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Are we trying to do this in the very near term, or should I try to resurrect my
BindToTree() patch so mDocument will _not_ be set on an element that's still
being parsed?

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This isn't very near term. Don't let this bug affect the BindToTree() patch, I'm
pretty sure that i can work with the currect state of affiars, but if i could
rely on mDocument then that'd be even better.

Comment 11

[Jonathan Watt [:jwatt]]

nsHTMLAttributes is gone.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The same code now lives in nsMappedAttributes/nsAttrAndChildArray