Added MemoryMonitor for Linux (with test)

Added MemoryMonitor for Linux (with test).

Extraction of common code also means some changes to the
Windows memory-monitor.

BUG=644397
Committed: https://crrev.com/8ceca0e580766a9f91cf655a52de1ebd97d8443d
Cr-Commit-Position: refs/heads/master@{#417607}

[bcwhite, 2016-09-06 19:30:53]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[bcwhite, 2016-09-06 19:31:28]

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

[bcwhite, 2016-09-06 19:31:29]

Chris, who is the best person to review these modules?

[commit-bot: I haz the power, 2016-09-06 19:32:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 20:35:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 20:35:49]

Dry run: This issue passed the CQ dry run.

[chrisha, 2016-09-07 15:37:04]

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
File components/memory_coordinator/browser/memory_monitor_linux.cc (right):

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.cc:73: return
(mem_info.total / (100 / kTargetAvailableMemoryPercentage)) >> 10;
You wouldn't need the 'must be divisible by' constraint if you rewrote this as:

(kTargetAvailableMemoryPercentage * mem_info.total / 100) >> 10

This could potentially overflow, but it is very unlikely if mem_info.total is
64GB. You could also avoid the overflow issue by simply casting to doubles and
doing the math over there.

Also, 10 should be defined as kPowerOfTwoMiB or something like that (lacking a
good name...)

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
File components/memory_coordinator/browser/memory_monitor_linux.h (right):

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.h:24: // no remainder
when 100 is divided by it.
Add a static_assert to enforce that fact?

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.h:55: int
target_free_mb_;
target_free_mb is maybe a little misleading? In the Windows implementation, the
free memory reported by the MM is in terms of total system memory remaining,
regardless of whether or not using it will incur paging. The MemoryCoordinator
provides the buffer from this limit, and the intent of the MemoryMonitor is to
draw a hard line in the sand.

In the Linux implementation, this is effectively a small buffer beyond the
OS-reported limit. Do we need this buffer?

Also, is there nothing we can do for pre 3.14 kernels? There could still be some
of the lying about, given that 3.13 is only 2.5 years old.

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.h:60: class
MEMORY_COORDINATOR_EXPORT MemoryMonitorLinuxDelegate {
MemoryMOnitorWin uses this as well. Worth promoting to a single shared
GetSystemMemoryInfoDelegate to be reused on both sides?

(ie. promote to get_system_memory_info_delegate.h|cc, move the singleton and
test utilities over there, etc)

[bcwhite, 2016-09-07 17:23:47]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 17:24:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-09-07 17:26:01]

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
File components/memory_coordinator/browser/memory_monitor_linux.cc (right):

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.cc:73: return
(mem_info.total / (100 / kTargetAvailableMemoryPercentage)) >> 10;
> You wouldn't need the 'must be divisible by' constraint if you rewrote this
as:
> 
> (kTargetAvailableMemoryPercentage * mem_info.total / 100) >> 10
> 
> This could potentially overflow, but it is very unlikely if mem_info.total is
> 64GB. You could also avoid the overflow issue by simply casting to doubles and
> doing the math over there.

Exactly.  I chose it this way so it couldn't overflow.  And given that the
values are coming from something completely outside our control (i.e. the user's
system), it seemed better to be safe.


> Also, 10 should be defined as kPowerOfTwoMiB or something like that (lacking a
> good name...)

Done.

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
File components/memory_coordinator/browser/memory_monitor_linux.h (right):

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.h:24: // no remainder
when 100 is divided by it.
On 2016/09/07 15:37:04, chrisha (slow) wrote:
> Add a static_assert to enforce that fact?

Done.

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.h:55: int
target_free_mb_;
> target_free_mb is maybe a little misleading? In the Windows implementation,
the
> free memory reported by the MM is in terms of total system memory remaining,
> regardless of whether or not using it will incur paging. The MemoryCoordinator
> provides the buffer from this limit, and the intent of the MemoryMonitor is to
> draw a hard line in the sand.

Renamed to "target_avail_mb_" and comment updated.


> Also, is there nothing we can do for pre 3.14 kernels? There could still be
some
> of the lying about, given that 3.13 is only 2.5 years old.

Done.

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.h:60: class
MEMORY_COORDINATOR_EXPORT MemoryMonitorLinuxDelegate {
On 2016/09/07 15:37:04, chrisha (slow) wrote:
> MemoryMOnitorWin uses this as well. Worth promoting to a single shared
> GetSystemMemoryInfoDelegate to be reused on both sides?
> 
> (ie. promote to get_system_memory_info_delegate.h|cc, move the singleton and
> test utilities over there, etc)

I wasn't sure how similar these will be.  It's referenced only within this file
so moving it increases the conceptual complexity.  And while the code is
duplicated, only once instance of it is ever compiled.

[chrisha, 2016-09-07 19:55:28]

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
File components/memory_coordinator/browser/memory_monitor_linux.h (right):

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.h:55: int
target_free_mb_;
On 2016/09/07 17:26:01, bcwhite wrote:
> > target_free_mb is maybe a little misleading? In the Windows implementation,
> the
> > free memory reported by the MM is in terms of total system memory remaining,
> > regardless of whether or not using it will incur paging. The
MemoryCoordinator
> > provides the buffer from this limit, and the intent of the MemoryMonitor is
to
> > draw a hard line in the sand.
> 
> Renamed to "target_avail_mb_" and comment updated.
> 
> 
> > Also, is there nothing we can do for pre 3.14 kernels? There could still be
> some
> > of the lying about, given that 3.13 is only 2.5 years old.
> 
> Done.

My question about the necessity of this still remains. This is a fudge factor
beyond the OS reported swapping cliff, no? If this is true, then I don't think
the fudge factor belongs here, rather it belongs in the MemoryCoordinator (the
client of this class).

The idea of the MemoryMonitor is for it to tell you *exactly* how far away you
are from a swap cliff. On Windows there's some "fudge" build into the monitor
because there's no way to get the OS to tell you straight up where that limit
is.

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.h:60: class
MEMORY_COORDINATOR_EXPORT MemoryMonitorLinuxDelegate {
On 2016/09/07 17:26:01, bcwhite wrote:
> On 2016/09/07 15:37:04, chrisha (slow) wrote:
> > MemoryMOnitorWin uses this as well. Worth promoting to a single shared
> > GetSystemMemoryInfoDelegate to be reused on both sides?
> > 
> > (ie. promote to get_system_memory_info_delegate.h|cc, move the singleton and
> > test utilities over there, etc)
> 
> I wasn't sure how similar these will be.  It's referenced only within this
file
> so moving it increases the conceptual complexity.  And while the code is
> duplicated, only once instance of it is ever compiled.

I think it's likely to be very common across the memory monitors for at least
ChromeOS, Linux, Win, Mac and Chromecast (judging by the current
MemoryPressureMonitor implementations).

Even though only one instance is compiled, I still think there's overhead in
maintaining multiple instances of the code itself.

[commit-bot: I haz the power, 2016-09-07 20:07:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 20:07:31]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-09-07 20:50:27]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 20:51:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-09-07 20:53:40]

I'll update the Windows code once you're satisfied with the Linux code.

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
File components/memory_coordinator/browser/memory_monitor_linux.h (right):

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.h:55: int
target_free_mb_;
> My question about the necessity of this still remains. This is a fudge factor
> beyond the OS reported swapping cliff, no? If this is true, then I don't think
> the fudge factor belongs here, rather it belongs in the MemoryCoordinator (the
> client of this class).

I don't have a strong opinion about it.  Perhaps I wasn't clear about it when I
brought it up in our 1:1 last week.


> The idea of the MemoryMonitor is for it to tell you *exactly* how far away you
> are from a swap cliff. On Windows there's some "fudge" build into the monitor
> because there's no way to get the OS to tell you straight up where that limit
> is.

Fair enough.  Overhead removed.

https://codereview.chromium.org/2310193002/diff/1/components/memory_coordinat...
components/memory_coordinator/browser/memory_monitor_linux.h:60: class
MEMORY_COORDINATOR_EXPORT MemoryMonitorLinuxDelegate {
On 2016/09/07 19:55:28, chrisha (slow) wrote:
> On 2016/09/07 17:26:01, bcwhite wrote:
> > On 2016/09/07 15:37:04, chrisha (slow) wrote:
> > > MemoryMOnitorWin uses this as well. Worth promoting to a single shared
> > > GetSystemMemoryInfoDelegate to be reused on both sides?
> > > 
> > > (ie. promote to get_system_memory_info_delegate.h|cc, move the singleton
and
> > > test utilities over there, etc)
> > 
> > I wasn't sure how similar these will be.  It's referenced only within this
> file
> > so moving it increases the conceptual complexity.  And while the code is
> > duplicated, only once instance of it is ever compiled.
> 
> I think it's likely to be very common across the memory monitors for at least
> ChromeOS, Linux, Win, Mac and Chromecast (judging by the current
> MemoryPressureMonitor implementations).
> 
> Even though only one instance is compiled, I still think there's overhead in
> maintaining multiple instances of the code itself.

Done.

[chrisha, 2016-09-07 21:25:09]

I don't think it was a problem of you not being clear when we talked, but rather
me not understanding fully. I distinctly remember you talking about the fudge
factor, but I guess I wasn't clear on its purpose. I've got a pretty thick skull
:)

https://codereview.chromium.org/2310193002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor.h (right):

https://codereview.chromium.org/2310193002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor.h:19: // A class for
fetching system information used by a memory monitor. This can
I'd move this to below the MemoryMonitor definition, as it's more of an
implementation detail.

https://codereview.chromium.org/2310193002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor_linux_unittest.cc
(right):

https://codereview.chromium.org/2310193002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_linux_unittest.cc:15: class
TestMemoryMonitorLinuxDelegate : public MemoryMonitorDelegate {
Move this to a test_memory_monitor_delegate.h/.cc, so it can be reused in Linux
and Windows too?

[commit-bot: I haz the power, 2016-09-07 22:10:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 22:10:25]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-09-08 14:09:23]

Description was changed from

==========
Added MemoryMonitor for Linux (with test)

BUG=644397
==========

to

==========
Added MemoryMonitor for Linux (with test).

Extraction of common code also means some changes to the
Windows memory-monitor.

BUG=644397
==========

[bcwhite, 2016-09-08 14:10:20]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 14:10:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 14:26:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 14:26:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bcwhite, 2016-09-08 15:05:31]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 15:05:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-09-08 15:06:24]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-09-08 16:24:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 16:24:40]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-09-08 16:36:49]

