Issue 1374283003: Reporting top cpu and memory consumers via rappor on chromeos

afakhry

afakhry@chromium.org changed reviewers: + nick@chromium.org

5 years, 1 month ago (2015-11-05 03:33:33 UTC) #1

afakhry

It's been a while since I last sent you a humongous CL! :) Please take ...

5 years, 1 month ago (2015-11-05 03:33:33 UTC) #2

ncarter (slow)

Wow, this is a big CL. Given my current load and travel plans, the earliest ...

5 years, 1 month ago (2015-11-05 18:10:43 UTC) #3

ncarter (slow)

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode167 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:167: task_records_by_memory_.insert(task_data); It looks like this logic means we just ...

5 years, 1 month ago (2015-11-10 23:29:57 UTC) #4

afakhry

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode167 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:167: task_records_by_memory_.insert(task_data); On 2015/11/10 23:29:56, ncarter (slow mon-tue) wrote: > ...

5 years, 1 month ago (2015-11-12 00:21:28 UTC) #5

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:167:
task_records_by_memory_.insert(task_data);
On 2015/11/10 23:29:56, ncarter (slow mon-tue) wrote:
> It looks like this logic means we just count the first 10 items, rather than
the
> top 10 items (or 10 random items, or 10 cost-weighted random items)? Is that
> right?
> 
> Assuming we were trying to sample the top 10: you can insert the item
> unconditionally, and then pop the lowest one off if it put us over
> kTopConsumersCount. I think a std::priority_queue ought to be the ideal
> container for that.

Thank you so much for catching that nasty bug! Indeed a priority queue is the
perfect container for this use case.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:291:
UMA_HISTOGRAM_ENUMERATION(kBrowserProcessCpuUsageHistogramName,
On 2015/11/10 23:29:56, ncarter wrote:
> Don't use a kConstant for the histogram name, just inline literal. I think
> otherwise you'll fail the presubmit check below?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&q=UMA...

That's weird! I didn't fail any presubmit checks at all! Is that a new check?

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:316:
rappor_service->RecordSampleObj(kCpuRapporMetric, sample.Pass());
On 2015/11/10 23:29:56, ncarter wrote:
> Doesn't this oversample processes with multiple tasks?

I'm not sure I understand what you need to know, but I'll try to explain. When
multiple samples map to the same Rappor metric during the same reporting
interval, Rappor will pick only a single one randomly for generating the report.
We sample all the tasks but we done't know which one rappor will be selecting at
the end.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:325:
rappor_service->RecordSampleObj(kMemoryRapporMetric, sample.Pass());
On 2015/11/10 23:29:56, ncarter wrote:
> Same question as above: doesn't this oversample groups with >1 task?

See above.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right):

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:44: // The CPU
usage of the task from most recent task manager refresh.
On 2015/11/10 23:29:56, ncarter (slow mon-tue) wrote:
> Is this a percentage? Clarify the comment.

Done.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:48: // refresh.
On 2015/11/10 23:29:56, ncarter (slow mon-tue) wrote:
> In bytes? Does this include shared memory?

Done.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:51: // True if the
task is running on backgrounded process.
On 2015/11/10 23:29:56, ncarter wrote:
> "running at background priority" might be a better way to phrase this.

Done.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:61: bool
background)
On 2015/11/10 23:29:56, ncarter (slow mon-tue) wrote:
> Should be in the .cc file.

Done.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:72: // to Google.
We observe state changes in the MetricsService which will
On 2015/11/10 23:29:56, ncarter (slow mon-tue) wrote:
> Is there a way to word this without saying "Google" ?

Done.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:100: if (lhs->cpu
== rhs->cpu)
On 2015/11/10 23:29:56, ncarter (slow mon-tue) wrote:
> This is a nontrivial function; shouldn't be in a header file.

Done and modified for both sorters to be a Less then comparison of task records
by cpu and memory respectively. So that we can use in the priority queue.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:140:
RANGE_CORES_0_CORES = 0,
On 2015/11/10 23:29:56, ncarter (slow mon-tue) wrote:
> I've never seen a machine with 0 cores. How would that work? If we need this
> bucket, should it be a _NA?

Done.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:141:
RANGE_CORES_1_TO_2_CORES,   // [1, 2]
On 2015/11/10 23:29:56, ncarter (slow mon-tue) wrote:
> Should 1 and 2 cores be separate buckets?

Done.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:142:
RANGE_CORES_2_TO_4_CORES,   // ]2, 4]
On 2015/11/10 23:29:56, ncarter (slow mon-tue) wrote:
> This "]2" notation (which I assume means an open interval) is unfamiliar to
me.
> The mathematical notation I'm familiar with is "(2, 4]", which would mean an
> interval that excludes 2 and includes 4.
> 
> For the cores, I'd just express them as closed intervals: RANGE_CORES_5_TO_8,
> 3_TO_4, etc, which I think is unambiguous.

Haha! Yes it means an open interval. We used to write it this way in math
classes back in Egypt! But I like your suggestion to make them unambiguous!
Done!

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:207: bool
is_monitoring_;
On 2015/11/10 23:29:56, ncarter wrote:
> This is kind of weird. Why are there redundant Start calls? Do they not
balance
> with the Stop calls? Should we stop monitoring on the first Stop() or the last
> Stop()?

The problem happens when Start() is called several times, you end up adding the
ResourceReporter as an observer of the task manager multiple times which will
cause it to crash of course.

See here, there is no code to handle multiple calls.

https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc
(right):

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc:49: //
Must be larger than |ResourceReporter::kTopConsumerCount|.
On 2015/11/10 23:29:56, ncarter wrote:
> Instead of commenting about this, you can static_assert() that it's true.

Actually we're already testing these assumptions at the beginning of
ResourceReporterTest.TestAll.

static assertion fails to compile unless I do this in resource_reporter.h:

  static const size_t kTopConsumersCount = 10U;

