Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
213093003:
cc: Skip analysis for narrow layers. (Closed)
|
Created:
6 years, 9 months ago by vmpstr Modified:
6 years, 8 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Skip analysis for narrow layers.
This patch ensures that we skip analysis for narrow layers. These layers
are unlikely to have solid color, and we shouldn't spend time analyzing
them. Additionally, we need to be cautious and set has_text to be true
by default, since for tiles that skipped analysis, we have to assume
that they have text.
BUG=341700
R=reveman@chromium.org, alokp@chromium.org
Patch Set 1 #
Total comments: 7
Patch Set 2 : alokp's review #
Total comments: 4
Patch Set 3 : update #
Messages
Total messages: 41 (0 generated)
Please take a look. On sky.com, this seems to skip about 25% of analysis.
https://codereview.chromium.org/213093003/diff/1/cc/resources/picture_pile_im... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/213093003/diff/1/cc/resources/picture_pile_im... cc/resources/picture_pile_impl.cc:379: PicturePileImpl::Analysis::Analysis() : is_solid_color(false), has_text(true) {} Please add a comment why the default value is true. https://codereview.chromium.org/213093003/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/213093003/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:36: const int kMinSizeToAnalyze = 256; kMinLengthToAnalyze? https://codereview.chromium.org/213093003/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:1171: // this tile is narrow, since more likely than not the tile would not be I do not get the connection between the tile being narrow and solid. Could you elaborate this a bit here?
PTAL https://codereview.chromium.org/213093003/diff/1/cc/resources/picture_pile_im... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/213093003/diff/1/cc/resources/picture_pile_im... cc/resources/picture_pile_impl.cc:379: PicturePileImpl::Analysis::Analysis() : is_solid_color(false), has_text(true) {} On 2014/03/27 05:20:45, Alok Priyadarshi wrote: > Please add a comment why the default value is true. Done. https://codereview.chromium.org/213093003/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/213093003/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:36: const int kMinSizeToAnalyze = 256; On 2014/03/27 05:20:45, Alok Priyadarshi wrote: > kMinLengthToAnalyze? Done. https://codereview.chromium.org/213093003/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:1171: // this tile is narrow, since more likely than not the tile would not be On 2014/03/27 05:20:45, Alok Priyadarshi wrote: > I do not get the connection between the tile being narrow and solid. Could you > elaborate this a bit here? It definitely doesn't mean that it's not solid, but I think we want to try a heuristic that just assumes they are not. The example is a page in the bug that has a lot of small layers, which means we spent all that extra time analyzing it. For the majority of the cases (1 root layer, 3 scrollbar layers), this should do the right thing... Did you want me to add a more elaborate comment here, or is your concern more with the whole idea behind this?
lgtm https://codereview.chromium.org/213093003/diff/1/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/213093003/diff/1/cc/resources/tile_manager.cc... cc/resources/tile_manager.cc:1171: // this tile is narrow, since more likely than not the tile would not be > Did you want me to add a more elaborate comment here, or is your concern more > with the whole idea behind this? I understand that it is just a heuristic and I am fine with it. I just wanted a comment about the justifications for that heuristic.
https://codereview.chromium.org/213093003/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/213093003/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:36: const int kMinLengthToAnalyze = 256; kMinDimensionForAnalysis? https://codereview.chromium.org/213093003/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:1176: std::min(pile_size.width(), pile_size.height()) >= kMinLengthToAnalyze; Comment mentions layer but you use the picture pile size. Are they always the same?
On 2014/03/27 17:45:16, Alok Priyadarshi wrote: > lgtm > > https://codereview.chromium.org/213093003/diff/1/cc/resources/tile_manager.cc > File cc/resources/tile_manager.cc (right): > > https://codereview.chromium.org/213093003/diff/1/cc/resources/tile_manager.cc... > cc/resources/tile_manager.cc:1171: // this tile is narrow, since more likely > than not the tile would not be > > Did you want me to add a more elaborate comment here, or is your concern more > > with the whole idea behind this? > > I understand that it is just a heuristic and I am fine with it. I just wanted a > comment about the justifications for that heuristic. I've elaborated in the comment.
PTAL https://codereview.chromium.org/213093003/diff/20001/cc/resources/tile_manage... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/213093003/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:36: const int kMinLengthToAnalyze = 256; On 2014/03/27 18:11:26, reveman wrote: > kMinDimensionForAnalysis? Done. https://codereview.chromium.org/213093003/diff/20001/cc/resources/tile_manage... cc/resources/tile_manager.cc:1176: std::min(pile_size.width(), pile_size.height()) >= kMinLengthToAnalyze; On 2014/03/27 18:11:26, reveman wrote: > Comment mentions layer but you use the picture pile size. Are they always the > same? I think it has to be the same. In any case, if it's smaller than the constant, that means the layer is smaller. I've added a comment to elaborate of that.
lgtm
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/213093003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on ios_dbg_simulator for step(s) base_unittests, components_unittests, crypto_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/213093003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/213093003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/213093003/40001
The CQ bit was unchecked by cmp@chromium.org
The CQ bit was checked by cmp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/213093003/40001
The CQ bit was unchecked by cmp@chromium.org
The CQ bit was checked by cmp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/213093003/40001
The CQ bit was unchecked by vmpstr@chromium.org
The CQ bit was checked by vmpstr@chromium.org
The CQ bit was unchecked by vmpstr@chromium.org
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/213093003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Internal error: failed to checkout. Please try again.
The CQ bit was checked by vmpstr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/213093003/40001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vmpstr@chromium.org/213093003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
On 2014/04/03 08:32:27, I haz the power (commit-bot) wrote: > Commit queue rejected this change because the description was changed > between the time the change entered the commit queue and the time it > was ready to commit. You can safely check the commit box again. moved this to https://codereview.chromium.org/224333002/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
