Add the hardwareclass UMA log field for Chrome OS builds....

Add the hardwareclass UMA log field for Chrome OS builds.

This field will be used to distinguish histograms across Chrome OS hardware
platforms.

BUG=none
TEST=unit tests, tested on target device by reviewing the logs

[petkov, 2010-05-27 22:34:00]

Client side changes for the Chrome OS hardware class field...

[jar (doing other things), 2010-05-28 00:15:57]

http://codereview.chromium.org/2324001/diff/1/5
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/2324001/diff/1/5#newcode73
chrome/browser/metrics/metrics_log.cc:73: WriteAttribute("hardwareclass",
base::SysInfo::GetHardwareClass());
I wasn't clear... but I think this call runs a command line command.  Is that
correct?
If so, you are running on the UI thread, and are not allowed to pause the
thread.  There are techniques for running a continuation after you complete a
(slow) asynchronous call, or you could have waited until you have the data... or
a bunch of other tricks.

Am I misunderstanding?

[petkov, 2010-06-01 05:43:22]

Thanks for the quick review. You're right, I'm executing an external utility on
the UI thread -- thanks for pointing this out.

Per our discussion, I've renamed the GetPluginList task and I'm running the
hardware class utility as part of it.

I hate the OS_CHROMEOS ifdefs...

I think it's ready for another review. Please take a look.

[tfarina, 2010-06-01 14:09:36]

http://codereview.chromium.org/2324001/diff/21002/25005
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/2324001/diff/21002/25005#newcode1
chrome/browser/metrics/metrics_log.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
nit: 2010

http://codereview.chromium.org/2324001/diff/21002/25006
File chrome/browser/metrics/metrics_log.h (right):

http://codereview.chromium.org/2324001/diff/21002/25006#newcode225
chrome/browser/metrics/metrics_log.h:225:
DISALLOW_EVIL_CONSTRUCTORS(MetricsLog);
please, since you are here, could you change this to DISALLOW_COPY_AND_ASSIGN?

[petkov, 2010-06-01 18:06:42]

tfarina's style comments addressed.

PS. For some reason gcl upload doesn't prompt me for a patch description...

http://codereview.chromium.org/2324001/diff/21002/25005
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/2324001/diff/21002/25005#newcode1
chrome/browser/metrics/metrics_log.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2010/06/01 14:09:37, tfarina wrote:
> nit: 2010

Done.

http://codereview.chromium.org/2324001/diff/21002/25006
File chrome/browser/metrics/metrics_log.h (right):

http://codereview.chromium.org/2324001/diff/21002/25006#newcode225
chrome/browser/metrics/metrics_log.h:225:
DISALLOW_EVIL_CONSTRUCTORS(MetricsLog);
On 2010/06/01 14:09:37, tfarina wrote:
> please, since you are here, could you change this to DISALLOW_COPY_AND_ASSIGN?

Done.

[jar (doing other things), 2010-06-01 18:25:29]

http://codereview.chromium.org/2324001/diff/21002/25003
File base/sys_info_chromeos.cc (right):

http://codereview.chromium.org/2324001/diff/21002/25003#newcode73
base/sys_info_chromeos.cc:73: static std::string hardware_class;
Do you know the format for this result?  If so, it would be more style compliant
to avoid using std::string in a static, and instead use a POD item, such as
char[...].
If the length is unknowable, I think it would be fine to new/malloc and leak
(with documentation), putting the result into a static char*.

If you go this direction, you can also change the return result to:

const char*

http://codereview.chromium.org/2324001/diff/21002/25008
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2324001/diff/21002/25008#newcode294
chrome/browser/metrics/metrics_service.cc:294: hardware_class_ =
data.hardware_class_;
I'm surprised to see the data being copied from an instance of this class, into
an instance of this class.  If we really had such a setup, I'd expect we'd just
use the old instance, and not create a new one (and copy in).

