Add detailed performance histograms to Files.app. (Abandoned)

Add detailed performance histograms to Files.app.

This patch adds following performance histograms:
- Load.Preference - time to load preferences
- Load.General - time to load general stuff (light)
- Load.Strings - time to load strings
- Load.EssentialUI - time to load essential UI (light)
- Load.AdditionalUI - time to load non-essential UI (moderate)
- Load.FileSystemUI - time to load file system related ui (heavy)
- Load.Display - time until displaying the basic UI

Also, it removes following histograms:
- Load.Parse - unused
- Load.Construct - unused
- Load.DOM - unused
- Load - unused

Finally, renames:
- Load.Roots -> UpdateRootsTime - the roots are updated not only during initialization.

TEST=Verify chrome://histogram if the histograms exist.
BUG=271409

[mtomasz, 2013-08-12 11:17:57]

@hirono: PTAL at JS.
@mpearson: PTAL at histograms.xml.

[hirono, 2013-08-12 16:53:51]

lgtm with nit. Thanks!

https://codereview.chromium.org/22815003/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/22815003/diff/10001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:214: var group = new
AsyncUtil.Group();
One more space is needed to indent?

[Mark P, 2013-08-12 21:35:39]

Please look over all these histograms to annotate them to say whether they
overlap with each other.  For instance, does "General" include "Preference",
etc?  Look at every pair.  People should be able to understand this breakdown /
what's included where without looking at the code.

Also, please consider revising the description of FileBrowser.Load to say how it
relates to the others.

--mark

[mtomasz, 2013-08-13 01:49:29]

@mpearson: PTAL. Thanks. It would be great if we could land it to M-30.

https://codereview.chromium.org/22815003/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

https://codereview.chromium.org/22815003/diff/10001/chrome/browser/resources/...
chrome/browser/resources/file_manager/js/file_manager.js:214: var group = new
AsyncUtil.Group();
On 2013/08/12 16:53:51, hirono wrote:
> One more space is needed to indent?

Done.

[Mark P, 2013-08-13 18:28:07]

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3888: -<histogram
name="FileBrowser.Load" units="milliseconds">
Please do not delete histograms.  Mark them as obsolete (see example elsewhere
in this file).

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3900: +    Load.EssentialUI. Does not
include Load.AdditionalUI nor Load.FileSystem,
Is this exactly (or nearly exactly) the sum of those four histograms, or does
something else take up non-trivial time?

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3923: +    is already available.
Please rephrase last sentence.  Do you mean:
This only measures the UI initialization step, the file system is already
available.
?

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3927: +<histogram
name="FileBrowser.Load.General" units="milliseconds">
Please come up with a better name.  General makes me think something like total,
but this is not like total at all.  Perhaps CommonData?

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3930: +    Files app.
Please be clear about whether this essential data includes preferences.

Also, does it include strings?

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3941: +  <summary>Time to load strings
during initialization of Files app.</summary>
Is it useful to separate general data from strings?  I'm having a really hard
time drawing a line that distinguishes one from the other.  If these measure
very different things in the code, perhaps you can give them better names that
make the meaning of the histograms more obvious.

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3947: +    initialization steps.
Is this basically General + AdditionalUI + FileSystem + FilesystemUI or are
there other things going on too?

[Mark P, 2013-08-13 18:37:21]

Also, your changelist description is out of date.

--mark

P.S.  I'm sorry I didn't see your last reply until this morning.  (It was sent
after I left for the day.)

[mtomasz, 2013-08-15 03:56:55]

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3888: -<histogram
name="FileBrowser.Load" units="milliseconds">
On 2013/08/13 18:28:07, Mark P wrote:
> Please do not delete histograms.  Mark them as obsolete (see example elsewhere
> in this file).

This histogram has never been used. Do we really want to keep it as obsolete? If
so, then why?

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3900: +    Load.EssentialUI. Does not
include Load.AdditionalUI nor Load.FileSystem,
On 2013/08/13 18:28:07, Mark P wrote:
> Is this exactly (or nearly exactly) the sum of those four histograms, or does
> something else take up non-trivial time?

We shouldn't sum here, since these tasks are called in parallel, and the sum
will be not reliable. For a user it is just time from launching to displaying.

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3923: +    is already available.
On 2013/08/13 18:28:07, Mark P wrote:
> Please rephrase last sentence.  Do you mean:
> This only measures the UI initialization step, the file system is already
> available.
> ?

Done.

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3927: +<histogram
name="FileBrowser.Load.General" units="milliseconds">
On 2013/08/13 18:28:07, Mark P wrote:
> Please come up with a better name.  General makes me think something like
total,
> but this is not like total at all.  Perhaps CommonData?

