Use HashMap<OwnPtr> in CollectionCache
Created attachment 117812 [details] Patch
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.
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.
Committed r102147: <http://trac.webkit.org/changeset/102147>
Wouldn't it be even better to just to use HashMap<AtomicStringImpl*, Vector<Element*> > ? Do we lack optimization for moving Vectors in HashMaps?
(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.
(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.
(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.
(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).
(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.