Summary: | Cleanup RenderSVGResourceClipper class. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Said Abou-Hallawa <sabouhallawa> | ||||||||||
Component: | SVG | Assignee: | Said Abou-Hallawa <sabouhallawa> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, darin, webkit-bug-importer, zimmermann | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Said Abou-Hallawa
2015-02-25 17:45:03 PST
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. |