Add several new TRACE call and UMA metrics for GetProfile()

Add several new TRACE call and UMA metrics for SetupProfile().

I've replaced measuring GetProfile() (which includes cached profiles) with SetupProfile() which is more useful to track. I've added metrics to track the expensive components under SetupProfile(), which are
-chrome::RegisterProfilePrefs
-chrome_prefs::CreateProfilePrefs
-ProfileImpl::OnPrefsLoaded.

There currently are UMA histograms called Startup.SlowStartupPreferenceLoading, but I'm setting up different versions of these. They are tracking almost but not exactly the same things, but the current ones are only triggered for slow startups.
I considered modifying these to add a control version, but I thought adding new ones was better because:
-The histograms I'm adding have better names since they are called what they are actually checking
-I'm measuring at what I think is a better place (inside the method itself instead of near the caller.
-The new histograms are easily comparable for the work I'm doing since they are for all data (can just check percentile data).

Also changed the TRACE argument from path.value().c_str() to path.MaybeAsASCII() since the former was only giving the address inside the trace (I'm guessing it never worked).

If someone wants to have themselves also be added as an owner to any of the new histograms let me know.

BUG=454789
Committed: https://crrev.com/058da313f57dc0ae8ccbc12cc6053b21fc324cf9
Cr-Commit-Position: refs/heads/master@{#317078}

[rkaplow, 2015-02-18 18:28:43]

rkaplow@chromium.org changed reviewers:
+ asvitkine@chromium.org, caitkp@chromium.org, gab@chromium.org,
noms@chromium.org

[rkaplow, 2015-02-18 18:28:44]

asvitkine@chromium.org: Please review changes in 
all
caitkp@chromium.org: Please review changes in 
 components/keyed_service/content/browser_context_dependency_manager.cc	
noms@chromium.org: Please review changes in 
browser/profiles
gab@chromium.org: Please review changes in 
browser/prefs

[gab, 2015-02-18 18:44:49]

prefs lgtm

I suggest also getting rid of Startup.SlowStartupPreferenceLoading if it's
mostly redundant now.

Removing all Startup.SlowStartup* histograms has already been discussed @
https://code.google.com/p/chromium/issues/detail?id=246555#c6 and removing this
specific one now seems obvious now that it's mostly replaced (especially since
the main person looking at this, i.e. yourself, doesn't find it useful).

Thanks,
Gab

[noms (inactive), 2015-02-18 18:55:25]

https://codereview.chromium.org/937713003/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/937713003/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.h:80: Profile* SetupProfile(const
base::FilePath& profile_dir);
I don't think this is right. Is it just for testing, so that you can add the
instrumentation? Can you just add the instrumentation in CreateProfileHelper
maybe instead? There is also https://codereview.chromium.org/929073002/ that is
touching this code, by the way.

If you really need to split this up, I would rename this method to
CreateAndInitializeProfile(), make it private, and maybe expand the comment a
lot. I'm worried people will call this function, which can double create
profiles with the same path. The original GetProfile() checked whether this path
was already used by a profile , and if the profile already existed, it returned
it. This method automatically tries to create a new profile for this path, which
is way scarier.

[rkaplow, 2015-02-18 18:55:42]

On 2015/02/18 18:44:49, gab wrote:
> prefs lgtm
> 
> I suggest also getting rid of Startup.SlowStartupPreferenceLoading if it's
> mostly redundant now.
> 
> Removing all Startup.SlowStartup* histograms has already been discussed @
> https://code.google.com/p/chromium/issues/detail?id=246555#c6 and removing
this
> specific one now seems obvious now that it's mostly replaced (especially since
> the main person looking at this, i.e. yourself, doesn't find it useful).
> 
> Thanks,
> Gab

Alexei had suggested taking over a CL he has worked on which added Control
groups. For what I'm doing it seems a bit too heavy handed, considering I'm only
looking at a couple of these, and they don't match exactly with what I want to
be looking at. Also, my new histograms have better long term names, if we want
to keep them around to make sure these numbers aren't going up.

I'm not sure of the original context of the class, but I think we may be able to
get the results of the information of the "slow startup" group server-side,
which is much more lightweight that keeping two seperate histograms. If we are
happy with that and there's no objections and I can remove the slowstartup
stuff.

[gab, 2015-02-18 19:49:38]

On 2015/02/18 18:55:42, rkaplow wrote:
> On 2015/02/18 18:44:49, gab wrote:
> > prefs lgtm
> > 
> > I suggest also getting rid of Startup.SlowStartupPreferenceLoading if it's
> > mostly redundant now.
> > 
> > Removing all Startup.SlowStartup* histograms has already been discussed @
> > https://code.google.com/p/chromium/issues/detail?id=246555#c6 and removing
> this
> > specific one now seems obvious now that it's mostly replaced (especially
since
> > the main person looking at this, i.e. yourself, doesn't find it useful).
> > 
> > Thanks,
> > Gab
> 
> Alexei had suggested taking over a CL he has worked on which added Control
> groups. For what I'm doing it seems a bit too heavy handed, considering I'm
only
> looking at a couple of these, and they don't match exactly with what I want to
> be looking at. Also, my new histograms have better long term names, if we want
> to keep them around to make sure these numbers aren't going up.
> 
> I'm not sure of the original context of the class, but I think we may be able
to
> get the results of the information of the "slow startup" group server-side,
> which is much more lightweight that keeping two seperate histograms. If we are
> happy with that and there's no objections and I can remove the slowstartup
> stuff.

You mean that we can server-side filter down to only the slow startups (as this
is the only remaining difference with the SlowStartup histogram)? If so, I
agree, let's do it.

[rkaplow, 2015-02-18 19:57:47]

New patchsets have been uploaded after l-g-t-m from gab@chromium.org

[rkaplow, 2015-02-18 19:58:57]

https://codereview.chromium.org/937713003/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/937713003/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.h:80: Profile* SetupProfile(const
base::FilePath& profile_dir);
On 2015/02/18 18:55:25, Monica Dinculescu wrote:
> I don't think this is right. Is it just for testing, so that you can add the
> instrumentation? Can you just add the instrumentation in CreateProfileHelper
> maybe instead? There is also https://codereview.chromium.org/929073002/ that
is
> touching this code, by the way.
> 
> If you really need to split this up, I would rename this method to
> CreateAndInitializeProfile(), make it private, and maybe expand the comment a
> lot. I'm worried people will call this function, which can double create
> profiles with the same path. The original GetProfile() checked whether this
path
> was already used by a profile , and if the profile already existed, it
returned
> it. This method automatically tries to create a new profile for this path,
which
> is way scarier.

