Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
379823002:
cc: skipping a horizontal sort by checking the value of vertical_density (Closed)
|
Created:
6 years, 5 months ago by hyunki Modified:
6 years, 5 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptioncc: skipping a horizontal sort by checking the value of vertical_density
When vertical_density is 1,
the value of horizontal_density from do_clustering() should be equal to 1.
Thus, it skips the horizontal sort process in this case.
When I checked logs in google.com and www.naver.com,
this condition (vertical_density equals to 1) is met
for more than 70% of cases.
BUG=312861
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282317
Patch Set 1 #Patch Set 2 : #Patch Set 3 : remove TODO comment line #
Total comments: 4
Patch Set 4 : address nits #
Total comments: 2
Patch Set 5 : Add a commnet to explain why early return can be possible #Patch Set 6 : Rebase #Patch Set 7 : Rebase #Messages
Total messages: 21 (0 generated)
PTAL. Thanks. :-)
https://codereview.chromium.org/379823002/diff/40001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/379823002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:119: if (vertical_density == 1) { nit: 1.f https://codereview.chromium.org/379823002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:125: std::vector<gfx::Rect> invalid_tiles_horizontal = invalid_tiles; Is this used? I think this might have been a typo, but can you fix that while you're here? And possibly rename invalid_tiles_vertical to something like invalid_tiles_copy. Either that or use invalid_tiles_horizontal below, whichever is more efficient.
Thanks. I updated two comments. PTAL. https://codereview.chromium.org/379823002/diff/40001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/379823002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:119: if (vertical_density == 1) { On 2014/07/09 23:29:11, vmpstr wrote: > nit: 1.f Done. https://codereview.chromium.org/379823002/diff/40001/cc/resources/picture_pil... cc/resources/picture_pile.cc:125: std::vector<gfx::Rect> invalid_tiles_horizontal = invalid_tiles; On 2014/07/09 23:29:11, vmpstr wrote: > Is this used? I think this might have been a typo, but can you fix that while > you're here? Sure. I think it is used. And possibly rename invalid_tiles_vertical to something like > invalid_tiles_copy. Either that or use invalid_tiles_horizontal below, whichever > is more efficient. I have changed it to invalid_tiles_horizontal. Done.
lgtm with nit https://codereview.chromium.org/379823002/diff/60001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/379823002/diff/60001/cc/resources/picture_pil... cc/resources/picture_pile.cc:119: if (vertical_density == 1.f) { nit: Add a comment here please explaining that if vertical density is optimal, then we can return early.
On 2014/07/09 23:47:14, vmpstr wrote: > lgtm with nit > > https://codereview.chromium.org/379823002/diff/60001/cc/resources/picture_pil... > File cc/resources/picture_pile.cc (right): > > https://codereview.chromium.org/379823002/diff/60001/cc/resources/picture_pil... > cc/resources/picture_pile.cc:119: if (vertical_density == 1.f) { > nit: Add a comment here please explaining that if vertical density is optimal, > then we can return early. Sure. Thanks for the review.
https://codereview.chromium.org/379823002/diff/60001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/379823002/diff/60001/cc/resources/picture_pil... cc/resources/picture_pile.cc:119: if (vertical_density == 1.f) { On 2014/07/09 23:47:14, vmpstr wrote: > nit: Add a comment here please explaining that if vertical density is optimal, > then we can return early. Done.
The CQ bit was checked by hyunki.baik@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hyunki.baik@samsung.com/379823002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for cc/resources/picture_pile.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file cc/resources/picture_pile.cc
Hunk #1 FAILED at 116.
1 out of 1 hunk FAILED -- saving rejects to file
cc/resources/picture_pile.cc.rej
Patch: cc/resources/picture_pile.cc
Index: cc/resources/picture_pile.cc
diff --git a/cc/resources/picture_pile.cc b/cc/resources/picture_pile.cc
index
6341e88b3688889df468543cecf2c652136f20c9..52b6b0642f03089f504cefeddc19c666d37b70ad
100644
--- a/cc/resources/picture_pile.cc
+++ b/cc/resources/picture_pile.cc
@@ -116,16 +116,21 @@ float ClusterTiles(const std::vector<gfx::Rect>&
invalid_tiles,
vertical_density = do_clustering(invalid_tiles_vertical,
&vertical_clustering);
+ // If vertical density is optimal, then we can return early.
+ if (vertical_density == 1.f) {
+ *record_rects = vertical_clustering;
+ return vertical_density;
+ }
+
// Now try again with a horizontal sort, see which one is best
- // TODO(humper): Heuristics for skipping this step?
std::vector<gfx::Rect> invalid_tiles_horizontal = invalid_tiles;
- std::sort(invalid_tiles_vertical.begin(),
- invalid_tiles_vertical.end(),
+ std::sort(invalid_tiles_horizontal.begin(),
+ invalid_tiles_horizontal.end(),
rect_sort_x);
float horizontal_density;
std::vector<gfx::Rect> horizontal_clustering;
- horizontal_density = do_clustering(invalid_tiles_vertical,
+ horizontal_density = do_clustering(invalid_tiles_horizontal,
&horizontal_clustering);
if (vertical_density < horizontal_density) {
The CQ bit was checked by hyunki.baik@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hyunki.baik@samsung.com/379823002/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win8_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26863) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/31302)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/26868)
The CQ bit was checked by hyunki.baik@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hyunki.baik@samsung.com/379823002/120001
The CQ bit was unchecked by hyunki.baik@samsung.com
The CQ bit was checked by hyunki.baik@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hyunki.baik@samsung.com/379823002/120001
Message was sent while issue was closed.
Change committed as 282317 |
