Fix free memory calculation.

Fix free memory calculation.

The term "free memory" can mean different things: it can mean
1) absolutely/pure free physical memory (not consumed by anything) or
2) it can include the inactive part(s) of OS (file) cache(s) or
3) it can mean the maximum amount of physical memory that can be
allocated quickly without swapping and thus can also include the
active part(s) of OS cache(s).

This CL fixes base::AmountOfAvailablePhysicalMemory() to function
according to the 2nd meaning on all platforms where it is possible
because this is what everyone needs most of the time. (Win platform
has already had such implementation but others haven't).

BUG=672745
Review-Url: https://codereview.chromium.org/2558043007
Cr-Original-Commit-Position: refs/heads/master@{#458291}
Committed: https://chromium.googlesource.com/chromium/src/+/a4258cad75d8cc63170555cc74bd584b5bfca965
Review-Url: https://codereview.chromium.org/2558043007
Cr-Commit-Position: refs/heads/master@{#458615}
Committed: https://chromium.googlesource.com/chromium/src/+/01ac10b7938701bc9f9f2c05faf18229f9573b4c

[Michael K. (Yandex Team), 2016-12-09 08:58:25]

Description was changed from

==========
Fix free memory calculation.

BUG=672745

See issue description for details.
==========

to

==========
Fix free memory calculation.

BUG=672745

See issue description for details.
==========

[Michael K. (Yandex Team), 2016-12-09 08:58:25]

mkolom@yandex-team.ru changed reviewers:
+ dcheng@chromium.org, jam@chromium.org, thakis@chromium.org

[Michael K. (Yandex Team), 2016-12-09 08:58:48]

PTAL

[Michael K. (Yandex Team), 2016-12-09 10:34:24]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 10:34:39]

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-12-09 10:34:40]

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

[commit-bot: I haz the power, 2016-12-09 10:34:41]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[jam, 2016-12-09 17:39:44]

please add a descriptive cl description. also please specify which files each
reviewer should look at.

[Michael K. (Yandex Team), 2016-12-29 10:43:00]

Description was changed from

==========
Fix free memory calculation.

BUG=672745

See issue description for details.
==========

to

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean absolutely/pure
free physical memory (not consumed by anything) or it can include the inactive
part(s) of OS (file) cache(s) or it can mean the maximum amount of physical
memory that can be allocated quickly without swapping and thus can also include
the active part(s) of OS cache(s).

This CL introduces 3 different functions for a free memory calculation for
different needs.
And also it fixes this bug: on all platforms except Windows
base::AmountOfAvailablePhysicalMemory() returned pure free memory but on Windows
one included the inactive part of OS cache.

BUG=672745
==========

[Michael K. (Yandex Team), 2016-12-29 10:55:30]

On 2016/12/09 17:39:44, jam wrote:
> please add a descriptive cl description.
Fixed

> also please specify which files each reviewer should look at.
I used git cl owners to find suitable reviewers: "
** You selected these owners **

thakis:
  base/process/process_metrics.h
  base/process/process_metrics_ios.cc
  base/process/process_metrics_linux.cc
  base/process/process_metrics_mac.cc
  base/process/process_metrics_unittest.cc
  base/process/process_metrics_win.cc
  base/sys_info.h
  base/sys_info_ios.mm
  base/sys_info_linux.cc
  base/sys_info_mac.mm
  base/sys_info_openbsd.cc
  base/sys_info_win.cc
  chrome/browser/chromeos/policy/device_status_collector.cc
  chrome/browser/metrics/leak_detector/leak_detector_controller.cc

jam:
  base/process/process_metrics.h
  base/process/process_metrics_ios.cc
  base/process/process_metrics_linux.cc
  base/process/process_metrics_mac.cc
  base/process/process_metrics_unittest.cc
  base/process/process_metrics_win.cc
  base/sys_info.h
  base/sys_info_ios.mm
  base/sys_info_linux.cc
  base/sys_info_mac.mm
  base/sys_info_openbsd.cc
  base/sys_info_win.cc
  chrome/browser/chromeos/policy/device_status_collector.cc
  chrome/browser/metrics/leak_detector/leak_detector_controller.cc
  components/memory_pressure/direct_memory_pressure_calculator_win.cc
  components/memory_pressure/direct_memory_pressure_calculator_win_unittest.cc
  content/browser/memory/memory_monitor_win.cc
  content/browser/memory/memory_monitor_win_unittest.cc
  extensions/browser/api/system_memory/memory_info_provider.cc

dcheng:
  base/process/process_metrics.h
  base/process/process_metrics_ios.cc
  base/process/process_metrics_linux.cc
  base/process/process_metrics_mac.cc
  base/process/process_metrics_unittest.cc
  base/process/process_metrics_win.cc
  base/sys_info.h
  base/sys_info_ios.mm
  base/sys_info_linux.cc
  base/sys_info_mac.mm
  base/sys_info_openbsd.cc
  base/sys_info_win.cc
"

I think all reviewers should look into changes in base/ headers also.

[Michael K. (Yandex Team), 2016-12-29 10:55:50]

PTAL

[jam, 2016-12-29 16:36:34]

On 2016/12/29 10:55:50, mkolom wrote:
> PTAL

I defer to base/OWNERS to review the changes there. Once they're happy I can
look at the files outside of base

[Michael K. (Yandex Team), 2017-01-12 11:36:19]

mkolom@yandex-team.ru changed reviewers:
+ atwilson@chromium.org, hokein.wu@gmail.com, mwlodar@google.com,
rmcilroy@chromium.org, rsesek@chromium.org

[Michael K. (Yandex Team), 2017-01-12 11:41:59]

Maybe these people will be also interested in:
* rmcilroy - base/sys_info*;
* rsesek - base/process/process_metrics*;
* atwilson - chrome/browser/chromeos/policy/device_status_collector.cc;
* mwlodar - chrome/browser/metrics/leak_detector/leak_detector_controller.cc;
* hokein.wu - extensions/browser/api/system_memory/memory_info_provider.cc.


Anyone, PTAL

[rmcilroy, 2017-01-12 15:16:29]

I'm not an owner of base, just base/android so you probably want to get someone
else to look at base/sys_info*

[Andrew T Wilson (Slow), 2017-01-12 16:02:40]

policy/ LGTM.

jinzhang@: FYI, if you start seeing weird "free memory" values in our chrome os
device reporting, we can see if this change is responsible.

[Michael K. (Yandex Team), 2017-01-24 06:28:14]

Sorry, is there anything I can do to draw your attention to this CL? :)
PTAL

[Alexei Svitkine (slow), 2017-01-24 18:44:51]

+primiano

Primiano, who are right folks on memory to review this?

On Tue, Jan 24, 2017 at 1:28 AM, <mkolom@yandex-team.ru> wrote:

> Sorry, is there anything I can do to draw your attention to this CL? :)
> PTAL
>
>
> https://codereview.chromium.org/2558043007/
>

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

[Primiano Tucci (use gerrit), 2017-01-25 05:07:21]

I  need to take a look in more detail to this, but at a first glance my feeling
is that this is adding more entropy to memory metrics.
How do we expect anybody using base/ to make a sense out of: "soft-free" vs
"hard-free" vs "pure-free"?
you are taking a complex problem (measuring memory) and just splashing all the
complexity on the client.

[Primiano Tucci (use gerrit), 2017-01-25 17:00:47]

Ok I took a look to this with fresh mind.
First of all, thanks for taking a look to this. I understand that this is a huge
headache and I am fully aware that the current situation is far from being
perfect.
Apologies if my previous reply was too negative. Let me retry with something
more constructive.

So here's my problem: I really need to be convinced that we need three different
notions of free memory in our codebase.
I do fully understand that depending on how you define "free" you can get a
different number.
I just don't think that we need to expose three of them in our codebase. I think
we need only a reasonable one. 
I'm not convinced that device_status_collector, leak_detector_controller,
memory_pressure_calculator, memory_monitor, memory_info_provider need a
different notion of free memory. 
If they do (I am not familiar with all of them, can you maybe expand a bit more
here?) I am more inclined to say that we should talk with the owners of them and
agree on *one* notion of free mem.

From a more practical viewpoint, I have some technical comments, which apply to
Linux/Android only, as I don't know the details of other platforms.
1)MemFree, what you call here AmountOfPureFreePhysicalMemory, is IMHO a useless
number and I nobody should need to look at this.
Once you start having some workload, the kernel will start paging in accessed
files in the pagecache and will persist them, even if unreferenced, just in
case.
the point is that there is no value in keeping "free" memory around for a kernel
when, instead, it can keep caches that might be reused in future, and just free
them on demand when needed. 
If anything depends on this value, very likely it's a bug in the code.
In other words, I'd NOT expose the notion of "pure free" memory, as that is just
too ambiguous and unactionable.