http://codereview.chromium.org/2324001/diff/21002/25008#newcode300
chrome/browser/metrics/metrics_service.cc:300: std::string hardware_class_;
nit: Class members should all be private.

Use accessors if you need to read/write them from other code.

http://codereview.chromium.org/2324001/diff/21002/25008#newcode778
chrome/browser/metrics/metrics_service.cc:778:
current_log_->RecordHardwareClass(hardware_class_);
Rather than adding to the log here, you should decide if you want this data in
all logs, or just in the initial logs.
There are areas where you should call for each of those choices (see, for
example, where the plugin list is added during initial log creation).

It turns out you actually do call this function later, so the recording here is
probably a duplication bug anyway.

http://codereview.chromium.org/2324001/diff/21002/25008#newcode1153
chrome/browser/metrics/metrics_service.cc:1153:
log->RecordHardwareClass(hardware_class_);
This is a more reasonable place to do the recording (into the initial log), but
perchance you should go deeper, and should include this data as part of the
RecordEnvironment() call.

[petkov, 2010-06-01 23:41:15]

Please take another look. I've addressed all comments except the one regarding
the call to RecordHardwareClass -- I'm not certain where the right place to add
this field is given all the entry points to creating a MetricsLog -- your
feedback would be appreciated. Thanks.

http://codereview.chromium.org/2324001/diff/21002/25003
File base/sys_info_chromeos.cc (right):

http://codereview.chromium.org/2324001/diff/21002/25003#newcode73
base/sys_info_chromeos.cc:73: static std::string hardware_class;
On 2010/06/01 18:25:29, jar wrote:
> Do you know the format for this result?  If so, it would be more style
compliant
> to avoid using std::string in a static, and instead use a POD item, such as
> char[...].
> If the length is unknowable, I think it would be fine to new/malloc and leak
> (with documentation), putting the result into a static char*.
> 
> If you go this direction, you can also change the return result to:
> 
> const char*

Since the only client of this value is metrics and it caches it anyway, I've
remove the caching logic.

http://codereview.chromium.org/2324001/diff/21002/25008
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2324001/diff/21002/25008#newcode294
chrome/browser/metrics/metrics_service.cc:294: hardware_class_ =
data.hardware_class_;
On 2010/06/01 18:25:29, jar wrote:
> I'm surprised to see the data being copied from an instance of this class,
into
> an instance of this class.  If we really had such a setup, I'd expect we'd
just
> use the old instance, and not create a new one (and copy in).

Done. A bit more ifdef OS_CHROMEOS now though.

http://codereview.chromium.org/2324001/diff/21002/25008#newcode300
chrome/browser/metrics/metrics_service.cc:300: std::string hardware_class_;
On 2010/06/01 18:25:29, jar wrote:
> nit: Class members should all be private.
> 
> Use accessors if you need to read/write them from other code.

Done. Made MetricsService a friend class for simplicity, is that OK?

http://codereview.chromium.org/2324001/diff/21002/25008#newcode778
chrome/browser/metrics/metrics_service.cc:778:
current_log_->RecordHardwareClass(hardware_class_);
On 2010/06/01 18:25:29, jar wrote:
> Rather than adding to the log here, you should decide if you want this data in
> all logs, or just in the initial logs.
> There are areas where you should call for each of those choices (see, for
> example, where the plugin list is added during initial log creation).
> 
> It turns out you actually do call this function later, so the recording here
is
> probably a duplication bug anyway.

You're probably right that this is not the only place I need to call this
method.

Per initial discussion, we'd like to include this field as part of every log
(otherwise correlation is difficult). Do you have any suggestion on where the
best place to put this in is?

http://codereview.chromium.org/2324001/diff/21002/25008#newcode1153
chrome/browser/metrics/metrics_service.cc:1153:
log->RecordHardwareClass(hardware_class_);
On 2010/06/01 18:25:29, jar wrote:
> This is a more reasonable place to do the recording (into the initial log),
but
> perchance you should go deeper, and should include this data as part of the
> RecordEnvironment() call.