When I do this it compiles, but then the linker complains about undefined
reference to kTopConsumersCount all over the place!

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/task_ma...
File chrome/browser/task_management/providers/web_contents/renderer_task.cc
(right):

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/renderer_task.cc:63:
return web_contents->GetVisibleURL().host();
On 2015/11/10 23:29:57, ncarter wrote:
> I recommend capturing the origin (GURL::GetOrigin()), not just the host. It'll
> make blaming extensions easier, for instance.
> 
> Are there any URLs you want to exclude from sampling (e.g. localhost
> equivalents, about:blank, file:// scheme URIs) ?

Done.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/task_ma...
File chrome/browser/task_management/task_manager_test_util.h (right):

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_test_util.h:6: #define
CHROME_BROWSER_TASK_MANAGEMENT_TASK_MANAGER_TEST_UTIL_H_
On 2015/11/10 23:29:57, ncarter wrote:
> This filename ought to be test_task_manager or fake_task_manager. That will
make
> it far easier for readers of _unittest.cc to figure out where to look to find
> this class.

Done.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_test_util.h:14: // This is a partial
stub implementation that can be used as a base class for
On 2015/11/10 23:29:57, ncarter wrote:
> What's the guiding principle behind what behavior is stubbed out vs. not? Add
> this to the comment.

I'm not sure what you mean. Most of the below functions do nothing or return
invalid values. Subclasses need to only extend the functions they're interested
in to simulate certain behavior.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/task_ma...
chrome/browser/task_management/task_manager_test_util.h:15: // implementations
of the task manager used in unit tests.
On 2015/11/10 23:29:57, ncarter wrote:
> Ideally this object would be useful without further subclassing. Is that
> possible?

Yes it is possible. Subclassing is sometimes needed when certain tests need to
do fake cpu and memory calculations like the case in our ResourceReporterTest.

https://codereview.chromium.org/1374283003/diff/240001/tools/metrics/rappor/r...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/1374283003/diff/240001/tools/metrics/rappor/r...
tools/metrics/rappor/rappor.xml:859: The rappor identifier of a Chrome task. In
most cases this is the hostname
On 2015/11/10 23:29:57, ncarter wrote:
> hostname? or origin?

Origin as you suggested is better.

https://codereview.chromium.org/1374283003/diff/240001/tools/metrics/rappor/r...
tools/metrics/rappor/rappor.xml:865: <flag>Bit 1: BACKGROUND</flag>
On 2015/11/10 23:29:57, ncarter wrote:
> Why is this two bits? Seems like it could be one.

I'm honestly not sure about this one, and I'd leave it to the rappor reviewers
later on.

ncarter (slow)

Haven't completed another pass, just responding to comments. https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode291 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:291: UMA_HISTOGRAM_ENUMERATION(kBrowserProcessCpuUsageHistogramName, ...

5 years, 1 month ago (2015-11-12 19:33:27 UTC) #6

Haven't completed another pass, just responding to comments.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:291:
UMA_HISTOGRAM_ENUMERATION(kBrowserProcessCpuUsageHistogramName,
On 2015/11/12 00:21:28, afakhry wrote:
> On 2015/11/10 23:29:56, ncarter wrote:
> > Don't use a kConstant for the histogram name, just inline literal. I think
> > otherwise you'll fail the presubmit check below?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&q=UMA...
> 
> That's weird! I didn't fail any presubmit checks at all! Is that a new check?

Ah, I looked at it more closely. The regex includes a literal double-quote
character, so if you use an indirect name, the check doesn't run at all.

I still think it's a good idea to use the literal notation (unless you really
need to increment the histogram in multiple places, which is very rare) -- it's
a tad easier to grep for when you're looking at histogram data and want to cross
reference the code.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:316:
rappor_service->RecordSampleObj(kCpuRapporMetric, sample.Pass());
On 2015/11/12 00:21:27, afakhry wrote:
> On 2015/11/10 23:29:56, ncarter wrote:
> > Doesn't this oversample processes with multiple tasks?
> 
> I'm not sure I understand what you need to know, but I'll try to explain. When
> multiple samples map to the same Rappor metric during the same reporting
> interval, Rappor will pick only a single one randomly for generating the
report.
> We sample all the tasks but we done't know which one rappor will be selecting
at
> the end.

By "oversampling", I mean that now a 100MB process with two tasks in it is twice
as likely to be sampled as a 100MB process with one task in it. That'll bias
your overall population estimate toward tasks that are actually consuming less
memory (since the two tasks average 50MB each, though it could be a 90/10
split).

It's still a little fuzzy to me what we're trying to estimate. Even if you
assume each task has its own process, logging an unweighted random element
within the top 10 consumers doesn't really agree with my mathematical intuition.
What value does the top-10 cutoff really provide, if we're just picking one of
the top ten at random?

afakhry

Another round please? :) https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode291 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:291: UMA_HISTOGRAM_ENUMERATION(kBrowserProcessCpuUsageHistogramName, On 2015/11/12 19:33:27, ncarter ...

5 years, 1 month ago (2015-11-13 19:41:13 UTC) #7

Another round please? :)

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:291:
UMA_HISTOGRAM_ENUMERATION(kBrowserProcessCpuUsageHistogramName,
On 2015/11/12 19:33:27, ncarter wrote:
> On 2015/11/12 00:21:28, afakhry wrote:
> > On 2015/11/10 23:29:56, ncarter wrote:
> > > Don't use a kConstant for the histogram name, just inline literal. I think
> > > otherwise you'll fail the presubmit check below?
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/PRESUBMIT.py&q=UMA...
> > 
> > That's weird! I didn't fail any presubmit checks at all! Is that a new
check?
> 
> Ah, I looked at it more closely. The regex includes a literal double-quote
> character, so if you use an indirect name, the check doesn't run at all.
> 
> I still think it's a good idea to use the literal notation (unless you really
> need to increment the histogram in multiple places, which is very rare) --
it's
> a tad easier to grep for when you're looking at histogram data and want to
cross
> reference the code.

Done.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:316:
rappor_service->RecordSampleObj(kCpuRapporMetric, sample.Pass());
On 2015/11/12 19:33:27, ncarter wrote:
> On 2015/11/12 00:21:27, afakhry wrote:
> > On 2015/11/10 23:29:56, ncarter wrote:
> > > Doesn't this oversample processes with multiple tasks?
> > 
> > I'm not sure I understand what you need to know, but I'll try to explain.
When
> > multiple samples map to the same Rappor metric during the same reporting
> > interval, Rappor will pick only a single one randomly for generating the
> report.
> > We sample all the tasks but we done't know which one rappor will be
selecting
> at
> > the end.
> 
> By "oversampling", I mean that now a 100MB process with two tasks in it is
twice
> as likely to be sampled as a 100MB process with one task in it. That'll bias
> your overall population estimate toward tasks that are actually consuming less
> memory (since the two tasks average 50MB each, though it could be a 90/10
> split).
> 
> It's still a little fuzzy to me what we're trying to estimate. Even if you
> assume each task has its own process, logging an unweighted random element
> within the top 10 consumers doesn't really agree with my mathematical
intuition.
> What value does the top-10 cutoff really provide, if we're just picking one of
> the top ten at random?

The top 10 limit is merely for us to limit our attention. When there are a huge
number of samples from many users, It's easy to say that these are the tasks
that show up in the top 10 list of consumers when the system is under pressure.

afakhry

afakhry@chromium.org changed reviewers: + holte@chromium.org

5 years, 1 month ago (2015-11-13 19:43:34 UTC) #8

afakhry

holte@ Can you please checkout the following files and make sure that our understanding of ...

5 years, 1 month ago (2015-11-13 19:43:35 UTC) #9

Steven Holte

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

5 years, 1 month ago (2015-11-13 20:21:09 UTC) #10

Steven Holte

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode236 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:236: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Why are you observing the metrics service? I ...

5 years, 1 month ago (2015-11-13 20:32:25 UTC) #11

Steven Holte

holte@chromium.org changed reviewers: - asvitkine@chromium.org

5 years, 1 month ago (2015-11-13 20:33:11 UTC) #12

afakhry

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode236 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:236: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2015/11/13 20:32:25, Steven Holte wrote: > Why ...

5 years, 1 month ago (2015-11-13 20:56:06 UTC) #13

