Bug 73784 – Use HashMap<OwnPtr> in CollectionCache

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

Use HashMap<OwnPtr> in CollectionCache

[Darin Adler]

Created attachment 117812 [details]
Patch

[Andreas Kling]

Comment on attachment 117812 [details]
Patch

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

> Source/WebCore/html/HTMLCollection.cpp:344
> +            OwnPtr<Vector<Element*> >& nameVector = m_info->nameCache.add(nameAttrVal.impl(), nullptr).first->second;
> +            if (!nameVector)
> +                nameVector = adoptPtr(new Vector<Element*>);

Given how much this pattern repeats, it seems like we could factor them into functions, e.g ensureNameCache() and ensureIdCache().
That would also make the "// add to foo cache" comments even more redundant.

[Darin Adler]

Comment on attachment 117812 [details]
Patch

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

>> Source/WebCore/html/HTMLCollection.cpp:344
>> +                nameVector = adoptPtr(new Vector<Element*>);
> 
> Given how much this pattern repeats, it seems like we could factor them into functions, e.g ensureNameCache() and ensureIdCache().
> That would also make the "// add to foo cache" comments even more redundant.

I think I can make a helper function that we can use at all 6 call sites.

[Darin Adler]

Committed r102147: <http://trac.webkit.org/changeset/102147>

[Antti Koivisto]

Wouldn't it be even better to just to use HashMap<AtomicStringImpl*, Vector<Element*> > ? Do we lack optimization for moving Vectors in HashMaps?

[Darin Adler]

(In reply to comment #5)
> Wouldn't it be even better to just to use HashMap<AtomicStringImpl*, Vector<Element*> > ? Do we lack optimization for moving Vectors in HashMaps?

I don’t know if it would be better. I don’t know how well we accommodate vectors when moving hash table entries around. Also, empty and deleted slots would use a lot more memory than they do for OwnPtr.

[Antti Koivisto]

(In reply to comment #6)
> I don’t know if it would be better. I don’t know how well we accommodate vectors when moving hash table entries around. Also, empty and deleted slots would use a lot more memory than they do for OwnPtr.

Empty vector is a pointer and a size_t. Considering that used values now use more memory the difference shouldn't be huge. Extra indirection is slower and the type looks uglier so it would be nice if the pure vector case was efficient too.

[Darin Adler]

(In reply to comment #7)
> (In reply to comment #6)
> > I don’t know if it would be better. I don’t know how well we accommodate vectors when moving hash table entries around. Also, empty and deleted slots would use a lot more memory than they do for OwnPtr.
> 
> Empty vector is a pointer and a size_t. Considering that used values now use more memory the difference shouldn't be huge. Extra indirection is slower and the type looks uglier so it would be nice if the pure vector case was efficient too.

Sure, sounds like a neat project.

[Darin Adler]

(In reply to comment #7)
> Empty vector is a pointer and a size_t.

I believe it’s a pointer (m_buffer) and two size_t's (m_capacity and m_size).

[Antti Koivisto]

(In reply to comment #9)
> (In reply to comment #7)
> > Empty vector is a pointer and a size_t.
> 
> I believe it’s a pointer (m_buffer) and two size_t's (m_capacity and m_size).

You are right. Optimizing m_capacity away from Vector in null case might be a good memory win.