2)There is instead a nuance between what you call soft-free and hard-free. On
Linux/Android it seems that your differentiator is the LRU heuristic (active vs
not active).
I buy this difference, but again I don't think we should expose both. Can we
make a choice here?
In all honesty your "soft" free memory seems to me the best tradeoff.

So, purely from a Linux/Android viewpoint, what I would do here is:
- Just keep AmountOfAvailablePhysicalMemory() (or if you want just
AmountOfAvailableMemory())
- Use the logic you did write for soft-free here.

Now the question is: what is the status on the other platforms? can the same
considerations apply? Can we end up with just one, better,
AmountOfAvailablePhysicalMemory ?

[Michael K. (Yandex Team), 2017-01-27 08:44:26]

I completely agree with you that it is better to have the only version of "free
memory" term.
And in most cases we need what I called 'soft' free memory here.
That I said and wrote here:
https://bugs.chromium.org/p/chromium/issues/detail?id=672745 . Have you read it?

(Of course it is better not to rely on the amount of free memory at all. But
sometimes we have to.)

The authors of device_status_collector.cc, leak_detector_controller.cc,
memory_info_provider.cc, direct_memory_pressure_calculator_win.cc,
memory_monitor_win.cc should read all the information here and then tell what
type of free memory they need to know.
(I'm not sure about chromeos leak_detector_controller.cc and
device_status_collector.cc because I'm not familiar with that OS and as I've
heard It can have some specificity).
(I'm not completely sure about memory_info_provider.cc: it's a JS
chrome.system.memory.getInfo function implementation and there is no clear spec
on it: https://developer.chrome.com/apps/system_memory. But I think the shown
implementation is Ok).

What about other OSes:
* Currently Win version already has correct ("soft")
AmountOfAvailablePhysicalMemory implementation. And it means (I believe) the
same as I've implemented here for linux (see msdn to be sure:
https://msdn.microsoft.com/en-us/library/windows/desktop/aa366770(v=vs.85).aspx
, ullAvailPhys);
* On Mac (and iOS) it isn't possible to make similar calculations unfortunately
(I've made some investigations). The shown AmountOfSoftFreePhysicalMemory
underestimates the amount of free memory: it doesn't take into account the
inactive file-backed memory - MacOS doesn't give us such information. But it is
better to underestimate than to overestimate;
* OpenBSD... I'm not familiar with that OS. I couldn't find a way to calculate
soft and hard free memory on it. Thus corresponding implementations
underestimate the numbers.

[Andrew T Wilson (Slow), 2017-01-27 11:10:07]

Owner of DeviceStatusCollector here - we want "soft" free memory to be
consistent with Windows  - can you update the patch accordingly? Thanks for
clarifying.

[Michael K. (Yandex Team), 2017-02-06 09:34:12]

mkolom@yandex-team.ru changed reviewers:
+ sque@chromium.org, wfh@chromium.org

[Michael K. (Yandex Team), 2017-02-06 09:35:32]

mkolom@yandex-team.ru changed reviewers:
- rmcilroy@chromium.org

[Michael K. (Yandex Team), 2017-02-06 09:36:30]

Waiting for leak_detector_controller.cc owners: sque@ or wfh@.
PTAL

[Will Harris, 2017-02-06 18:03:06]

On 2017/02/06 09:36:30, mkolom wrote:
> Waiting for leak_detector_controller.cc owners: sque@ or wfh@.
> PTAL

We were added two minutes before you said you were 'waiting for' us...? It
doesn't seem the comments from primiano have been fully addressed yet. Can you
take a look again? Once l-g-t-m obtained from other parts then I'll take a look.
Thanks!

[Michael K. (Yandex Team), 2017-02-07 03:23:49]

On 2017/02/06 18:03:06, Will Harris wrote:
> We were added two minutes before you said you were 'waiting for' us...?
Yes:( I don't know why but `git cl owner` didn't suggest you (and sque@) for
reviewing.
Maybe because of unusual
chrome/browser/metrics/leak_detector/leak_detector_controller.cc contents: "
file://components/metrics/leak_detector/OWNERS"
...Ok, never mind.

> doesn't seem the comments from primiano have been fully addressed yet. Can you
> take a look again? Once l-g-t-m obtained from other parts then I'll take a
look.
Now we have to decide whether we need three versions of
AmountOfAvailablePhysicalMemory() or we can stay with one - "Soft" version.
Can you read the discussion here then take a look at leak_detector_controller.cc
and say what function version you need?

Then I will rework the CL.

[Michael K. (Yandex Team), 2017-02-09 02:31:39]

wfh@ , PTAL

[Simon Que, 2017-02-09 04:16:11]

On 2017/02/09 02:31:39, mkolom wrote:
> wfh@ , PTAL

I'm okay with using the Soft version in leak_detector_controller.cc.

[Michael K. (Yandex Team), 2017-02-09 13:52:33]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 13:52:54]

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, 2017-02-09 16:12:11]

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

[commit-bot: I haz the power, 2017-02-09 16:12:12]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Michael K. (Yandex Team), 2017-02-10 17:36:50]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 17:37:21]

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, 2017-02-10 20:58:11]

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

[commit-bot: I haz the power, 2017-02-10 20:58:12]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Will Harris, 2017-02-10 21:40:33]

It seems this CL has changed quite a bit since I last looked at it, but no
longer makes any changes to leak_detector. Can you update the CL description,
and also make sure all comments in source files are updated?

[Michael K. (Yandex Team), 2017-02-13 05:00:00]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 05:00:16]

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, 2017-02-13 07:22:55]

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

[commit-bot: I haz the power, 2017-02-13 07:22:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Michael K. (Yandex Team), 2017-02-13 09:22:12]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 09:22:28]

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, 2017-02-13 09:50:14]

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

[commit-bot: I haz the power, 2017-02-13 09:50:15]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Michael K. (Yandex Team), 2017-02-13 10:20:59]

Description was changed from

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean absolutely/pure
free physical memory (not consumed by anything) or it can include the inactive
part(s) of OS (file) cache(s) or it can mean the maximum amount of physical
memory that can be allocated quickly without swapping and thus can also include
the active part(s) of OS cache(s).

This CL introduces 3 different functions for a free memory calculation for
different needs.
And also it fixes this bug: on all platforms except Windows
base::AmountOfAvailablePhysicalMemory() returned pure free memory but on Windows
one included the inactive part of OS cache.

BUG=672745
==========

to

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean 1) absolutely/pure
free physical memory (not consumed by anything) or 2) it can include the
inactive part(s) of OS (file) cache(s) or 3) it can mean the maximum amount of
physical memory that can be allocated quickly without swapping and thus can also
include the active part(s) of OS cache(s).

This CL fixes base::AmountOfAvailablePhysicalMemory() to function according to
the 2nd meaning on all platforms where it is possible because this is what
everyone needs most of the time.
(Win platform has already had such implementation but others haven't).

BUG=672745
==========

[Michael K. (Yandex Team), 2017-02-13 10:26:13]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 10:26: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, 2017-02-13 11:59:21]

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

[commit-bot: I haz the power, 2017-02-13 11:59:22]

Dry run: This issue passed the CQ dry run.

[Michael K. (Yandex Team), 2017-02-13 13:32:19]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 13:32:35]

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, 2017-02-13 13:35:49]

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

[commit-bot: I haz the power, 2017-02-13 13:35:50]

Dry run: This issue passed the CQ dry run.

[Michael K. (Yandex Team), 2017-02-13 14:58:44]

I believe I've fixed all the notes.

PTAL

[Will Harris, 2017-02-13 17:46:22]

wfh@chromium.org changed reviewers:
- sque@chromium.org, wfh@chromium.org

[Michael K. (Yandex Team), 2017-02-15 02:19:06]

mkolom@yandex-team.ru changed reviewers:
+ primiano@chromium.org

[Michael K. (Yandex Team), 2017-02-15 02:21:16]

Primiano or anyone, PTAL

[Primiano Tucci (use gerrit), 2017-02-15 11:18:34]

The linux changes look mostly good with some comments here and there. Can't
speak for ios or win as I know less nitty gritty details there

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics.h:281: int zero;
what is this?

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics.h:282: // On Windows this field has different
meaning from the Linux one.
nit: add a newline.
Also would be nice to explain what the meaning is, instead of saying "it's
different"

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics_linux.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_linux.cc:538: #if defined(OS_LINUX) ||
defined(OS_ANDROID)
i think you can ditch the ifdef at this point, can't think to many example of
_linux && !OS_LINUX && !OS_ANDROID (CrOs counts as OS_LINUX)

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_linux.cc:572: #if defined(OS_LINUX) ||
defined(OS_ANDROID)
ditto, here an below

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_unittest.cc:109: "MemAvailable:    2746720 kB\n"
since this is not avialable on all kernels we support would be great that this
test did cover the case of MemAvailable being absent,  checking that the
fallback logic is sane.