Steven Holte

On 2015/11/13 20:56:06, afakhry wrote: > https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc > File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): > > https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode236 > ...

5 years, 1 month ago (2015-11-13 21:33:47 UTC) #14

Steven Holte

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode371 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:371: for (const auto& task_data : task_records_by_cpu_) { The code ...

5 years, 1 month ago (2015-11-13 21:42:18 UTC) #15

ncarter (slow)

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode316 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:316: rappor_service->RecordSampleObj(kCpuRapporMetric, sample.Pass()); On 2015/11/13 19:41:12, afakhry wrote: > On ...

5 years, 1 month ago (2015-11-13 22:44:33 UTC) #16

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:316:
rappor_service->RecordSampleObj(kCpuRapporMetric, sample.Pass());
On 2015/11/13 19:41:12, afakhry wrote:
> On 2015/11/12 19:33:27, ncarter wrote:
> > On 2015/11/12 00:21:27, afakhry wrote:
> > > On 2015/11/10 23:29:56, ncarter wrote:
> > > > Doesn't this oversample processes with multiple tasks?
> > > 
> > > I'm not sure I understand what you need to know, but I'll try to explain.
> When
> > > multiple samples map to the same Rappor metric during the same reporting
> > > interval, Rappor will pick only a single one randomly for generating the
> > report.
> > > We sample all the tasks but we done't know which one rappor will be
> selecting
> > at
> > > the end.
> > 
> > By "oversampling", I mean that now a 100MB process with two tasks in it is
> twice
> > as likely to be sampled as a 100MB process with one task in it. That'll bias
> > your overall population estimate toward tasks that are actually consuming
less
> > memory (since the two tasks average 50MB each, though it could be a 90/10
> > split).
> > 
> > It's still a little fuzzy to me what we're trying to estimate. Even if you
> > assume each task has its own process, logging an unweighted random element
> > within the top 10 consumers doesn't really agree with my mathematical
> intuition.
> > What value does the top-10 cutoff really provide, if we're just picking one
of
> > the top ten at random?
> 
> The top 10 limit is merely for us to limit our attention. When there are a
huge
> number of samples from many users, It's easy to say that these are the tasks
> that show up in the top 10 list of consumers when the system is under
pressure.

The current strategy still seems a little heuristic to me -- maybe that's fine,
or maybe we'll need a couple of iterations of this data pipeline
before we get data that we really understand how to interpret. Either way,
it seems like launching & iterating is the way to go, so I'm not going
to hold this CL up on my concerns.

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right):

https://codereview.chromium.org/1374283003/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:207: bool
is_monitoring_;
On 2015/11/12 00:21:28, afakhry wrote:
> On 2015/11/10 23:29:56, ncarter wrote:
> > This is kind of weird. Why are there redundant Start calls? Do they not
> balance
> > with the Stop calls? Should we stop monitoring on the first Stop() or the
last
> > Stop()?
> 
> The problem happens when Start() is called several times, you end up adding
the
> ResourceReporter as an observer of the task manager multiple times which will
> cause it to crash of course.
> 
> See here, there is no code to handle multiple calls.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

Are we just coding around a bug in the MetricsService? Should this bool be added
to the MetricsService? Seems reasonable for an observer to expect balanced
Start/Stop calls.

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:193: default:
Are you sure you want to sample all task types?

I imagine that Zygotes, for example, will show up when there are fewer than 10
renderer processes.

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:371: for (const
auto& task_data : task_records_by_cpu_) {
On 2015/11/13 21:42:18, Steven Holte wrote:
> The code for creating the samples looks fine, but I'm not sure I understand
the
> implications of your sampling strategy.  I think it's probably worth setting
up
> a meeting with Ananth/Ilya to discuss how you would actually be able to
analyze
> this data.

Feel free to include me in this meeting, if it happens.

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:379:
CPU_RANGES_NUM);
Dumb question: would it make sense to record the memory in the cpu sample as
well (and vice versa)?

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right):

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:9: #include
<vector>
#include <string> here (& can remove from the .cc)

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:58:
TaskRecord(task_management::TaskId the_id,
ctors before data members
https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc
(right):

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc:69: //
task_management::TestTaskManager:
Seems like the code under test calls GetType() too, should we add an override
for that (this would let us exercise the BROWSER and GPU codepaths too) ?

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc:88: }
This came out pretty nice -- no dependency on actual task manager Task objects.
Glad to see this architecture pan out.

afakhry

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode193 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:193: default: On 2015/11/13 22:44:32, ncarter wrote: > Are you ...

5 years, 1 month ago (2015-11-16 18:44:20 UTC) #17

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:193: default:
On 2015/11/13 22:44:32, ncarter wrote:
> Are you sure you want to sample all task types?
> 
> I imagine that Zygotes, for example, will show up when there are fewer than 10
> renderer processes.

Done.

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:236:
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
On 2015/11/13 20:56:06, afakhry wrote:
> > On 2015/11/13 20:32:25, Steven Holte wrote:
> > > Why are you observing the metrics service?  I would discourage tieing a
> > > collection activities to the state of the service.
> > 
> > The collection activities is done only for the purpose of reporting via
Rappor.
> > If Rappor reporting is disabled, then there's no need at all for doing this
> > work.
>
> Metrics code isn't like debug logging, where it makes sense to disable the
code
> for production.  The UMA enabled case is still a production use case, so if
the
> overhead of the collection code is significant enough that you would want to
> avoid it for non-UMA users, you need to avoid it for UMA users too.  Whether
or
> not the collection system is actually doing anything with the data it's given
> should be considered an implementation detail.

There are two things here. First thing is the stats calculations done by the
task manager for the various tasks running on the system, and the second being
the processing of these stats we do here in this system (ResourceReporter).
The task manager has been recently rewritten from scratch optimized specifically
for such use cases. Stats calculations will not introduce any jank for users.
The processing we do here is trivial.
Despite of that, I strongly believe that if this system won't be doing the task
which is the only reason for its existence (reporting samples), then there is no
need for running it altogether.

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:379:
CPU_RANGES_NUM);
On 2015/11/13 22:44:32, ncarter wrote:
> Dumb question: would it make sense to record the memory in the cpu sample as
> well (and vice versa)?

I thought about that, but since top CPU tasks are probably different than top
memory tasks, I preferred to keep separate rappor metric for each.

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right):

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:9: #include
<vector>
On 2015/11/13 22:44:32, ncarter wrote:
> #include <string> here (& can remove from the .cc)

Done.

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:42: std::string
rappor_sample;
On 2015/11/13 21:42:18, Steven Holte wrote:
> Similar to my other comment, this should probably be called simplified_task_id
> or something similar, since this not a Sample, it's a value you intend to
record
> in a sample.

