Issue 1891913002: Support saving browser metrics to disk and reading them during next run.

bcwhite

Patchset #2 (id:20001) has been deleted

4 years, 8 months ago (2016-04-15 14:29:47 UTC) #1

bcwhite

bcwhite@chromium.org changed reviewers: + isherman@chromium.org

4 years, 8 months ago (2016-04-15 19:33:08 UTC) #3

Ilya Sherman

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_base.h File base/metrics/histogram_base.h (right): https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_base.h#newcode207 base/metrics/histogram_base.h:207: virtual std::unique_ptr<HistogramSamples> SnapshotDifference() const = 0; Hmm, I think ...

4 years, 8 months ago (2016-04-19 00:57:46 UTC) #5

bcwhite

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_base.h File base/metrics/histogram_base.h (right): https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_base.h#newcode207 base/metrics/histogram_base.h:207: virtual std::unique_ptr<HistogramSamples> SnapshotDifference() const = 0; On 2016/04/19 00:57:46, ...

4 years, 8 months ago (2016-04-19 16:33:38 UTC) #6

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_base.h:207: virtual std::unique_ptr<HistogramSamples>
SnapshotDifference() const = 0;
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> Hmm, I think the names SnapshotDelta() vs SnapshotDifference() are too
similar,
> and the don't really indicate which one does what.  I'm trying to think of a
> better name for this method.  Maybe SnapshotDeltaIdempotently()?  It's kind of
> complicated, but at least it will nudge the reader to think a little harder
> about what's going on.  It's honestly kind of a weird thing to have both
methods
> in this API.  I need to think more about whether/why both are needed.

How about "UploadDelta" (for the previously existing method) to indicate that
it's specifically for uploading to UMA?

Both are needed because sometimes metrics data is read-only and sometimes it is
read/write with each successive call returning only what has changed since the
last call.  The Histograms and the SnapshotManager don't know which.

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.h:65: // PrepareDifference() is works
like PrepareDelta() except that it does
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> nit: s/is works/works

Done.

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.h:76: void PrepareDifference(const
HistogramBase* histogram);
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> It looks like this method might never be used.  Did I miss the use of it, or
is
> that true?

It's used in FileMetricsProvider for atomic files.  It previously called
"PrepareAbsolute()" but that won't be correct for browser metrics stored to disk
because browser metrics will certainly have had uploads done of them and we
don't want to include what was previously sent.

Before this, only setup metrics were saved to a file. They were never uploaded
so "previously sent" would always be zero.  For them, Difference and Absolute
are equivalent.

Absolute will still be required for chrome://histogarms.

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.cc:579: return false;
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> Hmm.  Could this be a DCHECK instead?  That is, are there legitimate expected
> use cases where the destination would not be set?

Right now, it's mostly unset because the experiment is only at 10%.  It could
also be unset if for some reason the process starts to exit (crashes?) before
higher layers start and set the destination.

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:246: void
SetPersistentLocation(FilePath location);
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> nit: Are FilePaths usually passed by copy or by const ref?

I've seen it both ways.  If I make this a ref then I have to do a copy during
the implementation whereas it is currently std::move'd.

I think this is more efficient because FilePaths are often created on-the-fly
and so the compiler can just move the result of that creation into the
parameter, perhaps without ever needing to create the actual object in memory.

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:252: bool
WritePersistentLocation();
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> nit: Perhaps "WriteTo" rather than just "Write"?

Done.

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:273: FilePath
persistent_location_;
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> nit: Can this be const?

Const values have to be set in the constructor.  This isn't because that
information isn't known until the higher layers get started.

https://codereview.chromium.org/1891913002/diff/60001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:46: // Set the destination for
where these will be persistent. This shouldn't
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> nit: s/persistent/persisted

Done.

https://codereview.chromium.org/1891913002/diff/60001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_field_trials.cc:58: }
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> I think I'd prefer to always set the location from the correct location, and
to
> just call a boolean Enable() method based on field trial state.  WDYT?

I could add Disable/Enable methods to the GlobalHistogramAllocator class.  I'll
look into it.

https://codereview.chromium.org/1891913002/diff/60001/chrome/browser/metrics/...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/1891913002/diff/60001/chrome/browser/metrics/...
chrome/browser/metrics/chrome_metrics_service_client.cc:173:
.AddExtension(FILE_PATH_LITERAL(".pma"));
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> What is ".pma"?  Is this just an extension that you're making up for the
> purposes of this code?  If so, could we just omit the extension entirely?

PersistentMemoryAllocator.  Just a dump of memory managed by that class.

The extension name is being defined as a constant inside the FileMetricsProvider
as part of a different CL.  It is used to recognize metrics file in some
situations.

https://codereview.chromium.org/1891913002/diff/60001/components/metrics/file...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/60001/components/metrics/file...
components/metrics/file_metrics_provider.cc:186:
snapshot_manager->PrepareDifferenceTakingOwnership(std::move(histogram));
On 2016/04/19 00:57:46, Ilya Sherman wrote:
> So, why is it important here that we take the (idempotent) difference, rather
> than a non-idempotent delta?  It seems kind of wrong to me, but maybe there's
> something I'm just not thinking of properly.

Because in some situations, the data may be read-only.  SetupMetrics is not
writable because it's created by an Administrator process.

Though...  The current implementation is to copy the file into RAM anyway so any
written changes would be discarded anyway.  Using PrepareDelta() could cause a
problem if we ever go back to using a memory-mapped file instead of RAM copy,
but I guess I could back-out the PrepareDifference changes.

Thoughts?

Ilya Sherman

Thanks, Brian. https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_snapshot_manager.h File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_snapshot_manager.h#newcode76 base/metrics/histogram_snapshot_manager.h:76: void PrepareDifference(const HistogramBase* histogram); On 2016/04/19 16:33:37, ...

4 years, 8 months ago (2016-04-20 07:12:15 UTC) #7

Thanks, Brian.

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.h:76: void PrepareDifference(const
HistogramBase* histogram);
On 2016/04/19 16:33:37, bcwhite wrote:
> On 2016/04/19 00:57:46, Ilya Sherman wrote:
> > It looks like this method might never be used.  Did I miss the use of it, or
> is
> > that true?
> 
> It's used in FileMetricsProvider for atomic files.  It previously called
> "PrepareAbsolute()" but that won't be correct for browser metrics stored to
disk
> because browser metrics will certainly have had uploads done of them and we
> don't want to include what was previously sent.
> 
> Before this, only setup metrics were saved to a file. They were never uploaded
> so "previously sent" would always be zero.  For them, Difference and Absolute
> are equivalent.
> 
> Absolute will still be required for chrome://histogarms.

FileMetricsProvider appears to use the TakingOwnership version, but not the
non-taking-ownership version.  Unless I'm still overlooking the use, of course
:)

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.h:76: void PrepareDifference(const
HistogramBase* histogram);
On 2016/04/19 16:33:37, bcwhite wrote:
> On 2016/04/19 00:57:46, Ilya Sherman wrote:
> > It looks like this method might never be used.  Did I miss the use of it, or
> is
> > that true?
> 
> It's used in FileMetricsProvider for atomic files.  It previously called
> "PrepareAbsolute()" but that won't be correct for browser metrics stored to
disk
> because browser metrics will certainly have had uploads done of them and we
> don't want to include what was previously sent.

For the same reason, we wouldn't want to potentially upload what was written out
to disk.  That's why I'm really uncomfortable with PrepareDifference() -- it
seems very easy to misuse it.

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:246: void
SetPersistentLocation(FilePath location);
On 2016/04/19 16:33:38, bcwhite wrote:
> On 2016/04/19 00:57:46, Ilya Sherman wrote:
> > nit: Are FilePaths usually passed by copy or by const ref?
> 
> I've seen it both ways.  If I make this a ref then I have to do a copy during
> the implementation whereas it is currently std::move'd.
> 
> I think this is more efficient because FilePaths are often created on-the-fly
> and so the compiler can just move the result of that creation into the
> parameter, perhaps without ever needing to create the actual object in memory.

There was recently a thread on chromium-dev about a similar topic, but for
strings [1].  The conclusion there was to prefer passing by const-ref.  I'd
think the same would apply to FilePath params.  Does that make sense?

[1]
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZoAfW7GXUVI

https://codereview.chromium.org/1891913002/diff/60001/components/metrics/file...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/60001/components/metrics/file...
components/metrics/file_metrics_provider.cc:186:
snapshot_manager->PrepareDifferenceTakingOwnership(std::move(histogram));
On 2016/04/19 16:33:38, bcwhite wrote:
> On 2016/04/19 00:57:46, Ilya Sherman wrote:
> > So, why is it important here that we take the (idempotent) difference,
rather
> > than a non-idempotent delta?  It seems kind of wrong to me, but maybe
there's
> > something I'm just not thinking of properly.
> 
> Because in some situations, the data may be read-only.  SetupMetrics is not
> writable because it's created by an Administrator process.
> 
> Though...  The current implementation is to copy the file into RAM anyway so
any
> written changes would be discarded anyway.  Using PrepareDelta() could cause a
> problem if we ever go back to using a memory-mapped file instead of RAM copy,
> but I guess I could back-out the PrepareDifference changes.
> 
> Thoughts?

I'd prefer to back-out the PrepareDifference changes for now if they're not
needed.

For a read-only file, I don't understand how PrepareDelta() could possibly be
different than PrepareAbsolute() -- if it's strictly read-only, then there can't
have been any deltas performed on it.  Right?

bcwhite

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_snapshot_manager.h File base/metrics/histogram_snapshot_manager.h (right): https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_snapshot_manager.h#newcode76 base/metrics/histogram_snapshot_manager.h:76: void PrepareDifference(const HistogramBase* histogram); On 2016/04/20 07:12:14, Ilya Sherman ...

4 years, 8 months ago (2016-04-20 16:47:46 UTC) #9

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.h:76: void PrepareDifference(const
HistogramBase* histogram);
On 2016/04/20 07:12:14, Ilya Sherman wrote:
> On 2016/04/19 16:33:37, bcwhite wrote:
> > On 2016/04/19 00:57:46, Ilya Sherman wrote:
> > > It looks like this method might never be used.  Did I miss the use of it,
or
> > is
> > > that true?
> > 
> > It's used in FileMetricsProvider for atomic files.  It previously called
> > "PrepareAbsolute()" but that won't be correct for browser metrics stored to
> disk
> > because browser metrics will certainly have had uploads done of them and we
> > don't want to include what was previously sent.
> > 
> > Before this, only setup metrics were saved to a file. They were never
uploaded
> > so "previously sent" would always be zero.  For them, Difference and
Absolute
> > are equivalent.
> > 
> > Absolute will still be required for chrome://histogarms.
> 
> FileMetricsProvider appears to use the TakingOwnership version, but not the
> non-taking-ownership version.  Unless I'm still overlooking the use, of course
> :)

Oh, I see.  You're correct.  Those methods are just wrappers around an internal
method that does the real work so I think they're worth having in order to keep
the API complete.

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/histogram_...
base/metrics/histogram_snapshot_manager.h:76: void PrepareDifference(const
HistogramBase* histogram);
On 2016/04/20 07:12:14, Ilya Sherman wrote:
> On 2016/04/19 16:33:37, bcwhite wrote:
> > On 2016/04/19 00:57:46, Ilya Sherman wrote:
> > > It looks like this method might never be used.  Did I miss the use of it,
or
> > is
> > > that true?
> > 
> > It's used in FileMetricsProvider for atomic files.  It previously called
> > "PrepareAbsolute()" but that won't be correct for browser metrics stored to
> disk
> > because browser metrics will certainly have had uploads done of them and we
> > don't want to include what was previously sent.
> 
> For the same reason, we wouldn't want to potentially upload what was written
out
> to disk.  That's why I'm really uncomfortable with PrepareDifference() -- it
> seems very easy to misuse it.

