Construct a field trial to see if HIGH or MEDIUM memory model "works better"...

Construct a field trial to see if HIGH or MEDIUM memory model "works better"

Includes definition of a FieldTrial class to support this.
I have thoughts in my head about how this will eventually
extend to be controllable via UMA (as well as being able
to run tests defined at compile time, as was done in this
example.


r=mbelshe, mmentovai

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=3604

[jar (doing other things), 2008-10-18 20:01:56]

I have edited the sconsscript files to seemingly make this
work on linux (or so the try server says), but I don't yet
have the mac build adjusted (but perhaps the changes are
not yet critical to that build).  

Mark-  Do you have suggestions regarding this landing safely
in mac world and not breaking the build or hindering 
porting efforts?

Mike- Could you could review the code with regard to
connectness etc. in a Windows world? 

Thanks,

Jim

[Mike Belshe, 2008-10-18 21:32:52]

LGTM.

Question - do we need to measure memory usage differently for the different
memory models?  Today everyone is basically using medium, very very few alter
the default.  The reason for doing this is to reduce memory usage by the
browser.  Should we modify the memory histograms to also report differently
based on memory model?  If it turns out there is a difference in perf, we'll
probably want to know what the tradeoff is?

Mike

http://codereview.chromium.org/7638/diff/39/227
File base/field_trial.h (right):

http://codereview.chromium.org/7638/diff/39/227#newcode28
Line 28: class FieldTrial :public base::RefCounted<FieldTrial> {
nit: spacing after :

http://codereview.chromium.org/7638/diff/39/227#newcode52
Line 52: class FieldTrialList {
You could derive from NotThreadSafe.

http://codereview.chromium.org/7638/diff/39/227#newcode55
Line 55: FieldTrialList();
Generally with a singleton, you'll make the constructor private, and then
provide an accessor like "Get()", which would create the singleton if necessary
and then return it.

http://codereview.chromium.org/7638/diff/39/227#newcode82
Line 82: static FieldTrialList* global_;  // The singleton of this class.
You could use the Singleton template class here (see singleton.h).

http://codereview.chromium.org/7638/diff/39/236
File base/field_trial_unittest.cc (right):

http://codereview.chromium.org/7638/diff/39/236#newcode70
Line 70: // Very surprising to get here. Probability should be around 1 in 2 **
250.
Amusing.... :-)

[jar (doing other things), 2008-10-19 07:22:56]

Several suggestions taken from Mike.

Mike: We should chat about which memory histograms to split up for this field
trial.  I can land any/all additional splits in a small additional CL.   Thanks
for the suggestion!

http://codereview.chromium.org/7638/diff/39/227
File base/field_trial.h (right):

http://codereview.chromium.org/7638/diff/39/227#newcode28
Line 28: class FieldTrial :public base::RefCounted<FieldTrial> {
On 2008/10/18 21:32:52, mbelshe wrote:
> nit: spacing after :

Done.

http://codereview.chromium.org/7638/diff/39/227#newcode52
Line 52: class FieldTrialList {
On 2008/10/18 21:32:52, mbelshe wrote:
> You could derive from NotThreadSafe.

Done.

http://codereview.chromium.org/7638/diff/39/227#newcode55
Line 55: FieldTrialList();
On 2008/10/18 21:32:52, mbelshe wrote:
> Generally with a singleton, you'll make the constructor private, and then
> provide an accessor like "Get()", which would create the singleton if
necessary
> and then return it.

That would have left me with the need to call for destruction somehow (which was
taken care of by the scoped construction/destruction in main).

I didn't realize I could handle the destruction with the singleton class (per
suggestion below), but that would have made it harder to test (I'd need to add
pseudo-destruction ClearForTest() method).

I'll definitely take your advice (including use of singleton) if I make this
class thread safe.

Perhaps it is worth doing in the next CL (involving splitting up some other
histograms to participate in this field trial).

http://codereview.chromium.org/7638/diff/39/227#newcode82
Line 82: static FieldTrialList* global_;  // The singleton of this class.
On 2008/10/18 21:32:52, mbelshe wrote:
> You could use the Singleton template class here (see singleton.h). 

Good idea.  See comment above.

http://codereview.chromium.org/7638/diff/39/236
File base/field_trial_unittest.cc (right):

http://codereview.chromium.org/7638/diff/39/236#newcode70
Line 70: // Very surprising to get here. Probability should be around 1 in 2 **
250.
On 2008/10/18 21:32:52, mbelshe wrote:
> Amusing.... :-)

WTC told me that as long as I stayed under 1 in 2 ** 80, that the crypto tests
would be more likely to fail... so I took the "risk" ;-)

[Mark Mentovai, 2008-10-19 17:06:25]

We don't build any of these files on the Mac, so there shouldn't be a problem
for us.

[jar (doing other things), 2008-11-13 23:20:09]

I believe all of this is landed already (on trunk and dev release), except for

chrome/browser/render_widget_host_view_win.cc

Which apparently missed the dev channel build.

Please be sure that that file lands on the dev channel branch properly.

LGTM.

Jim