Done.

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:58:
TaskRecord(task_management::TaskId the_id,
On 2015/11/13 22:44:32, ncarter wrote:
> ctors before data members
> https://google.github.io/styleguide/cppguide.html#Declaration_Order

I thought in structs we put the member variable first. Done.

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc
(right):

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc:69: //
task_management::TestTaskManager:
On 2015/11/13 22:44:32, ncarter wrote:
> Seems like the code under test calls GetType() too, should we add an override
> for that (this would let us exercise the BROWSER and GPU codepaths too) ?

Done.

https://codereview.chromium.org/1374283003/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter_unittest.cc:88: }
On 2015/11/13 22:44:32, ncarter wrote:
> This came out pretty nice -- no dependency on actual task manager Task
objects.
> Glad to see this architecture pan out.

Yeah me too! Writing tests against this interface has been simplified a lot.

https://codereview.chromium.org/1374283003/diff/280001/tools/metrics/rappor/r...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/1374283003/diff/280001/tools/metrics/rappor/r...
tools/metrics/rappor/rappor.xml:941: The rappor identifier of a Chrome task. In
most cases this is the origin
On 2015/11/13 21:42:18, Steven Holte wrote:
> It's not quite clear what you mean by "rappor identifier", since Rappor
doesn't
> know what a task is.  This should probably be called a
simplified/canonicalized
> task identifier.

Done.

afakhry

afakhry@chromium.org changed reviewers: + mpearson@chromium.org

5 years, 1 month ago (2015-11-16 20:10:37 UTC) #18

Mark P

https://codereview.chromium.org/1374283003/diff/300001/chrome/browser/chromeos/resource_reporter/resource_reporter.h File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right): https://codereview.chromium.org/1374283003/diff/300001/chrome/browser/chromeos/resource_reporter/resource_reporter.h#newcode100 chrome/browser/chromeos/resource_reporter/resource_reporter.h:100: enum CpuUsageRange { please put a warning here that ...

5 years, 1 month ago (2015-11-16 20:31:21 UTC) #20

afakhry

https://codereview.chromium.org/1374283003/diff/300001/chrome/browser/chromeos/resource_reporter/resource_reporter.h File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right): https://codereview.chromium.org/1374283003/diff/300001/chrome/browser/chromeos/resource_reporter/resource_reporter.h#newcode100 chrome/browser/chromeos/resource_reporter/resource_reporter.h:100: enum CpuUsageRange { On 2015/11/16 20:31:21, Mark P wrote: ...

5 years, 1 month ago (2015-11-16 22:33:49 UTC) #21

https://codereview.chromium.org/1374283003/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right):

https://codereview.chromium.org/1374283003/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:100: enum
CpuUsageRange {
On 2015/11/16 20:31:21, Mark P wrote:
> please put a warning here that these should be renamed or modified or
reordered
> because they're written to logs.  The only permitted modification is to the
> insert new elements immediately before the last.
> 
> ditto below

Done.

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

https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:29892: +<histogram
name="OfflinePages.DeletePage.TotalPageSizeAsPercentageOfFreeSpace"
On 2015/11/16 20:31:21, Mark P wrote:
> This move change should go away after you sync and rebase.

Done.

https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38957: +    The cpu usage rage reported
for the browser process when the system is under
On 2015/11/16 20:31:21, Mark P wrote:
> rage?
> rate?  range?
> 
> ditto below

range. Done.

https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38958: +    moderate memory pressure or
higher.
On 2015/11/16 20:31:21, Mark P wrote:
> Please allude to how this is defined.  For instance, is this absolute memory
> usage (depends just on chrome) or memory usage as a percent of all system
memory
> (again depends just on chrome) or percent or amount of free memory on the
system
> (which depends on what other apps are open)?
> 
> please put the same information in all the relevant histograms.
>  

It's the total percentage of memory used by the system (chrome, apps,
extensions, and all processes). The thresholds defining moderate and critical
memory pressures are dependent on a flag.

Done.

https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38958: +    moderate memory pressure or
higher.
On 2015/11/16 20:31:21, Mark P wrote:
> please mention when this is emitted.  On every pageload?  Only on the
transition
> from not-under-pressure to under-pressure?  etc?
> 
> ditto all histograms below

Done.

https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38958: +    moderate memory pressure or
higher.
On 2015/11/16 20:31:21, Mark P wrote:
> how is cpu usage measured on multi-core machines?  Is it amount as a percent
of
> the sum of processing power of all cores?

It is a percent of the total CPU power. So if a process is using 100% of a core
in a dual-core CPU, then this process is actually using 50%.

https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38971: +<histogram
name="ResourceReporter.GpuProcess.CpuUsage"
On 2015/11/16 20:31:21, Mark P wrote:
> Doesn't it make more sense to call GpuUsage, not CpuUsage, as the description
> implies that it's the processor in the GPU chip?

The GPU process runs on the CPU not the GPU.

https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38984: +    The memory usage range
reported for the GPU process when the system is under
On 2015/11/16 20:31:21, Mark P wrote:
> This is system ram, not memory on the GPU, right?  Be clear.

Right. Done.

https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:72368: +  <int value="0"
label="RANGE_0_TO_10_PERCENT"/>
On 2015/11/16 20:31:21, Mark P wrote:
> you don't need ugly enum-like labels here.  Do something reasonable.

I saw other enums in this file using enum-like naming, so I assumed this is the
convention.

Mark P

two minor comments with content; otherwise histograms.xml lgtm --mark https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histograms/histograms.xml#newcode38958 tools/metrics/histograms/histograms.xml:38958: ...

5 years, 1 month ago (2015-11-16 23:42:18 UTC) #22

two minor comments with content; otherwise histograms.xml lgtm

--mark

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

https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38958: +    moderate memory pressure or
higher.
On 2015/11/16 22:33:49, afakhry wrote:
> On 2015/11/16 20:31:21, Mark P wrote:
> > how is cpu usage measured on multi-core machines?  Is it amount as a percent
> of
> > the sum of processing power of all cores?
> 
> It is a percent of the total CPU power. So if a process is using 100% of a
core
> in a dual-core CPU, then this process is actually using 50%.

Acknowledged.  The current explanation is fine then.

https://codereview.chromium.org/1374283003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38971: +<histogram
name="ResourceReporter.GpuProcess.CpuUsage"
On 2015/11/16 22:33:49, afakhry wrote:
> On 2015/11/16 20:31:21, Mark P wrote:
> > Doesn't it make more sense to call GpuUsage, not CpuUsage, as the
description
> > implies that it's the processor in the GPU chip?
> 
> The GPU process runs on the CPU not the GPU.

Ah, silly me confusing GPU process and GPU chip/processor.

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

https://codereview.chromium.org/1374283003/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:38979: +    The memory usage range
reported for the browser process when the chromeos
consider saying "system memory" here to be parallel the phrasing in the
GpuProcess histogram.

https://codereview.chromium.org/1374283003/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:72400: +  <int value="0" label="CPU
usage that is in the range between 0% and 10%"/>
These labels will be long icky to read in the dashboard.  Can't you simply do
0-10%
10-30%
30-60%
>60%
?

analogous suggestion in the enum below.

afakhry

Description was changed from ========== Reporting top cpu and memory consumers via rappor on chromeos ...

5 years, 1 month ago (2015-11-17 22:53:34 UTC) #23

afakhry

nick@ and holte@, I made the below changes: - Removed the dependency on the metric ...

5 years, 1 month ago (2015-11-17 23:01:55 UTC) #24

Steven Holte

https://codereview.chromium.org/1374283003/diff/340001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/340001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode241 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:241: task_records_by_cpu_.back()->cpu_weight = cpu_current_weight--; I thought the idea was to ...

5 years, 1 month ago (2015-11-17 23:42:04 UTC) #25

afakhry

https://codereview.chromium.org/1374283003/diff/340001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/340001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode241 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:241: task_records_by_cpu_.back()->cpu_weight = cpu_current_weight--; On 2015/11/17 23:42:04, Steven Holte wrote: ...

5 years, 1 month ago (2015-11-18 01:06:51 UTC) #26

Steven Holte

https://codereview.chromium.org/1374283003/diff/340001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/340001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode241 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:241: task_records_by_cpu_.back()->cpu_weight = cpu_current_weight--; On 2015/11/18 01:06:51, afakhry wrote: > ...

5 years, 1 month ago (2015-11-18 02:32:35 UTC) #27

ncarter (slow)

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode241 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:241: task_records_by_cpu_.back()->cpu_weight = cpu_current_weight--; This seems like a totally arbitrary ...

5 years, 1 month ago (2015-11-18 22:17:15 UTC) #28

afakhry

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode241 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:241: task_records_by_cpu_.back()->cpu_weight = cpu_current_weight--; On 2015/11/18 22:17:15, ncarter wrote: > ...

5 years, 1 month ago (2015-11-19 01:36:55 UTC) #29

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:241:
task_records_by_cpu_.back()->cpu_weight = cpu_current_weight--;
On 2015/11/18 22:17:15, ncarter wrote:
> This seems like a totally arbitrary assignment of weight. Why do it this way,
> versus weighting by the record's |cpu| value?

Bad judgement from my side. Removed.

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:409:
GetCpuUsageRange(sampled_cpu_task->cpu),
On 2015/11/18 22:17:15, ncarter wrote:
> If we're already doing a weighted choice of the task, what are the
implications
> for logging the weight (which influenced the choice) as a field? Naively, I
> would expect that when aggregating this field (e.g. if we wanted to estimate
the
> average CPU/memory usage for a particular site) we'd need to take the sample
> weight into account. Is this something commonly encountered by other reppor
use
> cases, or do we need to do something special?
> 
> E.g., suppose we're sampling two tab scenario, where http://tab1.com is 100MB
50% of
> the time and 200MB 50% of the time, and http://tab2.com is 100MB 100% of the
time, then
> tab1.com's average memory usage is 150MB. But we'll see samples according to
the
> following distribution (ignoring memory pressure events):
>   25% 100MB http://tab1.com
>   33% 200MB http://tab1.com
>   42% 100MB http://tab2.com 
> 
> So, assuming that we're ultimately going to see a dashboard that tells us
> "tab1.com appears in XX% of memory pressure scenarios, and consumes an average
> of YYY megabytes," YYY ought to be 150MB (the true average) and not 157MB
(which
> is the average of the samples).

We're doing something special here to account for (as far as I understood) a
limitation in the Rappor dashboard, which currently doesn't support the analysis
of multi-dimentional Rappor metrics we're using here (ones with different flag
fields and string fields).