I'm not following.  Each metric on disk has both the "active" sample count and
the "logged" sample count side-by-side.  For anything involving logging to UMA,
the difference between the two should be used.  Generally we want to update the
"logged" count at the same time (aka "Delta") but if we can't (say, because the
file is not writable) then something less invasive is necessary (aka
"Difference").  In the latter case, it's assumed that the data will be discarded
thereafter, never to be uploaded again.  Perhaps that should be in the comments.

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/persistent...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1891913002/diff/60001/base/metrics/persistent...
base/metrics/persistent_histogram_allocator.h:246: void
SetPersistentLocation(FilePath location);
On 2016/04/20 07:12:14, Ilya Sherman wrote:
> On 2016/04/19 16:33:38, bcwhite wrote:
> > On 2016/04/19 00:57:46, Ilya Sherman wrote:
> > > nit: Are FilePaths usually passed by copy or by const ref?
> > 
> > I've seen it both ways.  If I make this a ref then I have to do a copy
during
> > the implementation whereas it is currently std::move'd.
> > 
> > I think this is more efficient because FilePaths are often created
on-the-fly
> > and so the compiler can just move the result of that creation into the
> > parameter, perhaps without ever needing to create the actual object in
memory.
> 
> There was recently a thread on chromium-dev about a similar topic, but for
> strings [1].  The conclusion there was to prefer passing by const-ref.  I'd
> think the same would apply to FilePath params.  Does that make sense?
> 
> [1]
>
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/ZoAfW7GXUVI

Interesting.  Better to pass-by-ref for maintainability reasons.  Done.

https://codereview.chromium.org/1891913002/diff/60001/components/metrics/file...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/60001/components/metrics/file...
components/metrics/file_metrics_provider.cc:186:
snapshot_manager->PrepareDifferenceTakingOwnership(std::move(histogram));
On 2016/04/20 07:12:14, Ilya Sherman wrote:
> On 2016/04/19 16:33:38, bcwhite wrote:
> > On 2016/04/19 00:57:46, Ilya Sherman wrote:
> > > So, why is it important here that we take the (idempotent) difference,
> rather
> > > than a non-idempotent delta?  It seems kind of wrong to me, but maybe
> there's
> > > something I'm just not thinking of properly.
> > 
> > Because in some situations, the data may be read-only.  SetupMetrics is not
> > writable because it's created by an Administrator process.
> > 
> > Though...  The current implementation is to copy the file into RAM anyway so
> any
> > written changes would be discarded anyway.  Using PrepareDelta() could cause
a
> > problem if we ever go back to using a memory-mapped file instead of RAM
copy,
> > but I guess I could back-out the PrepareDifference changes.
> > 
> > Thoughts?
> 
> I'd prefer to back-out the PrepareDifference changes for now if they're not
> needed.

Done.


> For a read-only file, I don't understand how PrepareDelta() could possibly be
> different than PrepareAbsolute() -- if it's strictly read-only, then there
can't
> have been any deltas performed on it.  Right?

Uploads could have been done on the data _before_ it was written out to disk. 
It's dumped as a single memory block with both "active" and "logged" counts held
side-by-side.

Ilya Sherman

Thanks Brian. I still have questions about the PrepareDifference semantics, but since that's not part ...

4 years, 8 months ago (2016-04-20 17:10:33 UTC) #10

Thanks Brian.  I still have questions about the PrepareDifference semantics, but
since that's not part of this CL anymore, I'll let them rest for now.  If you do
bring back this concept in a later CL, we can discuss it further at that time =)

https://codereview.chromium.org/1891913002/diff/140001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:719: if
(persistent_location_.empty())
I'd still prefer the location to be guaranteed to be set, and to track
enabled/disabled state explicitly.

(I don't actually see a way for the location to be unset, except in case of an
error.  If this is really intended to be error-handling code, then please update
the comment.  And, we should think carefully about what might cause that error
to come up, and whether we're willing to live with simply dropping metrics in
that case.  It certainly seems oddly asymmetric to log an error below, but not
to log one here, if this is error-handling code.)

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName);
Could you pass the destination file name here, so that it is indeed passed to
the constructor, and therefore possible to mark as const?

https://codereview.chromium.org/1891913002/diff/140001/components/metrics/fil...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:191: // discarded after the
reporting is complete.
To clarify: What happens to the actual backing file?  Are the file contents
themselves discarded after reporting is complete?  If not, what mechanism is in
place to ensure that we don't doubly upload data that was persisted to a file?

bcwhite

https://codereview.chromium.org/1891913002/diff/140001/base/metrics/persistent_histogram_allocator.cc File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/1891913002/diff/140001/base/metrics/persistent_histogram_allocator.cc#newcode719 base/metrics/persistent_histogram_allocator.cc:719: if (persistent_location_.empty()) On 2016/04/20 17:10:33, Ilya Sherman wrote: > ...

4 years, 8 months ago (2016-04-22 15:18:05 UTC) #13

https://codereview.chromium.org/1891913002/diff/140001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:719: if
(persistent_location_.empty())
On 2016/04/20 17:10:33, Ilya Sherman wrote:
> I'd still prefer the location to be guaranteed to be set, and to track
> enabled/disabled state explicitly.

Enable/Disable: Done.

> (I don't actually see a way for the location to be unset, except in case of an
> error.  If this is really intended to be error-handling code, then please
update
> the comment.  And, we should think carefully about what might cause that error
> to come up, and whether we're willing to live with simply dropping metrics in
> that case.  It certainly seems oddly asymmetric to log an error below, but not
> to log one here, if this is error-handling code.)

It can't be unset (at least by existing code) but there may be paths where it
never gets set at all.  I don't think a log message is necessary but I don't
mind adding one.

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName);
On 2016/04/20 17:10:33, Ilya Sherman wrote:
> Could you pass the destination file name here, so that it is indeed passed to
> the constructor, and therefore possible to mark as const?

This has been moved to it's rightful final resting place in
browser_main_runner.cc.  The pathname can't be set from content/ because it
doesn't have access to the destination directory as that is defined in chrome/.

https://codereview.chromium.org/1891913002/diff/140001/components/metrics/fil...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:191: // discarded after the
reporting is complete.
On 2016/04/20 17:10:33, Ilya Sherman wrote:
> To clarify: What happens to the actual backing file?  Are the file contents
> themselves discarded after reporting is complete?  If not, what mechanism is
in
> place to ensure that we don't doubly upload data that was persisted to a file?

The backing file is atomically overwritten (using the ImportantFileWriter) upon
each exit.

The FileMetricsProvider has logic that prevents multiple reads of the same
content by persistently tracking the last-read time and comparing it against the
current last-modified time of the file.

Alexei Svitkine (slow)

asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org

4 years, 8 months ago (2016-04-22 15:27:38 UTC) #14

Alexei Svitkine (slow)

I didn't look at this CL in detail, but I have some high level questions: ...

4 years, 8 months ago (2016-04-22 15:27:39 UTC) #15

bcwhite

> - If this doesn't persist things during a crash, what's the point? Because on ...

4 years, 8 months ago (2016-04-22 16:04:36 UTC) #16

Ilya Sherman

Btw, I'm in Montreal this week. Feel free to come find me if you'd like ...

4 years, 8 months ago (2016-04-25 19:48:41 UTC) #17

bcwhite

>Btw, I'm in Montreal this week. Feel free to come find >me if you'd like ...

4 years, 8 months ago (2016-04-25 20:37:18 UTC) #18

>Btw, I'm in Montreal this week.  Feel free to come find
>me if you'd like to talk allocator! =)

I'm only in the office on Thursday & Friday.  I'll find you then.

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName);
On 2016/04/25 19:48:40, Ilya Sherman wrote:
> On 2016/04/22 15:18:05, bcwhite wrote:
> > On 2016/04/20 17:10:33, Ilya Sherman wrote:
> > > Could you pass the destination file name here, so that it is indeed passed
> to
> > > the constructor, and therefore possible to mark as const?
> > 
> > This has been moved to it's rightful final resting place in
> > browser_main_runner.cc.  The pathname can't be set from content/ because it
> > doesn't have access to the destination directory as that is defined in
> chrome/.
> 
> Is there not an embedder interface that could be used to provide the
destination
> directory?  That would be the more typical design structure for accessing data
> that's only known to the chrome/ layer.

I don't understand what you mean by that.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:755: // Stop if no destination is
set, perhaps because it has not been enabled.
On 2016/04/25 19:48:40, Ilya Sherman wrote:
> I was kind of expecting that you'd check the enabled state explicitly, and
> return early (sans warning) in just that case.

"Enabled" has nothing to do with being able to save the state.  It could have
been enabled and then disabled.  If it was never enabled then the amount of data
to save will be very small.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:758: << " to file because no
location was set.";
On 2016/04/25 19:48:40, Ilya Sherman wrote:
> I still think this should be an error rather than a warning.  What is a code
> path that could result in WriteToPersistentLocation() being called without a
> persistent location being set, and why is it desirable for that code path to
> exist?

Unlike below, where there is a specific local failure, there is no "error" here.
 This condition is dealing, safely, with how an external caller is using this
module.  This module shouldn't be judging if something outside is wrong.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:398: static void Disable();
On 2016/04/25 19:48:40, Ilya Sherman wrote:
> Rather than having both an Enable() and a Disable() method, I'd prefer to just
> have a single Enable() method, since disabled is the default state.

The default is "enabled" because that is the normal way of operating.  Only this
special case of an experiment creates the allocator but doesn't use it.

I could change it to "Enabled(bool enable)" if you prefer.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:402: static bool IsEnabled();
On 2016/04/25 19:48:40, Ilya Sherman wrote:
> This method appears to be unused.

I was using it...  Code must have changed.  Still, seems like it's good to be
symmetrical.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:410: static
GlobalHistogramAllocator* GetEvenIfDisabled();
On 2016/04/25 19:48:40, Ilya Sherman wrote:
> IMO this method is not needed.  Get() should always return the allocator, and
> the enabled/disabled state should control how the allocator behaves.  That
said,
> it's fairly surprising that some code expects to work with the allocator even
> when it's disabled.  Is enabled/disabled the wrong metaphor here?  Or should
the
> code that tries to use the allocator when its disabled be refactored so that
it
> uses some other object, which is always enabled?

Get() always returns an active allocator.  Existing code makes this call to know
whether to allocate persistantly or not.  If there is no persistent allocator,
this returns null.  Disabling the allocator acheives the same result.

However, there needs to be a way to get an allocator (even if disabled) if one
exists in order to call other methods on it.  It's not uncommen for objects to
be accessed for some things even when they're not "active" in their primary
role.

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:35:
base::GlobalHistogramAllocator::Enable();
On 2016/04/25 19:48:40, Ilya Sherman wrote:
> It seems odd to have this as a static method.  Why not just Get() the
allocator
> below, and then call Enable() on it?

I considered both.

The general differentiation has been that methods that operate on the allocator
object are normal while methods that join it to the global (singleton) scope are
static.

The "global" allocator object knows nothing about whether it's tied into the
metrics system, just that it needs some additional code to support that role.