See above -- it should be part of every log, so it doesn't really belong to
RecordEnvironment which is relevant only for the initial log.

[jar (doing other things), 2010-06-02 01:57:20]

http://codereview.chromium.org/2324001/diff/21002/25008
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2324001/diff/21002/25008#newcode300
chrome/browser/metrics/metrics_service.cc:300: std::string hardware_class_;
Private is a really a good thing... and friends subvert that, and should not be
used that way.  

Looking closely, you probably don't even need this container object, as PostTask
can care multiple arguments without having to have you bundle and unbundle them.

Bottom line: You don't need to put them in in an instance... code will be much
simpler.

See my comments at the PostTask site below.

You will need to list the arguments on separate lines (if you're going to ifdef
the passage, and the prototype).



On 2010/06/01 23:41:15, petkov wrote:
> On 2010/06/01 18:25:29, jar wrote:
> > nit: Class members should all be private.
> > 
> > Use accessors if you need to read/write them from other code.
> 
> Done. Made MetricsService a friend class for simplicity, is that OK?
>

http://codereview.chromium.org/2324001/diff/58001/45008#newcode344
chrome/browser/metrics/metrics_service.cc:344:
local_state->RegisterInt64Pref(prefs::kStabilityLaunchTimeSec, 0);
Instead of having to encapsulate this into an object, you can just pass multiple
arguments, and PostTask will take care of the passing.  The call will look
something like:
callback_loop->PostTask(FROM_HERE,
                        plugins,
                        hardware_class);

Also note that the code at written leaks the |data| structure, but you could
have fixed that by not doing the new ... in this PostTask argument.

http://codereview.chromium.org/2324001/diff/58001/45008#newcode785
chrome/browser/metrics/metrics_service.cc:785: #if defined(OS_WIN)
This method can then be made to take two arguments, plugins and hardware_class.

You can decided if you want to use ifdefs, or just pass some empty string or
such as a signal that you don't have the hardware_class value.


Also note that if you stayed with the encapsulation, then you'd also need to
|delete data| or else it would leak.

[petkov, 2010-06-02 04:58:34]

Please take another look. In terms of data transfer, these are the only changes
relative to the original MetricsService code:

- renamed GetPluginListTask to InitTask
- renamed GetPluginListTaskComplete to InitTaskComplete
- renamed OnGetPluginListTaskComplete to OnInitTaskComplete
- added the hardware_class/hardware_class_ parameter/field to all three (just
like plugin/plugin_).

[jar (doing other things), 2010-06-02 06:20:55]

The flow is looking much better, and most of the issues are resolved.  Thanks!

As noted below, I still think you're double-writing in the initial log, and not
writing at all in the ongoing log (and I think you wanted it in both logs).  One
relatively easy way to check this sort of thing via Chromium command line
--enable-logging and --log-level=0, and then manually inspect the
client-side-log (it is written as plain ASCII to the log file... but you do have
to hack around in browser_main to simulate enabling of UMA-uploading in a
non-official build).


As one additional attempt at simplifying:

I don't like the ugliness caused by the ifdef's and I wish this code could be
mode cleaner in that regard.  IMO, you should always pass around hardware_class,
but use an empty string when you don't have chromeOS populating it.  When we
finally get do to the write to the log, test for an empty string, and only write
the tag/value when you have a non-empty string.  ALmost all of the ifdefs will
go away with that strategy.

...and the performance impact of creating a null string once every 30 minutes,
and/or passing it around, will be inconsequential.

http://codereview.chromium.org/2324001/diff/66002/62008
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2324001/diff/66002/62008#newcode780
chrome/browser/metrics/metrics_service.cc:780:
current_log_->RecordHardwareClass(hardware_class_);
As per my previous comment, I *think* it is a mistake to call
RecordHardwareClass() here.  This path is only executed within the first minute
of operation.  As a result, this will only add this data to the initial log.