That's why we're randomly selecting one task to report in each metric, cpu and
memory. What we are interested in is seeing the most frequent offenders when the
system is under moderate or critical memory pressure. Hence, the sampling to
select a single task to report via Rappor out of the top 10 ones, based on the
most recent measurements done by the task manager's Refresh() right before the
memory pressure event happens.

Ideally, we'd want to see which usage range bucket the reported task falls into,
but that currently is not supported by the dashboard. holte@ can correct me here
if I'm wrong.

I honestly didn't get the relevance of the example you gave above to what we're
doing here, as we're not trying to average anything over time. Just the most
recent measurements before the memory pressure event is hit are what we're
interested in.

Steven Holte

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode409 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:409: GetCpuUsageRange(sampled_cpu_task->cpu), On 2015/11/18 22:17:15, ncarter wrote: > If we're ...

5 years, 1 month ago (2015-11-19 22:05:32 UTC) #30

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:409:
GetCpuUsageRange(sampled_cpu_task->cpu),
On 2015/11/18 22:17:15, ncarter wrote:
> If we're already doing a weighted choice of the task, what are the
implications
> for logging the weight (which influenced the choice) as a field? Naively, I
> would expect that when aggregating this field (e.g. if we wanted to estimate
the
> average CPU/memory usage for a particular site) we'd need to take the sample
> weight into account. Is this something commonly encountered by other reppor
use
> cases, or do we need to do something special?
> 
> E.g., suppose we're sampling two tab scenario, where http://tab1.com is 100MB
50% of
> the time and 200MB 50% of the time, and http://tab2.com is 100MB 100% of the
time, then
> tab1.com's average memory usage is 150MB. But we'll see samples according to
the
> following distribution (ignoring memory pressure events):
>   25% 100MB http://tab1.com
>   33% 200MB http://tab1.com
>   42% 100MB http://tab2.com 
> 
> So, assuming that we're ultimately going to see a dashboard that tells us
> "tab1.com appears in XX% of memory pressure scenarios, and consumes an average
> of YYY megabytes," YYY ought to be 150MB (the true average) and not 157MB
(which
> is the average of the samples).

You are correct that this sampling scheme doesn't let us compute the average
memory consumed by a particular process, and that in order to compute that we
would need to know the weights that each sample was sampled with.

However, I don't know if that stat is actually that interesting of metric for
this use case, or if including a Rappor-noised indicator of the weight would
give enough data to estimate it with any accuracy.

For more detail about why it might not be interesting metric, consider a task
which is ubiquitous, but has some pathological bad state where it uses way too
much memory.  We might expect that for many reports this task would be very
small say 5MB, but in 1% of cases it's ballooned to 505MB.  Knowing that it uses
10MB on average doesn't tell you much about whether or not it's a problematic
task.

That said, I wouldn't be against including the total amount of memory / CPU used
by all tasks in the reports as an extra field.  If we start sending the other
fields as non-noised, that would probably be enough to compute the averages.

https://codereview.chromium.org/1374283003/diff/380001/tools/metrics/rappor/r...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/1374283003/diff/380001/tools/metrics/rappor/r...
tools/metrics/rappor/rappor.xml:1003: Bitfield for the tasks CPU usage range.
Fix this summary.

ncarter (slow)

lgtm https://codereview.chromium.org/1374283003/diff/380001/chrome/browser/task_management/sampling/task_group_sampler.cc File chrome/browser/task_management/sampling/task_group_sampler.cc (right): https://codereview.chromium.org/1374283003/diff/380001/chrome/browser/task_management/sampling/task_group_sampler.cc#newcode43 chrome/browser/task_management/sampling/task_group_sampler.cc:43: process_(base::Process::Open(proc_id)), If we're doing base::Process::Open here, that gives ...

5 years, 1 month ago (2015-11-20 18:44:45 UTC) #31