Enable/Disable doesn't need any knowledge from inside a GlobalHistogramAllocator
object.  It does some management on a global instance of such an object but
makes no call into it.

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:144: if (allocator)
On 2016/04/25 19:48:40, Ilya Sherman wrote:
> In what case(s) might the allocator be null?  (And why is the answer here
> different than the answer in chrome_browser_field_trials.cc?)

I don't imagine it will be.  But this code is so far away from where it is set
that I prefer to be fail-safe.

Who knows what code paths could be across the myriad of Chrome products, not to
mention all the tests.

https://codereview.chromium.org/1891913002/diff/220001/content/browser/browse...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/content/browser/browse...
content/browser/browser_main_runner.cc:66: // BrowserMetrics usage peaked around
95% of 2MiB as of 2016-02-20.
On 2016/04/25 19:48:41, Ilya Sherman wrote:
> nit: "95% of 2MiB" is confusing -- it kind of sounds like it might mean 1.9MiB
> (since 1.9 is 95% of 2).  Could you please rephrase this bit?

That is what it means.  Better to just say "1.9MiB".

https://codereview.chromium.org/1891913002/diff/220001/content/browser/browse...
content/browser/browser_main_runner.cc:241: if (allocator)
On 2016/04/25 19:48:41, Ilya Sherman wrote:
> Under what circumstances can the allocator be null here?

It will be null if the allocator has been disabled.

Ilya Sherman

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_browser_field_trials.cc File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_browser_field_trials.cc#newcode42 chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName); On 2016/04/25 20:37:17, bcwhite wrote: > On 2016/04/25 ...

4 years, 8 months ago (2016-04-25 21:12:11 UTC) #19

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName);
On 2016/04/25 20:37:17, bcwhite wrote:
> On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > On 2016/04/22 15:18:05, bcwhite wrote:
> > > On 2016/04/20 17:10:33, Ilya Sherman wrote:
> > > > Could you pass the destination file name here, so that it is indeed
passed
> > to
> > > > the constructor, and therefore possible to mark as const?
> > > 
> > > This has been moved to it's rightful final resting place in
> > > browser_main_runner.cc.  The pathname can't be set from content/ because
it
> > > doesn't have access to the destination directory as that is defined in
> > chrome/.
> > 
> > Is there not an embedder interface that could be used to provide the
> destination
> > directory?  That would be the more typical design structure for accessing
data
> > that's only known to the chrome/ layer.
> 
> I don't understand what you mean by that.

For example, the ChromeMetricsServiceClient class implements the
metrics::MetricsServiceClient interface to provide data to the //metrics
component, even though that data is only available from the //chrome layer.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:755: // Stop if no destination is
set, perhaps because it has not been enabled.
On 2016/04/25 20:37:17, bcwhite wrote:
> On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > I was kind of expecting that you'd check the enabled state explicitly, and
> > return early (sans warning) in just that case.
> 
> "Enabled" has nothing to do with being able to save the state.  It could have
> been enabled and then disabled.  If it was never enabled then the amount of
data
> to save will be very small.

Why would we enable and then subsequently disable it?  The only reason we even
have enable/disable state both as viable possibilities is that we're not
shipping to 100% of users yet.  But that's a one-time decision, and should only
be able to flip in a single direction.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:758: << " to file because no
location was set.";
On 2016/04/25 20:37:17, bcwhite wrote:
> On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > I still think this should be an error rather than a warning.  What is a code
> > path that could result in WriteToPersistentLocation() being called without a
> > persistent location being set, and why is it desirable for that code path to
> > exist?
> 
> Unlike below, where there is a specific local failure, there is no "error"
here.
>  This condition is dealing, safely, with how an external caller is using this
> module.  This module shouldn't be judging if something outside is wrong.

If I'm understanding you correctly, then I disagree: IMO this module absolutely
should document its invariants, and fail ungracefully if those invariants are
not satisfied.  I would much rather find broken invariants due to failing
DCHECKs and crash reports rather than by eventually noticing that we seem to
sometimes be dropping data on the floor.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:398: static void Disable();
On 2016/04/25 20:37:17, bcwhite wrote:
> On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > Rather than having both an Enable() and a Disable() method, I'd prefer to
just
> > have a single Enable() method, since disabled is the default state.
> 
> The default is "enabled" because that is the normal way of operating.  Only
this
> special case of an experiment creates the allocator but doesn't use it.
> 
> I could change it to "Enabled(bool enable)" if you prefer.

Sorry, I don't follow.  How is the experiment a special case?  There's only a
single allocator, and it's currently disabled by default, and enabled for X% of
users (for some value of X).  Am I understanding that correctly?

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:402: static bool IsEnabled();
On 2016/04/25 20:37:17, bcwhite wrote:
> On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > This method appears to be unused.
> 
> I was using it...  Code must have changed.  Still, seems like it's good to be
> symmetrical.

I strongly disagree that it's worthwhile to land dead code.  This adds work for
the compiler, for maintainers of the code, etc., with no clear benefits.  If the
code is ever needed, we can add it at that time.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:410: static
GlobalHistogramAllocator* GetEvenIfDisabled();
On 2016/04/25 20:37:17, bcwhite wrote:
> On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > IMO this method is not needed.  Get() should always return the allocator,
and
> > the enabled/disabled state should control how the allocator behaves.  That
> said,
> > it's fairly surprising that some code expects to work with the allocator
even
> > when it's disabled.  Is enabled/disabled the wrong metaphor here?  Or should
> the
> > code that tries to use the allocator when its disabled be refactored so that
> it
> > uses some other object, which is always enabled?
> 
> Get() always returns an active allocator.  Existing code makes this call to
know
> whether to allocate persistantly or not.  If there is no persistent allocator,
> this returns null.  Disabling the allocator acheives the same result.
> 
> However, there needs to be a way to get an allocator (even if disabled) if one
> exists in order to call other methods on it.  It's not uncommen for objects to
> be accessed for some things even when they're not "active" in their primary
> role.

IMO this is a poor design pattern, and I'd much rather separate out those roles
into distinct classes.  I'll admit that I haven't deeply thought about the
specific method calls involved here; so if you disagree, please feel free to
explain to me why this design makes the most sense here.

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:35:
base::GlobalHistogramAllocator::Enable();
On 2016/04/25 20:37:17, bcwhite wrote:
> On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > It seems odd to have this as a static method.  Why not just Get() the
> allocator
> > below, and then call Enable() on it?
> 
> I considered both.
> 
> The general differentiation has been that methods that operate on the
allocator
> object are normal while methods that join it to the global (singleton) scope
are
> static.
> 
> The "global" allocator object knows nothing about whether it's tied into the
> metrics system, just that it needs some additional code to support that role.
> 
> Enable/Disable doesn't need any knowledge from inside a
GlobalHistogramAllocator
> object.  It does some management on a global instance of such an object but
> makes no call into it.

I don't really follow, sorry.  In any case, I don't care so much about whether
the method itself is static or not.  But I do think it makes more sense for the
allocator to track, internally, whether it's enabled or not, rather than
tracking that state via global variables.  Basically, global variables are
harder to reason about than instance variables, so I'd prefer to use instance
variables when possible.

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:144: if (allocator)
On 2016/04/25 20:37:17, bcwhite wrote:
> On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > In what case(s) might the allocator be null?  (And why is the answer here
> > different than the answer in chrome_browser_field_trials.cc?)
> 
> I don't imagine it will be.  But this code is so far away from where it is set
> that I prefer to be fail-safe.
> 
> Who knows what code paths could be across the myriad of Chrome products, not
to
> mention all the tests.

You're trading predictable crashes or DCHECK failures in the case of bugs, to
mysterious behavior in the case of bugs.  I'd much rather have a clear
indication that something is awry, rather than potentially papering over a real
issue.  If tests are failing, we should fix the tests, rather than bending the
production code to their whims.

https://codereview.chromium.org/1891913002/diff/220001/content/browser/browse...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/content/browser/browse...
content/browser/browser_main_runner.cc:241: if (allocator)
On 2016/04/25 20:37:17, bcwhite wrote:
> On 2016/04/25 19:48:41, Ilya Sherman wrote:
> > Under what circumstances can the allocator be null here?
> 
> It will be null if the allocator has been disabled.

Yikes, that is so confusing that it confused me even immediately after having
read through this CL.  I'd much rather test allocator->IsEnabled() here... or
else internally to WriteToPersistentLocation().

bcwhite

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_browser_field_trials.cc File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_browser_field_trials.cc#newcode42 chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName); On 2016/04/25 21:12:10, Ilya Sherman wrote: > On ...

4 years, 8 months ago (2016-04-26 13:32:30 UTC) #20

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName);
On 2016/04/25 21:12:10, Ilya Sherman wrote:
> On 2016/04/25 20:37:17, bcwhite wrote:
> > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > On 2016/04/22 15:18:05, bcwhite wrote:
> > > > On 2016/04/20 17:10:33, Ilya Sherman wrote:
> > > > > Could you pass the destination file name here, so that it is indeed
> passed
> > > to
> > > > > the constructor, and therefore possible to mark as const?
> > > > 
> > > > This has been moved to it's rightful final resting place in
> > > > browser_main_runner.cc.  The pathname can't be set from content/ because
> it
> > > > doesn't have access to the destination directory as that is defined in
> > > chrome/.
> > > 
> > > Is there not an embedder interface that could be used to provide the
> > destination
> > > directory?  That would be the more typical design structure for accessing
> data
> > > that's only known to the chrome/ layer.
> > 
> > I don't understand what you mean by that.
> 
> For example, the ChromeMetricsServiceClient class implements the
> metrics::MetricsServiceClient interface to provide data to the //metrics
> component, even though that data is only available from the //chrome layer.

When things can be instantiated at any time, that's possible.  This has to be
instantiated as early in the start-up sequence as possible.

Chrome/ code could later hook into the object to inject an object that provides
this kind of information but that's not really any different than just injecting
the information when there is only one entry.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:755: // Stop if no destination is
set, perhaps because it has not been enabled.
On 2016/04/25 21:12:11, Ilya Sherman wrote:
> On 2016/04/25 20:37:17, bcwhite wrote:
> > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > I was kind of expecting that you'd check the enabled state explicitly, and
> > > return early (sans warning) in just that case.
> > 
> > "Enabled" has nothing to do with being able to save the state.  It could
have
> > been enabled and then disabled.  If it was never enabled then the amount of
> data
> > to save will be very small.
> 
> Why would we enable and then subsequently disable it?  The only reason we even
> have enable/disable state both as viable possibilities is that we're not
> shipping to 100% of users yet.  But that's a one-time decision, and should
only
> be able to flip in a single direction.

The PHA is also used in setup.exe and in subprocesses.  In those cases, they
always start enabled and remain that way.  The many tests that use the PHA also
work that way.  To start disabled would to go against the standard use-case.

Supporting enabled/disabled is a hack to deal with the limitations of not being
able to determine "should I" or "shouldn't I" at the moment when the decision
needs to be made.

When the experiment is removed, I expect enable/disable will also be removed. 
Perhaps Activate/Deactivate would have been a better choice in names.  It's
always operational; it's just not always hooked in.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:758: << " to file because no
location was set.";
On 2016/04/25 21:12:11, Ilya Sherman wrote:
> On 2016/04/25 20:37:17, bcwhite wrote:
> > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > I still think this should be an error rather than a warning.  What is a
code
> > > path that could result in WriteToPersistentLocation() being called without
a
> > > persistent location being set, and why is it desirable for that code path
to
> > > exist?
> > 
> > Unlike below, where there is a specific local failure, there is no "error"
> here.
> >  This condition is dealing, safely, with how an external caller is using
this
> > module.  This module shouldn't be judging if something outside is wrong.
> 
> If I'm understanding you correctly, then I disagree: IMO this module
absolutely
> should document its invariants, and fail ungracefully if those invariants are
> not satisfied.  I would much rather find broken invariants due to failing
> DCHECKs and crash reports rather than by eventually noticing that we seem to
> sometimes be dropping data on the floor.