Was just in error it was not private, it's intended to just be used by
GetProfile. It makes sense to me to split this logic out into a private method
and have it be measured specifically. Renamed as per your suggestion as well.

Measuring CreateProfileHelper is not the same as the AddProfile() call also
requires measurement. I'm actually also adding instrumentation in
CreateProfileHelper.

[noms (inactive), 2015-02-18 21:32:19]

profiles lgtm

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.h:79: 
nit: extra \n

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.h:261: // handles the management.
nit: I would change this to "Synchronously creates and returns a profile..."

[Cait (Slow), 2015-02-18 21:41:27]

components/ lgtm

[Alexei Svitkine (slow), 2015-02-18 21:48:04]

https://codereview.chromium.org/937713003/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/937713003/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:286: // TODO(): http://crbug/160553 -
Bad things happen if we can't write to
TODO()'s should always have a name. I suggest putting a profile/ owner - like
noms@. (Even if she doesn't have cycles for this any time soon - the TODO should
have an owner.)

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:273: path.MaybeAsASCII());
What happens if the path is not ASCII?

From the header docs:

"This should only be used for cases where the FilePath is representing a
known-ASCII filename."

This doesn't seem like the case here. I think a safe UTF8 conversion should be
done.

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:400: profile_path.MaybeAsASCII());
Ditto.

[rkaplow, 2015-02-19 00:17:31]

New patchsets have been uploaded after l-g-t-m from
noms@chromium.org,caitkp@chromium.org

[rkaplow, 2015-02-19 00:18:57]

https://codereview.chromium.org/937713003/diff/20001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/937713003/diff/20001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:286: // TODO(): http://crbug/160553 -
Bad things happen if we can't write to
On 2015/02/18 21:48:03, Alexei Svitkine wrote:
> TODO()'s should always have a name. I suggest putting a profile/ owner - like
> noms@. (Even if she doesn't have cycles for this any time soon - the TODO
should
> have an owner.)

