move intensive computing/rendering from bench constructor to onPreDraw.

When skia run bench cases to test performance, it will run constructors for all cases one by one, then getName to skip unnecessary cases according to command line parameters, so these constructors should be lightweight enough to avoid redundant computing. Unfortunately, some constructors contain intensive computing/rendering. They are very heavy, maybe much heavier than need-to-run bench case itself. And these redundant computation will be run every time you run bench, even you just test a single simple case. Moreover, it will mislead the real hotspot/bottleneck of the case itself. 
For example, run a lightweight case, say, region_intersectsrgn_16, the hot spots are gles operation, SuperBlitter, SkRTree... introduced by irrelevant cases' constructors. These redundant computation will mislead performance tuning. 
So we can move these intensive computation to onPreDraw() of these case. They will be executed only if this case should be run.

Committed: http://code.google.com/p/skia/source/detail?r=10486

[robertphillips, 2013-07-29 11:57:10]

lgtm + nits (most of them aren't yours at all but it would be nice to have them
fixed as long as you're modifying the code).

https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp
File bench/BitmapRectBench.cpp (right):

https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp#...
bench/BitmapRectBench.cpp:47: SkRect      fSrcR, fDstR;
kWidth, kHeight

https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp#...
bench/BitmapRectBench.cpp:60: protected:
SK_OVERRIDE

https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp#...
bench/BitmapRectBench.cpp:67: 
SK_OVERRIDE

https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp#...
bench/BitmapRectBench.cpp:71: fBitmap.eraseColor(SK_ColorBLACK);
rename to draw_into_bitmap

https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp#...
bench/BitmapRectBench.cpp:87: 
SK_OVERRIDE

https://codereview.chromium.org/20997003/diff/6001/bench/RTreeBench.cpp
File bench/RTreeBench.cpp (right):

https://codereview.chromium.org/20997003/diff/6001/bench/RTreeBench.cpp#newco...
bench/RTreeBench.cpp:94: protected:
SK_OVERRIDE

https://codereview.chromium.org/20997003/diff/6001/bench/RTreeBench.cpp#newco...
bench/RTreeBench.cpp:97: }
SK_OVERRIDE