I can't say I agree but I understand your point.  Done.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:398: static void Disable();
On 2016/04/25 21:12:11, Ilya Sherman wrote:
> On 2016/04/25 20:37:17, bcwhite wrote:
> > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > Rather than having both an Enable() and a Disable() method, I'd prefer to
> just
> > > have a single Enable() method, since disabled is the default state.
> > 
> > The default is "enabled" because that is the normal way of operating.  Only
> this
> > special case of an experiment creates the allocator but doesn't use it.
> > 
> > I could change it to "Enabled(bool enable)" if you prefer.
> 
> Sorry, I don't follow.  How is the experiment a special case?  There's only a
> single allocator, and it's currently disabled by default, and enabled for X%
of
> users (for some value of X).  Am I understanding that correctly?

See discussion in persistent_histogram_allocator.cc...

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:402: static bool IsEnabled();
On 2016/04/25 21:12:11, Ilya Sherman wrote:
> On 2016/04/25 20:37:17, bcwhite wrote:
> > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > This method appears to be unused.
> > 
> > I was using it...  Code must have changed.  Still, seems like it's good to
be
> > symmetrical.
> 
> I strongly disagree that it's worthwhile to land dead code.  This adds work
for
> the compiler, for maintainers of the code, etc., with no clear benefits.  If
the
> code is ever needed, we can add it at that time.

Done.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:410: static
GlobalHistogramAllocator* GetEvenIfDisabled();
On 2016/04/25 21:12:11, Ilya Sherman wrote:
> On 2016/04/25 20:37:17, bcwhite wrote:
> > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > IMO this method is not needed.  Get() should always return the allocator,
> and
> > > the enabled/disabled state should control how the allocator behaves.  That
> > said,
> > > it's fairly surprising that some code expects to work with the allocator
> even
> > > when it's disabled.  Is enabled/disabled the wrong metaphor here?  Or
should
> > the
> > > code that tries to use the allocator when its disabled be refactored so
that
> > it
> > > uses some other object, which is always enabled?
> > 
> > Get() always returns an active allocator.  Existing code makes this call to
> know
> > whether to allocate persistantly or not.  If there is no persistent
allocator,
> > this returns null.  Disabling the allocator acheives the same result.
> > 
> > However, there needs to be a way to get an allocator (even if disabled) if
one
> > exists in order to call other methods on it.  It's not uncommen for objects
to
> > be accessed for some things even when they're not "active" in their primary
> > role.
> 
> IMO this is a poor design pattern, and I'd much rather separate out those
roles
> into distinct classes.  I'll admit that I haven't deeply thought about the
> specific method calls involved here; so if you disagree, please feel free to
> explain to me why this design makes the most sense here.

The whole GlobalHistogramAllocator is already separate out from the main class
to support all things "global".  

Once the experiment is over, this call can be removed along with enable/disable.

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:35:
base::GlobalHistogramAllocator::Enable();
On 2016/04/25 21:12:11, Ilya Sherman wrote:
> On 2016/04/25 20:37:17, bcwhite wrote:
> > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > It seems odd to have this as a static method.  Why not just Get() the
> > allocator
> > > below, and then call Enable() on it?
> > 
> > I considered both.
> > 
> > The general differentiation has been that methods that operate on the
> allocator
> > object are normal while methods that join it to the global (singleton) scope
> are
> > static.
> > 
> > The "global" allocator object knows nothing about whether it's tied into the
> > metrics system, just that it needs some additional code to support that
role.
> > 
> > Enable/Disable doesn't need any knowledge from inside a
> GlobalHistogramAllocator
> > object.  It does some management on a global instance of such an object but
> > makes no call into it.
> 
> I don't really follow, sorry.  In any case, I don't care so much about whether
> the method itself is static or not.  But I do think it makes more sense for
the
> allocator to track, internally, whether it's enabled or not, rather than
> tracking that state via global variables.  Basically, global variables are
> harder to reason about than instance variables, so I'd prefer to use instance
> variables when possible.

My style matches yours but I've been previously asked specifically to use
anonymous globals where possible so that's why it's done this way.

To internally track enabled/disabled would require it added to the
PersistentHistogramAllocator main (base) class even though it doesn't really
make sense to be there.  It would also require additional plumbing in the
callers which generally don't expect such a call to fail; they instead expect
that an allocator may not exist at all (i.e. Get() may return null).

Again, significant complication for something that will likely be removed in the
future.

https://codereview.chromium.org/1891913002/diff/220001/content/browser/browse...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/content/browser/browse...
content/browser/browser_main_runner.cc:241: if (allocator)
On 2016/04/25 21:12:11, Ilya Sherman wrote:
> On 2016/04/25 20:37:17, bcwhite wrote:
> > On 2016/04/25 19:48:41, Ilya Sherman wrote:
> > > Under what circumstances can the allocator be null here?
> > 
> > It will be null if the allocator has been disabled.
> 
> Yikes, that is so confusing that it confused me even immediately after having
> read through this CL.  I'd much rather test allocator->IsEnabled() here... or
> else internally to WriteToPersistentLocation().

Acknowledged.

Ilya Sherman

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_browser_field_trials.cc File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_browser_field_trials.cc#newcode42 chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName); On 2016/04/26 13:32:29, bcwhite wrote: > On 2016/04/25 ...

4 years, 8 months ago (2016-04-26 20:01:24 UTC) #21

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName);
On 2016/04/26 13:32:29, bcwhite wrote:
> On 2016/04/25 21:12:10, Ilya Sherman wrote:
> > On 2016/04/25 20:37:17, bcwhite wrote:
> > > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > > On 2016/04/22 15:18:05, bcwhite wrote:
> > > > > On 2016/04/20 17:10:33, Ilya Sherman wrote:
> > > > > > Could you pass the destination file name here, so that it is indeed
> > passed
> > > > to
> > > > > > the constructor, and therefore possible to mark as const?
> > > > > 
> > > > > This has been moved to it's rightful final resting place in
> > > > > browser_main_runner.cc.  The pathname can't be set from content/
because
> > it
> > > > > doesn't have access to the destination directory as that is defined in
> > > > chrome/.
> > > > 
> > > > Is there not an embedder interface that could be used to provide the
> > > destination
> > > > directory?  That would be the more typical design structure for
accessing
> > data
> > > > that's only known to the chrome/ layer.
> > > 
> > > I don't understand what you mean by that.
> > 
> > For example, the ChromeMetricsServiceClient class implements the
> > metrics::MetricsServiceClient interface to provide data to the //metrics
> > component, even though that data is only available from the //chrome layer.
> 
> When things can be instantiated at any time, that's possible.  This has to be
> instantiated as early in the start-up sequence as possible.
> 
> Chrome/ code could later hook into the object to inject an object that
provides
> this kind of information but that's not really any different than just
injecting
> the information when there is only one entry.

Hmm, I guess I didn't look very carefully at where this setup was happening. 
Why are you choosing to implement this here, rather than a tiny bit later, in
ChromeBrowserMainParts::SetupMetricsAndFieldTrials(), where all of the other
metrics code is set up?

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:755: // Stop if no destination is
set, perhaps because it has not been enabled.
On 2016/04/26 13:32:30, bcwhite wrote:
> On 2016/04/25 21:12:11, Ilya Sherman wrote:
> > On 2016/04/25 20:37:17, bcwhite wrote:
> > > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > > I was kind of expecting that you'd check the enabled state explicitly,
and
> > > > return early (sans warning) in just that case.
> > > 
> > > "Enabled" has nothing to do with being able to save the state.  It could
> have
> > > been enabled and then disabled.  If it was never enabled then the amount
of
> > data
> > > to save will be very small.
> > 
> > Why would we enable and then subsequently disable it?  The only reason we
even
> > have enable/disable state both as viable possibilities is that we're not
> > shipping to 100% of users yet.  But that's a one-time decision, and should
> only
> > be able to flip in a single direction.
> 
> The PHA is also used in setup.exe and in subprocesses.  In those cases, they
> always start enabled and remain that way.  The many tests that use the PHA
also
> work that way.  To start disabled would to go against the standard use-case.
> 
> Supporting enabled/disabled is a hack to deal with the limitations of not
being
> able to determine "should I" or "shouldn't I" at the moment when the decision
> needs to be made.
> 
> When the experiment is removed, I expect enable/disable will also be removed. 
> Perhaps Activate/Deactivate would have been a better choice in names.  It's
> always operational; it's just not always hooked in.

Okay, I see your point about this code being used both by Chrome and setup.exe. 
Thanks for explaining that to me.

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1891913002/diff/220001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:398: static void Disable();
On 2016/04/26 13:32:30, bcwhite wrote:
> On 2016/04/25 21:12:11, Ilya Sherman wrote:
> > On 2016/04/25 20:37:17, bcwhite wrote:
> > > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > > Rather than having both an Enable() and a Disable() method, I'd prefer
to
> > just
> > > > have a single Enable() method, since disabled is the default state.
> > > 
> > > The default is "enabled" because that is the normal way of operating. 
Only
> > this
> > > special case of an experiment creates the allocator but doesn't use it.
> > > 
> > > I could change it to "Enabled(bool enable)" if you prefer.
> > 
> > Sorry, I don't follow.  How is the experiment a special case?  There's only
a
> > single allocator, and it's currently disabled by default, and enabled for X%
> of
> > users (for some value of X).  Am I understanding that correctly?
> 
> See discussion in persistent_histogram_allocator.cc...

Okay, yeah, I see your point.  Thanks.

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/220001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:35:
base::GlobalHistogramAllocator::Enable();
On 2016/04/26 13:32:30, bcwhite wrote:
> On 2016/04/25 21:12:11, Ilya Sherman wrote:
> > On 2016/04/25 20:37:17, bcwhite wrote:
> > > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > > It seems odd to have this as a static method.  Why not just Get() the
> > > allocator
> > > > below, and then call Enable() on it?
> > > 
> > > I considered both.
> > > 
> > > The general differentiation has been that methods that operate on the
> > allocator
> > > object are normal while methods that join it to the global (singleton)
scope
> > are
> > > static.
> > > 
> > > The "global" allocator object knows nothing about whether it's tied into
the
> > > metrics system, just that it needs some additional code to support that
> role.
> > > 
> > > Enable/Disable doesn't need any knowledge from inside a
> > GlobalHistogramAllocator
> > > object.  It does some management on a global instance of such an object
but
> > > makes no call into it.
> > 
> > I don't really follow, sorry.  In any case, I don't care so much about
whether
> > the method itself is static or not.  But I do think it makes more sense for
> the
> > allocator to track, internally, whether it's enabled or not, rather than
> > tracking that state via global variables.  Basically, global variables are
> > harder to reason about than instance variables, so I'd prefer to use
instance
> > variables when possible.
> 
> My style matches yours but I've been previously asked specifically to use
> anonymous globals where possible so that's why it's done this way.
> 
> To internally track enabled/disabled would require it added to the
> PersistentHistogramAllocator main (base) class even though it doesn't really
> make sense to be there.  It would also require additional plumbing in the
> callers which generally don't expect such a call to fail; they instead expect
> that an allocator may not exist at all (i.e. Get() may return null).
> 
> Again, significant complication for something that will likely be removed in
the
> future.

