Summary: | Shrink ElementAttributeData by factoring out Attr object count. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||
Component: | DOM | Assignee: | 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
Andreas Kling
2012-05-07 14:00:41 PDT
Created attachment 140584 [details]
Pretentious patch
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. Created attachment 140704 [details]
Presumptuous patch
Committed r116419: <http://trac.webkit.org/changeset/116419> Committed r116427: <http://trac.webkit.org/changeset/116427> Additional revision to fix Chrome ninja build. 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; (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. |