Windows changes added.

https://codereview.chromium.org/2310193002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor.h (right):

https://codereview.chromium.org/2310193002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor.h:19: // A class for
fetching system information used by a memory monitor. This can
On 2016/09/07 21:25:09, chrisha (slow) wrote:
> I'd move this to below the MemoryMonitor definition, as it's more of an
> implementation detail.

Done.

https://codereview.chromium.org/2310193002/diff/40001/components/memory_coord...
File components/memory_coordinator/browser/memory_monitor_linux_unittest.cc
(right):

https://codereview.chromium.org/2310193002/diff/40001/components/memory_coord...
components/memory_coordinator/browser/memory_monitor_linux_unittest.cc:15: class
TestMemoryMonitorLinuxDelegate : public MemoryMonitorDelegate {
On 2016/09/07 21:25:09, chrisha (slow) wrote:
> Move this to a test_memory_monitor_delegate.h/.cc, so it can be reused in
Linux
> and Windows too?

Done.

[chrisha, 2016-09-08 21:07:02]

lgtm with a nit

https://codereview.chromium.org/2310193002/diff/80001/components/memory_coord...
File components/memory_coordinator/browser/test_memory_monitor.h (right):

https://codereview.chromium.org/2310193002/diff/80001/components/memory_coord...
components/memory_coordinator/browser/test_memory_monitor.h:29: // values into
|mem_info_|;
Ahh... I had forgotten this detail. Somewhat reduces the utility of this.

Maybe consider having the various "Set" fields be defined in the .h behind
precompiler guards? Would prevent you having to then define these per platform
via inheritance.

Or simply expose a getter for the underlying mem_info_, and modify fields
directly in the unittests?

(I don't feel too strongly about this, so I'll leave it up to you to decide.)

[bcwhite, 2016-09-09 12:54:16]

https://codereview.chromium.org/2310193002/diff/80001/components/memory_coord...
File components/memory_coordinator/browser/test_memory_monitor.h (right):

https://codereview.chromium.org/2310193002/diff/80001/components/memory_coord...
components/memory_coordinator/browser/test_memory_monitor.h:29: // values into
|mem_info_|;
\> Ahh... I had forgotten this detail. Somewhat reduces the utility of this.
> 
> Maybe consider having the various "Set" fields be defined in the .h behind
> precompiler guards? Would prevent you having to then define these per platform
> via inheritance.
> 
> Or simply expose a getter for the underlying mem_info_, and modify fields
> directly in the unittests?

I could.  I know that "protected" isn't desired, but it's test code and seems a
lot cleaner than #ifdef.  Accessing the raw structure through get/set would
remove the derived class but not the method.  There would still have to be the
function.

I don't have a strong opinion either but I don't think any other solution is
cleaner, except maybe no test base class at all.

[chrisha, 2016-09-09 13:26:11]

https://codereview.chromium.org/2310193002/diff/80001/components/memory_coord...
File components/memory_coordinator/browser/test_memory_monitor.h (right):

https://codereview.chromium.org/2310193002/diff/80001/components/memory_coord...
components/memory_coordinator/browser/test_memory_monitor.h:29: // values into
|mem_info_|;
> I don't have a strong opinion either but I don't think any other solution is
> cleaner, except maybe no test base class at all.

IMO opinion, cleanest is simply to have an accessor for the mem_info_ that
allows direct access to its internal fields from unittests. No precompiler code,
no inheritance, no duplicated code per platform, and readability is nearly the
same.

From this:

delegate->SetTotalMemoryKB(...);

to this:

delegate->mem_info()->total = ...;

[bcwhite, 2016-09-09 14:32:53]

https://codereview.chromium.org/2310193002/diff/80001/components/memory_coord...
File components/memory_coordinator/browser/test_memory_monitor.h (right):

https://codereview.chromium.org/2310193002/diff/80001/components/memory_coord...
components/memory_coordinator/browser/test_memory_monitor.h:29: // values into
|mem_info_|;
On 2016/09/09 13:26:11, chrisha (slow) wrote:
> > I don't have a strong opinion either but I don't think any other solution is
> > cleaner, except maybe no test base class at all.
> 
> IMO opinion, cleanest is simply to have an accessor for the mem_info_ that
> allows direct access to its internal fields from unittests. No precompiler
code,
> no inheritance, no duplicated code per platform, and readability is nearly the
> same.
> 
> From this:
> 
> delegate->SetTotalMemoryKB(...);
> 
> to this:
> 
> delegate->mem_info()->total = ...;

For "total", that's okay because it is by itself.  But, under Linux, setting
"free" alone is not enough; three values need to be set.  To avoid mistakes, a
"helper function" would need to be created so that all three get set together
(or more should the algorithm change).  That helper function would look just
like the current SetFreeMemoryKB(f, c, b) method.  That's no longer so clean.

[bcwhite, 2016-09-09 15:42:50]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 15:43:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-09-09 16:00:39]

