Remove the unmaintained performance monitor.

Remove the unmaintained performance monitor.

It's not launched nor are there plans to do so. The last commit
was Dec, 2012 and there was a thread about removing it on chromium-dev in May.

My immediate use-case is to quelch the periodic idle wakeups
due to the performance monitor, but at this point, simply removing it seems
preferable to digging in to fix the timer.

BUG=411488, 130212
Committed: https://crrev.com/7f48a174b4c63c11ee0cbbfee4125b89a963b3c0
Cr-Commit-Position: refs/heads/master@{#294298}

[tonyg, 2014-09-06 01:02:49]

tonyg@chromium.org changed reviewers:
+ erikkay@chromium.org, jam@chromium.org, rdevlin.cronin@chromium.org,
thestig@chromium.org

[Lei Zhang, 2014-09-06 03:31:18]

thestig@chromium.org changed reviewers:
+ yoz@chromium.org
- erikkay@chromium.org

[Lei Zhang, 2014-09-06 03:31:18]

-erikkay, +yoz

[Devlin, 2014-09-08 15:30:03]

On 2014/09/06 03:31:18, Lei Zhang wrote:
> -erikkay, +yoz

Two main things I see missing that should be removed:
- chrome/browser/resources/performance_monitor
- third_party/flot/ (and references)

[jam, 2014-09-08 19:34:41]

lgtm

[tonyg, 2014-09-09 01:01:04]

> Two main things I see missing that should be removed:
> - chrome/browser/resources/performance_monitor
> - third_party/flot/ (and references)

Good catch. Done.

In addition, Oystein also reminded me that there was some useful
enabled-by-default functionality here for histograms which we might also extend
in the future to take traces when we detect something like a pegged CPU. I've
updated the patch to leave that skeleton in, while still removing the rest.

[Devlin, 2014-09-09 01:45:46]

On 2014/09/09 01:01:04, tonyg wrote:
> > Two main things I see missing that should be removed:
> > - chrome/browser/resources/performance_monitor
> > - third_party/flot/ (and references)
> 
> Good catch. Done.
> 
> In addition, Oystein also reminded me that there was some useful
> enabled-by-default functionality here for histograms which we might also
extend
> in the future to take traces when we detect something like a pegged CPU. I've
> updated the patch to leave that skeleton in, while still removing the rest.

I could be wrong (it's been awhile since I looked at the code), but I thought
that really, the only polling aspect of the PerformanceMonitor was in checking
the CPU cycles (with the rest - the bulk of the logic - being observer-style
behavior).  If the point of this CL is to prevent the PerformanceMonitor from
waking us up, is this sufficient?  Is it okay since we're reducing the poll
time?

[tonyg, 2014-09-09 01:50:48]

On 2014/09/09 01:45:46, Devlin wrote:
> On 2014/09/09 01:01:04, tonyg wrote:
> > > Two main things I see missing that should be removed:
> > > - chrome/browser/resources/performance_monitor
> > > - third_party/flot/ (and references)
> > 
> > Good catch. Done.
> > 
> > In addition, Oystein also reminded me that there was some useful
> > enabled-by-default functionality here for histograms which we might also
> extend
> > in the future to take traces when we detect something like a pegged CPU.
I've
> > updated the patch to leave that skeleton in, while still removing the rest.
> 
> I could be wrong (it's been awhile since I looked at the code), but I thought
> that really, the only polling aspect of the PerformanceMonitor was in checking
> the CPU cycles (with the rest - the bulk of the logic - being observer-style
> behavior).  If the point of this CL is to prevent the PerformanceMonitor from
> waking us up, is this sufficient?  Is it okay since we're reducing the poll
> time?

Yes, I could just change the interval and not remove it. But removing it seemed
like a good thing to do at this point. Up to other folks on the thread.

[Devlin, 2014-09-09 01:53:59]

On 2014/09/09 01:50:48, tonyg wrote:
> On 2014/09/09 01:45:46, Devlin wrote:
> > On 2014/09/09 01:01:04, tonyg wrote:
> > > > Two main things I see missing that should be removed:
> > > > - chrome/browser/resources/performance_monitor
> > > > - third_party/flot/ (and references)
> > > 
> > > Good catch. Done.
> > > 
> > > In addition, Oystein also reminded me that there was some useful
> > > enabled-by-default functionality here for histograms which we might also
> > extend
> > > in the future to take traces when we detect something like a pegged CPU.
> I've
> > > updated the patch to leave that skeleton in, while still removing the
rest.
> > 
> > I could be wrong (it's been awhile since I looked at the code), but I
thought
> > that really, the only polling aspect of the PerformanceMonitor was in
checking
> > the CPU cycles (with the rest - the bulk of the logic - being observer-style
> > behavior).  If the point of this CL is to prevent the PerformanceMonitor
from
> > waking us up, is this sufficient?  Is it okay since we're reducing the poll
> > time?
> 
> Yes, I could just change the interval and not remove it. But removing it
seemed
> like a good thing to do at this point. Up to other folks on the thread.

My point was just that the motivation of this CL was to prevent the polling
(which was interfering with idle).  But in the latest patch, it looks like the
polling behavior is still present.  As such, should we either:
a) Reconsider the polling behavior (there's probably a more elegant way to do
this nowadays, especially if we don't need super-frequent polling), or
b) Reconsider ripping the whole thing out?

Just want to make sure that the core issue being cited in the issue is actually
solved by the CL. :)

[tonyg, 2014-09-09 02:00:16]

On 2014/09/09 01:53:59, Devlin wrote:
> On 2014/09/09 01:50:48, tonyg wrote:
> > On 2014/09/09 01:45:46, Devlin wrote:
> > > On 2014/09/09 01:01:04, tonyg wrote:
> > > > > Two main things I see missing that should be removed:
> > > > > - chrome/browser/resources/performance_monitor
> > > > > - third_party/flot/ (and references)
> > > > 
> > > > Good catch. Done.
> > > > 
> > > > In addition, Oystein also reminded me that there was some useful
> > > > enabled-by-default functionality here for histograms which we might also
> > > extend
> > > > in the future to take traces when we detect something like a pegged CPU.
> > I've
> > > > updated the patch to leave that skeleton in, while still removing the
> rest.
> > > 
> > > I could be wrong (it's been awhile since I looked at the code), but I
> thought
> > > that really, the only polling aspect of the PerformanceMonitor was in
> checking
> > > the CPU cycles (with the rest - the bulk of the logic - being
observer-style
> > > behavior).  If the point of this CL is to prevent the PerformanceMonitor
> from
> > > waking us up, is this sufficient?  Is it okay since we're reducing the
poll
> > > time?
> > 
> > Yes, I could just change the interval and not remove it. But removing it
> seemed
> > like a good thing to do at this point. Up to other folks on the thread.
> 
> My point was just that the motivation of this CL was to prevent the polling
> (which was interfering with idle).  But in the latest patch, it looks like the
> polling behavior is still present.  As such, should we either:
> a) Reconsider the polling behavior (there's probably a more elegant way to do
> this nowadays, especially if we don't need super-frequent polling), or
> b) Reconsider ripping the whole thing out?
> 
> Just want to make sure that the core issue being cited in the issue is
actually
> solved by the CL. :)

So, there were two poll intervals in the system before. One every 10s and one
every 120s. The UMA metrics were only updated every 120s, but it still polled
every 10s. This patch removes the 10s interval altogether so there is only a
120s UMA interval.

[Devlin, 2014-09-09 02:02:16]

On 2014/09/09 02:00:16, tonyg wrote:
> On 2014/09/09 01:53:59, Devlin wrote:
> > On 2014/09/09 01:50:48, tonyg wrote:
> > > On 2014/09/09 01:45:46, Devlin wrote:
> > > > On 2014/09/09 01:01:04, tonyg wrote:
> > > > > > Two main things I see missing that should be removed:
> > > > > > - chrome/browser/resources/performance_monitor
> > > > > > - third_party/flot/ (and references)
> > > > > 
> > > > > Good catch. Done.
> > > > > 
> > > > > In addition, Oystein also reminded me that there was some useful
> > > > > enabled-by-default functionality here for histograms which we might
also
> > > > extend
> > > > > in the future to take traces when we detect something like a pegged
CPU.
> > > I've
> > > > > updated the patch to leave that skeleton in, while still removing the
> > rest.
> > > > 
> > > > I could be wrong (it's been awhile since I looked at the code), but I
> > thought
> > > > that really, the only polling aspect of the PerformanceMonitor was in
> > checking
> > > > the CPU cycles (with the rest - the bulk of the logic - being
> observer-style
> > > > behavior).  If the point of this CL is to prevent the PerformanceMonitor
> > from
> > > > waking us up, is this sufficient?  Is it okay since we're reducing the
> poll
> > > > time?
> > > 
> > > Yes, I could just change the interval and not remove it. But removing it
> > seemed
> > > like a good thing to do at this point. Up to other folks on the thread.
> > 
> > My point was just that the motivation of this CL was to prevent the polling
> > (which was interfering with idle).  But in the latest patch, it looks like
the
> > polling behavior is still present.  As such, should we either:
> > a) Reconsider the polling behavior (there's probably a more elegant way to
do
> > this nowadays, especially if we don't need super-frequent polling), or
> > b) Reconsider ripping the whole thing out?
> > 
> > Just want to make sure that the core issue being cited in the issue is
> actually
> > solved by the CL. :)
> 
> So, there were two poll intervals in the system before. One every 10s and one
> every 120s. The UMA metrics were only updated every 120s, but it still polled
> every 10s. This patch removes the 10s interval altogether so there is only a
> 120s UMA interval.