This corresponds the name used in the code. Note, these histograms are for our
internal use, for our performance automated tool.

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3930: +    Files app.
On 2013/08/13 18:28:07, Mark P wrote:
> Please be clear about whether this essential data includes preferences.
> 
> Also, does it include strings?

I am confused. This does not use strings. These are only histograms for our
internal use. Do we need to be so detailed here?

Note, that these histogram names correspond initialization steps in Files app.
See:
https://sites.google.com/a/google.com/chromeos-team-tok/files-app/initialization.
It is complex. I am not sure if we want to explain everything in histogram
descriptions.

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3941: +  <summary>Time to load strings
during initialization of Files app.</summary>
On 2013/08/13 18:28:07, Mark P wrote:
> Is it useful to separate general data from strings?  I'm having a really hard
> time drawing a line that distinguishes one from the other.  If these measure
> very different things in the code, perhaps you can give them better names that
> make the meaning of the histograms more obvious.

As before. We want to load strings in parallel to other steps, that's why this
step is separated from other steps. The histogram names correspond the
initialization steps in Files app. Refer to
https://sites.google.com/a/google.com/chromeos-team-tok/files-app/initialization.

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3947: +    initialization steps.
On 2013/08/13 18:28:07, Mark P wrote:
> Is this basically General + AdditionalUI + FileSystem + FilesystemUI or are
> there other things going on too?

As in the comment. It is said that it includes all initialization steps. But
some of the steps are done in parallel, so the total time is not equal to sum of
them.

To sum up, the initialization process in Files.app is asynchronous, running
steps in dependency order. Therefore the histograms can't be obvious without
understanding the logic, or explaining the entire logic in comments. But, these
histograms are necessary to track performance regressions in each of the
initialization steps, that's why we have them here.

[Mark P, 2013-08-15 15:22:50]

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3888: -<histogram
name="FileBrowser.Load" units="milliseconds">
On 2013/08/15 03:56:55, mtomasz wrote:
> On 2013/08/13 18:28:07, Mark P wrote:
> > Please do not delete histograms.  Mark them as obsolete (see example
elsewhere
> > in this file).
> 
> This histogram has never been used.

Oh, in that case (because it was never emitted to), go ahead and delete it.

> Do we really want to keep it as obsolete? If
> so, then why?

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3900: +    Load.EssentialUI. Does not
include Load.AdditionalUI nor Load.FileSystem,
On 2013/08/15 03:56:55, mtomasz wrote:
> On 2013/08/13 18:28:07, Mark P wrote:
> > Is this exactly (or nearly exactly) the sum of those four histograms, or
does
> > something else take up non-trivial time?
> 
> We shouldn't sum here, since these tasks are called in parallel, and the sum
> will be not reliable. For a user it is just time from launching to displaying.

Can you please put some of this explanation in the description?  Instead of
"This includes Load.General, Load.Preference, Load.Strings and
Load.EssentialUI."
it might be better to say for instance something like
"During this time, the tasks measured by the Load.General, Load.Preference,
Load.Strings and Load.EssentialUI histograms are performed.  Some of these
happen in parallel; this histogram is not the sum of those times."

I don't like the passive voice in the first sentence of my suggestion (if you
can rewrite it that would be good), but I think it captures the message.

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3927: +<histogram
name="FileBrowser.Load.General" units="milliseconds">
On 2013/08/15 03:56:55, mtomasz wrote:
> On 2013/08/13 18:28:07, Mark P wrote:
> > Please come up with a better name.  General makes me think something like
> total,
> > but this is not like total at all.  Perhaps CommonData?
> 
> This corresponds the name used in the code.

Okay, that's a plausible reason to keep the name.

> Note, these histograms are for our
> internal use, for our performance automated tool.

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3930: +    Files app.
On 2013/08/15 03:56:55, mtomasz wrote:
> On 2013/08/13 18:28:07, Mark P wrote:
> > Please be clear about whether this essential data includes preferences.
> > 
> > Also, does it include strings?
> 
> I am confused. This does not use strings. These are only histograms for our
> internal use. Do we need to be so detailed here?

Histograms often live long past the date that the original coder moves onto
another project.  Later people, whether newbies to the project or people from
another area of Chrome trying to investigate the scope of a performance problem,
need to have descriptions they can understand.  It's basically the same reason
we comment code well.
 
> Note, that these histogram names correspond initialization steps in Files app.
> See:
>
https://sites.google.com/a/google.com/chromeos-team-tok/files-app/initialization.
> It is complex. I am not sure if we want to explain everything in histogram
> descriptions.