See below where I *think* you are doing this for the initial log *again*.  I
don't think you record this data at all in the ongoing logs (but I think you
suggested that you wanted to do so).

http://codereview.chromium.org/2324001/diff/66002/62008#newcode1155
chrome/browser/metrics/metrics_service.cc:1155:
log->RecordHardwareClass(hardware_class_);
I think this is the second time you are adding hardware_class to the *initial*
log, as suggested by the name of this method.  I think you *wanted* to add it to
the ongoing log, but this is not the place to do so.

Care should be taken that the log writing is added as a sibling to the other
items (appversion, etc.) that are currently written at the head of each log (in
fact, they are written during the constructor for a MetricsLog), and such is
written for both initial and ongoing logs.  You should also check to see if the
logreader (server side) needs to parse out these general values ahead of time,
or if these siblings can usefully appear anywhere in the XML file

[petkov, 2010-06-03 05:07:19]

Well, it seems that converting metrics log to using a tree-based IR and
flattening at the very end is not a major change. I've put the change in and
conditionalized it based on OS_CHROMEOS. This way, if you're nervous about the
IR change, we can try it in Chrome OS first and then enable it if needed for all
platforms. It seems that the only difference in the XML output with and without
the tree IR change is a trailing \n in the final output (hence, the unit test
updates).

I've tested the CL on a Chrome OS device by inspecting the log output for an
initial log and several ongoing logs -- all seems fine.

Please take a look.

[petkov, 2010-06-04 20:01:16]

Some thoughts...

http://codereview.chromium.org/2324001/diff/72001/73004
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73004#newcode79
chrome/browser/metrics/metrics_log.cc:79: if (writer_)
A possible enhancement is to set all these members to NULL after freeing them.

http://codereview.chromium.org/2324001/diff/72001/73004#newcode110
chrome/browser/metrics/metrics_log.cc:110: result = xmlNodeDump(buffer_, doc_,
root, /* level */ 0, /* format */ 1);
A possible enhancement is to free doc_ (and writer_) after this call to reduce
peak memory usage. Freeing would need to be done both here and in the destructor
though in case somebody delete the object without calling CloseLog first.

[jar (doing other things), 2010-06-05 08:34:13]

There are a few minor nits, and one more significant perf concern.  My fear is
that that the tree structure will grow large, and the freeing of a log will
involve a LOT of tiny frees.  The potential is for this action to instigate a
TCMalloc "garbage collection" on the UI thread.  I *think* this is going to be
rare/ok... but it would be nice to add a histogram to watch this in the field. 
We have historically had bugs which pushed a LOT of events into the logs, and
eventually exceeded the event-limit imposed by the the server!?!  As a result,
we now impose the limit client side, just before we try to compress a log... but
that is probably late enough that when we hit such scenarios, then a major GC
event might take place.

One other alternative is to add an event counter so that we can avoid getting
into this "too many events" situation, and not have to "fix things" via a
massive discard at log-closing-time.

IMO, it would be sufficient to add a timing histogram, and file a bug to
preclude ultra-large-logs (with many nodes) before we consider transition to
your new node-based XML code outside of ChromeOS.

http://codereview.chromium.org/2324001/diff/72001/73002
File base/sys_info_chromeos.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73002#newcode72
base/sys_info_chromeos.cc:72: std::string hardware_class;
nit: Perhaps you could add something like:

DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));

That might not work in unit tests, and you may have to be a little more clever,
but it should at a minimum appear in a comment, and better yet appear in an
assertion, that this slow-running-code should only be run on less sensitive
thread.  

For instance, perhaps you could say:

DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));

http://codereview.chromium.org/2324001/diff/72001/73004
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73004#newcode79
chrome/browser/metrics/metrics_log.cc:79: if (writer_)
+1
On 2010/06/04 20:01:16, petkov wrote:
> A possible enhancement is to set all these members to NULL after freeing them.
>

