This bug to address Darin's comment in https://bugs.webkit.org/show_bug.cgi?id=141776#c5. Comment on attachment 246862 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246862&action=review > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:260 > +ClipperData* RenderSVGResourceClipper::addRendererToClipper(const RenderObject& object) The pointer this returns is always non-null. This function should return a reference instead of a pointer. > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:264 > + if (!m_clipper.contains(&object)) > + m_clipper.set(&object, std::make_unique<ClipperData>()); > + return m_clipper.get(&object); This code does double hash table lookups. You just moved it, but the performance problem can be fixed, like this: auto& slot = m_clipper.add(&object, nullptr).iterator->value; if (!slot) slot = std::make_unique<ClipperData>(); return slot.get(); That will only do a single hash table lookup. In the future we might invent even cleaner ways to write this. We could also consider making this a HashMap<const RenderObject*, ClipperData> instead. If we did that, then the code would be even simpler: return m_clipper.add(&object, ClipperData()).iterator->value; The map would be larger, because each slot would have an entire ClipperData object. And rehashing would be slower, but we would avoid the overhead of an additional memory block for every slot that is in use.
Created attachment 247379 [details] Patch
(In reply to comment #0) > This bug to address Darin's comment in > https://bugs.webkit.org/show_bug.cgi?id=141776#c5. > > Comment on attachment 246862 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=246862&action=review > > > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:260 > > +ClipperData* RenderSVGResourceClipper::addRendererToClipper(const RenderObject& object) > > The pointer this returns is always non-null. This function should return a > reference instead of a pointer. > Done. > > Source/WebCore/rendering/svg/RenderSVGResourceClipper.cpp:264 > > + if (!m_clipper.contains(&object)) > > + m_clipper.set(&object, std::make_unique<ClipperData>()); > > + return m_clipper.get(&object); > > This code does double hash table lookups. You just moved it, but the > performance problem can be fixed, like this: > > auto& slot = m_clipper.add(&object, nullptr).iterator->value; > if (!slot) > slot = std::make_unique<ClipperData>(); > return slot.get(); > > That will only do a single hash table lookup. In the future we might invent > even cleaner ways to write this. > > We could also consider making this a HashMap<const RenderObject*, > ClipperData> instead. If we did that, then the code would be even simpler: > > return m_clipper.add(&object, ClipperData()).iterator->value; > > The map would be larger, because each slot would have an entire ClipperData > object. And rehashing would be slower, but we would avoid the overhead of an > additional memory block for every slot that is in use. Thanks for the advice. This change looks much better and cleaner than the code I modified. I changed the type of m_clipper to be as you suggested. ClipperData is a structure which only one item of type std::unique_ptr<>. So sizeof(ClipperData) is 8 and sizeof(ClipperData*) is also 8. So I think the map size will be the same with this change.
Created attachment 247385 [details] Patch
Comment on attachment 247385 [details] Patch Looks like this doesn’t compile on Windows. It seems that Windows is having trouble because it is compiling the assignment operator for ClipperData rather than a move operator. We might be able to fix this using std::unique_ptr<ImageBuffer> directly or as a typedef instead of wrapping it in a class. Also, we don’t need to use WTF_MAKE_FAST_ALLOCATED in the ClipperData struct any more since we are not allocating it as a separate object on the heap.
Created attachment 247427 [details] Patch
Created attachment 247429 [details] Patch
Comment on attachment 247429 [details] Patch Clearing flags on attachment: 247429 Committed r180685: <http://trac.webkit.org/changeset/180685>
All reviewed patches have been landed. Closing bug.
<rdar://problem/19971911>