Okay, I agree that if the base PHA class doesn't normally need to track
enabled/disabled state, then an anonymous global is an ok compromise.

https://codereview.chromium.org/1891913002/diff/260001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:50: GlobalHistogramAllocator*
g_allocator_disabled;
Please use a boolean for enabled/disabled state, rather than having two separate
pointers.

https://codereview.chromium.org/1891913002/diff/260001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1891913002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:406: static
GlobalHistogramAllocator* GetEvenIfDisabled();
Looking at the uses, it looks like you could replace this method with a
GetName() method, which would get the allocator name even if it's disabled.  I
think that would be a less confusing API.  Heck, the name of the global
allocator could just be a static constant exported by this class, rather than
something passed to the constructor.

bcwhite

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_browser_field_trials.cc File chrome/browser/chrome_browser_field_trials.cc (right): https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_browser_field_trials.cc#newcode42 chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName); On 2016/04/26 20:01:24, Ilya Sherman wrote: > On ...

4 years, 8 months ago (2016-04-26 21:04:26 UTC) #22

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName);
On 2016/04/26 20:01:24, Ilya Sherman wrote:
> On 2016/04/26 13:32:29, bcwhite wrote:
> > On 2016/04/25 21:12:10, Ilya Sherman wrote:
> > > On 2016/04/25 20:37:17, bcwhite wrote:
> > > > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > > > On 2016/04/22 15:18:05, bcwhite wrote:
> > > > > > On 2016/04/20 17:10:33, Ilya Sherman wrote:
> > > > > > > Could you pass the destination file name here, so that it is
indeed
> > > passed
> > > > > to
> > > > > > > the constructor, and therefore possible to mark as const?
> > > > > > 
> > > > > > This has been moved to it's rightful final resting place in
> > > > > > browser_main_runner.cc.  The pathname can't be set from content/
> because
> > > it
> > > > > > doesn't have access to the destination directory as that is defined
in
> > > > > chrome/.
> > > > > 
> > > > > Is there not an embedder interface that could be used to provide the
> > > > destination
> > > > > directory?  That would be the more typical design structure for
> accessing
> > > data
> > > > > that's only known to the chrome/ layer.
> > > > 
> > > > I don't understand what you mean by that.
> > > 
> > > For example, the ChromeMetricsServiceClient class implements the
> > > metrics::MetricsServiceClient interface to provide data to the //metrics
> > > component, even though that data is only available from the //chrome
layer.
> > 
> > When things can be instantiated at any time, that's possible.  This has to
be
> > instantiated as early in the start-up sequence as possible.
> > 
> > Chrome/ code could later hook into the object to inject an object that
> provides
> > this kind of information but that's not really any different than just
> injecting
> > the information when there is only one entry.
> 
> Hmm, I guess I didn't look very carefully at where this setup was happening. 
> Why are you choosing to implement this here, rather than a tiny bit later, in
> ChromeBrowserMainParts::SetupMetricsAndFieldTrials(), where all of the other
> metrics code is set up?

It's important to start it as early as possible so that it catches as many
histograms as possible.  Once it's created, it can never be moved so if it gets
created on the heap before the allocator gets created, it'll never be
persistent.

https://codereview.chromium.org/1891913002/diff/260001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:50: GlobalHistogramAllocator*
g_allocator_disabled;
On 2016/04/26 20:01:24, Ilya Sherman wrote:
> Please use a boolean for enabled/disabled state, rather than having two
separate
> pointers.

Done.

https://codereview.chromium.org/1891913002/diff/260001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.h (right):

https://codereview.chromium.org/1891913002/diff/260001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.h:406: static
GlobalHistogramAllocator* GetEvenIfDisabled();
On 2016/04/26 20:01:24, Ilya Sherman wrote:
> Looking at the uses, it looks like you could replace this method with a
> GetName() method, which would get the allocator name even if it's disabled.  I
> think that would be a less confusing API.  Heck, the name of the global
> allocator could just be a static constant exported by this class, rather than
> something passed to the constructor.

The name can't be a constant here because the name changes depending on what
it's global for:  BrowserMetrics, SetupMetrics, RendererMetrics, etc.

I looked at GetName() but that seems odd since there is already a non-static
Name() method that returns the same thing.  It would also muddy the distinction
between normal/static.  Normal methods go into the object while static methods
manage the singleton's connection to global activity.

Part of the reason of creating the GlobalHistogramManager derived class was to
move Import() away from being a static method that acted on the global object.

I also think GetEvenIfDisabled() works better for the future because when it
stops being an experiment, the only change necessary will be to replace calls to
it with calls to Get().

Alexei Svitkine (slow)

On 2016/04/22 16:04:36, bcwhite wrote: > > - If we're reading the metrics from the ...

4 years, 8 months ago (2016-04-26 21:23:30 UTC) #23

bcwhite

On 2016/04/26 21:23:30, Alexei Svitkine wrote: > On 2016/04/22 16:04:36, bcwhite wrote: > > > ...

4 years, 8 months ago (2016-04-26 21:30:06 UTC) #24

Alexei Svitkine (slow)

Okay - I've added Ilya to that meeting. On Tue, Apr 26, 2016 at 5:30 ...

4 years, 7 months ago (2016-04-27 13:48:11 UTC) #25

Okay - I've added Ilya to that meeting.

On Tue, Apr 26, 2016 at 5:30 PM, <bcwhite@chromium.org> wrote:

> On 2016/04/26 21:23:30, Alexei Svitkine wrote:
> > On 2016/04/22 16:04:36, bcwhite wrote:
> > > > - If we're reading the metrics from the last session on the next
> start up,
> > do
> > > we
> > > > have a mechanism to ensure they're associated with the previous
> system
> > > profile?
> > > > We don't want to report metrics from the previous session with the
> current
> > > > (version, experiments, etc) data.
> > >
> > > Not yet. The allocators can preserve this information but there has to
> be
> > some
> > > way to specify that during the upload.
> >
> > I don't think we should enable this until we have this mechanism in
> place.
> > Otherwise, the data will incorrect and surprising.
> >
> > For example, if a histogram is only logged when a certain trial is
> enabled, if
> > you're now picking it up in the next session from the previous process
> where
> the
> > trial is in a different state, people will see it getting logged for a
> trial
> > that's disabled.
> >
> > So please work on getting that mechanism in place before enabling this
> (even
> > behind a trial). Note: I'm fine with CLs being landed in the meanwhile to
> > support this, but we shouldn't enable this for users until we have the
> mechanism
> > to associate the metrics from the previous session with the system
> profile
> from
> > the previous session. (On the other hand, you could add histograms
> *about*
> those
> > and log them - i.e. counting how many histograms we received from such a
> > persistent file from the last session.)
>
> As you noted previously, the only values that would persist are those that
> changed after the final metrics upload that is done as part of the shutdown
> process.
>
> I was thinking we could discuss how to do this when we meet Thursday
> morning.
>
>
> https://codereview.chromium.org/1891913002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Ilya Sherman

Probably makes sense to discuss in person tomorrow. Just jotting down some thoughts now so ...

4 years, 7 months ago (2016-04-27 20:31:33 UTC) #26

Probably makes sense to discuss in person tomorrow.  Just jotting down some
thoughts now so as to remember them later.

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
File chrome/browser/chrome_browser_field_trials.cc (right):

https://codereview.chromium.org/1891913002/diff/140001/chrome/browser/chrome_...
chrome/browser/chrome_browser_field_trials.cc:42: kAllocatorName);
On 2016/04/26 21:04:26, bcwhite wrote:
> On 2016/04/26 20:01:24, Ilya Sherman wrote:
> > On 2016/04/26 13:32:29, bcwhite wrote:
> > > On 2016/04/25 21:12:10, Ilya Sherman wrote:
> > > > On 2016/04/25 20:37:17, bcwhite wrote:
> > > > > On 2016/04/25 19:48:40, Ilya Sherman wrote:
> > > > > > On 2016/04/22 15:18:05, bcwhite wrote:
> > > > > > > On 2016/04/20 17:10:33, Ilya Sherman wrote:
> > > > > > > > Could you pass the destination file name here, so that it is
> indeed
> > > > passed
> > > > > > to
> > > > > > > > the constructor, and therefore possible to mark as const?
> > > > > > > 
> > > > > > > This has been moved to it's rightful final resting place in
> > > > > > > browser_main_runner.cc.  The pathname can't be set from content/
> > because
> > > > it
> > > > > > > doesn't have access to the destination directory as that is
defined
> in
> > > > > > chrome/.
> > > > > > 
> > > > > > Is there not an embedder interface that could be used to provide the
> > > > > destination
> > > > > > directory?  That would be the more typical design structure for
> > accessing
> > > > data
> > > > > > that's only known to the chrome/ layer.
> > > > > 
> > > > > I don't understand what you mean by that.
> > > > 
> > > > For example, the ChromeMetricsServiceClient class implements the
> > > > metrics::MetricsServiceClient interface to provide data to the //metrics
> > > > component, even though that data is only available from the //chrome
> layer.
> > > 
> > > When things can be instantiated at any time, that's possible.  This has to
> be
> > > instantiated as early in the start-up sequence as possible.
> > > 
> > > Chrome/ code could later hook into the object to inject an object that
> > provides
> > > this kind of information but that's not really any different than just
> > injecting
> > > the information when there is only one entry.
> > 
> > Hmm, I guess I didn't look very carefully at where this setup was happening.

> > Why are you choosing to implement this here, rather than a tiny bit later,
in
> > ChromeBrowserMainParts::SetupMetricsAndFieldTrials(), where all of the other
> > metrics code is set up?
> 
> It's important to start it as early as possible so that it catches as many
> histograms as possible.  Once it's created, it can never be moved so if it
gets
> created on the heap before the allocator gets created, it'll never be
> persistent.

Are there actually histograms that are recorded between
BrowserMainRunnerImpl::Initialize() and when the rest of the metrics
initialization happens?  Similarly, are there any metrics recorded after the
metrics system is generally shut down?  jar@ thought pretty carefully about
where metrics initialization and shutdown happen, and I would be kind of
surprised if there were metrics being recorded before/after those.  (Especially
after, since they'd be uniformly discarded currently.)

https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:50: bool g_allocator_enabled;
Sorry, I'm going to go back to my previous question about this: Why wouldn't it
make sense to track internally to the GlobalHistogramAllocator object?  You
mentioned that you'd need to plumb it through to the base class, but I don't
actually follow why that would be necessary.

https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:736: #else
Please DCHECK(g_allocator_enabled) in this code.

bcwhite

Description was changed from ========== Persist browser metrics to disk at exit and read them ...

4 years, 7 months ago (2016-04-29 17:14:27 UTC) #29

bcwhite

https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persistent_histogram_allocator.cc File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persistent_histogram_allocator.cc#newcode50 base/metrics/persistent_histogram_allocator.cc:50: bool g_allocator_enabled; On 2016/04/27 20:31:33, Ilya Sherman wrote: > ...

4 years, 7 months ago (2016-05-02 14:46:47 UTC) #31

Ilya Sherman

Thanks, Brian. https://codereview.chromium.org/1891913002/diff/420001/base/metrics/histogram_base.h File base/metrics/histogram_base.h (right): https://codereview.chromium.org/1891913002/diff/420001/base/metrics/histogram_base.h#newcode208 base/metrics/histogram_base.h:208: // method or to SnapshotDelta() should be ...

4 years, 7 months ago (2016-05-04 06:48:08 UTC) #32

bcwhite

https://codereview.chromium.org/1891913002/diff/420001/base/metrics/histogram_base.h File base/metrics/histogram_base.h (right): https://codereview.chromium.org/1891913002/diff/420001/base/metrics/histogram_base.h#newcode208 base/metrics/histogram_base.h:208: // method or to SnapshotDelta() should be done as ...

4 years, 7 months ago (2016-05-04 14:12:51 UTC) #33

https://codereview.chromium.org/1891913002/diff/420001/base/metrics/histogram...
File base/metrics/histogram_base.h (right):

https://codereview.chromium.org/1891913002/diff/420001/base/metrics/histogram...
base/metrics/histogram_base.h:208: // method or to SnapshotDelta() should be
done as the result would include
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> nit: "done as" -> "done, as"

I believe that would be incorrect.  You'd have the comma if the clauses were
reversed:  As the result would include data previously returned, no further
calls...

https://codereview.chromium.org/1891913002/diff/420001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager.h (right):

https://codereview.chromium.org/1891913002/diff/420001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.h:76: void PrepareFinalDelta(const
HistogramBase* histogram);
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> nit: Only the TakingOwnership() method is used, so please omit this method. 
It
> can always be added later if a use case comes up for it.

