A host of micro-optimizations and a refactor of TimeForBoundsToIntersect

A host of micro-optimizations and a refactor of TimeForBoundsToIntersect

I used perf to profile the compositor thread and addressed performance problems
with the top fuctions.  The result is about a 2.5x performance boost.
(~1.3ms -> ~.5ms) on List of Pokemon for UpdateTilePriorities.

Most of the optimizations are inlining small functions.  The refactor of
TimeForBoundsToIntersect also eliminates many unnecessary float-double
conversions.


BUG=172406
NOTRY=true


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179770

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180229

[danakj, 2013-01-28 22:43:21]

ui/gfx/ LGTM

[enne (OOO), 2013-01-28 22:55:10]

https://codereview.chromium.org/12084031/diff/1/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12084031/diff/1/cc/picture_layer_tiling.cc#ne...
cc/picture_layer_tiling.cc:297: for (TileMap::const_iterator it =
tiles_.begin();
This was already applied in your last UpdateTileProperties patch, right?

[enne (OOO), 2013-01-30 06:53:55]

Please add a comment when you have updated a patch.  I am not trying to be rude,
but this is the third time in a row I have asked.

https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:7: #include "base/debug/trace_event.h"
Please don't add this unnecessary include.

https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:336: priority.distance_to_visible_in_pixels =
Please don't create unnecessary diff churn by moving things around.

[tfarina, 2013-01-30 14:55:02]

https://codereview.chromium.org/12084031/diff/5001/ui/gfx/rect_f.h
File ui/gfx/rect_f.h (right):

https://codereview.chromium.org/12084031/diff/5001/ui/gfx/rect_f.h#newcode92
ui/gfx/rect_f.h:92: r.x() * x_scale, r.y() * y_scale,
nit: this does fit above, so, please move it above.

[whunt, 2013-01-30 21:07:33]

https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_impl.cc
File cc/picture_layer_impl.cc (right):

https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_impl.cc#n...
cc/picture_layer_impl.cc:7: #include "base/debug/trace_event.h"
On 2013/01/30 06:53:56, enne wrote:
> Please don't add this unnecessary include.

No idea how that got there.  I'm not even sure what that header does.  Probably
part of the merge, I'll remove it.

https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:336: priority.distance_to_visible_in_pixels =
On 2013/01/30 06:53:56, enne wrote:
> Please don't create unnecessary diff churn by moving things around.

This is a performance optimization.

[enne (OOO), 2013-01-30 21:50:12]

https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_tiling.cc
File cc/picture_layer_tiling.cc (right):

https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_tiling.cc...
cc/picture_layer_tiling.cc:336: priority.distance_to_visible_in_pixels =
On 2013/01/30 21:07:33, whunt wrote:
> On 2013/01/30 06:53:56, enne wrote:
> > Please don't create unnecessary diff churn by moving things around.
> 
> This is a performance optimization.

Could you explain?

[enne (OOO), 2013-01-30 22:11:27]

lgtm

[commit-bot: I haz the power, 2013-01-30 22:17:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/12084031/22011

[whunt, 2013-01-30 23:39:23]

On 2013/01/30 21:50:12, enne wrote:
> https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_tiling.cc
> File cc/picture_layer_tiling.cc (right):
> 
>
https://codereview.chromium.org/12084031/diff/5001/cc/picture_layer_tiling.cc...
> cc/picture_layer_tiling.cc:336: priority.distance_to_visible_in_pixels =
> On 2013/01/30 21:07:33, whunt wrote:
> > On 2013/01/30 06:53:56, enne wrote:
> > > Please don't create unnecessary diff churn by moving things around.
> > 
> > This is a performance optimization.
> 
> Could you explain?

Yes, sorry for the terse reply at first.  The Manhattan distance function is
inlined but the second call isn't.  There's a flush at the call boundary for
those values.

[commit-bot: I haz the power, 2013-01-31 02:09:36]

Change committed as 179770

[enne (OOO), 2013-02-01 03:35:56]

lgtm

[commit-bot: I haz the power, 2013-02-01 03:38:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/12084031/5003

[commit-bot: I haz the power, 2013-02-01 03:38:34]

Failed to apply patch for cc/picture_layer_tiling.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/picture_layer_tiling.cc
  Hunk #2 FAILED at 329.
  Hunk #3 succeeded at 426 (offset 31 lines).
  Hunk #4 succeeded at 463 (offset 31 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
cc/picture_layer_tiling.cc.rej

Patch:       cc/picture_layer_tiling.cc
Index: cc/picture_layer_tiling.cc
diff --git a/cc/picture_layer_tiling.cc b/cc/picture_layer_tiling.cc
index
a8000ecfce972f69b76d48aee4d1ea710360c708..185c609dbedb46b7c24b82c7f20cffe5f9e873d5
100644
--- a/cc/picture_layer_tiling.cc
+++ b/cc/picture_layer_tiling.cc
@@ -4,6 +4,7 @@
 
 #include "cc/picture_layer_tiling.h"
 
+#include "base/debug/trace_event.h"
 #include "cc/math_util.h"
 #include "ui/gfx/point_conversions.h"
 #include "ui/gfx/rect_conversions.h"
@@ -328,6 +329,7 @@ void PictureLayerTiling::UpdateTilePriorities(
     const gfx::Transform& last_screen_transform,
     const gfx::Transform& current_screen_transform,
     double time_delta) {
+  TRACE_EVENT0("cc", "PictureLayerTiling::UpdateTilePriorities");
   gfx::Rect content_rect = ContentRect();
   if (content_rect.IsEmpty())
     return;
@@ -394,12 +396,12 @@ void PictureLayerTiling::UpdateTilePriorities(
           last_scale,
           last_scale) + last_offset;
 
-      float time_to_visible_in_seconds =
-          TilePriority::TimeForBoundsToIntersect(
-              last_screen_rect, current_screen_rect, time_delta, view_rect);
       float distance_to_visible_in_pixels =
           TilePriority::manhattanDistance(current_screen_rect, view_rect);
 
+      float time_to_visible_in_seconds =
+          TilePriority::TimeForBoundsToIntersect(
+              last_screen_rect, current_screen_rect, time_delta, view_rect);
       TilePriority priority(
           resolution_,
           time_to_visible_in_seconds,
@@ -431,11 +433,12 @@ void PictureLayerTiling::UpdateTilePriorities(
       gfx::RectF last_screen_rect  = MathUtil::mapClippedRect(
           last_screen_transform, last_layer_content_rect);
 
+      float distance_to_visible_in_pixels =
+          TilePriority::manhattanDistance(current_screen_rect, view_rect);
+
       float time_to_visible_in_seconds =
           TilePriority::TimeForBoundsToIntersect(
               last_screen_rect, current_screen_rect, time_delta, view_rect);
-      float distance_to_visible_in_pixels =
-          TilePriority::manhattanDistance(current_screen_rect, view_rect);
 
       TilePriority priority(
           resolution_,

[commit-bot: I haz the power, 2013-02-01 18:24:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/12084031/5003

[commit-bot: I haz the power, 2013-02-01 18:24:27]

Failed to apply patch for cc/picture_layer_tiling.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/picture_layer_tiling.cc
  Hunk #2 FAILED at 329.
  Hunk #3 succeeded at 426 (offset 31 lines).
  Hunk #4 succeeded at 463 (offset 31 lines).
  1 out of 4 hunks FAILED -- saving rejects to file
cc/picture_layer_tiling.cc.rej

Patch:       cc/picture_layer_tiling.cc
Index: cc/picture_layer_tiling.cc
diff --git a/cc/picture_layer_tiling.cc b/cc/picture_layer_tiling.cc
index
a8000ecfce972f69b76d48aee4d1ea710360c708..185c609dbedb46b7c24b82c7f20cffe5f9e873d5
100644
--- a/cc/picture_layer_tiling.cc
+++ b/cc/picture_layer_tiling.cc
@@ -4,6 +4,7 @@
 
 #include "cc/picture_layer_tiling.h"
 
+#include "base/debug/trace_event.h"
 #include "cc/math_util.h"
 #include "ui/gfx/point_conversions.h"
 #include "ui/gfx/rect_conversions.h"
@@ -328,6 +329,7 @@ void PictureLayerTiling::UpdateTilePriorities(
     const gfx::Transform& last_screen_transform,
     const gfx::Transform& current_screen_transform,
     double time_delta) {
+  TRACE_EVENT0("cc", "PictureLayerTiling::UpdateTilePriorities");
   gfx::Rect content_rect = ContentRect();
   if (content_rect.IsEmpty())
     return;
@@ -394,12 +396,12 @@ void PictureLayerTiling::UpdateTilePriorities(
           last_scale,
           last_scale) + last_offset;
 
-      float time_to_visible_in_seconds =
-          TilePriority::TimeForBoundsToIntersect(
-              last_screen_rect, current_screen_rect, time_delta, view_rect);
       float distance_to_visible_in_pixels =
           TilePriority::manhattanDistance(current_screen_rect, view_rect);
 
+      float time_to_visible_in_seconds =
+          TilePriority::TimeForBoundsToIntersect(
+              last_screen_rect, current_screen_rect, time_delta, view_rect);
       TilePriority priority(
           resolution_,
           time_to_visible_in_seconds,
@@ -431,11 +433,12 @@ void PictureLayerTiling::UpdateTilePriorities(
       gfx::RectF last_screen_rect  = MathUtil::mapClippedRect(
           last_screen_transform, last_layer_content_rect);
 
+      float distance_to_visible_in_pixels =
+          TilePriority::manhattanDistance(current_screen_rect, view_rect);
+
       float time_to_visible_in_seconds =
           TilePriority::TimeForBoundsToIntersect(
               last_screen_rect, current_screen_rect, time_delta, view_rect);
-      float distance_to_visible_in_pixels =
-          TilePriority::manhattanDistance(current_screen_rect, view_rect);
 
       TilePriority priority(
           resolution_,

[commit-bot: I haz the power, 2013-02-01 18:58:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/12084031/13004

[commit-bot: I haz the power, 2013-02-01 22:53:02]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[whunt, 2013-02-02 00:51:37]

On 2013/02/01 22:53:02, I haz the power (commit-bot) wrote:
> Retried try job too often on win_rel for step(s) browser_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

I'm getting this issue on all my patches, is there a way to push it thought?

[danakj, 2013-02-02 00:52:40]

You can put NOTRY=true in your description to not run tests.

[commit-bot: I haz the power, 2013-02-02 01:11:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/whunt@chromium.org/12084031/13004

[commit-bot: I haz the power, 2013-02-02 01:52:01]

Change committed as 180229