Right.  And 120s is sufficiently limited?  (Will assume "yes" and review).

[Devlin, 2014-09-09 02:12:58]

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
File chrome/browser/performance_monitor/performance_monitor.cc (right):

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.cc:10: #include
"base/bind.h"
Do we really need all these includes still?

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.cc:61:
base::TimeDelta::FromSeconds(kGatherIntervalInSeconds),
We can probably safely move this constant into this file now.

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
File chrome/browser/performance_monitor/performance_monitor.h (right):

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.h:5: #ifndef
CHROME_BROWSER_PERFORMANCE_MONITOR_PERFORMANCE_MONITOR_H_
I think that if we remove 90% of this, we can probably safely move it up to
chrome/browser (rather than having its own folder).  I don't feel strongly
though, so up to you.

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.h:10: #include
"base/memory/scoped_ptr.h"
Don't need.

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.h:12: #include
"base/time/time.h"
don't need.

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.h:19: namespace
performance_monitor {
Same comment as on having its own folder.

[tonyg, 2014-09-09 18:28:10]

The trybots noticed that flot is used elsewhere so I brought it back in addition
to addressing the comments below.

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
File chrome/browser/performance_monitor/performance_monitor.cc (right):

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.cc:10: #include
"base/bind.h"
On 2014/09/09 02:12:57, Devlin wrote:
> Do we really need all these includes still?

Good catch, I removed most of them.

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.cc:61:
base::TimeDelta::FromSeconds(kGatherIntervalInSeconds),
On 2014/09/09 02:12:57, Devlin wrote:
> We can probably safely move this constant into this file now.

Done.

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
File chrome/browser/performance_monitor/performance_monitor.h (right):

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.h:5: #ifndef
CHROME_BROWSER_PERFORMANCE_MONITOR_PERFORMANCE_MONITOR_H_
On 2014/09/09 02:12:57, Devlin wrote:
> I think that if we remove 90% of this, we can probably safely move it up to
> chrome/browser (rather than having its own folder).  I don't feel strongly
> though, so up to you.

I'll leave it here. As Erik mentioned, it may grow again at some point.

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.h:10: #include
"base/memory/scoped_ptr.h"
On 2014/09/09 02:12:57, Devlin wrote:
> Don't need.

Done.

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.h:12: #include
"base/time/time.h"
On 2014/09/09 02:12:58, Devlin wrote:
> don't need.

Done.

https://codereview.chromium.org/547063003/diff/80001/chrome/browser/performan...
chrome/browser/performance_monitor/performance_monitor.h:19: namespace
performance_monitor {
On 2014/09/09 02:12:57, Devlin wrote:
> Same comment as on having its own folder.

Acknowledged.

[Devlin, 2014-09-10 21:40:57]

lgtm

https://codereview.chromium.org/547063003/diff/100001/chrome/browser/performa...
File chrome/browser/performance_monitor/performance_monitor.cc (right):

https://codereview.chromium.org/547063003/diff/100001/chrome/browser/performa...
chrome/browser/performance_monitor/performance_monitor.cc:32:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
Since you're here, mind updating this (and its friends) to be
CHECK_CURRENTLY_ON?

https://codereview.chromium.org/547063003/diff/100001/chrome/browser/performa...
File chrome/browser/performance_monitor/performance_monitor.h (right):

https://codereview.chromium.org/547063003/diff/100001/chrome/browser/performa...
chrome/browser/performance_monitor/performance_monitor.h:21: const int
kGatherIntervalInSeconds = 120;
Do we need this here, or could it be in an anonymous namespace in the .cc?

[tonyg, 2014-09-10 21:54:56]

On 2014/09/10 21:40:57, Devlin wrote:
> lgtm
> 
>
https://codereview.chromium.org/547063003/diff/100001/chrome/browser/performa...
> File chrome/browser/performance_monitor/performance_monitor.cc (right):
> 
>
https://codereview.chromium.org/547063003/diff/100001/chrome/browser/performa...
> chrome/browser/performance_monitor/performance_monitor.cc:32:
> CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
> Since you're here, mind updating this (and its friends) to be
> CHECK_CURRENTLY_ON?

Done.

> 
>
https://codereview.chromium.org/547063003/diff/100001/chrome/browser/performa...
> File chrome/browser/performance_monitor/performance_monitor.h (right):
> 
>
https://codereview.chromium.org/547063003/diff/100001/chrome/browser/performa...
> chrome/browser/performance_monitor/performance_monitor.h:21: const int
> kGatherIntervalInSeconds = 120;
> Do we need this here, or could it be in an anonymous namespace in the .cc?

Done.

[tonyg, 2014-09-10 22:03:15]

The CQ bit was checked by tonyg@chromium.org

[commit-bot: I haz the power, 2014-09-10 22:04:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tonyg@chromium.org/547063003/140001

[commit-bot: I haz the power, 2014-09-11 02:27:42]

Committed patchset #8 (id:140001) as 4073ed588b5576ea048046600a6f22161f133416

[commit-bot: I haz the power, 2014-09-11 03:09:20]

Patchset 8 (id:??) landed as
https://crrev.com/7f48a174b4c63c11ee0cbbfee4125b89a963b3c0
Cr-Commit-Position: refs/heads/master@{#294298}