<shudder>  I hate inconsistent APIs.  But done.

https://codereview.chromium.org/1891913002/diff/420001/base/metrics/histogram...
File base/metrics/histogram_unittest.cc (right):

https://codereview.chromium.org/1891913002/diff/420001/base/metrics/histogram...
base/metrics/histogram_unittest.cc:188: std::unique_ptr<HistogramSamples>
samples = histogram->SnapshotFinalDelta();
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> Please include an additional test where the code first calls SnapshotDelta(). 
> It's important to test both the codepath where there was no delta previously
> recorded, and where there was one.

Done.

https://codereview.chromium.org/1891913002/diff/420001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/420001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:50: bool g_allocator_enabled;
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> Hmm, where are these variables initialized?

The "enabled" flag is initialized when g_allocator is loaded.

Global variables are always initialized to zero if there is no explicit value.

I'll add initializers to make it clear.

https://codereview.chromium.org/1891913002/diff/420001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:743: // Stop if no destination is
set, perhaps because it has not been enabled.
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> nit: Please omit the "perhaps because it has not been enabled" clause, since
> that's DCHECKed above.

Done.

https://codereview.chromium.org/1891913002/diff/420001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/1891913002/diff/420001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:144: if (allocator)
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> nit: This if stmt seems unnecessary -- the allocator should always exist by
this
> point, right?

Based on current code, a full browser instantiation should but that's done far
away and isn't something I feel we should rely upon.  Get() is explicitly
allowed to return null.  A test, for example, would likely not create an
allocator.  The single-instruction test condition is worth avoiding some
potential headaches.

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:282: // before the first call to
this method.
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> Hrm, why not clear them when they're reported?

They have to live until the reporting is complete which goes beyond when the
method that loads them to the HSM returns.  Ownership of the (heap) histogram
objects was passed to the HSM but they still point to memory managed by an
Allocator and so that can't be released until some time later.

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:284: for (std::unique_ptr<FileInfo>&
file : files_for_previous_)
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> nit: "const auto&"?