https://codereview.chromium.org/20997003/diff/6001/bench/RTreeBench.cpp#newco...
bench/RTreeBench.cpp:98: virtual void onPreDraw() {
Use SkMWCRandom instead

https://codereview.chromium.org/20997003/diff/6001/bench/RTreeBench.cpp#newco...
bench/RTreeBench.cpp:106: 
SK_OVERRIDE

https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp
File bench/RepeatTileBench.cpp (right):

https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
bench/RepeatTileBench.cpp:88: SkString    fName;
line up all the member variable names

https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
bench/RepeatTileBench.cpp:90: SkBitmap::Config fConfig;
fIsOpaque

https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
bench/RepeatTileBench.cpp:109: protected:
SK_OVERRIDE

https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
bench/RepeatTileBench.cpp:113: 
SK_OVERRIDE

https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
bench/RepeatTileBench.cpp:118: 
rename to draw_into_bitmap

https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
bench/RepeatTileBench.cpp:122: SkBitmap tmp;
rename to convert_to_index666

https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
bench/RepeatTileBench.cpp:133: 
SK_OVERRIDE

[tomhudson, 2013-07-29 12:33:24]

Before we land this I wanted to dig through history, but it looks like git log
cuts off at the start of this year when we moved off of SVN - how do we go back
further in time? Should we have just kept old checkouts around?

I seem to recall about two years ago that we made an effort to move
computationally expensive things *into* the constructor. Now, that may have been
moving them *out* of onDraw(), and perhaps onPreDraw() didn't exist then - this
seems like a very good thing to do. But I'd like to understand the history.

[yunchao, 2013-07-30 03:11:20]

On 2013/07/29 11:57:10, robertphillips wrote:
> lgtm + nits (most of them aren't yours at all but it would be nice to have
them
> fixed as long as you're modifying the code).
> 
> https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp
> File bench/BitmapRectBench.cpp (right):
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp#...
> bench/BitmapRectBench.cpp:47: SkRect      fSrcR, fDstR;
> kWidth, kHeight
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp#...
> bench/BitmapRectBench.cpp:60: protected:
> SK_OVERRIDE
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp#...
> bench/BitmapRectBench.cpp:67: 
> SK_OVERRIDE
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp#...
> bench/BitmapRectBench.cpp:71: fBitmap.eraseColor(SK_ColorBLACK);
> rename to draw_into_bitmap
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/BitmapRectBench.cpp#...
> bench/BitmapRectBench.cpp:87: 
> SK_OVERRIDE
> 
> https://codereview.chromium.org/20997003/diff/6001/bench/RTreeBench.cpp
> File bench/RTreeBench.cpp (right):
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RTreeBench.cpp#newco...
> bench/RTreeBench.cpp:94: protected:
> SK_OVERRIDE
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RTreeBench.cpp#newco...
> bench/RTreeBench.cpp:97: }
> SK_OVERRIDE
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RTreeBench.cpp#newco...
> bench/RTreeBench.cpp:98: virtual void onPreDraw() {
> Use SkMWCRandom instead
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RTreeBench.cpp#newco...
> bench/RTreeBench.cpp:106: 
> SK_OVERRIDE
> 
> https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp
> File bench/RepeatTileBench.cpp (right):
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
> bench/RepeatTileBench.cpp:88: SkString    fName;
> line up all the member variable names
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
> bench/RepeatTileBench.cpp:90: SkBitmap::Config fConfig;
> fIsOpaque
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
> bench/RepeatTileBench.cpp:109: protected:
> SK_OVERRIDE
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
> bench/RepeatTileBench.cpp:113: 
> SK_OVERRIDE
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
> bench/RepeatTileBench.cpp:118: 
> rename to draw_into_bitmap
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
> bench/RepeatTileBench.cpp:122: SkBitmap tmp;
> rename to convert_to_index666
> 
>
https://codereview.chromium.org/20997003/diff/6001/bench/RepeatTileBench.cpp#...
> bench/RepeatTileBench.cpp:133: 
> SK_OVERRIDE

Yes, I didn't add/remove code, just move intensive computing code from its
constructors to onPreDraw. Then these code will not executed unless these cases
are really need to run. I followed you suggestions in these three files: lower
case with underscores separated words for static non-class fuctions name,
Sk_OVERRIDE for onGetName, onPreDraw, onDraw, etc. And use SkMWCRandom instead
of SkRandom. Please help to review it.

[yunchao, 2013-07-30 03:26:12]

On 2013/07/29 12:33:24, tomhudson wrote:
> Before we land this I wanted to dig through history, but it looks like git log
> cuts off at the start of this year when we moved off of SVN - how do we go
back
> further in time? Should we have just kept old checkouts around?
> 
> I seem to recall about two years ago that we made an effort to move
> computationally expensive things *into* the constructor. Now, that may have
been
> moving them *out* of onDraw(), and perhaps onPreDraw() didn't exist then -
this
> seems like a very good thing to do. But I'd like to understand the history.

I don't know the history, but I think constructors should be lightweight enough.
Otherwise, these heavy computation will be executed every time when we run
bench, even though sometimes we just want to run a single case with --match
parameter. They are totally redundant in this situation. Also, they will mislead
profiling and performance tuning when we found some cases show bad performance. 
And preDraw/onPreDraw function provide a mechanism to do the init/setup work
outside timer(also outside to-be-test rendering/computing). And they are not
executed if cases are not meant to test. It is a good design, we should place
intensive init code here, making constructors lightweight enough.

[tomhudson, 2013-07-30 07:16:06]

On 2013/07/30 03:26:12, yunchao wrote:

> It is a good design, we should place
> intensive init code here, making constructors lightweight enough.

Absolutely, yes, this is a good design, but I want to make the change in the
context of the history, and the git history seems to be broken for me.

[yunchao, 2013-08-01 02:18:15]

On 2013/07/30 07:16:06, tomhudson wrote:
> On 2013/07/30 03:26:12, yunchao wrote:
> 
> > It is a good design, we should place
> > intensive init code here, making constructors lightweight enough.
> 
> Absolutely, yes, this is a good design, but I want to make the change in the
> context of the history, and the git history seems to be broken for me.

tomhudson, robertphillips, humper, reed and other guys, any suggestions about
this patch?

[reed1, 2013-08-01 14:27:23]

lgtm

[robertphillips, 2013-08-01 14:39:26]

lgtm

[yunchao, 2013-08-01 15:34:33]

On 2013/08/01 14:39:26, robertphillips wrote:
> lgtm

Hi, robertphillips or other reviewers, could you help to land it if it is OK, I
don't have the privilege.

[commit-bot: I haz the power, 2013-08-01 15:36:24]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/yunchao.he@intel.com/20997003/12001

[commit-bot: I haz the power, 2013-08-01 15:58:11]

Change committed as 10486