Summary: | Convert OwnPtr to std::unique_ptr in WebCore/graphics/ | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||
Component: | New Bugs | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, dino, kondapallykalyan, noam, roger_fong, sergio, simon.fraser | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | 157749 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2014-12-28 02:13:58 PST
Created attachment 243780 [details]
Patch
Created attachment 243781 [details]
Patch
Comment on attachment 243781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243781&action=review > Source/WebCore/platform/graphics/GlyphMetricsMap.h:114 > return page; > } else > - m_pages = adoptPtr(new HashMap<int, OwnPtr<GlyphMetricsPage>>); > + m_pages = std::make_unique<HashMap<int, std::unique_ptr<GlyphMetricsPage>>>(); > page = new GlyphMetricsPage; > - m_pages->set(pageNumber, adoptPtr(page)); > + m_pages->set(pageNumber, std::unique_ptr<GlyphMetricsPage>(page)); We are really trying to avoid this idiom where we adopt by invoking the std::unique_ptr constructor. This function also does double hash table lookup and it need not do that. I suggest this code instead: if (!m_pages) m_pages = std::make_unique<HashMap<int, std::unique_ptr<GlyphMetricsPage>>>(); auto& pageInMap = m_pages->add(pageNumber, nullptr).iterator->value; if (!pageInMap) pageInMap = std::make_unique<GlyphMetricsPage>(); page = pageInMap; This will set up the page in the map with only a single hash table lookup. > Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:345 > + m_children.set(fontData, std::unique_ptr<GlyphPageTreeNode>(child)); Again, this is not the idiom we want. Instead we should put the GlyphPageTreeNode into a local variable that is already a unique_ptr. One way to do this would be to have both a unique_ptr and a raw pointer pointing at the same thing. But even better, it’s possible to rearrange this function so it won’t have a raw pointer and also eliminate the double hashing it does. We would figure out where the child goes right at the start, replacing the "foundChild" code. auto& child = fontData ? m_children.add(fontData, nullptr).iterator->value : m_systemFallbackChild; if (child) return child.get(); child = std::make_unique<GlyphPageTreeNode>(); ... return child.get(); The other code in the function would stay the same. > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.h:40 > + explicit Extensions3DOpenGL(GraphicsContext3D*); This argument should really be a reference rather than a pointer. But no need to change it right now. > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.h:61 > // This class only needs to be instantiated by GraphicsContext3D implementations. I don’t think this comment makes sense any more. > Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.h:62 > friend class GraphicsContext3D; I don’t think this line of code is helpful any more. Comment on attachment 243781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243781&action=review >> Source/WebCore/platform/graphics/opengl/Extensions3DOpenGL.h:61 >> // This class only needs to be instantiated by GraphicsContext3D implementations. > > I don’t think this comment makes sense any more. The comment *might* have some value, but it belongs up at the constructor, I think. Created attachment 243788 [details]
Patch
Comment on attachment 243781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243781&action=review >> Source/WebCore/platform/graphics/GlyphMetricsMap.h:114 >> + m_pages->set(pageNumber, std::unique_ptr<GlyphMetricsPage>(page)); > > We are really trying to avoid this idiom where we adopt by invoking the std::unique_ptr constructor. This function also does double hash table lookup and it need not do that. I suggest this code instead: > > if (!m_pages) > m_pages = std::make_unique<HashMap<int, std::unique_ptr<GlyphMetricsPage>>>(); > auto& pageInMap = m_pages->add(pageNumber, nullptr).iterator->value; > if (!pageInMap) > pageInMap = std::make_unique<GlyphMetricsPage>(); > page = pageInMap; > > This will set up the page in the map with only a single hash table lookup. Thank you for your suggestion. New patch uses this code. >> Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:345 >> + m_children.set(fontData, std::unique_ptr<GlyphPageTreeNode>(child)); > > Again, this is not the idiom we want. Instead we should put the GlyphPageTreeNode into a local variable that is already a unique_ptr. > > One way to do this would be to have both a unique_ptr and a raw pointer pointing at the same thing. > > But even better, it’s possible to rearrange this function so it won’t have a raw pointer and also eliminate the double hashing it does. We would figure out where the child goes right at the start, replacing the "foundChild" code. > > auto& child = fontData ? m_children.add(fontData, nullptr).iterator->value : m_systemFallbackChild; > if (child) > return child.get(); > > child = std::make_unique<GlyphPageTreeNode>(); > > ... > > return child.get(); > > The other code in the function would stay the same. I try to apply your second suggestion to new patch. Thanks. Comment on attachment 243788 [details] Patch Clearing flags on attachment: 243788 Committed r177786: <http://trac.webkit.org/changeset/177786> All reviewed patches have been landed. Closing bug. This commit made a build break on win port. I landed two fixes for win port though, second fix is not tested on win bot yet. https://trac.webkit.org/changeset/177787 https://trac.webkit.org/changeset/177788 Comment on attachment 243788 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243788&action=review > Source/WebCore/platform/graphics/GlyphMetricsMap.h:-110 > - return page; Previously, there was an early return here, which meant that we would only do the for loop below (the one initializing the page with unknown metrics) if the page is new. After your change, we always fill the page with unknown metrics, even if it is an existing page :/ I will fix. |