afakhry

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode409 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:409: GetCpuUsageRange(sampled_cpu_task->cpu), On 2015/11/19 22:05:32, Steven Holte wrote: > On ...

5 years, 1 month ago (2015-11-21 01:32:58 UTC) #32

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:409:
GetCpuUsageRange(sampled_cpu_task->cpu),
On 2015/11/19 22:05:32, Steven Holte wrote:
> On 2015/11/18 22:17:15, ncarter wrote:
> > If we're already doing a weighted choice of the task, what are the
> implications
> > for logging the weight (which influenced the choice) as a field? Naively, I
> > would expect that when aggregating this field (e.g. if we wanted to estimate
> the
> > average CPU/memory usage for a particular site) we'd need to take the sample
> > weight into account. Is this something commonly encountered by other reppor
> use
> > cases, or do we need to do something special?
> > 
> > E.g., suppose we're sampling two tab scenario, where http://tab1.com is
100MB
> 50% of
> > the time and 200MB 50% of the time, and http://tab2.com is 100MB 100% of the
> time, then
> > tab1.com's average memory usage is 150MB. But we'll see samples according to
> the
> > following distribution (ignoring memory pressure events):
> >   25% 100MB http://tab1.com
> >   33% 200MB http://tab1.com
> >   42% 100MB http://tab2.com 
> > 
> > So, assuming that we're ultimately going to see a dashboard that tells us
> > "tab1.com appears in XX% of memory pressure scenarios, and consumes an
average
> > of YYY megabytes," YYY ought to be 150MB (the true average) and not 157MB
> (which
> > is the average of the samples).
> 
> You are correct that this sampling scheme doesn't let us compute the average
> memory consumed by a particular process, and that in order to compute that we
> would need to know the weights that each sample was sampled with.
> 
> However, I don't know if that stat is actually that interesting of metric for
> this use case, or if including a Rappor-noised indicator of the weight would
> give enough data to estimate it with any accuracy.
> 
> For more detail about why it might not be interesting metric, consider a task
> which is ubiquitous, but has some pathological bad state where it uses way too
> much memory.  We might expect that for many reports this task would be very
> small say 5MB, but in 1% of cases it's ballooned to 505MB.  Knowing that it
uses
> 10MB on average doesn't tell you much about whether or not it's a problematic
> task.
> 
> That said, I wouldn't be against including the total amount of memory / CPU
used
> by all tasks in the reports as an extra field.  If we start sending the other
> fields as non-noised, that would probably be enough to compute the averages.

What's the difference between sending a Rappor-noised weight and a Rappor-noised
memory usage range that we already do, considering that we're not interested in
the average usage, but rather the use @ bad memory pressure?

https://codereview.chromium.org/1374283003/diff/380001/chrome/browser/task_ma...
File chrome/browser/task_management/sampling/task_group_sampler.cc (right):

https://codereview.chromium.org/1374283003/diff/380001/chrome/browser/task_ma...
chrome/browser/task_management/sampling/task_group_sampler.cc:43:
process_(base::Process::Open(proc_id)),
On 2015/11/20 18:44:45, ncarter wrote:
> If we're doing base::Process::Open here, that gives us our own durable handle
to
> the pid, so we can pass that to process_metrics_. It's always better to use a
> handle that you own, because using a handle that's been closed can result in a
> crash.
> 
> I suggest .Pass()-ing a base::Process into the TaskGroupSampler ctor
(replacing
> both proc_id and proc_handle). Have the calling function do the Open call. 

Thanks for this suggestion. It matches our decision to rely on the process id in
all the new task manager code. The only thing that still used a process handle
is the ProcessMetrics.

Done. Please take a look.

https://codereview.chromium.org/1374283003/diff/380001/tools/metrics/rappor/r...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/1374283003/diff/380001/tools/metrics/rappor/r...
tools/metrics/rappor/rappor.xml:1003: Bitfield for the tasks CPU usage range.
On 2015/11/19 22:05:32, Steven Holte wrote:
> Fix this summary.

Done.

afakhry

On 2015/11/21 01:32:58, afakhry wrote: > https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc > File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): > > https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode409 > ...

5 years ago (2015-11-23 20:01:33 UTC) #33

On 2015/11/21 01:32:58, afakhry wrote:
>
https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeo...
> File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):
> 
>
https://codereview.chromium.org/1374283003/diff/360001/chrome/browser/chromeo...
> chrome/browser/chromeos/resource_reporter/resource_reporter.cc:409:
> GetCpuUsageRange(sampled_cpu_task->cpu),
> On 2015/11/19 22:05:32, Steven Holte wrote:
> > On 2015/11/18 22:17:15, ncarter wrote:
> > > If we're already doing a weighted choice of the task, what are the
> > implications
> > > for logging the weight (which influenced the choice) as a field? Naively,
I
> > > would expect that when aggregating this field (e.g. if we wanted to
estimate
> > the
> > > average CPU/memory usage for a particular site) we'd need to take the
sample
> > > weight into account. Is this something commonly encountered by other
reppor
> > use
> > > cases, or do we need to do something special?
> > > 
> > > E.g., suppose we're sampling two tab scenario, where http://tab1.com is
> 100MB
> > 50% of
> > > the time and 200MB 50% of the time, and http://tab2.com is 100MB 100% of
the
> > time, then
> > > tab1.com's average memory usage is 150MB. But we'll see samples according
to
> > the
> > > following distribution (ignoring memory pressure events):
> > >   25% 100MB http://tab1.com
> > >   33% 200MB http://tab1.com
> > >   42% 100MB http://tab2.com 
> > > 
> > > So, assuming that we're ultimately going to see a dashboard that tells us
> > > "tab1.com appears in XX% of memory pressure scenarios, and consumes an
> average
> > > of YYY megabytes," YYY ought to be 150MB (the true average) and not 157MB
> > (which
> > > is the average of the samples).
> > 
> > You are correct that this sampling scheme doesn't let us compute the average
> > memory consumed by a particular process, and that in order to compute that
we
> > would need to know the weights that each sample was sampled with.
> > 
> > However, I don't know if that stat is actually that interesting of metric
for
> > this use case, or if including a Rappor-noised indicator of the weight would
> > give enough data to estimate it with any accuracy.
> > 
> > For more detail about why it might not be interesting metric, consider a
task
> > which is ubiquitous, but has some pathological bad state where it uses way
too
> > much memory.  We might expect that for many reports this task would be very
> > small say 5MB, but in 1% of cases it's ballooned to 505MB.  Knowing that it
> uses
> > 10MB on average doesn't tell you much about whether or not it's a
problematic
> > task.
> > 
> > That said, I wouldn't be against including the total amount of memory / CPU
> used
> > by all tasks in the reports as an extra field.  If we start sending the
other
> > fields as non-noised, that would probably be enough to compute the averages.
> 
> What's the difference between sending a Rappor-noised weight and a
Rappor-noised
> memory usage range that we already do, considering that we're not interested
in
> the average usage, but rather the use @ bad memory pressure?
> 
>
https://codereview.chromium.org/1374283003/diff/380001/chrome/browser/task_ma...
> File chrome/browser/task_management/sampling/task_group_sampler.cc (right):
> 
>
https://codereview.chromium.org/1374283003/diff/380001/chrome/browser/task_ma...
> chrome/browser/task_management/sampling/task_group_sampler.cc:43:
> process_(base::Process::Open(proc_id)),
> On 2015/11/20 18:44:45, ncarter wrote:
> > If we're doing base::Process::Open here, that gives us our own durable
handle
> to
> > the pid, so we can pass that to process_metrics_. It's always better to use
a
> > handle that you own, because using a handle that's been closed can result in
a
> > crash.
> > 
> > I suggest .Pass()-ing a base::Process into the TaskGroupSampler ctor
> (replacing
> > both proc_id and proc_handle). Have the calling function do the Open call. 
> 
> Thanks for this suggestion. It matches our decision to rely on the process id
in
> all the new task manager code. The only thing that still used a process handle
> is the ProcessMetrics.
> 
> Done. Please take a look.
> 
>
https://codereview.chromium.org/1374283003/diff/380001/tools/metrics/rappor/r...
> File tools/metrics/rappor/rappor.xml (right):
> 
>
https://codereview.chromium.org/1374283003/diff/380001/tools/metrics/rappor/r...
> tools/metrics/rappor/rappor.xml:1003: Bitfield for the tasks CPU usage range.
> On 2015/11/19 22:05:32, Steven Holte wrote:
> > Fix this summary.
> 
> Done.

