Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(675)

Issue 15741010: Make WidgetHierarchyUpdatesSuspensionScope use swap instead of copy (Closed)

Created:
7 years, 7 months ago by dmichael (off chromium)
Modified:
7 years, 7 months ago
CC:
blink-reviews, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering
Base URL:
https://chromium.googlesource.com/chromium/blink@master
Visibility:
Public.

Description

Make WidgetHierarchyUpdatesSuspensionScope use swap instead of copy This is equivalent behavior code that is unambiguously better performance. swap is O(1) time and memory, copying a hash table is at least linear with the number of elements. (Caveat: I don't know how big that map can get, so this may not have any noticeable impact on performance). BUG= Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150931

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -2 lines) Patch
M Source/core/rendering/RenderWidget.cpp View 1 chunk +2 lines, -2 lines 2 comments Download

Messages

Total messages: 6 (0 generated)
dmichael (off chromium)
Just a really straightforward optimization I spotted. I don't know how big that map gets, ...
7 years, 7 months ago (2013-05-22 17:42:03 UTC) #1
leviw_travelin_and_unemployed
lgtm
7 years, 7 months ago (2013-05-22 21:19:35 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/15741010/1
7 years, 7 months ago (2013-05-22 21:24:11 UTC) #3
commit-bot: I haz the power
Change committed as 150931
7 years, 7 months ago (2013-05-22 23:12:43 UTC) #4
eseidel
https://codereview.chromium.org/15741010/diff/1/Source/core/rendering/RenderWidget.cpp File Source/core/rendering/RenderWidget.cpp (right): https://codereview.chromium.org/15741010/diff/1/Source/core/rendering/RenderWidget.cpp#newcode57 Source/core/rendering/RenderWidget.cpp:57: widgetNewParentMap().swap(map); This extends the lifetime of widgets in this ...
7 years, 7 months ago (2013-05-23 03:12:21 UTC) #5
dmichael (off chromium)
7 years, 7 months ago (2013-05-23 15:52:57 UTC) #6
Message was sent while issue was closed.
https://codereview.chromium.org/15741010/diff/1/Source/core/rendering/RenderW...
File Source/core/rendering/RenderWidget.cpp (right):

https://codereview.chromium.org/15741010/diff/1/Source/core/rendering/RenderW...
Source/core/rendering/RenderWidget.cpp:57: widgetNewParentMap().swap(map);
On 2013/05/23 03:12:22, eseidel wrote:
> This extends the lifetime of widgets in this map to the end of this function
> instead of being cleared immediately, no?  I'm not sure this matters... but it
> may change behavior?
Note how the old code copies the map in line 56. The map's type is:
    typedef HashMap<RefPtr<Widget>, FrameView*> WidgetToParentMap;
So the old code copied the RefPtr<Widget>s and the FrameView raw pointers to the
new local hash map, which lives until the end of scope.

When we call swap, the static map from widgetNewParentMap gets the contents of
the empty |map|, so it's the same as clearing it. |map| gets the contents of the
static map from widgetNewParentMap. Because the map just holds pointer types,
this has the same effect as copying (without the memory and time overhead of
filling a second hash map, copying ptrs, and extra refcounting). The behavior
should be equivalent. Please let me know if I missed something.

Powered by Google App Engine
This is Rietveld 408576698