Thanks for the reference.  I can understand wanting to keep histogram names in
parallel with the names of the functions in the code.

But please rewrite the description to be clear that it doesn't include loading
strings, loading preferences, or any filesystem or ui elements.

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3941: +  <summary>Time to load strings
during initialization of Files app.</summary>
On 2013/08/15 03:56:55, mtomasz wrote:
> On 2013/08/13 18:28:07, Mark P wrote:
> > Is it useful to separate general data from strings?  I'm having a really
hard
> > time drawing a line that distinguishes one from the other.  If these measure
> > very different things in the code, perhaps you can give them better names
that
> > make the meaning of the histograms more obvious.
> 
> As before. We want to load strings in parallel to other steps, that's why this
> step is separated from other steps. The histogram names correspond the
> initialization steps in Files app. Refer to
>
https://sites.google.com/a/google.com/chromeos-team-tok/files-app/initialization.

Okay.  I think this histogram name and description is fine then if you make the
correction I suggested above regarding the "General" histogram.

https://codereview.chromium.org/22815003/diff/21001/tools/metrics/histograms/...
tools/metrics/histograms/histograms.xml:3947: +    initialization steps.
On 2013/08/15 03:56:55, mtomasz wrote:
> On 2013/08/13 18:28:07, Mark P wrote:
> > Is this basically General + AdditionalUI + FileSystem + FilesystemUI or are
> > there other things going on too?
> 
> As in the comment. It is said that it includes all initialization steps. But
> some of the steps are done in parallel, so the total time is not equal to sum
of
> them.
> 
> To sum up, the initialization process in Files.app is asynchronous, running
> steps in dependency order. Therefore the histograms can't be obvious without
> understanding the logic, or explaining the entire logic in comments. But,
these
> histograms are necessary to track performance regressions in each of the
> initialization steps, that's why we have them here.

Can you please add something like
"Note that many initialization steps run in parallel."

[Mark P, 2013-10-17 17:40:06]

Is this changelist still pending?  The last message I see is some comments from
me that haven't been replied to.

--mark

[mtomasz, 2013-10-21 00:26:50]

On 2013/10/17 17:40:06, Mark P wrote:
> Is this changelist still pending?  The last message I see is some comments
from
> me that haven't been replied to.
> 
> --mark

Yes. This is not a priority now, I'll address them soon.

[Mark P, 2014-03-13 17:23:08]

On 2013/10/21 00:26:50, mtomasz wrote:
> On 2013/10/17 17:40:06, Mark P wrote:
> > Is this changelist still pending?  The last message I see is some comments
> from
> > me that haven't been replied to.
> > 
> > --mark
> 
> Yes. This is not a priority now, I'll address them soon.

This change as been sitting in my inbox for 7 months now.  Is it still relevant
/ do you still plan to get to it?

(I'm trying to clean up my codereview inbox.)

thanks,
mark

[mtomasz, 2014-03-14 02:35:39]

On 2014/03/13 17:23:08, Mark P wrote:
> On 2013/10/21 00:26:50, mtomasz wrote:
> > On 2013/10/17 17:40:06, Mark P wrote:
> > > Is this changelist still pending?  The last message I see is some comments
> > from
> > > me that haven't been replied to.
> > > 
> > > --mark
> > 
> > Yes. This is not a priority now, I'll address them soon.
> 
> This change as been sitting in my inbox for 7 months now.  Is it still
relevant
> / do you still plan to get to it?
> 
> (I'm trying to clean up my codereview inbox.)
> 
> thanks,
> mark

There are plans, but I'm very busy with other things at this moment. Files app
has changed since then, and I need to fix this patch.

[mtomasz, 2014-03-14 02:36:12]

On 2014/03/14 02:35:39, mtomasz wrote:
> On 2014/03/13 17:23:08, Mark P wrote:
> > On 2013/10/21 00:26:50, mtomasz wrote:
> > > On 2013/10/17 17:40:06, Mark P wrote:
> > > > Is this changelist still pending?  The last message I see is some
comments
> > > from
> > > > me that haven't been replied to.
> > > > 
> > > > --mark
> > > 
> > > Yes. This is not a priority now, I'll address them soon.
> > 
> > This change as been sitting in my inbox for 7 months now.  Is it still
> relevant
> > / do you still plan to get to it?
> > 
> > (I'm trying to clean up my codereview inbox.)
> > 
> > thanks,
> > mark
> 
> There are plans, but I'm very busy with other things at this moment. Files app
> has changed since then, and I need to fix this patch.

Removing from reviewers as for now, to clean your inbox.