holte@ Friendly ping. Thanks!

Daniel Erat

i didn't look at the tests yet. https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode405 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:405: chromeos::ResourceReporter::GetInstance()->StartMonitoring(); it ...

5 years ago (2015-11-23 23:57:20 UTC) #37

afakhry

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeos/chrome_browser_main_chromeos.cc#newcode405 chrome/browser/chromeos/chrome_browser_main_chromeos.cc:405: chromeos::ResourceReporter::GetInstance()->StartMonitoring(); On 2015/11/23 23:57:19, Daniel Erat wrote: > it ...

5 years ago (2015-11-24 01:52:34 UTC) #38

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:405:
chromeos::ResourceReporter::GetInstance()->StartMonitoring();
On 2015/11/23 23:57:19, Daniel Erat wrote:
> it seems a bit weird to me that everything else in this message gets run
before
> ChromeBrowserMainPartsLinux::PreMainMessageLoopRun(), but this runs after. if
> that's intentional and not a bug, please add a comment here explaining why.

This is an arbitrary decision. It doesn't matter if it's before or after.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/OWNERS (right):

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/OWNERS:1: afakhry@chromium.org
On 2015/11/23 23:57:19, Daniel Erat wrote:
> can you find a second person to be an owner of this? (ideally not me, since i
> don't really spend any time working on chrome-for-chrome-os nowadays)

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right):

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:7: #include
<stdint.h>
On 2015/11/23 23:57:19, Daniel Erat wrote:
> nit: cstdint

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:31: // 800 MB in
bytes.
On 2015/11/23 23:57:19, Daniel Erat wrote:
> nit: these comments aren't very useful; i'd just put one at the top like
> "Various sizes in bytes." and then delete the lines between all of these (or
> better, just get rid of these entirely and inline the values below; these will
> never change and look like they're only used once)

Done. Went with the first suggestion.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:66: const int
kMinimumTimeBetweenReportsInMS = 1 * 24 * 60 * 60 * 1000;
On 2015/11/23 23:57:19, Daniel Erat wrote:
> nit: "Ms" looks like it's more common than "MS" in the chrome source. you're
> also using e.g. "Cpu" rather than "CPU" elsewhere

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:69: struct
TaskRecordByCpuSorter {
On 2015/11/23 23:57:19, Daniel Erat wrote:
> would a name like "TaskRecordCpuLessThan" be clearer?

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:79: struct
TaskRecordByMemorySorter {
On 2015/11/23 23:57:19, Daniel Erat wrote:
> ditto here with "TaskRecordMemoryLessThan"

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:142: auto itr =
task_records_.find(id);
On 2015/11/23 23:57:19, Daniel Erat wrote:
> nit: i think that "it" is much more common for iterators in the chromium
source
> (also below)

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:197:
last_browser_process_memory_ = memory_usage != -1 ? memory_usage : 0;
On 2015/11/23 23:57:19, Daniel Erat wrote:
> memory_usage >= 0? (also below)

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:327:
is_first_memory_pressure_event_ = false;
On 2015/11/23 23:57:19, Daniel Erat wrote:
> i think it's unexpected to have a Should/Is method mutating internal state
like
> this. i'd rather that you had some have_seen_first_memory_pressure_event_ or
> similar member that got updated by OnMemoryPressure().

Thanks! |have_seen_first_memory_pressure_event_| is indeed a much better name.
Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.cc:338: for (const
auto& task_data : task_records_by_cpu_) {
On 2015/11/23 23:57:19, Daniel Erat wrote:
> it's non-obvious to me what this is doing without staring at the code for a
> while. please add a comment summarizing it.

This is an algorithm for doing weighted random sampling, taking the CPU usage of
the tasks as their weights. I added a comment, but I doubt it helps much. Maybe
add a link to this page: https://en.wikipedia.org/wiki/Reservoir_sampling ?

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right):

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:23: }
On 2015/11/23 23:57:20, Daniel Erat wrote:
> i think it's generally considered fine (or even preferable) to include base
> headers inside of header files instead of forward-declaring their objects. i'd
> recommend just adding a #include <base/memory/singleton.h> to this file.

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:25: using
MemoryPressureLevel = base::MemoryPressureListener::MemoryPressureLevel;
On 2015/11/23 23:57:20, Daniel Erat wrote:
> please don't pull things out into the top-level namespace in header files.
> MemoryPressureLevel only appears in a single place in this file, so i think
it'd
> be better to just fully-qualify it there.

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:38: explicit
TaskRecord(task_management::TaskId the_id);
On 2015/11/23 23:57:20, Daniel Erat wrote:
> s/the_id/task_id/ ?

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:40:
TaskRecord(task_management::TaskId the_id,
On 2015/11/23 23:57:20, Daniel Erat wrote:
> same here

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:42: double
the_cpu,
On 2015/11/23 23:57:20, Daniel Erat wrote:
> why "the_" on all of these? something like "cpu_percent" would be clearer.
> please also add a comment documenting whether this is [0.0, 1.0] or [0.0,
> 100.0].
> 
> note also that it's safe to use same name for a member and c'tor parameter and
> then do something like:
> 
>     cpu(cpu)
> 
> in the initialization list.

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:43: int64_t
the_memory,
On 2015/11/23 23:57:20, Daniel Erat wrote:
> include the units here. "memory_bytes"? "memory_kb"?

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:59: // invalid.
See TaskManagerInterface::GetPhysicalMemoryUsage().
On 2015/11/23 23:57:20, Daniel Erat wrote:
> does this mean "-1 is used to represent missing values", or something else?

It means that the memory usage calculation for this task is not ready yet. This
happens at the value before the very first calculation is complete and the
result is received on the UI thread.

I clarified the comment a bit.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:90: enum
TaskProcessPriority {
On 2015/11/23 23:57:20, Daniel Erat wrote:
> please use enum classes for all of these to make it harder to mess up the
> typing. see "Enumerated Type Classes and Enum Bases" at
> https://chromium-cpp.appspot.com/.

I think it would be better not to use enum classes here. We need to convert
these enum values to their integral values when creating a Rappor sample and an
UMA histogram stat. Is it worth to clutter the code with static_cast<>'s here
and there?

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:92: BACKGROUND,
On 2015/11/23 23:57:20, Daniel Erat wrote:
> i'd recommend explicitly assigning e.g. BACKGROUND = 1 here to make it clearer
> that these are fixed values, and also doing the same to the other enums.

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:147: static
CpuUsageRange GetCpuUsageRange(double cpu);
On 2015/11/23 23:57:20, Daniel Erat wrote:
> any reason not to just put these into the anonymous namespace in the .cc file?

These are exposed for testing.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:151: // into.
On 2015/11/23 23:57:20, Daniel Erat wrote:
> grammar nit: remove "into" since this comment already includes "in which"
> earlier

Done.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:190: const
CpuCoresNumberRange system_cpu_cores_range_;
On 2015/11/23 23:57:20, Daniel Erat wrote:
> what does this represent? the range that includes the number of cores present
on
> the system?

Yes. I added a comment.

https://codereview.chromium.org/1374283003/diff/440001/chrome/browser/chromeo...
chrome/browser/chromeos/resource_reporter/resource_reporter.h:192: // the most
recent reading for the browser and GPU processes to be reported as
On 2015/11/23 23:57:20, Daniel Erat wrote:
> nit: s/the/The/

Done.

Daniel Erat

just a few more comments https://codereview.chromium.org/1374283003/diff/460001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/460001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode87 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:87: int64_t memory_bytest, fix the ...

5 years ago (2015-11-24 22:03:52 UTC) #39

afakhry

https://codereview.chromium.org/1374283003/diff/460001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc File chrome/browser/chromeos/resource_reporter/resource_reporter.cc (right): https://codereview.chromium.org/1374283003/diff/460001/chrome/browser/chromeos/resource_reporter/resource_reporter.cc#newcode87 chrome/browser/chromeos/resource_reporter/resource_reporter.cc:87: int64_t memory_bytest, On 2015/11/24 22:03:52, Daniel Erat wrote: > ...

5 years ago (2015-11-24 22:49:49 UTC) #40

Daniel Erat

thanks! lgtm with one comment. https://codereview.chromium.org/1374283003/diff/480001/chrome/browser/chromeos/resource_reporter/resource_reporter.h File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right): https://codereview.chromium.org/1374283003/diff/480001/chrome/browser/chromeos/resource_reporter/resource_reporter.h#newcode95 chrome/browser/chromeos/resource_reporter/resource_reporter.h:95: RANGE_0_TO_10_PERCENT = 0, i ...

5 years ago (2015-11-25 01:20:04 UTC) #41

afakhry

https://codereview.chromium.org/1374283003/diff/480001/chrome/browser/chromeos/resource_reporter/resource_reporter.h File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right): https://codereview.chromium.org/1374283003/diff/480001/chrome/browser/chromeos/resource_reporter/resource_reporter.h#newcode95 chrome/browser/chromeos/resource_reporter/resource_reporter.h:95: RANGE_0_TO_10_PERCENT = 0, On 2015/11/25 01:20:04, Daniel Erat wrote: ...

5 years ago (2015-11-25 01:43:26 UTC) #42

Daniel Erat

https://codereview.chromium.org/1374283003/diff/480001/chrome/browser/chromeos/resource_reporter/resource_reporter.h File chrome/browser/chromeos/resource_reporter/resource_reporter.h (right): https://codereview.chromium.org/1374283003/diff/480001/chrome/browser/chromeos/resource_reporter/resource_reporter.h#newcode95 chrome/browser/chromeos/resource_reporter/resource_reporter.h:95: RANGE_0_TO_10_PERCENT = 0, On 2015/11/25 01:43:26, afakhry wrote: > ...

5 years ago (2015-11-25 15:39:28 UTC) #43

afakhry

I also caught a bug that I'm not sure how it slipped in earlier reviews ...

5 years ago (2015-11-25 19:50:41 UTC) #44

afakhry

The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, nick@chromium.org, holte@chromium.org, derat@chromium.org ...

5 years ago (2015-11-30 18:58:48 UTC) #47

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1374283003/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1374283003/580001

5 years ago (2015-11-30 19:05:18 UTC) #48

commit-bot: I haz the power

Description was changed from ========== Reporting top cpu and memory consumers via rappor on chromeos ...

5 years ago (2015-11-30 21:09:06 UTC) #49

commit-bot: I haz the power

Description was changed from ========== Reporting top cpu and memory consumers via rappor on chromeos ...

5 years ago (2015-11-30 21:10:07 UTC) #51

commit-bot: I haz the power

5 years ago (2015-11-30 21:10:08 UTC) #52

Message was sent while issue was closed.

Patchset 29 (id:??) landed as
https://crrev.com/c305019fc15adad7c35276fd3eb68617adc79732
Cr-Commit-Position: refs/heads/master@{#362220}

Issue 1374283003: Reporting top cpu and memory consumers via rappor on chromeos (Closed)

Description

Patch Set 1 #

Patch Set 2 : [WIP] Added the rappor metrics and tested the code manually. #

Patch Set 3 : [WIP]: Delete mistakenly added rappor.old.xml #

Patch Set 4 : [WIP]: Started working on unittests ... #

Patch Set 5 : [WIP] Test up and running #

Patch Set 6 : [WIP]: Almost there, remaining few TODOs. #

Patch Set 7 : [WIP]: Added listening to the MetricsService. #

Patch Set 8 : Finished the system and tested it with user-opting in. #

Patch Set 9 : Modified the memory ranges #

Patch Set 10 : Rebase #

Patch Set 11 : TaskManager now can tell if process if foreground or background #

Patch Set 12 : Now using a multi-dimentional rappor metric #

Patch Set 13 : browser/gpu stats by UMA histograms, buckets for cpu cores. #

Patch Set 14 : Nick's comments #

Patch Set 15 : Using literals for UMA histograms #

Patch Set 16 : nick's and holte's comments #

Patch Set 17 : mpearson's comments #

Patch Set 18 : report once per day, no dependency on the metric service, use weighted random sampling #

Patch Set 19 : holte's comments #

Patch Set 20 : Removed arbitrary given weights #

Patch Set 21 : Passing a base::Process instead #

Patch Set 22 : Rebase #

Patch Set 23 : Add OWNERS file. #

Patch Set 24 : derat's comments #

Patch Set 25 : more comments #

Patch Set 26 : consistent enum names. #

Patch Set 27 : Fixed flags fields enum values. #

Patch Set 28 : skip memory pressure events if task manager never refreshed. #

Patch Set 29 : Turn off ResourceReporter on task_management CrOs browser_tests. #

Messages