http://codereview.chromium.org/2324001/diff/72001/73004#newcode110
chrome/browser/metrics/metrics_log.cc:110: result = xmlNodeDump(buffer_, doc_,
root, /* level */ 0, /* format */ 1);
Good suggestion.  

...but...

You should benchmark the speed of the free however.  If all nodes are kept
during the construction of this tree, then the recursive free of
many-thousands-of-nodes (corresponding to a lot of events, for example), could
be time consuming, and perhaps should not happen on the UI thread (if it really
can take a long time... and I'm not sure how many frees in a single TCMalloc
bucket are needed before the thread will push its local cache of free items (via
a lock) into the central cache.  

One simple way to monitor this would be at least to add a histogram for the time
taken to do the set of resource frees (where ever we do perform them).

I've never actually worried about this historically, but I *think* the code used
to have a linear buffer, which should not have been expensive to free.



On 2010/06/04 20:01:16, petkov wrote:
> A possible enhancement is to free doc_ (and writer_) after this call to reduce
> peak memory usage. Freeing would need to be done both here and in the
destructor
> though in case somebody delete the object without calling CloseLog first.
>

http://codereview.chromium.org/2324001/diff/72001/73007
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73007#newcode292
chrome/browser/metrics/metrics_service.cc:292: :
hardware_class_(hardware_class), plugins_(plugins) {}
style nit: initializers go one per line (when we are forced to wrap the first
one).

[petkov, 2010-06-05 18:25:16]

Thanks again for the review. It seems that we won't be able to complete this
code review before you go OOO (or I'll have to switch reviewers), so asking some
questions now in case you get a chance to respond before leaving.

http://codereview.chromium.org/2324001/diff/72001/73002
File base/sys_info_chromeos.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73002#newcode72
base/sys_info_chromeos.cc:72: std::string hardware_class;
I don't think I can/should access ChromeThread from a "base" package routine. I
see a couple of options:

1. Just leave it as a comment -- note that there's already a comment about
invoking an external utility in the header file, or
2. Move this utility out of base/sys_info into metrics_service which is
currently its only client.

Which one would you prefer?

On 2010/06/05 08:34:14, jar wrote:
> nit: Perhaps you could add something like:
> 
> DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
> 
> That might not work in unit tests, and you may have to be a little more
clever,
> but it should at a minimum appear in a comment, and better yet appear in an
> assertion, that this slow-running-code should only be run on less sensitive
> thread.  
> 
> For instance, perhaps you could say:
> 
> DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));

http://codereview.chromium.org/2324001/diff/72001/73004
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73004#newcode110
chrome/browser/metrics/metrics_log.cc:110: result = xmlNodeDump(buffer_, doc_,
root, /* level */ 0, /* format */ 1);
I assume this is just a simple UMA_HISTOGRAM_TIMES call?