The CQ bit was unchecked by bcwhite@chromium.org

[bcwhite, 2016-09-09 16:00:41]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2016-09-09 16:00:41]

The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org
Link to the patchset: https://codereview.chromium.org/2310193002/#ps100001
(title: "rebased")

[commit-bot: I haz the power, 2016-09-09 16:00:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 16:57:28]

Description was changed from

==========
Added MemoryMonitor for Linux (with test).

Extraction of common code also means some changes to the
Windows memory-monitor.

BUG=644397
==========

to

==========
Added MemoryMonitor for Linux (with test).

Extraction of common code also means some changes to the
Windows memory-monitor.

BUG=644397
==========

[commit-bot: I haz the power, 2016-09-09 16:57:29]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-09-09 16:59:59]

Description was changed from

==========
Added MemoryMonitor for Linux (with test).

Extraction of common code also means some changes to the
Windows memory-monitor.

BUG=644397
==========

to

==========
Added MemoryMonitor for Linux (with test).

Extraction of common code also means some changes to the
Windows memory-monitor.

BUG=644397

Committed: https://crrev.com/8ceca0e580766a9f91cf655a52de1ebd97d8443d
Cr-Commit-Position: refs/heads/master@{#417607}
==========

[commit-bot: I haz the power, 2016-09-09 17:00:01]

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