https://codereview.chromium.org/2558043007/diff/140001/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/sys_info_linux.cc...
base/sys_info_linux.cc:62: info.inactive_file) *
is this git cl formatted? sounds a bit strange that the 1024 is on the other
line

[Michael K. (Yandex Team), 2017-02-17 13:38:48]

PTAL

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics.h:281: int zero;
On 2017/02/15 11:18:33, Primiano Tucci wrote:
> what is this?

Done.

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics.h:282: // On Windows this field has different
meaning from the Linux one.
On 2017/02/15 11:18:33, Primiano Tucci wrote:
> nit: add a newline.
> Also would be nice to explain what the meaning is, instead of saying "it's
> different"

Done.

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics_linux.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_linux.cc:538: #if defined(OS_LINUX) ||
defined(OS_ANDROID)
On 2017/02/15 11:18:33, Primiano Tucci wrote:
> i think you can ditch the ifdef at this point, can't think to many example of
> _linux && !OS_LINUX && !OS_ANDROID (CrOs counts as OS_LINUX)

Done.

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_linux.cc:572: #if defined(OS_LINUX) ||
defined(OS_ANDROID)
On 2017/02/15 11:18:33, Primiano Tucci wrote:
> ditto, here an below

Done.

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_unittest.cc:109: "MemAvailable:    2746720 kB\n"
When MemAvailable is absent, meminfo.available is zero.
("Process metrics" are "as is", there are no magic formulas etc).
What do you want to check?
I agree it would be great if we can check this logic: base/sys_info_linux.cc
It should be done in base/sys_info_unittest.cc
But I'm not sure what is the best way to do it... You may suggest.

https://codereview.chromium.org/2558043007/diff/140001/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/sys_info_linux.cc...
base/sys_info_linux.cc:62: info.inactive_file) *
I've checked: Yes, clang-format wants it in that way.

[Primiano Tucci (use gerrit), 2017-02-20 14:13:28]

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_unittest.cc:109: "MemAvailable:    2746720 kB\n"
On 2017/02/17 13:38:48, mkolom wrote:
> When MemAvailable is absent, meminfo.available is zero.
> ("Process metrics" are "as is", there are no magic formulas etc).
> What do you want to check?

I want to check that when we are in kernels that don't produce MemAvailable,
AmountOfAvailablePhysicalMemory still returns a valid number != 0, which is
calculated using your fallback logic..

> I agree it would be great if we can check this logic: base/sys_info_linux.cc
> It should be done in base/sys_info_unittest.cc
> But I'm not sure what is the best way to do it... You may suggest.

You arelady have valid_input1 and valid_input2 where only one has MemAvailable
(good)
I would like to see a test that uses that as a mock input for
AmountOfAvailablePhysicalMemory() and checks that we get the right number.

https://codereview.chromium.org/2558043007/diff/160001/base/process/process_m...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2558043007/diff/160001/base/process/process_m...
base/process/process_metrics.h:284: int zero;
Why this needs to be exposed at an API level? I understand if you need this to
do some calculation, but I am not sure we want to expose this detail through the
public-facing API.

https://codereview.chromium.org/2558043007/diff/160001/base/process/process_m...
base/process/process_metrics.h:292: // On Linux "available" includes _all_
file-backed memory (active + inactive).
note that you are saying "on linux" but this line is really about windows only
(look at the #ifdef)

[Michael K. (Yandex Team), 2017-02-20 14:33:14]

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_unittest.cc:109: "MemAvailable:    2746720 kB\n"
On 2017/02/20 14:13:27, Primiano Tucci wrote:
> On 2017/02/17 13:38:48, mkolom wrote:
> > When MemAvailable is absent, meminfo.available is zero.
> > ("Process metrics" are "as is", there are no magic formulas etc).
> > What do you want to check?
> 
> I want to check that when we are in kernels that don't produce MemAvailable,
> AmountOfAvailablePhysicalMemory still returns a valid number != 0, which is
> calculated using your fallback logic..
> 
> > I agree it would be great if we can check this logic: base/sys_info_linux.cc
> > It should be done in base/sys_info_unittest.cc
> > But I'm not sure what is the best way to do it... You may suggest.
> 
> You arelady have valid_input1 and valid_input2 where only one has MemAvailable
> (good)
> I would like to see a test that uses that as a mock input for
> AmountOfAvailablePhysicalMemory() and checks that we get the right number.

Acknowledged.

https://codereview.chromium.org/2558043007/diff/160001/base/process/process_m...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2558043007/diff/160001/base/process/process_m...
base/process/process_metrics.h:284: int zero;
You're right: currently nobody uses it.
...Except process_metrics_unittest: "zero" memory is very similar to "free" on
Windows. And sometimes "free" can be null ( == 0) and zero can be null, but zero
+ free > 0.
Maybe in a future somebody will want "zero" memory.
(My thoughts: "free" is useless on Win without zero).

Ok, I can remove it.

https://codereview.chromium.org/2558043007/diff/160001/base/process/process_m...
base/process/process_metrics.h:292: // On Linux "available" includes _all_
file-backed memory (active + inactive).
Because I wanted to explain what "different" exactly means.

Ok, I'll put that sentence in the linux section.

[Michael K. (Yandex Team), 2017-03-01 07:03:56]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 07:04:17]

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, 2017-03-01 07:18:18]

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

[commit-bot: I haz the power, 2017-03-01 07:18:19]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)

[Michael K. (Yandex Team), 2017-03-01 07:19:52]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 07:20:14]

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, 2017-03-01 07:53:31]

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

[commit-bot: I haz the power, 2017-03-01 07:53:32]

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

[Michael K. (Yandex Team), 2017-03-01 09:57:58]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 09:58:09]

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, 2017-03-01 10:35:57]

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

[commit-bot: I haz the power, 2017-03-01 10:35:58]

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

[Michael K. (Yandex Team), 2017-03-01 10:46:23]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 10:46:39]

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, 2017-03-01 11:45:49]

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

[commit-bot: I haz the power, 2017-03-01 11:45:51]

Dry run: This issue passed the CQ dry run.

[Michael K. (Yandex Team), 2017-03-01 11:50:18]

Sorry for the delay.

Primiano, PTAL

[Michael K. (Yandex Team), 2017-03-07 10:00:26]

Sorry for the persistence.

PTAL

[Primiano Tucci (use gerrit), 2017-03-08 12:36:41]

I think the test to check that we are retro-compat with old kernels is still
missing (Or maybe I am misreading the code?) See comment inline

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_unittest.cc:109: "MemAvailable:    2746720 kB\n"
On 2017/02/20 14:33:13, Michael Kolomeytsev wrote:
> On 2017/02/20 14:13:27, Primiano Tucci wrote:
> > On 2017/02/17 13:38:48, mkolom wrote:
> > > When MemAvailable is absent, meminfo.available is zero.
> > > ("Process metrics" are "as is", there are no magic formulas etc).
> > > What do you want to check?
> > 
> > I want to check that when we are in kernels that don't produce MemAvailable,
> > AmountOfAvailablePhysicalMemory still returns a valid number != 0, which is
> > calculated using your fallback logic..
> > 
> > > I agree it would be great if we can check this logic:
base/sys_info_linux.cc
> > > It should be done in base/sys_info_unittest.cc
> > > But I'm not sure what is the best way to do it... You may suggest.
> > 
> > You arelady have valid_input1 and valid_input2 where only one has
MemAvailable
> > (good)
> > I would like to see a test that uses that as a mock input for
> > AmountOfAvailablePhysicalMemory() and checks that we get the right number.
> 
> Acknowledged.

I might be missing something but... where did this happen?

https://codereview.chromium.org/2558043007/diff/220001/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