putting rogerta because I'm a bad person.

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:273: path.MaybeAsASCII());
On 2015/02/18 21:48:03, Alexei Svitkine wrote:
> What happens if the path is not ASCII?
> 
> From the header docs:
> 
> "This should only be used for cases where the FilePath is representing a
> known-ASCII filename."
> 
> This doesn't seem like the case here. I think a safe UTF8 conversion should be
> done.

If it is not ASCII then it returns the empty string, which is not so bad:
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_pa...
Worst case, you just don't see the arg in the trace.

all the UTC method have 'unsafe' in the name. Do you have a link for how to
convert to UTF8 safely?

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.cc (right):

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.cc:400: profile_path.MaybeAsASCII());
On 2015/02/18 21:48:03, Alexei Svitkine wrote:
> Ditto.

Also ditto - any suggestions?

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.h:79: 
On 2015/02/18 21:32:19, Monica Dinculescu wrote:
> nit: extra \n

Done.

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.h:261: // handles the management.
On 2015/02/18 21:32:19, Monica Dinculescu wrote:
> nit: I would change this to "Synchronously creates and returns a profile..."

Done.

[Alexei Svitkine (slow), 2015-02-19 14:44:59]

lgtm

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/937713003/diff/60001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:273: path.MaybeAsASCII());
On 2015/02/19 00:18:57, rkaplow wrote:
> On 2015/02/18 21:48:03, Alexei Svitkine wrote:
> > What happens if the path is not ASCII?
> > 
> > From the header docs:
> > 
> > "This should only be used for cases where the FilePath is representing a
> > known-ASCII filename."
> > 
> > This doesn't seem like the case here. I think a safe UTF8 conversion should
be
> > done.
> 
> If it is not ASCII then it returns the empty string, which is not so bad:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_pa...
> Worst case, you just don't see the arg in the trace.
> 
> all the UTC method have 'unsafe' in the name. Do you have a link for how to
> convert to UTF8 safely?

According to the comment for AsUTF8Unsafe(), it's only unsafe non-Mac, non-CrOS
POSIX systems - and only then diminishingly so - i.e. only on systems that don't
use utf8.

So I think using that is fine here. Though I'm also OK with MaybeAsASCII() per
your explanation above, with a slight preference for utf8.

https://codereview.chromium.org/937713003/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/937713003/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:286: // TODO(rogerta):
http://crbug/160553 - Bad things happen if we can't write to
Nit: Wrapping.

https://codereview.chromium.org/937713003/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/937713003/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.h:259: // Synchronously Creates and
returns a profile. This handles both the full
Nit: Don't capitalize creates.

https://codereview.chromium.org/937713003/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.h:260: // creation the management.
I don't understand this sentence. "This handles both the full creation the
management."? Is there a word missing?

Also, I'm not sure what "management" means in this context.

[rkaplow, 2015-02-19 15:40:58]

New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org

[rkaplow, 2015-02-19 15:42:51]

https://codereview.chromium.org/937713003/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/937713003/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile_impl.cc:286: // TODO(rogerta):
http://crbug/160553 - Bad things happen if we can't write to
On 2015/02/19 14:44:59, Alexei Svitkine wrote:
> Nit: Wrapping.

Done.

https://codereview.chromium.org/937713003/diff/80001/chrome/browser/profiles/...
File chrome/browser/profiles/profile_manager.h (right):

https://codereview.chromium.org/937713003/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.h:259: // Synchronously Creates and
returns a profile. This handles both the full
On 2015/02/19 14:44:59, Alexei Svitkine wrote:
> Nit: Don't capitalize creates.

Done.

https://codereview.chromium.org/937713003/diff/80001/chrome/browser/profiles/...
chrome/browser/profiles/profile_manager.h:260: // creation the management.
On 2015/02/19 14:44:59, Alexei Svitkine wrote:
> I don't understand this sentence. "This handles both the full creation the
> management."? Is there a word missing?
> 
> Also, I'm not sure what "management" means in this context.

lost an "and" there in the editing. Added a bit more detail.

[rkaplow, 2015-02-19 16:19:33]

The CQ bit was checked by rkaplow@chromium.org

[commit-bot: I haz the power, 2015-02-19 16:20:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/937713003/100001

[commit-bot: I haz the power, 2015-02-19 18:18:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-19 18:20:14]

Patchset 6 (id:??) landed as
https://crrev.com/058da313f57dc0ae8ccbc12cc6053b21fc324cf9
Cr-Commit-Position: refs/heads/master@{#317078}