Bug 142032 – Cleanup RenderSVGResourceClipper class.

• Component: SVG
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 247379 [details]
Patch

[Said Abou-Hallawa]

(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.

[Said Abou-Hallawa]

Created attachment 247385 [details]
Patch

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 247427 [details]
Patch

[Said Abou-Hallawa]

Created attachment 247429 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 247429 [details]
Patch

Clearing flags on attachment: 247429

Committed r180685: <http://trac.webkit.org/changeset/180685>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/19971911>