https://codereview.chromium.org/2558043007/diff/220001/base/sys_info_linux.cc...
base/sys_info_linux.cc:61: int64_t SysInfo::AmountOfAvailablePhysicalMemory(
this one seems an internal implementation detail, could this be implemented up
in the anonymous namespace and not exposed to the header?

https://codereview.chromium.org/2558043007/diff/220001/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/220001/base/sys_info_unittest...
base/sys_info_unittest.cc:46: EXPECT_LT(amount, info.available * 1024);
shouldn't this be s/LT/LE/ ?

[Michael K. (Yandex Team), 2017-03-09 07:21:15]

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_unittest.cc:109: "MemAvailable:    2746720 kB\n"
See the new SysInfoTest.AmountOfAvailablePhysicalMemory test in
base/sys_info_unittest.cc.

I've put the test there because it tests the code from the corresponding
base/sys_info.cc.
And it seems logical:
* here we test that we can parse system/proc data into a SystemMemoryInfoKB
structure. And we check various cases including whether MemAvailable is present
or not;
* there (in sys_info_unittest.cc) we test that AmountOfAvailablePhysicalMemory
(which depends on SystemMemoryInfoKB structure) works correctly in both cases:
whether MemAvailable is present or not.

Don't you agree it's ok?

https://codereview.chromium.org/2558043007/diff/220001/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

https://codereview.chromium.org/2558043007/diff/220001/base/sys_info_linux.cc...
base/sys_info_linux.cc:61: int64_t SysInfo::AmountOfAvailablePhysicalMemory(
On 2017/03/08 12:36:41, Primiano Tucci (slow - perf) wrote:
> this one seems an internal implementation detail, could this be implemented up
> in the anonymous namespace and not exposed to the header? 

It is used in sys_info_unittest.cc in the new
SysInfoTest.AmountOfAvailablePhysicalMemory and should be declared in the
header.
But I've moved it to the private section of the class and added that test as
friend. See base/sys_info.h.

https://codereview.chromium.org/2558043007/diff/220001/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/220001/base/sys_info_unittest...
base/sys_info_unittest.cc:46: EXPECT_LT(amount, info.available * 1024);
On 2017/03/08 12:36:41, Primiano Tucci (slow - perf) wrote:
> shouldn't this be s/LT/LE/ ?

Only when active file-backed memory is zero (what seems unreal). That's by our
own definition: see base/sys_info_linux.cc.  But if you think that can be real -
I'll s/LT/LE.

[Michael K. (Yandex Team), 2017-03-09 07:23:30]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 07:23:49]

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, 2017-03-09 07:29:00]

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

[commit-bot: I haz the power, 2017-03-09 07:29:01]

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

[Michael K. (Yandex Team), 2017-03-09 07:51:07]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 07:51:28]

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, 2017-03-09 09:35:36]

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

[commit-bot: I haz the power, 2017-03-09 09:35:37]

Dry run: This issue passed the CQ dry run.

[Michael K. (Yandex Team), 2017-03-09 09:47:53]

PTAL

[Primiano Tucci (use gerrit), 2017-03-09 10:24:03]

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_unittest.cc:109: "MemAvailable:    2746720 kB\n"
On 2017/03/09 07:21:15, Michael Kolomeytsev wrote:
> See the new SysInfoTest.AmountOfAvailablePhysicalMemory test in
> base/sys_info_unittest.cc.
> 
> I've put the test there because it tests the code from the corresponding
> base/sys_info.cc.
> And it seems logical:
> * here we test that we can parse system/proc data into a SystemMemoryInfoKB
> structure. And we check various cases including whether MemAvailable is
present
> or not;
> * there (in sys_info_unittest.cc) we test that AmountOfAvailablePhysicalMemory
> (which depends on SystemMemoryInfoKB structure) works correctly in both cases:
> whether MemAvailable is present or not.
> 
> Don't you agree it's ok?

The problem of that test you wrote is that what covers really depends on the
kernel it's being run onto. So if there is a bug in the logic, we might or might
not spot it, depending on the bots where it runs, which is not desirable. So, to
be clear, it's good to have that test that to check that the api works
end-to-end but you still lack coverage in this file for the case we discussed
above.
 
Sorry maybe I haven't been clear. Let me try to rephrase my statement above.
The coverage I would like to see in this test is:
1) Given a fixed /proc/meminfo input (like the ones here in these lines) that
contains MemAvailable, check that the number returned is sound and matches
MemAvailable (- the math on active file file).
2) Same thing, but with another fixed /proc/meminfo input which does NOT have
the MemAvailable line, to check that the fallback logic doesn't get accidentally
broken.

Now, I see here coverage for 1) below, which is good. I still don't see any
coverage for 2.

https://codereview.chromium.org/2558043007/diff/220001/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/220001/base/sys_info_unittest...
base/sys_info_unittest.cc:46: EXPECT_LT(amount, info.available * 1024);
On 2017/03/09 07:21:15, Michael Kolomeytsev wrote:
> On 2017/03/08 12:36:41, Primiano Tucci (slow - perf) wrote:
> > shouldn't this be s/LT/LE/ ?
> 
> Only when active file-backed memory is zero (what seems unreal). That's by our
> own definition: see base/sys_info_linux.cc.  But if you think that can be real
-
> I'll s/LT/LE.
> 
> 

Ah you are right, forgot about the -active_file part.

[Michael K. (Yandex Team), 2017-03-09 10:40:57]

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
File base/process/process_metrics_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/140001/base/process/process_m...
base/process/process_metrics_unittest.cc:109: "MemAvailable:    2746720 kB\n"
On 2017/03/09 10:24:03, Primiano Tucci (slow - perf) wrote:
> The problem of that test you wrote is that what covers really depends on the
> kernel it's being run onto.
Not exactly. See below.

> Sorry maybe I haven't been clear. Let me try to rephrase my statement above.
> The coverage I would like to see in this test is:
> 1) Given a fixed /proc/meminfo input (like the ones here in these lines) that
> contains MemAvailable, check that the number returned is sound and matches
> MemAvailable (- the math on active file file).
> 2) Same thing, but with another fixed /proc/meminfo input which does NOT have
> the MemAvailable line, to check that the fallback logic doesn't get
accidentally
> broken.
> 
> Now, I see here coverage for 1) below, which is good. I still don't see any
> coverage for 2.

There is the coverage for case #2 in base/sys_info_unittest.cc.
I've made a comment there. Plz pay attention.

But Ok, I'll also add expectations on AmountOfAvailablePhysicalMemory here. It
isn't hard and will prevent possible questions by other people in the future.

https://codereview.chromium.org/2558043007/diff/260001/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/260001/base/sys_info_unittest...
base/sys_info_unittest.cc:48: info.available = 0;
The answer on yours:
> 2) Same thing, but with another fixed /proc/meminfo input which does NOT have
> the MemAvailable line, to check that the fallback logic doesn't get
accidentally
> broken.

> Now, I see here coverage for 1) below, which is good. I still don't see any
> coverage for 2.

If we are running on modern kernel/bot with MemAvailable, we check both
variants:
* inside this "if" block - case #1;
* zero out info.available and just after this block - case #2.

[Primiano Tucci (use gerrit), 2017-03-09 11:50:16]

Anyways, assuming the test gets fixed as we discussed, *linux*.cc LGTM (not an
owner though).
Can't speak for ios and win where I know less. would be great if somebody could
cover those.

[Michael K. (Yandex Team), 2017-03-09 12:59:56]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 13:00:14]

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

[Michael K. (Yandex Team), 2017-03-09 13:14:19]

Reviewers are looked for the following:
* base/process/process_metrics_MAC.cc and base/process/process_metrics_WIN.cc
* base/sys_info_MAC.cc, base/sys_info_IOS.cc and base/sys_info_WIN.cc

georgesak? brucedawson? danakj? dcheng?

[Michael K. (Yandex Team), 2017-03-09 13:15:57]

mkolom@yandex-team.ru changed reviewers:
+ brucedawson@chromium.org, danakj@chromium.org
- atwilson@chromium.org, hokein.wu@gmail.com, jam@chromium.org,
mwlodar@google.com, thakis@chromium.org

[Michael K. (Yandex Team), 2017-03-09 13:17:39]

PTAL

[commit-bot: I haz the power, 2017-03-09 14:02:17]

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

[commit-bot: I haz the power, 2017-03-09 14:02:18]

Dry run: This issue passed the CQ dry run.

[Michael K. (Yandex Team), 2017-03-09 14:35:54]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 14:36:09]

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, 2017-03-09 15:07:15]

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

[commit-bot: I haz the power, 2017-03-09 15:07:17]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Michael K. (Yandex Team), 2017-03-09 16:05:40]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 16:06:11]

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, 2017-03-09 16:55:48]

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

[commit-bot: I haz the power, 2017-03-09 16:55:49]

Dry run: This issue passed the CQ dry run.

[Michael K. (Yandex Team), 2017-03-09 16:56:53]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 16:57:35]

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, 2017-03-09 17:03:17]

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

[commit-bot: I haz the power, 2017-03-09 17:03:18]

Dry run: This issue passed the CQ dry run.

[brucedawson, 2017-03-09 19:17:52]

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
base/process/process_metrics.h:281: int free;
What is this actually used for? The purpose of the CL is to change
base::AmountOfAvailablePhysicalMemory() so that it returns available memory and
I worry that having 'free' memory available is just creating a footgun - an
inappropriate value that developers can query when they really want 'available'
memory.

