Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
79173005:
Improve DamageTracker performance. (Closed)
|
Created:
7 years, 1 month ago by ostap Modified:
7 years ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImprove DamageTracker performance.
Use single rect_history_ map for layer damage computation with extra
boolean per region to mark regions for layers that were deleted.
Reduces number of map search ~3 times and insertions/deletions ~5 times
(on the mobile.pcmag.com page).
DamageTracker unit test passes.
BUG=316180
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237174
Patch Set 1 #
Total comments: 15
Patch Set 2 : Update by review comments. #Patch Set 3 : hash_map is replaced by sorted vector #
Total comments: 6
Patch Set 4 : Updated by review comments. #Patch Set 5 : Fix typos in comments. #Patch Set 6 : Fix clang compile issues. #Messages
Total messages: 16 (0 generated)
Don't know who should review this. Git cl script suggested David or Dana.
+shawnsingh
some style nits, but shawnsingh is the right person to review this https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:229: if (it->second.updated_) {} here, since else has them https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:354: &replica_is_new); indenting off, git cl format please https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.h File cc/trees/damage_tracker.h (right): https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.h#new... cc/trees/damage_tracker.h:63: struct RectMapData { formatting is off, please run git cl format on the CL.
Two main comments - one about mailboxing, the other about template containers. Other comments are just minor nits. I like this idea a lot =) Hopefully we can find a way to address my template container concerns without defeating the purpose of this optimization... https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:106: // - To correctly manage exposed rects, two RectMaps are maintained: "two RectMaps" -- this line of comment should also be updated. https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:108: // 1. All existing from the previous fram regions in map history are Some typos and clarifications - let's say "All existing rects from the previous frame are marked as not updated" https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:160: *layer_is_new = iter == rect_history_.end(); For clarity here let's use parentheses around the boolean condition. https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:211: void DamageTracker::PrepareRectHistoryForUpdate() { Let's use an integer RectMapData::mailboxId_ instead of a boolean RectMapData.updated_ - that way, we can avoid this entire loop that exists simply to invalidate each rect. Instead, incrementing the mailbox ID would be equivalent to automatically dirtying all entries in the map. https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:235: rect_history_.erase(erase_it); I don't know about base::HashMap, but for std:: containers, we cannot continue to use an iterator after adding/removing elements of a list. There is a good chance we will want to change container types some time for crbug.com/230050 - probably a std::vector, so unfortunately I think this code needs to change. Right now I can't think of how to implement this more robustly without adding more queries to the container. Let's discuss ideas if you have any =)
https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:106: // - To correctly manage exposed rects, two RectMaps are maintained: On 2013/11/21 09:56:03, shawnsingh wrote: > > "two RectMaps" -- this line of comment should also be updated. Done. https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:108: // 1. All existing from the previous fram regions in map history are On 2013/11/21 09:56:03, shawnsingh wrote: > > Some typos and clarifications - let's say "All existing rects from the previous > frame are marked as not updated" Done. https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:160: *layer_is_new = iter == rect_history_.end(); On 2013/11/21 09:56:03, shawnsingh wrote: > For clarity here let's use parentheses around the boolean condition. Done. https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:229: if (it->second.updated_) On 2013/11/21 00:13:40, danakj wrote: > {} here, since else has them Done. https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:235: rect_history_.erase(erase_it); On 2013/11/21 09:56:03, shawnsingh wrote: > > I don't know about base::HashMap, but for std:: containers, we cannot continue > to use an iterator after adding/removing elements of a list. There is a good > chance we will want to change container types some time for crbug.com/230050 - > probably a std::vector, so unfortunately I think this code needs to change. > > Right now I can't think of how to implement this more robustly without adding > more queries to the container. Let's discuss ideas if you have any =) Done. https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.cc#ne... cc/trees/damage_tracker.cc:354: &replica_is_new); On 2013/11/21 00:13:40, danakj wrote: > indenting off, git cl format please Done. https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.h File cc/trees/damage_tracker.h (right): https://codereview.chromium.org/79173005/diff/1/cc/trees/damage_tracker.h#new... cc/trees/damage_tracker.h:63: struct RectMapData { On 2013/11/21 00:13:40, danakj wrote: > formatting is off, please run git cl format on the CL. Great! Didn't know about "cl format"! Done
I have played a bit more with this and it appears that for small number of elements (less than 100) binary search in sorted vector is always better than hash_map. Inserts and deletes are also better with sorted vector in this case because layer ids are coming incrementally and layers are deleted in groups.
On 2013/11/22 21:15:57, ostap wrote: > I have played a bit more with this and it appears that for small number of > elements (less than 100) binary search in sorted vector is always better than > hash_map. Inserts and deletes are also better with sorted vector in this case > because layer ids are coming incrementally and layers are deleted in groups. Will take a look soon - in the meantime can you please give more details about the performance numbers you have measured?
On 2013/11/22 21:19:55, shawnsingh wrote: > On 2013/11/22 21:15:57, ostap wrote: > > I have played a bit more with this and it appears that for small number of > > elements (less than 100) binary search in sorted vector is always better than > > hash_map. Inserts and deletes are also better with sorted vector in this case > > because layer ids are coming incrementally and layers are deleted in groups. > > Will take a look soon - in the meantime can you please give more details about > the performance numbers you have measured? Just simple run of DamageTracker unit tests 1000 times: without change: time ./cc_unittests --gtest_filter=DamageTrackerTest* --gtest_repeat=1000 >/dev/null real 0m7.198s user 0m7.156s sys 0m0.036s with change: time ./cc_unittests --gtest_filter=DamageTrackerTest* --gtest_repeat=1000 >/dev/null real 0m6.388s user 0m6.360s sys 0m0.024s I'm looking how to add perftest for this, because unittests do lots of IO and other things.
On 2013/11/22 21:19:55, shawnsingh wrote: > On 2013/11/22 21:15:57, ostap wrote: > Will take a look soon - in the meantime can you please give more details about > the performance numbers you have measured? Effect is also visible on the existing LayerTreeHost perf tests. I ran them like: ./cc_perftests --gtest_filter=LayerTreeHost* Without patch: Note: Google Test filter = LayerTreeHost* [==========] Running 7 tests from 2 test cases. [----------] Global test environment set-up. [----------] 5 tests from LayerTreeHostPerfTestJsonReader [ RUN ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread *RESULT layer_tree_host_frame_time: 10_10_single_thread= 1734.0068359375 us [ OK ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread (2092 ms) [ RUN ] LayerTreeHostPerfTestJsonReader.TenTenThreadedImplSide *RESULT layer_tree_host_frame_time: 10_10_threaded_impl_side= 764.348876953125 us [ OK ] LayerTreeHostPerfTestJsonReader.TenTenThreadedImplSide (2086 ms) [ RUN ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread_FullDamageEachFrame *RESULT layer_tree_host_frame_time: 10_10_single_thread_full_damage_each_frame= 2555.91015625 us [ OK ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread_FullDamageEachFrame (2085 ms) [ RUN ] LayerTreeHostPerfTestJsonReader.TenTenThreadedImplSide_FullDamageEachFrame *RESULT layer_tree_host_frame_time: 10_10_threaded_impl_side_full_damage_each_frame= 1526.359130859375 us [ OK ] LayerTreeHostPerfTestJsonReader.TenTenThreadedImplSide_FullDamageEachFrame (2094 ms) [ RUN ] LayerTreeHostPerfTestJsonReader.HeavyPageThreadedImplSide *RESULT layer_tree_host_frame_time: heavy_page= 5348.681640625 us *RESULT layer_tree_host_commit_time: heavy_page= 3094.59326171875 us [ OK ] LayerTreeHostPerfTestJsonReader.HeavyPageThreadedImplSide (2268 ms) [----------] 5 tests from LayerTreeHostPerfTestJsonReader (10628 ms total) [----------] 2 tests from LayerTreeHostPerfTestLeafInvalidates [ RUN ] LayerTreeHostPerfTestLeafInvalidates.TenTenSingleThread *RESULT layer_tree_host_frame_time: 10_10_single_thread_leaf_invalidates= 2514.739990234375 us [ OK ] LayerTreeHostPerfTestLeafInvalidates.TenTenSingleThread (2079 ms) [ RUN ] LayerTreeHostPerfTestLeafInvalidates.TenTenThreadedImplSide *RESULT layer_tree_host_frame_time: 10_10_threaded_impl_side_leaf_invalidates= 758.6197509765625 us [ OK ] LayerTreeHostPerfTestLeafInvalidates.TenTenThreadedImplSide (2072 ms) [----------] 2 tests from LayerTreeHostPerfTestLeafInvalidates (4152 ms total) [----------] Global test environment tear-down [==========] 7 tests from 2 test cases ran. (14780 ms total) [ PASSED ] 7 tests. With patch: Note: Google Test filter = LayerTreeHost* [==========] Running 7 tests from 2 test cases. [----------] Global test environment set-up. [----------] 5 tests from LayerTreeHostPerfTestJsonReader [ RUN ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread *RESULT layer_tree_host_frame_time: 10_10_single_thread= 1643.525390625 us [ OK ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread (2070 ms) [ RUN ] LayerTreeHostPerfTestJsonReader.TenTenThreadedImplSide *RESULT layer_tree_host_frame_time: 10_10_threaded_impl_side= 662.5145874023438 us [ OK ] LayerTreeHostPerfTestJsonReader.TenTenThreadedImplSide (2087 ms) [ RUN ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread_FullDamageEachFrame *RESULT layer_tree_host_frame_time: 10_10_single_thread_full_damage_each_frame= 2443.28662109375 us [ OK ] LayerTreeHostPerfTestJsonReader.TenTenSingleThread_FullDamageEachFrame (2066 ms) [ RUN ] LayerTreeHostPerfTestJsonReader.TenTenThreadedImplSide_FullDamageEachFrame *RESULT layer_tree_host_frame_time: 10_10_threaded_impl_side_full_damage_each_frame= 1434.452880859375 us [ OK ] LayerTreeHostPerfTestJsonReader.TenTenThreadedImplSide_FullDamageEachFrame (2083 ms) [ RUN ] LayerTreeHostPerfTestJsonReader.HeavyPageThreadedImplSide *RESULT layer_tree_host_frame_time: heavy_page= 5209.6103515625 us *RESULT layer_tree_host_commit_time: heavy_page= 2988.524169921875 us [ OK ] LayerTreeHostPerfTestJsonReader.HeavyPageThreadedImplSide (2265 ms) [----------] 5 tests from LayerTreeHostPerfTestJsonReader (10571 ms total) [----------] 2 tests from LayerTreeHostPerfTestLeafInvalidates [ RUN ] LayerTreeHostPerfTestLeafInvalidates.TenTenSingleThread *RESULT layer_tree_host_frame_time: 10_10_single_thread_leaf_invalidates= 2412.4794921875 us [ OK ] LayerTreeHostPerfTestLeafInvalidates.TenTenSingleThread (2067 ms) [ RUN ] LayerTreeHostPerfTestLeafInvalidates.TenTenThreadedImplSide *RESULT layer_tree_host_frame_time: 10_10_threaded_impl_side_leaf_invalidates= 662.0650024414062 us [ OK ] LayerTreeHostPerfTestLeafInvalidates.TenTenThreadedImplSide (2072 ms) [----------] 2 tests from LayerTreeHostPerfTestLeafInvalidates (4139 ms total) [----------] Global test environment tear-down [==========] 7 tests from 2 test cases ran. (14711 ms total) [ PASSED ] 7 tests.
Looking really great, and it's very satisfying to see the perf improvements =) I'm predicting this is the last round of comments, and will be ready to go after that. https://codereview.chromium.org/79173005/diff/180001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/79173005/diff/180001/cc/trees/damage_tracker.... cc/trees/damage_tracker.cc:164: SortedRectMap::iterator it = lower_bound(rect_history_.begin(), let's explicitly write std::lower_bound here, and everywhere else we're using std:: algorithm stuff. https://codereview.chromium.org/79173005/diff/180001/cc/trees/damage_tracker.... cc/trees/damage_tracker.cc:229: SortedRectMap::iterator copy_pos; Something doesn't seem right here, copy_pos could be uninitialized when it gets tested for equality and incremented below. I understand that the intention of this code is to find layers who were outdated and remove them. But I am finding it hard to decipher how this code actually does that. Could you please add a quick comment inside the for-loop explaining what each iteration does, and if possible, use static inlined helper functions whose names can help self-document the code, too? https://codereview.chromium.org/79173005/diff/180001/cc/trees/damage_tracker.h File cc/trees/damage_tracker.h (right): https://codereview.chromium.org/79173005/diff/180001/cc/trees/damage_tracker.... cc/trees/damage_tracker.h:76: unsigned int mailboxId_; Let's declare mailboxId_ above rect_, on the very tiny chance that the data alignment helps in some cases.
https://codereview.chromium.org/79173005/diff/180001/cc/trees/damage_tracker.cc File cc/trees/damage_tracker.cc (right): https://codereview.chromium.org/79173005/diff/180001/cc/trees/damage_tracker.... cc/trees/damage_tracker.cc:164: SortedRectMap::iterator it = lower_bound(rect_history_.begin(), On 2013/11/23 01:32:01, shawnsingh wrote: > let's explicitly write std::lower_bound here, and everywhere else we're using > std:: algorithm stuff. Done. https://codereview.chromium.org/79173005/diff/180001/cc/trees/damage_tracker.... cc/trees/damage_tracker.cc:229: SortedRectMap::iterator copy_pos; On 2013/11/23 01:32:01, shawnsingh wrote: > Something doesn't seem right here, copy_pos could be uninitialized when it gets > tested for equality and incremented below. copy_pos is always initialized to rect_history_.begin() . Probably I should rewrite the assignment. > I understand that the intention of this code is to find layers who were outdated > and remove them. But I am finding it hard to decipher how this code actually > does that. Could you please add a quick comment inside the for-loop explaining > what each iteration does, and if possible, use static inlined helper functions > whose names can help self-document the code, too? Done I changed it a bit to look more like std::remove_if . Might be it is easier to understand this way. https://codereview.chromium.org/79173005/diff/180001/cc/trees/damage_tracker.h File cc/trees/damage_tracker.h (right): https://codereview.chromium.org/79173005/diff/180001/cc/trees/damage_tracker.... cc/trees/damage_tracker.h:76: unsigned int mailboxId_; On 2013/11/23 01:32:01, shawnsingh wrote: > Let's declare mailboxId_ above rect_, on the very tiny chance that the data > alignment helps in some cases. Done.
LGTM I understand the loop now, thanks for all the adjustments.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/79173005/290001
Retried try job too often on android_clang_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sl.ostapenko@samsung.com/79173005/310001
Message was sent while issue was closed.
Change committed as 237174 |
