Bug 85825 – Shrink ElementAttributeData by factoring out Attr object count.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Andreas Kling]

Yup.

[Andreas Kling]

Created attachment 140584 [details]
Pretentious patch

[Antti Koivisto]

Comment on attachment 140584 [details]
Pretentious patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140584&action=review

I think the storage should be bit more optimized for size. Looks great otherwise.

> Source/WebCore/dom/ElementAttributeData.cpp:36
> +typedef HashMap<QualifiedName, RefPtr<Attr> > AttrMap;
> +typedef HashMap<Element*, OwnPtr<AttrMap> > AttrMapMap;

I know Attrs are super rare and shouldn't really be used by anyone, but this still seems overly bloaty. HashMaps have high fixed initial cost (64 slots by default, lots of overhead). Since the name -> Attr map is basically always tiny I think you should use Vector instead.

[Andreas Kling]

Created attachment 140704 [details]
Presumptuous patch

[Andreas Kling]

Committed r116419: <http://trac.webkit.org/changeset/116419>

[Stephen Chenney]

Committed r116427: <http://trac.webkit.org/changeset/116427>

[Stephen Chenney]

Additional revision to fix Chrome ninja build.

[Ojan Vafai]

Comment on attachment 140704 [details]
Presumptuous patch

View in context: https://bugs.webkit.org/attachment.cgi?id=140704&action=review

Nice patch! Just saw this in beverloo's weekly summary and had a few thoughts. Feel free to ignore them if you've already investigated this. Although, I'd be especially interested if you did find a perf issue that required the hasAttrList method. It would surprise me a little.

> Source/WebCore/dom/ElementAttributeData.cpp:47
> +    if (!element->hasAttrList())

Do we even need the hasAttrList method? Could we just check attrListMap().contains? Are you worried that would be too slow? Could we try it with just using contains and see if it regresses any perf tests?

It's not a big deal, but it would remove a little bit of complexity from Node.h and save a bit from NodeFlags for something else, which is nice if we can get away with it.

> Source/WebCore/dom/ElementAttributeData.cpp:59
> +    ASSERT(element);
> +    if (element->hasAttrList()) {
> +        ASSERT(attrListMap().contains(element));
> +        return attrListMap().get(element);
> +    }

Cleanup nit if you feel moved...this could just be:
if (AttrList* attrList = attrListForElement(element))
    return attrList;

[Andreas Kling]

(In reply to comment #7)
> (From update of attachment 140704 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140704&action=review
> 
> Nice patch! Just saw this in beverloo's weekly summary and had a few thoughts. Feel free to ignore them if you've already investigated this. Although, I'd be especially interested if you did find a perf issue that required the hasAttrList method. It would surprise me a little.
> 
> > Source/WebCore/dom/ElementAttributeData.cpp:47
> > +    if (!element->hasAttrList())
> 
> Do we even need the hasAttrList method? Could we just check attrListMap().contains? Are you worried that would be too slow? Could we try it with just using contains and see if it regresses any perf tests?
> 
> It's not a big deal, but it would remove a little bit of complexity from Node.h and save a bit from NodeFlags for something else, which is nice if we can get away with it.

That would introduce a hash lookup on every Element teardown (assuming the element has or has had attributes.) This is measurable on tests like PerformanceTests/DOM/CloneNodes where a lot of elements are created and destroyed. Also, it's not really any different from e.g the HasRareData flag.

> > Source/WebCore/dom/ElementAttributeData.cpp:59
> > +    ASSERT(element);
> > +    if (element->hasAttrList()) {
> > +        ASSERT(attrListMap().contains(element));
> > +        return attrListMap().get(element);
> > +    }
> 
> Cleanup nit if you feel moved...this could just be:
> if (AttrList* attrList = attrListForElement(element))
>     return attrList;

Sure, I'll do that. Thanks.

[David Kilzer (:ddkilzer)]

<rdar://problem/11625115>