The comment may actually be too much information for the variable declaration.
It is an implementation detail. But, I could see an argument either way.

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
File base/process/process_metrics_ios.cc (right):

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
base/process/process_metrics_ios.cc:31: : total(0), free(0), speculative(0),
file_backed(0), purgeable(0) {}
Consider zeroing these where the variables are declared instead of here - newly
available C++ feature that has been approved for use in Chromium. This would
reduce duplication across platform-specific source files.

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
File base/process/process_metrics_linux.cc (right):

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
base/process/process_metrics_linux.cc:548: reclaimable = 0;
This list of member variables to initialize makes the case for initializing them
where they are declared even stronger - this is unwieldy and unverifiable.

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
File base/process/process_metrics_win.cc (right):

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
base/process/process_metrics_win.cc:367: //   ullAvailPhys ==> available
This comment no longer mentions what maps to free - it should probably be
updated.

[Michael K. (Yandex Team), 2017-03-10 15:53:57]

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
base/process/process_metrics.h:281: int free;
It was used in first patchsets but wasn't used in this one (except a test).
Maybe "free" can be useful in a future but I agree it's not good to have unused
code: so I've removed it.

P.S.: SystemMemoryInfoKB is a collection of the low level metrics as-is. If a
developer wants just to query total and available memory - he/she should use
methods from base::SysInfo class. SystemMemoryInfoKB is for those who know what
they are doing.

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
File base/process/process_metrics_ios.cc (right):

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
base/process/process_metrics_ios.cc:31: : total(0), free(0), speculative(0),
file_backed(0), purgeable(0) {}
On 2017/03/09 19:17:52, brucedawson wrote:
> Consider zeroing these where the variables are declared instead of here -
newly
> available C++ feature that has been approved for use in Chromium. This would
> reduce duplication across platform-specific source files.

Done.

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
File base/process/process_metrics_linux.cc (right):

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
base/process/process_metrics_linux.cc:548: reclaimable = 0;
On 2017/03/09 19:17:52, brucedawson wrote:
> This list of member variables to initialize makes the case for initializing
them
> where they are declared even stronger - this is unwieldy and unverifiable.

Done.

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
File base/process/process_metrics_win.cc (right):

https://codereview.chromium.org/2558043007/diff/300001/base/process/process_m...
base/process/process_metrics_win.cc:367: //   ullAvailPhys ==> available
On 2017/03/09 19:17:52, brucedawson wrote:
> This comment no longer mentions what maps to free - it should probably be
> updated.

Nothing maps to "free" on Win now.

[Michael K. (Yandex Team), 2017-03-10 15:55:03]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 15:55:17]

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, 2017-03-10 15:57:46]

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

[commit-bot: I haz the power, 2017-03-10 15:57:48]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Michael K. (Yandex Team), 2017-03-10 16:20:02]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 16:20:19]

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, 2017-03-10 16:33:27]

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

[commit-bot: I haz the power, 2017-03-10 16:33:28]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_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_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[danakj, 2017-03-10 16:46:37]

Description was changed from

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean 1) absolutely/pure
free physical memory (not consumed by anything) or 2) it can include the
inactive part(s) of OS (file) cache(s) or 3) it can mean the maximum amount of
physical memory that can be allocated quickly without swapping and thus can also
include the active part(s) of OS cache(s).

This CL fixes base::AmountOfAvailablePhysicalMemory() to function according to
the 2nd meaning on all platforms where it is possible because this is what
everyone needs most of the time.
(Win platform has already had such implementation but others haven't).

BUG=672745
==========

to

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean
1) absolutely/pure free physical memory (not consumed by anything) or
2) it can include the inactive part(s) of OS (file) cache(s) or
3) it can mean the maximum amount of physical memory that can be
allocated quickly without swapping and thus can also include the
active part(s) of OS cache(s).

This CL fixes base::AmountOfAvailablePhysicalMemory() to function
according to the 2nd meaning on all platforms where it is possible
because this is what everyone needs most of the time. (Win platform
has already had such implementation but others haven't).

BUG=672745
==========

[Michael K. (Yandex Team), 2017-03-10 17:06:44]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 17:07:37]

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

[danakj, 2017-03-10 17:13:49]

I'm finding it problematic that we have two fields with the same name but
different meanings on different platforms ("available"). Anything to make them
not literally the same name seems important.

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
File base/process/process_metrics.h (left):

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
base/process/process_metrics.h:270: SystemMemoryInfoKB();
You need to keep the out-of-line constructor, it just doesn't need to do
anything and can be defined as "= default"

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
File base/process/process_metrics_mac.cc (right):

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
base/process/process_metrics_mac.cc:400:
static_cast<int>(vm_info.speculative_count * PAGE_SIZE / 1024);
should these fields on meminfo not be int64? if not..

should these all be saturated_cast? or checked_cast? should we encourage it to
not overflow by doing the division step first? or does the compiler do the right
thing?

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
File base/process/process_metrics_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
base/process/process_metrics_unittest.cc:362: #if defined(OS_WIN) ||
defined(OS_MACOSX) || defined(OS_IOS) || \
Doesn't OS_MACOSX include OS_IOS so you could just remove its mention here?

In fact, which OS is this excluding at this point, any?

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
base/process/process_metrics_unittest.cc:371: EXPECT_GT(info.available, 0);
linux and android have an available field too

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_linux.cc...
base/sys_info_linux.cc:54: NOTREACHED();
Do not write "NOTREACHED(); return;" Choose one: DCHECK or if (...) { return; }
Either this can happen or it can't.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_linux.cc...
base/sys_info_linux.cc:67: return info.available != 0
i suggest storing a temporary var with as MB then return that casted and * 1024.

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_mac.mm
File base/sys_info_mac.mm (right):

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_mac.mm#n...
base/sys_info_mac.mm:89: NOTREACHED();
the NOTREACHED+return anti-pattern is common in these files, can you remove the
ones you're touching at the least?

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_win.cc#n...
base/sys_info_win.cc:74: NOTREACHED();
same

[Michael K. (Yandex Team), 2017-03-10 18:22:38]

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
File base/process/process_metrics.h (left):

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
base/process/process_metrics.h:270: SystemMemoryInfoKB();
On 2017/03/10 17:13:48, danakj wrote:
> You need to keep the out-of-line constructor, it just doesn't need to do
> anything and can be defined as "= default"

Done.

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
File base/process/process_metrics_mac.cc (right):

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
base/process/process_metrics_mac.cc:400:
static_cast<int>(vm_info.speculative_count * PAGE_SIZE / 1024);
meminfo contains data in kilobytes. So it is hard to imagine that ordinary "int"
isn't enough.

What about safe casts... vm_info's fields are unsigned 32bit. And multiplying by
4k page size ... It's  easy to overflow(
Doing the division step first isn't right (3 * 4k / 1k != 3 / 1k * 4k)
It's better do PAGE_SIZE / 1024 * value.
With the assumption that PAGE_SIZE is divisible by 1024.

Done.

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
File base/process/process_metrics_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
base/process/process_metrics_unittest.cc:362: #if defined(OS_WIN) ||
defined(OS_MACOSX) || defined(OS_IOS) || \
You are right: IOS is MACOSX.
This should be excluded for BSDs.
I've copied the right condition from declaration in process_metrics.h.
Done.

https://codereview.chromium.org/2558043007/diff/340001/base/process/process_m...
base/process/process_metrics_unittest.cc:371: EXPECT_GT(info.available, 0);
Not always. We discussed it with primiano@.
When there is no MemAvailable it is zero.
So nothing to check for them.

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_linux.cc
File base/sys_info_linux.cc (right):

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_linux.cc...
base/sys_info_linux.cc:54: NOTREACHED();
On 2017/03/10 17:13:49, danakj wrote:
> Do not write "NOTREACHED(); return;" Choose one: DCHECK or if (...) { return;
}
> Either this can happen or it can't.
> 
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

(But there are a lot of already written code in such style even in base/ ...)

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_linux.cc...
base/sys_info_linux.cc:67: return info.available != 0
On 2017/03/10 17:13:49, danakj wrote:
> i suggest storing a temporary var with as MB then return that casted and *
1024.

Done.

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_mac.mm
File base/sys_info_mac.mm (right):

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_mac.mm#n...
base/sys_info_mac.mm:89: NOTREACHED();
On 2017/03/10 17:13:49, danakj wrote:
> the NOTREACHED+return anti-pattern is common in these files, can you remove
the
> ones you're touching at the least?

Done.

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_win.cc
File base/sys_info_win.cc (right):

https://codereview.chromium.org/2558043007/diff/340001/base/sys_info_win.cc#n...
base/sys_info_win.cc:74: NOTREACHED();
On 2017/03/10 17:13:49, danakj wrote:
> same

Done.

[Michael K. (Yandex Team), 2017-03-10 18:24:29]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 18:24:59]

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