Done.

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:296: auto temp = iter++;
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> "iter++" is strongly discouraged, because it can be significantly less
efficient
> than "++iter".  Please instead either "++iter" or "iter = 
> files_for_previous_.erase(temp)" at the end of the loop, depending on the if
> stmt. (For this particular loop, the efficiency difference is probably not
> significant, but IIRC it's recommended by the style guide to always use
> preincrement, so that you can't accidentally mess up when it does matter.)

Post-increment is definitely more efficient but ++iter won't work as it would
leave temp pointing to the wrong place.  I could do "temp=iter; ++iter" but
that's the same as "temp=iter++".

I can assign it from the erase() call except that's conditional so would require
an additional "else ++iter".  Maintenance wise, I think that would be a
detriment.  The increment should be at the top where the loop is defined for
clarity and less chance of errors in the future.

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:301: // Do the check in-line.
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> Hmm, this sounds a bit disconcerting.  Are we doing file I/O on the main
thread?
>  If so, let's ponder a way to structure this code so that we don't need to do
> so.

This method is called during startup and must return before the browser can
show.  Even if it were possible to move the checks to a background thread, the
main thread would still have to block until the background thread completed its
work.

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:354: for (std::unique_ptr<FileInfo>&
file : files_for_previous_) {
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> nit: "const auto&"?

Done.

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:358:
CreateAllocatorForFile(file.get());
On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> Is this doing file I/O on the main thread?

Yes, for the same reason as HasInitialStabilityMetrics().  It's not actually the
UI thread because at this point there is no UI.

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
File components/metrics/file_metrics_provider.h (right):

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.h:167: FileInfoList
files_for_previous_;
On 2016/05/04 06:48:08, Ilya Sherman (Away May 5-15) wrote:
> nit: s/for_previous/for_previous_runs

Done.

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/met...
File components/metrics/metrics_provider.h (right):

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/met...
components/metrics/metrics_provider.h:80: // Called during collection of
"stability" metrics to explicitly load
On 2016/05/04 06:48:08, Ilya Sherman (Away May 5-15) wrote:
> It's not actually "stability" that's relevant her.  Rather, this hook is for
the
> initial metrics log, associated with the previous session, right?  I'd clarify
> the method name to mention "initial" rather than "stability", and not mention
> "stability" in the docs.

Done.

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/met...
components/metrics/metrics_service.cc:1169:
provider->RecordStabilityHistogramSnapshots(&histogram_snapshot_manager_);
On 2016/05/04 06:48:08, Ilya Sherman (Away May 5-15) wrote:
> This doesn't seem like the right place to add this code.  You're interested in
> just recording these histograms for the initial log, right?

Correct.  This method is called only from PrepareInitialStabilityLog().

> I think it might be worthwhile to split this CL back into two parts, with the
> first CL just for saving the metrics, and the second for actually uploading
> them.  That way, you can land the first part while we figure out the details
for
> the second part.

I could but there's nothing waiting on the (small) part that writes the file
other than a small change to setup.exe to call it rather than do the same thing
in its own code.

https://codereview.chromium.org/1891913002/diff/420001/content/browser/browse...
File content/browser/browser_main_runner.cc (right):

https://codereview.chromium.org/1891913002/diff/420001/content/browser/browse...
content/browser/browser_main_runner.cc:66: // BrowserMetrics usage peaked around
1.9MiB as of 2016-02-20.
On 2016/05/04 06:48:08, Ilya Sherman (Away May 5-15) wrote:
> When you say that it peaked around 1.9MiB, do you mean on a test run on your
> machine, or globally across all Chrome users?

As reported in UMA across all (enrolled) users.

https://codereview.chromium.org/1891913002/diff/420001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1891913002/diff/420001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:55578: +    Records attempts to access a
file for the purpose of extracting metrics.
On 2016/05/04 06:48:08, Ilya Sherman (Away May 5-15) wrote:
> nit: s/metrics/stability metrics?

Done.

Ilya Sherman

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/file_metrics_provider.cc File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1891913002/diff/420001/components/metrics/file_metrics_provider.cc#newcode282 components/metrics/file_metrics_provider.cc:282: // before the first call to this method. On ...

4 years, 7 months ago (2016-05-05 07:22:37 UTC) #34

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:282: // before the first call to
this method.
On 2016/05/04 14:12:51, bcwhite wrote:
> On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> > Hrm, why not clear them when they're reported?
> 
> They have to live until the reporting is complete which goes beyond when the
> method that loads them to the HSM returns.  Ownership of the (heap) histogram
> objects was passed to the HSM but they still point to memory managed by an
> Allocator and so that can't be released until some time later.

FileInfo just provides file metadata, right?  How does that relate to the
Allocator?

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:296: auto temp = iter++;
On 2016/05/04 14:12:51, bcwhite wrote:
> On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> > "iter++" is strongly discouraged, because it can be significantly less
> efficient
> > than "++iter".  Please instead either "++iter" or "iter = 
> > files_for_previous_.erase(temp)" at the end of the loop, depending on the if
> > stmt. (For this particular loop, the efficiency difference is probably not
> > significant, but IIRC it's recommended by the style guide to always use
> > preincrement, so that you can't accidentally mess up when it does matter.)
> 
> Post-increment is definitely more efficient but ++iter won't work as it would
> leave temp pointing to the wrong place.  I could do "temp=iter; ++iter" but
> that's the same as "temp=iter++".
> 
> I can assign it from the erase() call except that's conditional so would
require
> an additional "else ++iter".  Maintenance wise, I think that would be a
> detriment.  The increment should be at the top where the loop is defined for
> clarity and less chance of errors in the future.

I disagree pretty strongly on this point.  For a loop that has an empty
increment statement, I expect to see the increment happen at the end of the
loop, rather than at the beginning.  (Also, unpacking the line "auto temp =
iter++" involves a lot of work, as I have to figure out what the semantics of
"temp" are, and I have to think about the semantics of pre- vs. post-increment. 
That's a lot of mental overhead when reading this line of code.)

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:301: // Do the check in-line.
On 2016/05/04 14:12:51, bcwhite wrote:
> On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> > Hmm, this sounds a bit disconcerting.  Are we doing file I/O on the main
> thread?
> >  If so, let's ponder a way to structure this code so that we don't need to
do
> > so.
> 
> This method is called during startup and must return before the browser can
> show.  Even if it were possible to move the checks to a background thread, the
> main thread would still have to block until the background thread completed
its
> work.

Erm, why does this block startup?  If that's true, that sounds like a (pretty
egregious) bug.

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager.cc (right):

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:38:
CHECK(!(hash_and_info.second.inconsistencies &
Should these be DCHECKs?  It's odd for them to be checks if they're only
executed when DCHECK is on.

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
File base/metrics/histogram_unittest.cc (right):

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
base/metrics/histogram_unittest.cc:203: }
Please test both the case where SnapshotFinalDelta() is called after a call to
SnapshotDelta(), and where it is called without any prior calls to
SanpshotDelta().  That is, please add an additional TEST_P for the other case.

https://codereview.chromium.org/1891913002/diff/440001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1891913002/diff/440001/components/metrics/met...
components/metrics/metrics_service.cc:1161: auto end =
base::StatisticsRecorder::end();
nit: Please move this so that it's scoped to within the for loop.

https://codereview.chromium.org/1891913002/diff/440001/components/metrics/met...
components/metrics/metrics_service.cc:1169:
provider->RecordInitialHistogramSnapshots(&histogram_snapshot_manager_);
Providers might choose to include metrics that aren't stability metrics, in case
of a crash, right?  So, what's the purpose of checking the
kUmaStabilityHistogramFlag above?

bcwhite

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/file_metrics_provider.cc File components/metrics/file_metrics_provider.cc (right): https://codereview.chromium.org/1891913002/diff/420001/components/metrics/file_metrics_provider.cc#newcode282 components/metrics/file_metrics_provider.cc:282: // before the first call to this method. On ...

4 years, 7 months ago (2016-05-05 16:01:17 UTC) #36

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:282: // before the first call to
this method.
On 2016/05/05 07:22:37, Ilya Sherman (Away May 5-15) wrote:
> On 2016/05/04 14:12:51, bcwhite wrote:
> > On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> > > Hrm, why not clear them when they're reported?
> > 
> > They have to live until the reporting is complete which goes beyond when the
> > method that loads them to the HSM returns.  Ownership of the (heap)
histogram
> > objects was passed to the HSM but they still point to memory managed by an
> > Allocator and so that can't be released until some time later.
> 
> FileInfo just provides file metadata, right?  How does that relate to the
> Allocator?

It has all the information related to the file, including the MemoryMappedFile
object, the file contents, and the PersistentHistogramAllocator object using
those (one or the other, actually).

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:296: auto temp = iter++;
> > Post-increment is definitely more efficient but ++iter won't work as it
would
> > leave temp pointing to the wrong place.  I could do "temp=iter; ++iter" but
> > that's the same as "temp=iter++".
> > 
> > I can assign it from the erase() call except that's conditional so would
> require
> > an additional "else ++iter".  Maintenance wise, I think that would be a
> > detriment.  The increment should be at the top where the loop is defined for
> > clarity and less chance of errors in the future.
> 
> I disagree pretty strongly on this point.  For a loop that has an empty
> increment statement, I expect to see the increment happen at the end of the
> loop, rather than at the beginning.

My expectations differ from yours, then, because if the increment could be done
at the end of the loop then it would have been defined in the if() itself.  It
can't be done at the end because the iterator may be invalidated in the body.

> (Also, unpacking the line "auto temp =
> iter++" involves a lot of work, as I have to figure out what the semantics of
> "temp" are, and I have to think about the semantics of pre- vs.
post-increment. 
> That's a lot of mental overhead when reading this line of code.)

The mental exercise will be greater parsing an if/else condition with one clause
doing an assignment from the return of erase() and the other doing a
pre-increment.

At least pre/post increment has consistent and long-standing known behavior
(back to the very first days of C).  I had to look up the return-code of
erase()... it changes depending on the parameter types.

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:301: // Do the check in-line.
On 2016/05/05 07:22:37, Ilya Sherman (Away May 5-15) wrote:
> On 2016/05/04 14:12:51, bcwhite wrote:
> > On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> > > Hmm, this sounds a bit disconcerting.  Are we doing file I/O on the main
> > thread?
> > >  If so, let's ponder a way to structure this code so that we don't need to
> do
> > > so.
> > 
> > This method is called during startup and must return before the browser can
> > show.  Even if it were possible to move the checks to a background thread,
the
> > main thread would still have to block until the background thread completed
> its
> > work.
> 
> Erm, why does this block startup?  If that's true, that sounds like a (pretty
> egregious) bug.

All metrics work is done on the main UI thread.  A once talked with Alexei about
moving it to a background thread but he felt is would be difficult and not worth
it.

The check for and reporting of InitialStabilityMetrics is done only once at
startup.  Even if the I/O could be moved to a background thread, there's no
guarantee it would be complete before this call is received and thus it would
still have to block.

The dumping of initial metrics is called in
ChromeBrowserMainParts::PreCreateThreadsImpl().  I don't see any way to avoid
delaying startup for reporting this.

I agree, though, that blocking startup could be a problem but should be
manageable if it is limited to only crashes and hangs.  We're not concerned with
saving metrics during a normal shutdown since metrics are gathered immediately
before a clean exit anyway.  We just make sure the metrics file is either never
written or deleted on exit.  Startup after a clean shutdown will do a stat()
looking for it but that is all.  If there was a crash/hang, then we pay the
penalty of cleaning up after the previous run.

I've added some timing histograms so we can at least have real information about
the severity of this problem before we decide exactly what to do about it.

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
File base/metrics/histogram_snapshot_manager.cc (right):

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
base/metrics/histogram_snapshot_manager.cc:38:
CHECK(!(hash_and_info.second.inconsistencies &
On 2016/05/05 07:22:37, Ilya Sherman (Away May 5-15) wrote:
> Should these be DCHECKs?  It's odd for them to be checks if they're only
> executed when DCHECK is on.

They could be.  Didn't seem to be any difference.  Done.

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
File base/metrics/histogram_unittest.cc (right):

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
base/metrics/histogram_unittest.cc:203: }
On 2016/05/05 07:22:37, Ilya Sherman (Away May 5-15) wrote:
> Please test both the case where SnapshotFinalDelta() is called after a call to
> SnapshotDelta(), and where it is called without any prior calls to
> SanpshotDelta().  That is, please add an additional TEST_P for the other case.

What is the benefit of that?  There is no difference in the code paths between
the two.  In one case, the "previously logged" samples are zero and in the other
it is not, but that is only data.

https://codereview.chromium.org/1891913002/diff/440001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1891913002/diff/440001/components/metrics/met...
components/metrics/metrics_service.cc:1161: auto end =
base::StatisticsRecorder::end();
On 2016/05/05 07:22:37, Ilya Sherman (Away May 5-15) wrote:
> nit: Please move this so that it's scoped to within the for loop.

Done.

https://codereview.chromium.org/1891913002/diff/440001/components/metrics/met...
components/metrics/metrics_service.cc:1169:
provider->RecordInitialHistogramSnapshots(&histogram_snapshot_manager_);
On 2016/05/05 07:22:37, Ilya Sherman (Away May 5-15) wrote:
> Providers might choose to include metrics that aren't stability metrics, in
case
> of a crash, right?  So, what's the purpose of checking the
> kUmaStabilityHistogramFlag above?

You mean on line 1163?  That's always been there and means that only a small
subset of all the histograms known to the StatisticsRecorder (basically
everything created by this particular process) will get reported here.

In the case of the providers, it's up to them what metrics to include.  The
different calls of RecordInitialHsitogramSnapshots() vs
RecordHistogramSnapshots() allows providers to choose.

Ilya Sherman

I guess I'm willing to agree to disagree about the remaining points of disagreement, so ...

4 years, 7 months ago (2016-05-05 22:22:53 UTC) #38

I guess I'm willing to agree to disagree about the remaining points of
disagreement, so LGTM % a final few nits.  Alexei, you might want to take one
more look just as an extra sanity check.

https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:50: bool g_allocator_enabled;
On 2016/05/02 14:46:47, bcwhite wrote:
> On 2016/04/27 20:31:33, Ilya Sherman wrote:
> > Sorry, I'm going to go back to my previous question about this: Why wouldn't
> it
> > make sense to track internally to the GlobalHistogramAllocator object?  You
> > mentioned that you'd need to plumb it through to the base class, but I don't
> > actually follow why that would be necessary.
> 
> It would have to be tested by CreateHistogram() which is in the base class and
> have it fail if not enabled.  Or the method would have to be overridden here,
> which would also mean it should probably be virtual.
> 
> Code calling this, however, doesn't expect CreateHistogram to fail unless
there
> is a problem, such as corrupted or full.  Calling code does, however, expect
> Get() to return null if it's not enabled so by testing g_enabled in Get(), all
> the operational semantics remain the same.

g_allocator_enabled is only accessed internally to this file, where every access
could be replaced by an access to g_allocator->enabled_.  What am I missing? 
(And, sorry, I thought I'd replied to this previously, but I guess I missed it.)

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/420001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:301: // Do the check in-line.
On 2016/05/05 16:01:17, bcwhite wrote:
> On 2016/05/05 07:22:37, Ilya Sherman (Away May 5-15) wrote:
> > On 2016/05/04 14:12:51, bcwhite wrote:
> > > On 2016/05/04 06:48:07, Ilya Sherman (Away May 5-15) wrote:
> > > > Hmm, this sounds a bit disconcerting.  Are we doing file I/O on the main
> > > thread?
> > > >  If so, let's ponder a way to structure this code so that we don't need
to
> > do
> > > > so.
> > > 
> > > This method is called during startup and must return before the browser
can
> > > show.  Even if it were possible to move the checks to a background thread,
> the
> > > main thread would still have to block until the background thread
completed
> > its
> > > work.
> > 
> > Erm, why does this block startup?  If that's true, that sounds like a
(pretty
> > egregious) bug.
> 
> All metrics work is done on the main UI thread.  A once talked with Alexei
about
> moving it to a background thread but he felt is would be difficult and not
worth
> it.
> 
> The check for and reporting of InitialStabilityMetrics is done only once at
> startup.  Even if the I/O could be moved to a background thread, there's no
> guarantee it would be complete before this call is received and thus it would
> still have to block.
> 
> The dumping of initial metrics is called in
> ChromeBrowserMainParts::PreCreateThreadsImpl().  I don't see any way to avoid
> delaying startup for reporting this.
> 
> I agree, though, that blocking startup could be a problem but should be
> manageable if it is limited to only crashes and hangs.  We're not concerned
with
> saving metrics during a normal shutdown since metrics are gathered immediately
> before a clean exit anyway.  We just make sure the metrics file is either
never
> written or deleted on exit.  Startup after a clean shutdown will do a stat()
> looking for it but that is all.  If there was a crash/hang, then we pay the
> penalty of cleaning up after the previous run.
> 
> I've added some timing histograms so we can at least have real information
about
> the severity of this problem before we decide exactly what to do about it.

Hmm, I see that you're right.  I still find this pretty surprising, but agree
that it's out of scope for this CL.  Thanks for adding the timing histograms.

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
File base/metrics/histogram_unittest.cc (right):

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
base/metrics/histogram_unittest.cc:203: }
On 2016/05/05 16:01:17, bcwhite wrote:
> On 2016/05/05 07:22:37, Ilya Sherman (Away May 5-15) wrote:
> > Please test both the case where SnapshotFinalDelta() is called after a call
to
> > SnapshotDelta(), and where it is called without any prior calls to
> > SanpshotDelta().  That is, please add an additional TEST_P for the other
case.
> 
> What is the benefit of that?  There is no difference in the code paths between
> the two.  In one case, the "previously logged" samples are zero and in the
other
> it is not, but that is only data.

Without looking at the implementation, it's not hard to imagine having different
code paths for the case where there is and where there isn't a prior call ot
SnapshotDelta().  I prefer to write tests defensively, since the implementation
can change.

https://codereview.chromium.org/1891913002/diff/440001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1891913002/diff/440001/components/metrics/met...
components/metrics/metrics_service.cc:1169:
provider->RecordInitialHistogramSnapshots(&histogram_snapshot_manager_);
On 2016/05/05 16:01:17, bcwhite wrote:
> On 2016/05/05 07:22:37, Ilya Sherman (Away May 5-15) wrote:
> > Providers might choose to include metrics that aren't stability metrics, in
> case
> > of a crash, right?  So, what's the purpose of checking the
> > kUmaStabilityHistogramFlag above?
> 
> You mean on line 1163?  That's always been there and means that only a small
> subset of all the histograms known to the StatisticsRecorder (basically
> everything created by this particular process) will get reported here.
> 
> In the case of the providers, it's up to them what metrics to include.  The
> different calls of RecordInitialHsitogramSnapshots() vs
> RecordHistogramSnapshots() allows providers to choose.

Ah, I was misreading the code.  Thanks for explaining that in more detail :)

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/fil...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:282: // before the first call to
this method.
Please document here why it's not appropriate for the data to be cleared sooner.

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:309: // one chance to record it no
point keeping it around for later. Also mark
nit: "it no" -> "it, so no"

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:331:
SCOPED_UMA_HISTOGRAM_TIMER("UMA.FileMetricsProvider.SnapshotTime");
Please also add a histogram to measure the snapshot time for all files, in case
we somehow pile up a lot of files, each of which is individually not too slow to
be read.  (Ditto for the other timing histograms.)

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/met...
components/metrics/metrics_service.cc:1163: it != end; ++it) {
nit: Why not just write "it != base::StatisticsRecorder::end()"?

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/met...
components/metrics/metrics_service.cc:1165:
base::Histogram::kUmaStabilityHistogramFlag) {
Optional nit: I personally prefer the style used on line 1148, which omits the
extra == check.

https://codereview.chromium.org/1891913002/diff/500001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1891913002/diff/500001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:55587: +    Records how much real time
was spent checking and mapping an initial metrics
nit: s/real/wall (applies throughout)

bcwhite

https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persistent_histogram_allocator.cc File base/metrics/persistent_histogram_allocator.cc (right): https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persistent_histogram_allocator.cc#newcode50 base/metrics/persistent_histogram_allocator.cc:50: bool g_allocator_enabled; On 2016/05/05 22:22:52, Ilya Sherman (Away May ...

4 years, 7 months ago (2016-05-06 16:59:28 UTC) #39

https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persisten...
File base/metrics/persistent_histogram_allocator.cc (right):

https://codereview.chromium.org/1891913002/diff/280001/base/metrics/persisten...
base/metrics/persistent_histogram_allocator.cc:50: bool g_allocator_enabled;
On 2016/05/05 22:22:52, Ilya Sherman (Away May 5-15) wrote:
> On 2016/05/02 14:46:47, bcwhite wrote:
> > On 2016/04/27 20:31:33, Ilya Sherman wrote:
> > > Sorry, I'm going to go back to my previous question about this: Why
wouldn't
> > it
> > > make sense to track internally to the GlobalHistogramAllocator object? 
You
> > > mentioned that you'd need to plumb it through to the base class, but I
don't
> > > actually follow why that would be necessary.
> > 
> > It would have to be tested by CreateHistogram() which is in the base class
and
> > have it fail if not enabled.  Or the method would have to be overridden
here,
> > which would also mean it should probably be virtual.
> > 
> > Code calling this, however, doesn't expect CreateHistogram to fail unless
> there
> > is a problem, such as corrupted or full.  Calling code does, however, expect
> > Get() to return null if it's not enabled so by testing g_enabled in Get(),
all
> > the operational semantics remain the same.
> 
> g_allocator_enabled is only accessed internally to this file, where every
access
> could be replaced by an access to g_allocator->enabled_.  What am I missing? 

That would work just fine.  The difference would be that the static
Enable/Disable methods would now have to go inside the global g_allocator
singleton to adjust the flag.  The separation has been that static methods
operate on the existence of the singleton but don't go inside it.

Or, those methods would have to become non-static and manipulating them would
then become GetEvenIfDisabled()->Enable()...  which just looks bad to me.

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
File base/metrics/histogram_unittest.cc (right):

https://codereview.chromium.org/1891913002/diff/440001/base/metrics/histogram...
base/metrics/histogram_unittest.cc:203: }
On 2016/05/05 22:22:52, Ilya Sherman (Away May 5-15) wrote:
> On 2016/05/05 16:01:17, bcwhite wrote:
> > On 2016/05/05 07:22:37, Ilya Sherman (Away May 5-15) wrote:
> > > Please test both the case where SnapshotFinalDelta() is called after a
call
> to
> > > SnapshotDelta(), and where it is called without any prior calls to
> > > SanpshotDelta().  That is, please add an additional TEST_P for the other
> case.
> > 
> > What is the benefit of that?  There is no difference in the code paths
between
> > the two.  In one case, the "previously logged" samples are zero and in the
> other
> > it is not, but that is only data.
> 
> Without looking at the implementation, it's not hard to imagine having
different
> code paths for the case where there is and where there isn't a prior call ot
> SnapshotDelta().  I prefer to write tests defensively, since the
implementation
> can change.

I agree which is why the second stage ignored an existing sample (1) added a new
sample (2).  There isn't any code difference between them but it expands the
scope of the test to effectively handle entries that previously had a Delta
operation done on them but now have no change, and vice-versa.

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/fil...
File components/metrics/file_metrics_provider.cc (right):

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:282: // before the first call to
this method.
On 2016/05/05 22:22:52, Ilya Sherman (Away May 5-15) wrote:
> Please document here why it's not appropriate for the data to be cleared
sooner.

Done.

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:309: // one chance to record it no
point keeping it around for later. Also mark
On 2016/05/05 22:22:52, Ilya Sherman (Away May 5-15) wrote:
> nit: "it no" -> "it, so no"

Done.

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/fil...
components/metrics/file_metrics_provider.cc:331:
SCOPED_UMA_HISTOGRAM_TIMER("UMA.FileMetricsProvider.SnapshotTime");
On 2016/05/05 22:22:52, Ilya Sherman (Away May 5-15) wrote:
> Please also add a histogram to measure the snapshot time for all files, in
case
> we somehow pile up a lot of files, each of which is individually not too slow
to
> be read.  (Ditto for the other timing histograms.)

Done.

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/met...
components/metrics/metrics_service.cc:1163: it != end; ++it) {
On 2016/05/05 22:22:52, Ilya Sherman (Away May 5-15) wrote:
> nit: Why not just write "it != base::StatisticsRecorder::end()"?

Ah.  Because end() actually has to create a new iterator (which involves
acquiring a lock and creating another end() iterator for the embedded std::map)
so it is best not to do that with each iteration through the loop.

I could change the SR to create the end() iterator during initialization, cache
it, and ever-after just return the cached version.
http://stackoverflow.com/questions/1432216/is-end-required-to-be-constant-in-...

But since the SR keeps all its member variables as static that can be pushed and
popped as more SRs are created (only in tests), the cached end() would require
the same.

So I just cached it in the loop.  :-)

https://codereview.chromium.org/1891913002/diff/500001/components/metrics/met...
components/metrics/metrics_service.cc:1165:
base::Histogram::kUmaStabilityHistogramFlag) {
On 2016/05/05 22:22:52, Ilya Sherman (Away May 5-15) wrote:
> Optional nit: I personally prefer the style used on line 1148, which omits the
> extra == check.

Me too, but it bites!  This particular kUma...Flag is NOT a single flag but a
merge of two flags.  Thus, omitting the == means matching anything that has at
least one of the bits set which, in this case, matches any
kUmaTargetedHistogramFlag (or: pretty much everything).

Ask me how I discovered this.  Go on...  I dare ya!

I'll add a comment.

https://codereview.chromium.org/1891913002/diff/500001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1891913002/diff/500001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:55587: +    Records how much real time
was spent checking and mapping an initial metrics
On 2016/05/05 22:22:52, Ilya Sherman (Away May 5-15) wrote:
> nit: s/real/wall (applies throughout)

Done.

Alexei Svitkine (slow)

https://codereview.chromium.org/1891913002/diff/520001/base/metrics/histogram.h File base/metrics/histogram.h (right): https://codereview.chromium.org/1891913002/diff/520001/base/metrics/histogram.h#newcode317 base/metrics/histogram.h:317: // Flag to indicate if PrepareFinalDelta has been previously ...

4 years, 7 months ago (2016-05-09 21:24:10 UTC) #41

bcwhite

https://codereview.chromium.org/1891913002/diff/520001/base/metrics/histogram.h File base/metrics/histogram.h (right): https://codereview.chromium.org/1891913002/diff/520001/base/metrics/histogram.h#newcode317 base/metrics/histogram.h:317: // Flag to indicate if PrepareFinalDelta has been previously ...

4 years, 7 months ago (2016-05-10 15:42:41 UTC) #42

Alexei Svitkine (slow)

Just a few more things. https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics/chrome_metrics_service_client.cc#newcode160 chrome/browser/metrics/chrome_metrics_service_client.cc:160: metrics::MetricsStateManager* metrics_state_manager) { Nit: ...

4 years, 7 months ago (2016-05-10 19:55:42 UTC) #43

bcwhite

https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics/chrome_metrics_service_client.cc#newcode160 chrome/browser/metrics/chrome_metrics_service_client.cc:160: metrics::MetricsStateManager* metrics_state_manager) { On 2016/05/10 19:55:42, Alexei Svitkine wrote: ...

4 years, 7 months ago (2016-05-11 15:47:13 UTC) #44

https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics...
File chrome/browser/metrics/chrome_metrics_service_client.cc (right):

https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:160:
metrics::MetricsStateManager* metrics_state_manager) {
On 2016/05/10 19:55:42, Alexei Svitkine wrote:
> Nit: How about just passing reporting_enabled bool?

I considered it.  It seemed better to pass the object rather than have the
caller need to know exactly what call to make on it.

I'll change it if you prefer.

https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:178:
.AddExtension(base::PersistentMemoryAllocator::kFileExtension);
On 2016/05/10 19:55:42, Alexei Svitkine wrote:
> Nit: Wonder if it would be shorter to have a static function in base to do
this
> that takes the name and parent file. Can look at doing it in a separate clean
up
> CL, not necessarily here.

Acknowledged.

https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics...
chrome/browser/metrics/chrome_metrics_service_client.cc:198:
base::DeleteFile(metrics_file, false);
On 2016/05/10 19:55:42, Alexei Svitkine wrote:
> Make this a post task to the blocking pool - no reason to do this
synchronously
> on start up.

Done.

https://codereview.chromium.org/1891913002/diff/540001/components/metrics/met...
File components/metrics/metrics_service.cc (right):

https://codereview.chromium.org/1891913002/diff/540001/components/metrics/met...
components/metrics/metrics_service.cc:1168:
base::Histogram::kUmaStabilityHistogramFlag) {
On 2016/05/10 19:55:42, Alexei Svitkine wrote:
> I would actually prefer we don't duplicate this logic that's already in
> histogram_snapshot_manager_.PrepareDeltas().
> 
> Looks like the only difference is you don't want StartDeltas/FinishDeltas be
> called before/after this and want to do it yourself. I suggest making another
> version of PrepareDeltas() functions that doesn't call those.
> 
> For example, PrepareDeltasNoStartFinish().
> 
> Then, PrepareDeltas() can call that one surrounded by StartFinish() and this
> file doesn't need to duplicate this logic.

Done.

Alexei Svitkine (slow)

https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics/chrome_metrics_service_client.cc#newcode160 chrome/browser/metrics/chrome_metrics_service_client.cc:160: metrics::MetricsStateManager* metrics_state_manager) { On 2016/05/11 15:47:12, bcwhite wrote: > ...

4 years, 7 months ago (2016-05-11 15:51:28 UTC) #45

bcwhite

https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1891913002/diff/540001/chrome/browser/metrics/chrome_metrics_service_client.cc#newcode160 chrome/browser/metrics/chrome_metrics_service_client.cc:160: metrics::MetricsStateManager* metrics_state_manager) { On 2016/05/11 15:51:28, Alexei Svitkine wrote: ...

4 years, 7 months ago (2016-05-11 17:58:34 UTC) #46

Alexei Svitkine (slow)

lgtm % remaining comment Curious to see what the timing histograms reveal once this lands. ...

4 years, 7 months ago (2016-05-11 18:09:20 UTC) #47

bcwhite

https://codereview.chromium.org/1891913002/diff/600001/chrome/browser/metrics/chrome_metrics_service_client.cc File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1891913002/diff/600001/chrome/browser/metrics/chrome_metrics_service_client.cc#newcode205 chrome/browser/metrics/chrome_metrics_service_client.cc:205: base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN) On 2016/05/11 18:09:20, Alexei Svitkine wrote: > Nit: ...

4 years, 7 months ago (2016-05-12 15:17:12 UTC) #49

bcwhite

The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, asvitkine@chromium.org Link to ...

4 years, 7 months ago (2016-05-13 13:46:34 UTC) #51