Rebuild test history databases when starting up performance_ui_tests.

Rebuild test history databases when starting up performance_ui_tests.

Previously, whenever the theme or history database format were modified, a
large binary database or pak file had to be committed. This was only done
intermittently, messing up some perf expectations. Since last June, this
has added over 100 megabytes to our repository weight.

By generating the SQLite databases and Cached Theme.pak files at startup
of the one test suite that uses them, we're guaranteed that we'll always
be using the current versions and won't add upgrade costs to our
performance_ui_tests. We'll also not continuously add to new binary
resources to the permanent git archive.

BUG=111570

[Paweł Hajdan Jr., 2013-04-26 23:19:32]

Overall, great change!

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
File chrome/test/perf/generate_profile.cc (right):

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
chrome/test/perf/generate_profile.cc:63: inline float RandomFloat() {
nit: Is this "inline" needed? Why? Same below.

Just checking, I'd like to understand what's going on here.

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
chrome/test/perf/generate_profile.cc:215: base::AtExitManager exit_manager;
This probably shouldn't be here now that it's not a standalone binary.

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
chrome/test/perf/generate_profile.cc:218: PLOG(ERROR) << "Unable to create
directory " << dst_dir.value().c_str();
return false

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
chrome/test/perf/generate_profile.cc:247: printf("Writing to disk\n");
Do we need this printf now?

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
File chrome/test/perf/generate_profile.h (right):

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
chrome/test/perf/generate_profile.h:23: const base::FilePath& dst_dir);
nit: Add WARN_UNUSED_RESULT from base/compiler_specific.h .

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/perf_test.cc
File chrome/test/perf/perf_test.cc (right):

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/perf_test...
chrome/test/perf/perf_test.cc:592: void SetPathForProfileType(ProfileType
profile_type, base::FilePath path) {
I don't really like this global state here.

The question is how to transfer the state from the new test suite to the tests.

I think this code should be moved to the file where the test suite is
implemented, so that the moment of initialization of these variables is more
clear.

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/perf_test.h
File chrome/test/perf/perf_test.h (right):

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/perf_test...
chrome/test/perf/perf_test.h:151: void SetPathForProfileType(ProfileType
profile_type, base::FilePath path);
nit: Why not const base::FilePath& path?

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/performan...
File chrome/test/perf/performance_ui_tests.cc (right):

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/performan...
chrome/test/perf/performance_ui_tests.cc:29: class PerfTest : public UITestSuite
{
Class name should include TestSuite. It's not a test, but a suite.

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/performan...
chrome/test/perf/performance_ui_tests.cc:40: exit(-1);
I prefer CHECK or LOG(FATAL) over LOG + exit. Totally fine to do that here.

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/performan...
chrome/test/perf/performance_ui_tests.cc:46: if (!GenerateProfile(TOP_SITES,
20000, default_path)) {
Let's extract 20000 to a named constant.

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/performan...
chrome/test/perf/performance_ui_tests.cc:177: return PerfTest(argc, argv).Run();
This (main) should move to a separate file, like run_all_perftests.cc

One advantage of doing so is you can extend the test suite in another binary.

[Randy Smith (Not in Mondays), 2013-04-29 18:49:13]

I'm very psyched you're doing this; thank you!

Do you have specific areas you'd like reviewed by the different reviewers?  I'm
not familiar with any of the code in this CL (yes, said with full knowledge that
I was probably the last person to touch generate_profile.cc :-}), so I'm not
sure where to focus, or if I should climb the unified learning curve.

One question that may have its own answer: I don't understand
test_support_ui_runner is being used.  I had a guess that it was being used to
run the generation step once for all performance tests, but it looks like that's
being run by the test suite initializer.  What's the purpose of
test_support_ui_runner?

https://codereview.chromium.org/14273023/diff/2006/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/14273023/diff/2006/chrome/chrome_tests.gypi#n...
chrome/chrome_tests.gypi:2299: # TODO(darin): Move other UIPerfTests here.
Just checking--this todo hasn't been completed by this CL, has it?

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
File chrome/test/perf/generate_profile.h (right):

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
chrome/test/perf/generate_profile.h:19: // Generates a user profile and history
by randomly generating data and feeding
I'm made nervous by the word "randomly" in here--are you confident that the
random variation in data won't induce a random variation in loading time?  If
not, can we make it non-random data (i.e. the same data will be generated each
time)?

[Elliot Glaysher, 2013-04-29 21:41:35]

ptal

rdsmith: the point of test_support_ui_runner is to provide the main() which
kicks up the testing machinery. Previously, test_support_ui implied that the
test_supprot_ui main() function would be linked in. Now, we want to use
PerfUITestSuite/run_all_perfuitests instead so we have the profiles build.

As for who should review what, I'm not sure. It seems like most of this stuff
hasn't been touched in a very long time and you (rdsmith@) were the person who
most recently hacked on this stuff. :-/

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
File chrome/test/perf/generate_profile.cc (right):

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
chrome/test/perf/generate_profile.cc:63: inline float RandomFloat() {
On 2013/04/26 23:19:32, Paweł Hajdan Jr. wrote:
> nit: Is this "inline" needed? Why? Same below.
> 
> Just checking, I'd like to understand what's going on here.

This isn't needed...in Release builds. At -O2, gcc will inline functions
whenever it thinks its a good idea. However, we care more about Debug
performance here. This was cheap and made a few seconds difference.

https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
chrome/test/perf/generate_profile.cc:247: printf("Writing to disk\n");
On 2013/04/26 23:19:32, Paweł Hajdan Jr. wrote:
> Do we need this printf now?

I've changed this to one printf at the beginning and at the end. I kept the
incremental printfs so the buildbot scripts won't kill the process for lack of
output.

[tonyg, 2013-04-30 02:33:12]

Awesome change. Just curious if this will work with the reference builds?

[Randy Smith (Not in Mondays), 2013-04-30 15:57:28]

On 2013/04/29 21:41:35, Elliot Glaysher wrote:
> ptal
> 
> rdsmith: the point of test_support_ui_runner is to provide the main() which
> kicks up the testing machinery. Previously, test_support_ui implied that the
> test_supprot_ui main() function would be linked in. Now, we want to use
> PerfUITestSuite/run_all_perfuitests instead so we have the profiles build.
> 
> As for who should review what, I'm not sure. It seems like most of this stuff
> hasn't been touched in a very long time and you (rdsmith@) were the person who
> most recently hacked on this stuff. :-/

Ok, I'll breakout my pitons and rope and start climbing the learning curve :-}. 
This may result in something like a 1d latency, at least for the first serious
round--I hope that's ok.
> 
>
https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
> File chrome/test/perf/generate_profile.cc (right):
> 
>
https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
> chrome/test/perf/generate_profile.cc:63: inline float RandomFloat() {
> On 2013/04/26 23:19:32, Paweł Hajdan Jr. wrote:
> > nit: Is this "inline" needed? Why? Same below.
> > 
> > Just checking, I'd like to understand what's going on here.
> 
> This isn't needed...in Release builds. At -O2, gcc will inline functions
> whenever it thinks its a good idea. However, we care more about Debug
> performance here. This was cheap and made a few seconds difference.
> 
>
https://codereview.chromium.org/14273023/diff/2006/chrome/test/perf/generate_...
> chrome/test/perf/generate_profile.cc:247: printf("Writing to disk\n");
> On 2013/04/26 23:19:32, Paweł Hajdan Jr. wrote:
> > Do we need this printf now?
> 
> I've changed this to one printf at the beginning and at the end. I kept the
> incremental printfs so the buildbot scripts won't kill the process for lack of
> output.

[Elliot Glaysher, 2013-04-30 17:51:27]

On 2013/04/30 02:33:12, tonyg wrote:
> Awesome change. Just curious if this will work with the reference builds?

I'm not entirely sure what you're asking. If the reference build is a
performance_ui_tests binary, it will build databases of the correct version.

[tonyg, 2013-04-30 17:53:59]

On 2013/04/30 17:51:27, Elliot Glaysher wrote:
> On 2013/04/30 02:33:12, tonyg wrote:
> > Awesome change. Just curious if this will work with the reference builds?
> 
> I'm not entirely sure what you're asking. If the reference build is a
> performance_ui_tests binary, it will build databases of the correct version.

There are reference builds of chrome checked in. At each revision, the
performance_ui_tests binary is built from ToT and then performance tests are run
against both the ToT chrome and the checked in reference build of chrome. My
question is whether everything will work properly with a ToT
performance_ui_tests and an old chrome.

[Randy Smith (Not in Mondays), 2013-04-30 21:27:36]

Ok, I think this looks basically good.  I reviewed the whole patch, but focussed
on the generate_profile changes.  A couple of high level questions:

* Have you explored what the current gap between the performance we're seeing on
startup and the place where the perf tests go red?  There's been enough work in
this area that I could imagine we've got too much buffer between how fast we're
running and when the test goes red--that's bad because it means we won't catch
performance regressions from our current point.  (Please note that I'm being a
howling hypocrite raising this issue, as (with reviewer approval) I dodged it
when I did my work last, for which you are welcome to heap me with scorn.  But
it seems a question at least worth asking.)

* Do the try jobs run these tests?  If not, I'd recommend explicitly testing on
Mac, since that was the one desktop platform on which generate_profile didn't
used to work.

I'd suggest you get other people to specifically look at the following areas, as
I suspect they're available and I don't know the code well.  But if I'm wrong
about there being experts in these areas, just let me know and I'll keep
climbing:
* PerfUITestSuite setup (Pawel's probably good for this, and it does look good
to my eye).
* The setup of the extensions in the complex theme, including
BuildCachedThemePakIn() and the expand template code.
* The transition from RefCountedBytes to RefCountedMemory.  I see why you're
doing it, and it makes sense, but that kind of code is subtle, and if someone
knowledgeable in the code areas in which you made the shift (TopSites?) could
look at it, I'd be happier.

https://codereview.chromium.org/14273023/diff/26002/chrome/test/perf/feature_...
File chrome/test/perf/feature_startup_test.cc (right):

https://codereview.chromium.org/14273023/diff/26002/chrome/test/perf/feature_...
chrome/test/perf/feature_startup_test.cc:47: profile_type_ = profile_type;
It looks like we initialize this and never use it--am I missing something?

https://codereview.chromium.org/14273023/diff/26002/chrome/test/perf/perf_ui_...
File chrome/test/perf/perf_ui_test_suite.cc (right):

https://codereview.chromium.org/14273023/diff/26002/chrome/test/perf/perf_ui_...
chrome/test/perf/perf_ui_test_suite.cc:59: // We do normal initialization
afterwards because we want
nit: Dangling sentence?

[Elliot Glaysher, 2013-04-30 21:55:03]

On 2013/04/30 17:53:59, tonyg wrote:
> There are reference builds of chrome checked in. At each revision, the
> performance_ui_tests binary is built from ToT and then performance tests are
run
> against both the ToT chrome and the checked in reference build of chrome. My
> question is whether everything will work properly with a ToT
> performance_ui_tests and an old chrome.

I don't think that any of the tests in start_tests.cc or
feature_startup_tests.cc that use the reference builds.

[jeremy, 2013-05-01 10:44:22]

No review comments but just an FYI - I'm working on moving startup_test.cc to
Telemetry.  Looks like the profile generation code is separable from the tests
which is great :)

[Elliot Glaysher, 2013-05-03 19:25:58]

+ sky: changes to TopSites/History
+ yoz: setup of extension code in perf_ui_test

Changes:
- I've manually verified that this works on Mac.
- It turns out that all perf tests that are supposed to run a complex theme have
been broken for at least a year and a half. Patchset 9 fixes this.

On 2013/04/30 21:27:36, rdsmith wrote:
> * Have you explored what the current gap between the performance we're seeing
on
> startup and the place where the perf tests go red?  There's been enough work
in
> this area that I could imagine we've got too much buffer between how fast
we're
> running and when the test goes red--that's bad because it means we won't catch
> performance regressions from our current point.  (Please note that I'm being a
> howling hypocrite raising this issue, as (with reviewer approval) I dodged it
> when I did my work last, for which you are welcome to heap me with scorn.  But
> it seems a question at least worth asking.)

I have not. (I also suspect that some of the data here is bogus given the linux
perf tests being broken per above for the last year.) The database also being
shrunk to what can be built in 30 seconds at startup might have a small impact
too. I think it'd be better to commit this, let it run on the bots for several
days/a week and then reset the expectations to a reasonable average of the last
week's runs.
 
> * Do the try jobs run these tests?  If not, I'd recommend explicitly testing
on
> Mac, since that was the one desktop platform on which generate_profile didn't
> used to work.

They do not and I've set up a mac build environment and manually tested that
this works.

[Randy Smith (Not in Mondays), 2013-05-03 20:11:47]

LGTM with nit below.  Your plan around the performance ratchet also SGTM. 
Thanks!

https://codereview.chromium.org/14273023/diff/63001/chrome/test/perf/perf_ui_...
File chrome/test/perf/perf_ui_test_suite.cc (right):

https://codereview.chromium.org/14273023/diff/63001/chrome/test/perf/perf_ui_...
chrome/test/perf/perf_ui_test_suite.cc:59: // We do normal initialization
afterwards because we want
Sentence still dangling :-}.

[Elliot Glaysher, 2013-05-06 20:02:46]

+ yoz, sky: as above

(I appear to have forgotten that just putting +person in a comment doesn't
actually add them as a reviewer.)

[Yoyo Zhou, 2013-05-06 20:31:02]

mostly LGTM

https://codereview.chromium.org/14273023/diff/63001/chrome/test/perf/perf_ui_...
File chrome/test/perf/perf_ui_test_suite.cc (right):

https://codereview.chromium.org/14273023/diff/63001/chrome/test/perf/perf_ui_...
chrome/test/perf/perf_ui_test_suite.cc:134: // Build the "Cached Theme.pak" file
in the template. (It's not committed
Should we do this before loading the extension?

[Elliot Glaysher, 2013-05-06 20:43:22]

https://codereview.chromium.org/14273023/diff/63001/chrome/test/perf/perf_ui_...
File chrome/test/perf/perf_ui_test_suite.cc (right):

https://codereview.chromium.org/14273023/diff/63001/chrome/test/perf/perf_ui_...
chrome/test/perf/perf_ui_test_suite.cc:134: // Build the "Cached Theme.pak" file
in the template. (It's not committed
On 2013/05/06 20:31:02, Yoyo Zhou wrote:
> Should we do this before loading the extension?

No, because the theme system takes the Extension object as input to build the
output pak file.

[sky, 2013-05-06 22:14:40]

Brett wrote this stuff, so:
sky->brettw

[Paweł Hajdan Jr., 2013-05-06 23:46:04]

LGTM with comments.

https://codereview.chromium.org/14273023/diff/63001/chrome/test/perf/generate...
File chrome/test/perf/generate_profile.h (right):

https://codereview.chromium.org/14273023/diff/63001/chrome/test/perf/generate...
chrome/test/perf/generate_profile.h:25: const base::FilePath& dst_dir);
nit: Add WARN_UNUSED_RESULT from base/compiler_specific.

https://codereview.chromium.org/14273023/diff/63001/chrome/test/perf/run_all_...
File chrome/test/perf/run_all_perfuitests.cc (right):

https://codereview.chromium.org/14273023/diff/63001/chrome/test/perf/run_all_...
chrome/test/perf/run_all_perfuitests.cc:8:
base::PlatformThread::SetName("Tests_Main");
Is this needed? If so, please rather make it part of the test suite.

[Elliot Glaysher, 2013-05-08 23:33:45]

ping: brettw

[Randy Smith (Not in Mondays), 2013-05-09 00:10:40]

On 2013/05/08 23:33:45, Elliot Glaysher wrote:
> ping: brettw

Brett's got a lot of filters in place to reduce his codereview spam.  I don't
fully grok the details, but I could imagine being added part way through a CL
might trigger them.  You might want to send a separate email or chat him.

[brettw, 2013-05-09 23:44:06]

owners lgtm

[commit-bot: I haz the power, 2013-05-10 16:31:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/14273023/92001

[commit-bot: I haz the power, 2013-05-10 16:31:04]

Can't process patch for file
chrome/test/data/profiles/profile_with_default_theme/Default/History Index
2013-02-journal.
Unexpected git diff output name Index 2013-02-journal
b/chrome/test/data/profiles/profile_with_default_theme/Default/History Index
2013-02-journal.

[Elliot Glaysher, 2013-05-10 16:35:49]

sky: could you stamp generate_profile?

[sky, 2013-05-10 19:20:54]

LGTM