Bug 85825

Summary: Shrink ElementAttributeData by factoring out Attr object count.
Product: WebKit Reporter: Andreas Kling <kling>
Component: DOMAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, schenney
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Pretentious patch
koivisto: review-
Presumptuous patch koivisto: review+

Description Andreas Kling 2012-05-07 14:00:41 PDT
Yup.
Comment 1 Andreas Kling 2012-05-07 14:03:20 PDT
Created attachment 140584 [details]
Pretentious patch
Comment 2 Antti Koivisto 2012-05-07 14:23:13 PDT
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.
Comment 3 Andreas Kling 2012-05-08 06:45:05 PDT
Created attachment 140704 [details]
Presumptuous patch
Comment 4 Andreas Kling 2012-05-08 07:16:45 PDT
Committed r116419: <http://trac.webkit.org/changeset/116419>
Comment 5 Stephen Chenney 2012-05-08 08:45:20 PDT
Committed r116427: <http://trac.webkit.org/changeset/116427>
Comment 6 Stephen Chenney 2012-05-08 08:45:44 PDT
Additional revision to fix Chrome ninja build.
Comment 7 Ojan Vafai 2012-05-16 23:08:48 PDT
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;
Comment 8 Andreas Kling 2012-05-17 15:52:18 PDT
(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.
Comment 9 David Kilzer (:ddkilzer) 2012-06-08 08:54:01 PDT
<rdar://problem/11625115>