[danakj, 2017-03-10 18:58:22]

On Fri, Mar 10, 2017 at 12:13 PM, <danakj@chromium.org> wrote:

> I'm finding it problematic that we have two fields with the same name but
> different meanings on different platforms ("available"). Anything to make
> them
> not literally the same name seems important.
>

Maybe you didn't see this comment at the top of my reply?

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

[Michael K. (Yandex Team), 2017-03-10 19:12:46]

On 2017/03/10 18:58:22, danakj wrote:
> On Fri, Mar 10, 2017 at 12:13 PM, <mailto:danakj@chromium.org> wrote:
> 
> > I'm finding it problematic that we have two fields with the same name but
> > different meanings on different platforms ("available"). Anything to make
> > them
> > not literally the same name seems important.
> >
> 
> Maybe you didn't see this comment at the top of my reply?

I'm sorry, I missed it.

I agree.
I'll rename it to "mem_available" on Linux.

[Michael K. (Yandex Team), 2017-03-10 19:35:54]

On 2017/03/10 19:12:46, Michael K. (Yandex Team) wrote:
> On 2017/03/10 18:58:22, danakj wrote:
> > Maybe you didn't see this comment at the top of my reply?
> 
> I'm sorry, I missed it.
> 
> I agree.
> I'll rename it to "mem_available" on Linux.

Vice versa. I'll rename Win's "available" to "phys_avail" not to touch existing
linux code.

[Michael K. (Yandex Team), 2017-03-10 19:48:08]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 19:48:33]

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, 2017-03-10 20:02:42]

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

[commit-bot: I haz the power, 2017-03-10 20:02:45]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Michael K. (Yandex Team), 2017-03-10 20:26:23]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 20:26:59]

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, 2017-03-10 20:48:01]

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

[commit-bot: I haz the power, 2017-03-10 20:48:03]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Michael K. (Yandex Team), 2017-03-10 20:58:50]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 20:59:15]

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, 2017-03-10 21:36:29]

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

[commit-bot: I haz the power, 2017-03-10 21:36:30]

Dry run: This issue passed the CQ dry run.

[Michael K. (Yandex Team), 2017-03-10 21:37:25]

PTAL

[brucedawson, 2017-03-11 01:42:08]

lgtm

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
File base/process/process_metrics_ios.cc (left):

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics_ios.cc:29: SystemMemoryInfoKB::SystemMemoryInfoKB()
: total(0), free(0) {}
Don't these need to be =defaulted in?

[Michael K. (Yandex Team), 2017-03-11 14:12:57]

On 2017/03/11 01:42:08, brucedawson wrote:
> lgtm
> 
>
https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
> File base/process/process_metrics_ios.cc (left):
> 
>
https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
> base/process/process_metrics_ios.cc:29:
SystemMemoryInfoKB::SystemMemoryInfoKB()
> : total(0), free(0) {}
> Don't these need to be =defaulted in?

Yes, It has been done in base/process/process_metrics.cc for all OSes.

[Michael K. (Yandex Team), 2017-03-11 14:15:59]

danakj, PTAL

[danakj, 2017-03-13 16:01:21]

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics.h:273: SystemMemoryInfoKB(const SystemMemoryInfoKB&
other);
btw I think you can just omit this and you'll get the implicit copy (and move)
constructors then?

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics.h:290: // Note: on Windows this field has a bit
different meaning from the Linux one
This line no longer needed? Or at least reworded

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
File base/process/process_metrics_ios.cc (right):

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics_ios.cc:112: meminfo->free = static_cast<int>(
So I'm not sure what you did to relieve my worry here. If this is indeed not
going to overflow ever, then just using saturated cast should be okay?

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics_ios.cc:112: meminfo->free = static_cast<int>(
same thing as the _mac file here re casting and int overflows

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
File base/process/process_metrics_mac.cc (right):

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics_mac.cc:399: PAGE_SIZE / 1024 * (vm_info.free_count
- vm_info.speculative_count));
Thanks, this seems good, can you use saturated_cast then too? It shouldn't hurt
but makes it impossible to have weirder problems.

https://codereview.chromium.org/2558043007/diff/440001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/2558043007/diff/440001/base/sys_info.h#newcode20
base/sys_info.h:20: FORWARD_DECLARE_TEST(SysInfoTest,
AmountOfAvailablePhysicalMemory);
I think you don't need these if the tests are in the base namespace? I've never
seen these before

https://codereview.chromium.org/2558043007/diff/440001/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/440001/base/sys_info_unittest...
base/sys_info_unittest.cc:20: TEST_F(SysInfoTest, NumProcs) {
can you wrap this file in

namespace base {
namespace {

... 

}  // namespace
}  // namespace base

and then remove all the base:: inside them?

[Michael K. (Yandex Team), 2017-03-14 03:49:09]

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics.h:273: SystemMemoryInfoKB(const SystemMemoryInfoKB&
other);
On 2017/03/13 16:01:20, danakj wrote:
> btw I think you can just omit this and you'll get the implicit copy (and move)
> constructors then?

Done.

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics.h:290: // Note: on Windows this field has a bit
different meaning from the Linux one
On 2017/03/13 16:01:20, danakj wrote:
> This line no longer needed? Or at least reworded

Done.

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
File base/process/process_metrics_ios.cc (right):

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics_ios.cc:112: meminfo->free = static_cast<int>(
On 2017/03/13 16:01:20, danakj wrote:
> same thing as the _mac file here re casting and int overflows

Done.

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics_ios.cc:112: meminfo->free = static_cast<int>(
On 2017/03/13 16:01:20, danakj wrote:
> So I'm not sure what you did to relieve my worry here. If this is indeed not
> going to overflow ever, then just using saturated cast should be okay?

Done.

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
File base/process/process_metrics_mac.cc (right):

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics_mac.cc:399: PAGE_SIZE / 1024 * (vm_info.free_count
- vm_info.speculative_count));
On 2017/03/13 16:01:21, danakj wrote:
> Thanks, this seems good, can you use saturated_cast then too? It shouldn't
hurt
> but makes it impossible to have weirder problems.

I think you are worried too much about these casts.
But Ok, I'll use saturated_cast.

https://codereview.chromium.org/2558043007/diff/440001/base/sys_info.h
File base/sys_info.h (right):

https://codereview.chromium.org/2558043007/diff/440001/base/sys_info.h#newcode20
base/sys_info.h:20: FORWARD_DECLARE_TEST(SysInfoTest,
AmountOfAvailablePhysicalMemory);
On 2017/03/13 16:01:21, danakj wrote:
> I think you don't need these if the tests are in the base namespace? I've
never
> seen these before

Done.

https://codereview.chromium.org/2558043007/diff/440001/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/440001/base/sys_info_unittest...
base/sys_info_unittest.cc:20: TEST_F(SysInfoTest, NumProcs) {
On 2017/03/13 16:01:21, danakj wrote:
> can you wrap this file in
> 
> namespace base {
> namespace {
> 
> ... 
> 
> }  // namespace
> }  // namespace base
> 
> and then remove all the base:: inside them?

Done.

[Michael K. (Yandex Team), 2017-03-14 03:52:49]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 03:52:59]

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

[Michael K. (Yandex Team), 2017-03-14 03:53:20]

danakj, PTAL

[commit-bot: I haz the power, 2017-03-14 03:58:43]

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

[commit-bot: I haz the power, 2017-03-14 03:58:44]

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

[Michael K. (Yandex Team), 2017-03-14 04:50:37]

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics.h:273: SystemMemoryInfoKB(const SystemMemoryInfoKB&
other);
On 2017/03/13 16:01:20, danakj wrote:
> btw I think you can just omit this and you'll get the implicit copy (and move)
> constructors then?

It doesn't work. So I'll revert.
(chromium-style plugin (aka findbadconstructs) says then that there is a complex
constructor with inlined body.
Btw this struct doesn't need move constructor: it is POD).

https://codereview.chromium.org/2558043007/diff/440001/base/sys_info_unittest.cc
File base/sys_info_unittest.cc (right):

https://codereview.chromium.org/2558043007/diff/440001/base/sys_info_unittest...
base/sys_info_unittest.cc:20: TEST_F(SysInfoTest, NumProcs) {
But without anonymous namespace otherwise SysInfo class will not be able to
"friend" a test.

[Michael K. (Yandex Team), 2017-03-14 04:51:12]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 04:51: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, 2017-03-14 05:06:14]

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

[commit-bot: I haz the power, 2017-03-14 05:06:16]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[dcheng, 2017-03-14 05:10:09]

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
File base/process/process_metrics.h (right):

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics.h:273: SystemMemoryInfoKB(const SystemMemoryInfoKB&
other);
On 2017/03/14 04:50:37, Michael K. (Yandex Team) wrote:
> On 2017/03/13 16:01:20, danakj wrote:
> > btw I think you can just omit this and you'll get the implicit copy (and
move)
> > constructors then?
> 
> It doesn't work. So I'll revert.
> (chromium-style plugin (aka findbadconstructs) says then that there is a
complex
> constructor with inlined body.
> Btw this struct doesn't need move constructor: it is POD).
> 
> 

Note: a struct isn't a POD if it has in-class member initializers (which this
one does).

[Michael K. (Yandex Team), 2017-03-14 05:40:59]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 05:41:15]

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, 2017-03-14 05:55:38]

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

[commit-bot: I haz the power, 2017-03-14 05:55:39]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Michael K. (Yandex Team), 2017-03-14 06:30:46]