On 2010/06/05 08:34:14, jar wrote:
> Good suggestion.  
> 
> ...but...
> 
> You should benchmark the speed of the free however.  If all nodes are kept
> during the construction of this tree, then the recursive free of
> many-thousands-of-nodes (corresponding to a lot of events, for example), could
> be time consuming, and perhaps should not happen on the UI thread (if it
really
> can take a long time... and I'm not sure how many frees in a single TCMalloc
> bucket are needed before the thread will push its local cache of free items
(via
> a lock) into the central cache.  
> 
> One simple way to monitor this would be at least to add a histogram for the
time
> taken to do the set of resource frees (where ever we do perform them).
> 
> I've never actually worried about this historically, but I *think* the code
used
> to have a linear buffer, which should not have been expensive to free.
> 
> 
> 
> On 2010/06/04 20:01:16, petkov wrote:
> > A possible enhancement is to free doc_ (and writer_) after this call to
reduce
> > peak memory usage. Freeing would need to be done both here and in the
> destructor
> > though in case somebody delete the object without calling CloseLog first.
> > 
> 
>

[jar (doing other things), 2010-06-06 06:30:19]

http://codereview.chromium.org/2324001/diff/72001/73002
File base/sys_info_chromeos.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73002#newcode72
base/sys_info_chromeos.cc:72: std::string hardware_class;
I liked your suggestion to move it to metrics_service, where the comment can be
made an assertion.

On 2010/06/05 18:25:16, petkov wrote:
> I don't think I can/should access ChromeThread from a "base" package routine.
I
> see a couple of options:
> 
> 1. Just leave it as a comment -- note that there's already a comment about
> invoking an external utility in the header file, or
> 2. Move this utility out of base/sys_info into metrics_service which is
> currently its only client.
> 
> Which one would you prefer?
> 
> On 2010/06/05 08:34:14, jar wrote:
> > nit: Perhaps you could add something like:
> > 
> > DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
> > 
> > That might not work in unit tests, and you may have to be a little more
> clever,
> > but it should at a minimum appear in a comment, and better yet appear in an
> > assertion, that this slow-running-code should only be run on less sensitive
> > thread.  
> > 
> > For instance, perhaps you could say:
> > 
> > DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
> 
>

http://codereview.chromium.org/2324001/diff/72001/73004
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73004#newcode110
chrome/browser/metrics/metrics_log.cc:110: result = xmlNodeDump(buffer_, doc_,
root, /* level */ 0, /* format */ 1);
Yes... just a simple UMA_HISTOGRAM_TIME call.  We can see how it does on
ChromeOS in the field, and get a better feel for its performance (especially
before moving it to the TOT for mainline Chromium).

On 2010/06/05 18:25:16, petkov wrote:
> I assume this is just a simple UMA_HISTOGRAM_TIMES call?
> 
> On 2010/06/05 08:34:14, jar wrote:
> > Good suggestion.  
> > 
> > ...but...
> > 
> > You should benchmark the speed of the free however.  If all nodes are kept
> > during the construction of this tree, then the recursive free of
> > many-thousands-of-nodes (corresponding to a lot of events, for example),
could
> > be time consuming, and perhaps should not happen on the UI thread (if it
> really
> > can take a long time... and I'm not sure how many frees in a single TCMalloc
> > bucket are needed before the thread will push its local cache of free items
> (via
> > a lock) into the central cache.  
> > 
> > One simple way to monitor this would be at least to add a histogram for the
> time
> > taken to do the set of resource frees (where ever we do perform them).
> > 
> > I've never actually worried about this historically, but I *think* the code
> used
> > to have a linear buffer, which should not have been expensive to free.
> > 
> > 
> > 
> > On 2010/06/04 20:01:16, petkov wrote:
> > > A possible enhancement is to free doc_ (and writer_) after this call to
> reduce
> > > peak memory usage. Freeing would need to be done both here and in the
> > destructor
> > > though in case somebody delete the object without calling CloseLog first.
> > > 
> > 
> > 
> 
>

[zel, 2010-06-07 21:00:44]

http://codereview.chromium.org/2324001/diff/72001/73002
File base/sys_info_chromeos.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73002#newcode71
base/sys_info_chromeos.cc:71: std::string SysInfo::GetHardwareClass() {
Why is this and the function able not under
#if defined(OS_CHROMEOS)

[petkov, 2010-06-07 21:57:52]

Adding zel@ as a reviewer since jar@ is OOO. I think I've addressed all of jar's
comments now. Please take a look.

http://codereview.chromium.org/2324001/diff/72001/73002
File base/sys_info_chromeos.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73002#newcode71
base/sys_info_chromeos.cc:71: std::string SysInfo::GetHardwareClass() {
On 2010/06/07 21:00:44, zel wrote:
> Why is this and the function able not under
> #if defined(OS_CHROMEOS)

Because this whole file is included in the build only from chromeos=1. However,
based on previous comments from jar@, I've moved this routine to metrics_service
and it's ifdef OS_CHROMEOS now.

http://codereview.chromium.org/2324001/diff/72001/73002#newcode72
base/sys_info_chromeos.cc:72: std::string hardware_class;
On 2010/06/06 06:30:19, jar wrote:
> I liked your suggestion to move it to metrics_service, where the comment can
be
> made an assertion.
> 
> On 2010/06/05 18:25:16, petkov wrote:
> > I don't think I can/should access ChromeThread from a "base" package
routine.
> I
> > see a couple of options:
> > 
> > 1. Just leave it as a comment -- note that there's already a comment about
> > invoking an external utility in the header file, or
> > 2. Move this utility out of base/sys_info into metrics_service which is
> > currently its only client.
> > 
> > Which one would you prefer?
> > 
> > On 2010/06/05 08:34:14, jar wrote:
> > > nit: Perhaps you could add something like:
> > > 
> > > DCHECK(ChromeThread::CurrentlyOn(ChromeThread::FILE));
> > > 
> > > That might not work in unit tests, and you may have to be a little more
> > clever,
> > > but it should at a minimum appear in a comment, and better yet appear in
an
> > > assertion, that this slow-running-code should only be run on less
sensitive
> > > thread.  
> > > 
> > > For instance, perhaps you could say:
> > > 
> > > DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::UI));
> > 
> > 
> 
> 

Done.

http://codereview.chromium.org/2324001/diff/72001/73004
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/2324001/diff/72001/73004#newcode79
chrome/browser/metrics/metrics_log.cc:79: if (writer_)
On 2010/06/05 08:34:14, jar wrote:
> +1
> On 2010/06/04 20:01:16, petkov wrote:
> > A possible enhancement is to set all these members to NULL after freeing
them.
> > 
> 
> 

Done.

http://codereview.chromium.org/2324001/diff/72001/73004#newcode110
chrome/browser/metrics/metrics_log.cc:110: result = xmlNodeDump(buffer_, doc_,
root, /* level */ 0, /* format */ 1);
Done -- I've added UMA_HISTOGRAM_TIMES calls to measure the XML doc writer
destruction time as well as the time to dump the tree into a character array.
Also, there's already a TODO(jar) in metrics_service on bounding the events so
that we can stop recording logs that are too big much sooner.


On 2010/06/06 06:30:19, jar wrote:
> Yes... just a simple UMA_HISTOGRAM_TIME call.  We can see how it does on
> ChromeOS in the field, and get a better feel for its performance (especially
> before moving it to the TOT for mainline Chromium).
> 
> On 2010/06/05 18:25:16, petkov wrote:
> > I assume this is just a simple UMA_HISTOGRAM_TIMES call?
> > 
> > On 2010/06/05 08:34:14, jar wrote:
> > > Good suggestion.  
> > > 
> > > ...but...
> > > 
> > > You should benchmark the speed of the free however.  If all nodes are kept
> > > during the construction of this tree, then the recursive free of
> > > many-thousands-of-nodes (corresponding to a lot of events, for example),
> could
> > > be time consuming, and perhaps should not happen on the UI thread (if it
> > really
> > > can take a long time... and I'm not sure how many frees in a single
TCMalloc
> > > bucket are needed before the thread will push its local cache of free
items
> > (via
> > > a lock) into the central cache.  
> > > 
> > > One simple way to monitor this would be at least to add a histogram for
the
> > time
> > > taken to do the set of resource frees (where ever we do perform them).
> > > 
> > > I've never actually worried about this historically, but I *think* the
code
> > used
> > > to have a linear buffer, which should not have been expensive to free.
> > > 
> > > 
> > > 
> > > On 2010/06/04 20:01:16, petkov wrote:
> > > > A possible enhancement is to free doc_ (and writer_) after this call to
> > reduce
> > > > peak memory usage. Freeing would need to be done both here and in the
> > > destructor
> > > > though in case somebody delete the object without calling CloseLog
first.
> > > > 
> > > 
> > > 
> > 
> > 
> 
>

[zel, 2010-06-07 22:25:22]

LGTM

[petkov, 2010-06-07 22:27:53]

Thank you! Assuming you've reviewed it carefully, could you please push the
patch for me?

On 2010/06/07 22:25:22, zel wrote:
> LGTM

[zel, 2010-06-08 00:02:00]

on the second thought, revoking my earlier LGTM

http://codereview.chromium.org/2324001/diff/102001/103004
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/2324001/diff/102001/103004#newcode1937
chrome/browser/metrics/metrics_service.cc:1937: CommandLine command(tool);
We don't want to run any external ChromeOS tools directly from Chrome. Instead,
you should expose a new method in chromeos::CrosLibrary. Please take a look at
implementation of chromeos::SyslogsLibrary::GetSyslogs() as an example.

[petkov, 2010-06-08 04:11:06]

On 2010/06/08 00:02:00, zel wrote:
> on the second thought, revoking my earlier LGTM
> 
> http://codereview.chromium.org/2324001/diff/102001/103004
> File chrome/browser/metrics/metrics_service.cc (right):
> 
> http://codereview.chromium.org/2324001/diff/102001/103004#newcode1937
> chrome/browser/metrics/metrics_service.cc:1937: CommandLine command(tool);
> We don't want to run any external ChromeOS tools directly from Chrome.
Instead,
> you should expose a new method in chromeos::CrosLibrary. Please take a look at
> implementation of chromeos::SyslogsLibrary::GetSyslogs() as an example.

Appreciate the feedback but... are you suggesting adding another *_library API
just for these 5 lines of code? How is running this Chrome OS utility different
than opening Chrome OS specific files in sys_info_chromeos.cc or the original (3
or so weeks ago) implementation of boot_times_loader.cc which executed "dmesg"
looking for Chrome OS specific strings (before we generated the firmware boot
data outside Chrome into files under /tmp)? What would be the purpose of the
extra complexity?

Either way, I don't have much choice and I'll need to do whatever you say but
please give me some additional guidance so that I don't end up with yet another
useless move and re-implementation of this routine.

[zel, 2010-06-08 15:30:21]

The rule of thumb is that we want to keep Chrome separated from the rest of
the ChromeOS via libcros.so. This lets us decouple Chrome from ChromeOS for
many scenarios like: development on Linux workstations, VM sandboxing on
buildbots (future), browser tests...


On Mon, Jun 7, 2010 at 9:11 PM, <petkov@chromium.org> wrote:

> On 2010/06/08 00:02:00, zel wrote:
>
>> on the second thought, revoking my earlier LGTM
>>
>
>  http://codereview.chromium.org/2324001/diff/102001/103004
>> File chrome/browser/metrics/metrics_service.cc (right):
>>
>
>  http://codereview.chromium.org/2324001/diff/102001/103004#newcode1937
>> chrome/browser/metrics/metrics_service.cc:1937: CommandLine command(tool);
>> We don't want to run any external ChromeOS tools directly from Chrome.
>>
> Instead,
>
>> you should expose a new method in chromeos::CrosLibrary. Please take a
>> look at
>> implementation of chromeos::SyslogsLibrary::GetSyslogs() as an example.
>>
>
> Appreciate the feedback but... are you suggesting adding another *_library
> API
> just for these 5 lines of code? How is running this Chrome OS utility
> different
> than opening Chrome OS specific files in sys_info_chromeos.cc or the
> original (3
> or so weeks ago) implementation of boot_times_loader.cc which executed
> "dmesg"
> looking for Chrome OS specific strings (before we generated the firmware
> boot
> data outside Chrome into files under /tmp)? What would be the purpose of
> the
> extra complexity?
>
> Either way, I don't have much choice and I'll need to do whatever you say
> but
> please give me some additional guidance so that I don't end up with yet
> another
> useless move and re-implementation of this routine.
>
>
>
> http://codereview.chromium.org/2324001/show
>

[petkov, 2010-06-08 17:56:36]

Based on our discussion, filed crosbug.com/3949

[petkov, 2010-06-09 17:32:26]

Landed by zel, closing the issue.

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