Enable UMA histogram logger for low memor kill events in ArcMetricsService.

1. Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

2. renamed UMA histogram ArcRuntime.* => Arc.*
3. Move ArcLowMemoryMonitor to components/ for better DEPS management.

BUG=597899
TEST="Tested on minnie"

Committed: https://crrev.com/b238d39b1f0e3dd475a91f5ac04534a084e12282
Cr-Commit-Position: refs/heads/master@{#383482}

[cylee1, 2016-03-22 21:54:03]

cylee@chromium.org changed reviewers:
+ hidehiko@chromium.org, rkaplow@chromium.org

[rkaplow, 2016-03-22 21:57:20]

https://codereview.chromium.org/1828533002/diff/30001/components/arc/metrics/...
File components/arc/metrics/arc_low_memory_killer_monitor.cc (right):

https://codereview.chromium.org/1828533002/diff/30001/components/arc/metrics/...
components/arc/metrics/arc_low_memory_killer_monitor.cc:103:
UMA_HISTOGRAM_LOWMEMORYKILL_TIMES(
are you deleting the definition of this?

https://codereview.chromium.org/1828533002/diff/30001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1828533002/diff/30001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:838: -<histogram
name="ArcRuntime.LowMemoryKiller.FreedSize" units="KB">
instead of deleting please mark as deprecated (and can add date)

[cylee1, 2016-03-23 21:46:59]

Ping hidehiko,
  Not sure how should I handle the DEPS change ?  Thanks.

https://codereview.chromium.org/1828533002/diff/30001/components/arc/metrics/...
File components/arc/metrics/arc_low_memory_killer_monitor.cc (right):

https://codereview.chromium.org/1828533002/diff/30001/components/arc/metrics/...
components/arc/metrics/arc_low_memory_killer_monitor.cc:103:
UMA_HISTOGRAM_LOWMEMORYKILL_TIMES(
On 2016/03/22 21:57:20, rkaplow wrote:
> are you deleting the definition of this?

No. It's defined on line 32.

https://codereview.chromium.org/1828533002/diff/30001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/1828533002/diff/30001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:838: -<histogram
name="ArcRuntime.LowMemoryKiller.FreedSize" units="KB">
On 2016/03/22 21:57:20, rkaplow wrote:
> instead of deleting please mark as deprecated (and can add date)

Done.

[cylee1, 2016-03-24 06:53:47]

cylee@chromium.org changed reviewers:
+ elijahtaylor@chromium.org

[cylee1, 2016-03-24 06:53:48]

+reviewers to speed up

[hidehiko, 2016-03-24 08:11:21]

Could you run trybots?

https://codereview.chromium.org/1828533002/diff/50001/components/arc/DEPS
File components/arc/DEPS (right):

https://codereview.chromium.org/1828533002/diff/50001/components/arc/DEPS#new...
components/arc/DEPS:10: "+third_party/re2/src/re2/re2.h"
"+third_party/re2" is fine.

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
File components/arc/metrics/arc_low_memory_killer_monitor.cc (right):

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
components/arc/metrics/arc_low_memory_killer_monitor.cc:107:
UMA_HISTOGRAM_CUSTOM_COUNTS(
Shouldn't this be out of the while loop?
If it is intentional to count less values than final oom_kills value, it's nice
to comment why.

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
File components/arc/metrics/arc_metrics_service.h (right):

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
components/arc/metrics/arc_metrics_service.h:40:
scoped_ptr<ArcLowMemoryKillerMonitor> low_memory_killer_minotor_;
You just can use ArcLowMemoryKillerMonitor directly here.

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
components/arc/metrics/arc_metrics_service.h:51: #endif  //
COMPONENTS_ARC_METRICS_ARC_METRICS_SERVICE_H
unnecessary trailing white space.

https://codereview.chromium.org/1828533002/diff/50001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1828533002/diff/50001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:838: +<histogram
name="Arc.LowMemoryKiller.Count">
It's better to have a description in your commit message.

[cylee1, 2016-03-24 14:20:47]

Description was changed from

==========
Enable UMA histogram logger for low memor kill events in ArcMetricsService.

BUG=none
==========

to

==========
Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

BUG=none
==========

[cylee1, 2016-03-24 14:36:06]

cylee@chromium.org changed reviewers:
+ tfarina@chromium.org

[cylee1, 2016-03-24 14:36:07]

Thanks for reviewing.

+ tfarina@ for DEPS.

https://codereview.chromium.org/1828533002/diff/50001/components/arc/DEPS
File components/arc/DEPS (right):

https://codereview.chromium.org/1828533002/diff/50001/components/arc/DEPS#new...
components/arc/DEPS:10: "+third_party/re2/src/re2/re2.h"
On 2016/03/24 08:11:21, hidehiko wrote:
> "+third_party/re2" is fine.

Done.

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
File components/arc/metrics/arc_low_memory_killer_monitor.cc (right):

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
components/arc/metrics/arc_low_memory_killer_monitor.cc:107:
UMA_HISTOGRAM_CUSTOM_COUNTS(
On 2016/03/24 08:11:21, hidehiko wrote:
> Shouldn't this be out of the while loop?
> If it is intentional to count less values than final oom_kills value, it's
nice
> to comment why.

Yes it's intentional. One of the reason is this dedicated loop may not exit when
shutting down chrome because of the blocking fgets(). As a workaround a
cumulative count should be acceptable. The existing metric
"Tabs.SadTab.KillCreated.OOM" is also cumulative.

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
File components/arc/metrics/arc_metrics_service.h (right):

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
components/arc/metrics/arc_metrics_service.h:40:
scoped_ptr<ArcLowMemoryKillerMonitor> low_memory_killer_minotor_;
On 2016/03/24 08:11:21, hidehiko wrote:
> You just can use ArcLowMemoryKillerMonitor directly here.

Done.

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
components/arc/metrics/arc_metrics_service.h:51: #endif  //
COMPONENTS_ARC_METRICS_ARC_METRICS_SERVICE_H
On 2016/03/24 08:11:21, hidehiko wrote:
> unnecessary trailing white space.

There's no trailing white space?
#endif<space><space>//<space>XXXXX 
seems to be the right format.

https://codereview.chromium.org/1828533002/diff/50001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1828533002/diff/50001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:838: +<histogram
name="Arc.LowMemoryKiller.Count">
On 2016/03/24 08:11:21, hidehiko wrote:
> It's better to have a description in your commit message.

Done.

[cylee1, 2016-03-24 14:39:22]

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
File components/arc/metrics/arc_metrics_service.h (right):

https://codereview.chromium.org/1828533002/diff/50001/components/arc/metrics/...
components/arc/metrics/arc_metrics_service.h:51: #endif  //
COMPONENTS_ARC_METRICS_ARC_METRICS_SERVICE_H
On 2016/03/24 14:36:06, cylee1 wrote:
> On 2016/03/24 08:11:21, hidehiko wrote:
> > unnecessary trailing white space.
> 
> There's no trailing white space?
> #endif<space><space>//<space>XXXXX 
> seems to be the right format.

Ah I know what happens. The original file doesn't contain a newline at EOF so
introduced the diff.

[rkaplow, 2016-03-24 15:04:59]

lgtm

[hidehiko, 2016-03-24 16:52:25]

Again, could you run trybots? I'd like to see it's green before my l-g-t-m.
Also, could you add TEST= line?

> Ah I know what happens. The original file doesn't contain a newline at EOF so
introduced the diff.

Oh, got it. Thank you for explanation!

https://codereview.chromium.org/1828533002/diff/50001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1828533002/diff/50001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:838: +<histogram
name="Arc.LowMemoryKiller.Count">
On 2016/03/24 14:36:07, cylee1 wrote:
> On 2016/03/24 08:11:21, hidehiko wrote:
> > It's better to have a description in your commit message.
> 
> Done.

Oops, sorry for my poor explanation. Would you mind also to mention about the
renaming from ArcRuntime to Arc?

[cylee1, 2016-03-24 17:41:09]

Description was changed from

==========
Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

BUG=none
==========

to

==========
Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

Also renamed ArcRuntime.* => Arc.*

BUG=none
TEST="Tested on minnie"
==========

[cylee1, 2016-03-24 22:31:26]

It's still failing
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

The include seems to be illegal, please suggest how to fix it

ERROR at //components/arc/metrics/arc_low_memory_killer_monitor.cc:23:11: Can't
include this header from here.
#include "third_party/re2/src/re2/re2.h"
          ^----------------------------
The target:
  //components/arc:arc
is including a file from the target:
  //third_party/re2:re2

It's usually best to depend directly on the destination target.
In some cases, the destination target is considered a subcomponent
of an intermediate target. In this case, the intermediate target
should depend publicly on the destination to forward the ability
to include headers.

Dependency chain (there may also be others):
  //components/arc:arc -->
  //components/exo:exo --[private]-->
  //gpu:gpu -->
  //gpu/command_buffer/service:service_sources --[private]-->
  //third_party/re2:re2

GN gen failed: 1

[cylee1, 2016-03-24 23:52:21]

https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
Also some browser test fails, but I don't understand the cause for 100%.

I can reproduce it with chromeos=1 flag on my desktop.
Some test (e.g., NaClBrowserTestGLibc.SysconfNprocessorsOnln) always timed out
(blocked in fgets()), but on real machine there's no problem since it's a
dedicated thread. In addition, the problem only happens on release binary, but
not debug binary.
Does our browser test have some rule like "always wait until all thread ends",
or "use single thread when testing" ? 
I could not figure out how to solve it. Using a nonblocking fgets() seems to
help, but it forms a busy loop. Also this looks like a testing harness problem
so I don't want to change my logic if possible.
Suggestion please !

[cylee1, 2016-03-25 00:05:15]

+bradnelson@

HI bradnelson,
  I have some problem regarding the test NaClBrowserTest_SysconfNprocessorsOnln.
This CL initiates a dedicated thread listening on kernel message. It uses a
blocking read on /dev/kmsg and the thread may never end. Somehow it causes the
mentioned test to block forever. I don't have enough knowledge on this test. Do
you have any idea why the problem and possible solution?
  Help appreciated. Thanks !

[hidehiko, 2016-03-25 04:51:57]

On 2016/03/25 00:05:15, cylee1 wrote:
> +bradnelson@
> 
> HI bradnelson,
>   I have some problem regarding the test
NaClBrowserTest_SysconfNprocessorsOnln.
> This CL initiates a dedicated thread listening on kernel message. It uses a
> blocking read on /dev/kmsg and the thread may never end. Somehow it causes the
> mentioned test to block forever. I don't have enough knowledge on this test.
Do
> you have any idea why the problem and possible solution?
>   Help appreciated. Thanks !

As for gn error, I think it is caused by missing deps in BUILD.gn?

As for NaCl test timeout;
- Does it happen without your CL w/ Release build, or only with yours?

Just I guess;
Your callback runs on a dedicated SequentialWorkerPool, which depends on
SimpleThread.
And in the comments of SimpleThread;

====
NOTE: You *MUST* call Join on the thread to clean up the underlying thread
// resources.
====
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/sim...

So, infinite loop can cause it?

[cylee1, 2016-03-25 05:18:10]

> As for gn error, I think it is caused by missing deps in BUILD.gn?
Thank you hidehiko and sorry, just found the stupid error.


> NOTE: You *MUST* call Join on the thread to clean up the underlying thread
> // resources.
> ====
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/sim...
> 
> So, infinite loop can cause it?

Hmm, but I meant not to join thread (I guess that's what CONTINUE_ON_SHUTDOWN
means?)
 I tried some similar tests in nacl_browsertest.cc, but only this one fails. It
seems to load different nacl-embedded webage and others are fine.
Let's see what bradnelson thinks.

[cylee1, 2016-03-25 05:18:50]

cylee@chromium.org changed reviewers:
+ bradnelson@chromium.org

[cylee, 2016-03-25 19:05:09]

Description was changed from

==========
Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

Also renamed ArcRuntime.* => Arc.*

BUG=none
TEST="Tested on minnie"
==========

to

==========
1. Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

2. renamed UMA histogram ArcRuntime.* => Arc.*
3. Move ArcLowMemoryMonitor to components/ for better DEPS management.

BUG=none
TEST="Tested on minnie"
==========

[cylee, 2016-03-25 20:12:26]

cylee@google.com changed reviewers:
+ cylee@google.com, mseaborn@chromium.org

[cylee, 2016-03-25 20:19:59]

Description was changed from

==========
1. Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

2. renamed UMA histogram ArcRuntime.* => Arc.*
3. Move ArcLowMemoryMonitor to components/ for better DEPS management.

BUG=none
TEST="Tested on minnie"
==========

to

==========
1. Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

2. renamed UMA histogram ArcRuntime.* => Arc.*
3. Move ArcLowMemoryMonitor to components/ for better DEPS management.

BUG=597899
TEST="Tested on minnie"
==========

[Mark Seaborn, 2016-03-25 21:26:25]

LGTM for the fflush() changes in chrome/test/nacl/nacl_browsertest.cc

https://codereview.chromium.org/1828533002/diff/190001/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/1828533002/diff/190001/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:195: // crbug.com/597899
This reference isn't really necessary -- the absence of a fflush(NULL) call
doesn't require an explanation. :-)

[cylee1, 2016-03-25 21:34:03]

Thanks great.

Hi Hidehiko, 
  Now the bots are green, PTAL !

https://codereview.chromium.org/1828533002/diff/190001/chrome/test/nacl/nacl_...
File chrome/test/nacl/nacl_browsertest.cc (right):

https://codereview.chromium.org/1828533002/diff/190001/chrome/test/nacl/nacl_...
chrome/test/nacl/nacl_browsertest.cc:195: // crbug.com/597899
On 2016/03/25 21:26:25, Mark Seaborn wrote:
> This reference isn't really necessary -- the absence of a fflush(NULL) call
> doesn't require an explanation. :-)

Ye.. But anyway I think it doesn't hurt to keep it.
In case somebody is tracing the log and wondering why the change. Or even
worse... if somebody wants to change it back ... ha..

[tfarina, 2016-03-27 08:03:59]

lgtm for the re2 dependency, though it should have been added to the DEPS file
when it was first used.

https://codereview.chromium.org/1828533002/diff/190001/components/arc/DEPS
File components/arc/DEPS (right):

https://codereview.chromium.org/1828533002/diff/190001/components/arc/DEPS#ne...
components/arc/DEPS:10: "+third_party/re2",
You are adding now but for a line that is already exist, I feel this
should be done separately from this change.

But since it is so small, I'm ok with it being part of this CL. Just wanted to
note that.

https://codereview.chromium.org/1828533002/diff/190001/components/arc/metrics...
File components/arc/metrics/arc_low_memory_killer_monitor.cc (right):

https://codereview.chromium.org/1828533002/diff/190001/components/arc/metrics...
components/arc/metrics/arc_low_memory_killer_monitor.cc:23: #include
"third_party/re2/src/re2/re2.h"
why is the presubmit complaining now? Wasn't it complaining before?

[hidehiko, 2016-03-28 04:18:43]

LGTM for arc.

https://codereview.chromium.org/1828533002/diff/190001/components/arc/metrics...
File components/arc/metrics/arc_low_memory_killer_monitor.cc (right):

https://codereview.chromium.org/1828533002/diff/190001/components/arc/metrics...
components/arc/metrics/arc_low_memory_killer_monitor.cc:23: #include
"third_party/re2/src/re2/re2.h"
On 2016/03/27 08:03:59, tfarina wrote:
> why is the presubmit complaining now? Wasn't it complaining before?
> 

FYI: this is moved from other location, that's why a new DEPS line is needed.

[cylee1, 2016-03-28 05:24:23]

Thanks ! going to submit the CL

https://codereview.chromium.org/1828533002/diff/190001/components/arc/DEPS
File components/arc/DEPS (right):

https://codereview.chromium.org/1828533002/diff/190001/components/arc/DEPS#ne...
components/arc/DEPS:10: "+third_party/re2",
On 2016/03/27 08:03:59, tfarina wrote:
> You are adding now but for a line that is already exist, I feel this
> should be done separately from this change.
> 
> But since it is so small, I'm ok with it being part of this CL. Just wanted to
> note that.

As explained by hidehiko, this file is moved from other location. That's why
this line is added in this CL.

[cylee1, 2016-03-28 05:24:36]

The CQ bit was checked by cylee@chromium.org

[cylee1, 2016-03-28 05:24:37]

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1828533002/#ps190001
(title: "Fix NaClBrowserTest")

[commit-bot: I haz the power, 2016-03-28 05:24:49]

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

[commit-bot: I haz the power, 2016-03-28 06:34:07]

Description was changed from

==========
1. Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

2. renamed UMA histogram ArcRuntime.* => Arc.*
3. Move ArcLowMemoryMonitor to components/ for better DEPS management.

BUG=597899
TEST="Tested on minnie"
==========

to

==========
1. Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

2. renamed UMA histogram ArcRuntime.* => Arc.*
3. Move ArcLowMemoryMonitor to components/ for better DEPS management.

BUG=597899
TEST="Tested on minnie"
==========

[commit-bot: I haz the power, 2016-03-28 06:34:08]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2016-03-28 06:36:05]

Description was changed from

==========
1. Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

2. renamed UMA histogram ArcRuntime.* => Arc.*
3. Move ArcLowMemoryMonitor to components/ for better DEPS management.

BUG=597899
TEST="Tested on minnie"
==========

to

==========
1. Enable UMA histogram logger for low memory kill events in ArcMetricsService.
- Arc.LowMemoryKiller.Count: cumulative count of #kills.
- Arc.LowMemoryKiller.FreedSize: Memory freed by each kill.
- Arc.LowMemoryKiller.TimeDelta: Elapsed time since last kill.

2. renamed UMA histogram ArcRuntime.* => Arc.*
3. Move ArcLowMemoryMonitor to components/ for better DEPS management.

BUG=597899
TEST="Tested on minnie"

Committed: https://crrev.com/b238d39b1f0e3dd475a91f5ac04534a084e12282
Cr-Commit-Position: refs/heads/master@{#383482}
==========

[commit-bot: I haz the power, 2016-03-28 06:36:07]

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