On 2017/03/14 05:10:09, dcheng wrote:
> Note: a struct isn't a POD if it has in-class member initializers (which this
> one does).

You're right, thx.

[Michael K. (Yandex Team), 2017-03-14 06:31:17]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 06:31:52]

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, 2017-03-14 06:46:21]

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

[commit-bot: I haz the power, 2017-03-14 06:46:22]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[Michael K. (Yandex Team), 2017-03-14 07:41:05]

https://codereview.chromium.org/2558043007/diff/500001/base/process/process_m...
File base/process/process_metrics_ios.cc (right):

https://codereview.chromium.org/2558043007/diff/500001/base/process/process_m...
base/process/process_metrics_ios.cc:114: CHECK_EQ(PAGE_SIZE, (PAGE_SIZE >> 10)
<< 10) << "Invalid page size";
jfyi: PAGE_SIZE isn't constexpr on ios(

[Michael K. (Yandex Team), 2017-03-14 07:41:20]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 07:41:35]

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, 2017-03-14 08:46:25]

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

[commit-bot: I haz the power, 2017-03-14 08:46:26]

Dry run: This issue passed the CQ dry run.

[Michael K. (Yandex Team), 2017-03-14 08:53:12]

danakj, PTAL

[Michael K. (Yandex Team), 2017-03-14 08:53:17]

danakj, PTAL

[danakj, 2017-03-14 14:41:46]

LGTM

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
File base/process/process_metrics_mac.cc (right):

https://codereview.chromium.org/2558043007/diff/440001/base/process/process_m...
base/process/process_metrics_mac.cc:399: PAGE_SIZE / 1024 * (vm_info.free_count
- vm_info.speculative_count));
On 2017/03/14 03:49:09, Michael K. (Yandex Team) wrote:
> On 2017/03/13 16:01:21, danakj wrote:
> > Thanks, this seems good, can you use saturated_cast then too? It shouldn't
> hurt
> > but makes it impossible to have weirder problems.
> 
> I think you are worried too much about these casts.
> But Ok, I'll use saturated_cast.

You're probably right, but then saturated_cast won't hurt either :)

https://codereview.chromium.org/2558043007/diff/500001/base/process/process_m...
File base/process/process_metrics_ios.cc (right):

https://codereview.chromium.org/2558043007/diff/500001/base/process/process_m...
base/process/process_metrics_ios.cc:114: CHECK_EQ(PAGE_SIZE, (PAGE_SIZE >> 10)
<< 10) << "Invalid page size";
On 2017/03/14 07:41:05, Michael K. (Yandex Team) wrote:
> jfyi: PAGE_SIZE isn't constexpr on ios(

nit: drop the "Invalid page size" it's not adding value and adds to the binary
size.

[Michael K. (Yandex Team), 2017-03-14 15:13:13]

The CQ bit was checked by mkolom@yandex-team.ru

[Michael K. (Yandex Team), 2017-03-14 15:13:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org, primiano@chromium.org, brucedawson@chromium.org,
danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2558043007/#ps520001
(title: "Remove additional logging in CHECK.")

[commit-bot: I haz the power, 2017-03-14 15:13:38]

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, 2017-03-14 15:20:56]

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

[commit-bot: I haz the power, 2017-03-14 15:20:59]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Michael K. (Yandex Team), 2017-03-14 15:37:30]

mkolom@yandex-team.ru changed reviewers:
+ chrisha@chromium.org, xiyuan@chromium.org
- asvitkine@chromium.org, dcheng@chromium.org, rsesek@chromium.org

[Michael K. (Yandex Team), 2017-03-14 15:38:54]

chrisha is for:
  components/memory_pressure/direct_memory_pressure_calculator_win.cc
  components/memory_pressure/direct_memory_pressure_calculator_win_unittest.cc
  content/browser/memory/memory_monitor_win.cc
  content/browser/memory/memory_monitor_win_unittest.cc

xiyuan@chromium.org is for:
 chrome/browser/ui/webui/about_ui.cc

PTAL

[xiyuan, 2017-03-14 17:25:46]

about_ui.cc lgtm

[Michael K. (Yandex Team), 2017-03-15 02:03:49]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 02:04:27]

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, 2017-03-15 02:58:09]

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

[commit-bot: I haz the power, 2017-03-15 02:58:11]

Dry run: This issue passed the CQ dry run.

[Michael K. (Yandex Team), 2017-03-16 02:10:42]

chrisha, PTAL
  components/memory_pressure/direct_memory_pressure_calculator_win.cc
  components/memory_pressure/direct_memory_pressure_calculator_win_unittest.cc
  content/browser/memory/memory_monitor_win.cc
  content/browser/memory/memory_monitor_win_unittest.cc

[Michael K. (Yandex Team), 2017-03-20 08:13:52]

mkolom@yandex-team.ru changed reviewers:
+ haraken@chromium.org, shrike@chromium.org

[Michael K. (Yandex Team), 2017-03-20 08:16:42]

shrike@ or chrisha@, PTAL:
 * components/memory_pressure/direct_memory_pressure_calculator_win.cc
 * components/memory_pressure/direct_memory_pressure_calculator_win_unittest.cc

haraken@ or chrisha@ PTAL:
 * content/browser/memory/memory_monitor_win.cc
 * content/browser/memory/memory_monitor_win_unittest.cc

Changes are quite trivial in that files.

[haraken, 2017-03-20 11:43:55]

> haraken@ or chrisha@ PTAL:
>  * content/browser/memory/memory_monitor_win.cc
>  * content/browser/memory/memory_monitor_win_unittest.cc

LGTM

[chrisha, 2017-03-20 18:28:52]

lgtm

[Michael K. (Yandex Team), 2017-03-20 18:47:18]

mkolom@yandex-team.ru changed reviewers:
- shrike@chromium.org

[Michael K. (Yandex Team), 2017-03-20 18:47:37]

The CQ bit was checked by mkolom@yandex-team.ru

[commit-bot: I haz the power, 2017-03-20 18:48:15]

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, 2017-03-20 19:19:40]

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

[commit-bot: I haz the power, 2017-03-20 19:19:43]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Michael K. (Yandex Team), 2017-03-20 19:39:49]

The CQ bit was checked by mkolom@yandex-team.ru

[commit-bot: I haz the power, 2017-03-20 19:40:07]

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, 2017-03-20 21:22:22]

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

[commit-bot: I haz the power, 2017-03-20 21:22:24]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Michael K. (Yandex Team), 2017-03-21 01:28:35]

The CQ bit was checked by mkolom@yandex-team.ru

[commit-bot: I haz the power, 2017-03-21 01:29:25]

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, 2017-03-21 03:13:31]

CQ is committing da patch.

Bot data: {"patchset_id": 520001, "attempt_start_ts": 1490059715407050,
"parent_rev": "cc1c52789b126b191dcfc7258e752de7e3cff549", "commit_rev":
"a4258cad75d8cc63170555cc74bd584b5bfca965"}

[commit-bot: I haz the power, 2017-03-21 03:14:36]

Description was changed from

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean
1) absolutely/pure free physical memory (not consumed by anything) or
2) it can include the inactive part(s) of OS (file) cache(s) or
3) it can mean the maximum amount of physical memory that can be
allocated quickly without swapping and thus can also include the
active part(s) of OS cache(s).

This CL fixes base::AmountOfAvailablePhysicalMemory() to function
according to the 2nd meaning on all platforms where it is possible
because this is what everyone needs most of the time. (Win platform
has already had such implementation but others haven't).

BUG=672745
==========

to

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean
1) absolutely/pure free physical memory (not consumed by anything) or
2) it can include the inactive part(s) of OS (file) cache(s) or
3) it can mean the maximum amount of physical memory that can be
allocated quickly without swapping and thus can also include the
active part(s) of OS cache(s).

This CL fixes base::AmountOfAvailablePhysicalMemory() to function
according to the 2nd meaning on all platforms where it is possible
because this is what everyone needs most of the time. (Win platform
has already had such implementation but others haven't).

BUG=672745

Review-Url: https://codereview.chromium.org/2558043007
Cr-Commit-Position: refs/heads/master@{#458291}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4258cad75d8cc63170555cc74bd...
==========

[commit-bot: I haz the power, 2017-03-21 03:14:40]

Committed patchset #27 (id:520001) as
https://chromium.googlesource.com/chromium/src/+/a4258cad75d8cc63170555cc74bd...

[Michael K. (Yandex Team), 2017-03-21 03:22:49]

Thanks everyone for your great patience!

[findit-for-me, 2017-03-21 04:54:00]

Findit identified this CL at revision 458291 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[Michael K. (Yandex Team), 2017-03-21 05:29:24]

On 2017/03/21 04:54:00, findit-for-me wrote:
> Findit identified this CL at revision 458291 as the culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

There is integer overflow. I'll fix it in a moment

[horo, 2017-03-21 05:31:32]

On 2017/03/21 05:29:24, Michael K. (Yandex Team) wrote:
> On 2017/03/21 04:54:00, findit-for-me wrote:
> > Findit identified this CL at revision 458291 as the culprit for
> > failures in the build cycles as shown on:
> >
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> 
> There is integer overflow. I'll fix it in a moment

Filed a bug https://crbug.com/703522

[Michael K. (Yandex Team), 2017-03-21 06:06:39]

On 2017/03/21 05:31:32, horo wrote:
> On 2017/03/21 05:29:24, Michael K. (Yandex Team) wrote:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> > 
> > There is integer overflow. I'll fix it in a moment
> 
> Filed a bug https://crbug.com/703522

https://codereview.chromium.org/2765793002/

[horo, 2017-03-21 07:00:37]

On 2017/03/21 06:06:39, Michael K. (Yandex Team) wrote:
> On 2017/03/21 05:31:32, horo wrote:
> > On 2017/03/21 05:29:24, Michael K. (Yandex Team) wrote:
> >
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> > > 
> > > There is integer overflow. I'll fix it in a moment
> > 
> > Filed a bug https://crbug.com/703522
> 
> https://codereview.chromium.org/2765793002/

Humm..Sorry.
There is no base OWNER in the current timezone.
So I (I'm sheriff now) would like to revert this patch.

Please re-land this CL with the fix.

[horo, 2017-03-21 07:01:26]

A revert of this CL (patchset #27 id:520001) has been created in
https://codereview.chromium.org/2766623002/ by horo@chromium.org.

The reason for reverting is: SysInfoTest.AmountOfAvailablePhysicalMemory is
failing

BUG=703522.

[Michael K. (Yandex Team), 2017-03-21 07:31:29]

Description was changed from

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean
1) absolutely/pure free physical memory (not consumed by anything) or
2) it can include the inactive part(s) of OS (file) cache(s) or
3) it can mean the maximum amount of physical memory that can be
allocated quickly without swapping and thus can also include the
active part(s) of OS cache(s).

This CL fixes base::AmountOfAvailablePhysicalMemory() to function
according to the 2nd meaning on all platforms where it is possible
because this is what everyone needs most of the time. (Win platform
has already had such implementation but others haven't).

BUG=672745

Review-Url: https://codereview.chromium.org/2558043007
Cr-Commit-Position: refs/heads/master@{#458291}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4258cad75d8cc63170555cc74bd...
==========

to

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean
1) absolutely/pure free physical memory (not consumed by anything) or
2) it can include the inactive part(s) of OS (file) cache(s) or
3) it can mean the maximum amount of physical memory that can be
allocated quickly without swapping and thus can also include the
active part(s) of OS cache(s).

This CL fixes base::AmountOfAvailablePhysicalMemory() to function
according to the 2nd meaning on all platforms where it is possible
because this is what everyone needs most of the time. (Win platform
has already had such implementation but others haven't).

BUG=672745

Review-Url: https://codereview.chromium.org/2558043007
Cr-Commit-Position: refs/heads/master@{#458291}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4258cad75d8cc63170555cc74bd...
==========

[Michael K. (Yandex Team), 2017-03-21 07:31:54]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 07:32:26]

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, 2017-03-21 08:28:36]

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

[commit-bot: I haz the power, 2017-03-21 08:28:38]

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

[Michael K. (Yandex Team), 2017-03-21 08:39:01]

The CQ bit was checked by mkolom@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 08:39:25]

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, 2017-03-21 09:07:09]

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

[commit-bot: I haz the power, 2017-03-21 09:07:11]

Dry run: This issue passed the CQ dry run.

[Michael K. (Yandex Team), 2017-03-21 10:29:04]

danakj@ PTAL at base/sys_info_unittest.cc (the last patchset).

There was an accident last night after the CL had been commited and it has been
reverted.
The logs:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

I'm sure there was integer overflow in the test.

But if you look into particular logs you will find strange numbers:
* vm916-m1 and vm924-m1 build bots have 15.67 GB total RAM. But from logs:
../../base/sys_info_unittest.cc:56: Failure
Expected: (amount) < (info.total * 1024), actual: 21963636736 vs 1561014272

"1561014272" - overflow, 21963636736 - 20.5 GB which is greater than 15.67 GB
mentioned above.
 
And there are other examples with the same symptom.

* But for slave117-c1 and slave118-c1 there is overflow only and "amount"
numbers are ok.

I don't think there is an error in base/sys_info_linux.cc logic.
It could be wrong description for vm916-m1 and vm924-m1 build bots
(https://build.chromium.org/p/chromium.linux/buildslaves/vm924-m1)
Or it could be an issue for the virtualization technology (I had such experience
in the past with virtuozzo/openvz e.x.).
 
Ok. Can I:
* Run that test on these exact build bots somehow?
* Get /proc/meminfo output from them?

[danakj, 2017-03-21 14:50:58]

The unit test changes LGTM

[Michael K. (Yandex Team), 2017-03-22 01:04:13]

The CQ bit was checked by mkolom@yandex-team.ru

[Michael K. (Yandex Team), 2017-03-22 01:04:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, brucedawson@chromium.org, xiyuan@chromium.org,
chrisha@chromium.org, haraken@chromium.org, atwilson@chromium.org
Link to the patchset: https://codereview.chromium.org/2558043007/#ps540001
(title: "Fix integer overflow in unittest.")

[commit-bot: I haz the power, 2017-03-22 01:05:20]

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, 2017-03-22 01:47:36]

CQ is committing da patch.

Bot data: {"patchset_id": 540001, "attempt_start_ts": 1490144653755810,
"parent_rev": "f8d3cf5cfc015e0cbffc2a1fa65b6b96f85fe353", "commit_rev":
"01ac10b7938701bc9f9f2c05faf18229f9573b4c"}

[commit-bot: I haz the power, 2017-03-22 01:48:29]

Description was changed from

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean
1) absolutely/pure free physical memory (not consumed by anything) or
2) it can include the inactive part(s) of OS (file) cache(s) or
3) it can mean the maximum amount of physical memory that can be
allocated quickly without swapping and thus can also include the
active part(s) of OS cache(s).

This CL fixes base::AmountOfAvailablePhysicalMemory() to function
according to the 2nd meaning on all platforms where it is possible
because this is what everyone needs most of the time. (Win platform
has already had such implementation but others haven't).

BUG=672745

Review-Url: https://codereview.chromium.org/2558043007
Cr-Commit-Position: refs/heads/master@{#458291}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4258cad75d8cc63170555cc74bd...
==========

to

==========
Fix free memory calculation.

The term "free memory" can mean different things: it can mean
1) absolutely/pure free physical memory (not consumed by anything) or
2) it can include the inactive part(s) of OS (file) cache(s) or
3) it can mean the maximum amount of physical memory that can be
allocated quickly without swapping and thus can also include the
active part(s) of OS cache(s).

This CL fixes base::AmountOfAvailablePhysicalMemory() to function
according to the 2nd meaning on all platforms where it is possible
because this is what everyone needs most of the time. (Win platform
has already had such implementation but others haven't).

BUG=672745

Review-Url: https://codereview.chromium.org/2558043007
Cr-Original-Commit-Position: refs/heads/master@{#458291}
Committed:
https://chromium.googlesource.com/chromium/src/+/a4258cad75d8cc63170555cc74bd...
Review-Url: https://codereview.chromium.org/2558043007
Cr-Commit-Position: refs/heads/master@{#458615}
Committed:
https://chromium.googlesource.com/chromium/src/+/01ac10b7938701bc9f9f2c05faf1...
==========

[commit-bot: I haz the power, 2017-03-22 01:48:32]

Committed patchset #28 (id:540001) as
https://chromium.googlesource.com/chromium/src/+/01ac10b7938701bc9f9f2c05faf1...