Fix free memory calculation.

base/process/process_metrics_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/process/process_metrics.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <sstream>
@@ skipping 88 matching lines @@
     "Inactive:       21221496 kB\n"
     "Active(anon):    5674352 kB\n"
     "Inactive(anon):   633992 kB\n";
   EXPECT_FALSE(ParseProcMeminfo(invalid_input1, &meminfo));
   EXPECT_FALSE(ParseProcMeminfo(invalid_input2, &meminfo));
   EXPECT_FALSE(ParseProcMeminfo(invalid_input3, &meminfo));
 
   std::string valid_input1 =
     "MemTotal:        3981504 kB\n"
     "MemFree:          140764 kB\n"
+    "MemAvailable:    2746720 kB\n"

[Primiano Tucci (use gerrit), 2017/02/15 11:18:33]

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.

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

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.

[Primiano Tucci (use gerrit), 2017/02/20 14:13:27]

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..
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.

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

Acknowledged.

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

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

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

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?

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

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.

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

Not exactly. See below.
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.

     "Buffers:          116480 kB\n"
     "Cached:           406160 kB\n"
     "SwapCached:        21304 kB\n"
     "Active:          3152040 kB\n"
     "Inactive:         472856 kB\n"
     "Active(anon):    2972352 kB\n"
     "Inactive(anon):   270108 kB\n"
     "Active(file):     179688 kB\n"
     "Inactive(file):   202748 kB\n"
     "Unevictable:           0 kB\n"
@@ skipping 45 matching lines @@
     "VmallocTotal:  3874808 kB\n"
     "VmallocUsed:      1416 kB\n"
     "VmallocChunk:  3872908 kB\n"
     "HugePages_Total:     0\n"
     "HugePages_Free:      0\n"
     "Hugepagesize:     4096 kB\n";
 
   EXPECT_TRUE(ParseProcMeminfo(valid_input1, &meminfo));
   EXPECT_EQ(meminfo.total, 3981504);
   EXPECT_EQ(meminfo.free, 140764);
+  EXPECT_EQ(meminfo.available, 2746720);
   EXPECT_EQ(meminfo.buffers, 116480);
   EXPECT_EQ(meminfo.cached, 406160);
   EXPECT_EQ(meminfo.active_anon, 2972352);
   EXPECT_EQ(meminfo.active_file, 179688);
   EXPECT_EQ(meminfo.inactive_anon, 270108);
   EXPECT_EQ(meminfo.inactive_file, 202748);
   EXPECT_EQ(meminfo.swap_total, 5832280);
   EXPECT_EQ(meminfo.swap_free, 3672368);
   EXPECT_EQ(meminfo.dirty, 184);
+  EXPECT_EQ(meminfo.reclaimable, 30936);
 #if defined(OS_CHROMEOS)
   EXPECT_EQ(meminfo.shmem, 140204);
   EXPECT_EQ(meminfo.slab, 54212);
 #endif
   EXPECT_TRUE(ParseProcMeminfo(valid_input2, &meminfo));
   EXPECT_EQ(meminfo.total, 255908);
   EXPECT_EQ(meminfo.free, 69936);
   EXPECT_EQ(meminfo.buffers, 15812);
   EXPECT_EQ(meminfo.cached, 115124);
   EXPECT_EQ(meminfo.swap_total, 524280);
@@ skipping 141 matching lines @@
 
   thread2.Stop();
   EXPECT_GE(metrics->GetCPUUsage(), 0.0);
 
   thread3.Stop();
   EXPECT_GE(metrics->GetCPUUsage(), 0.0);
 }
 
 #endif  // defined(OS_LINUX) || defined(OS_CHROMEOS)
 
-#if defined(OS_WIN) || (defined(OS_MACOSX) && !defined(OS_IOS)) || \
+#if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_IOS) || \
     defined(OS_LINUX) || defined(OS_ANDROID)
 TEST(SystemMetrics2Test, GetSystemMemoryInfo) {
   SystemMemoryInfoKB info;
   EXPECT_TRUE(GetSystemMemoryInfo(&info));
 
   // Ensure each field received a value.
   EXPECT_GT(info.total, 0);
+#if defined(OS_WIN)
+  EXPECT_GT(info.available, 0);
+  EXPECT_GT(info.free + info.zero, 0);
+#else
   EXPECT_GT(info.free, 0);
+#endif
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   EXPECT_GT(info.buffers, 0);
   EXPECT_GT(info.cached, 0);
   EXPECT_GT(info.active_anon, 0);
   EXPECT_GT(info.inactive_anon, 0);
   EXPECT_GT(info.active_file, 0);
   EXPECT_GT(info.inactive_file, 0);
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 
   // All the values should be less than the total amount of memory.
   EXPECT_LT(info.free, info.total);
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   EXPECT_LT(info.buffers, info.total);
   EXPECT_LT(info.cached, info.total);
   EXPECT_LT(info.active_anon, info.total);
   EXPECT_LT(info.inactive_anon, info.total);
   EXPECT_LT(info.active_file, info.total);
   EXPECT_LT(info.inactive_file, info.total);
 #endif  // defined(OS_LINUX) || defined(OS_ANDROID)
 
+#if defined(OS_MACOSX) || defined(OS_IOS)
+  EXPECT_GT(info.file_backed, 0);
+#endif
+
 #if defined(OS_CHROMEOS)
   // Chrome OS exposes shmem.
   EXPECT_GT(info.shmem, 0);
   EXPECT_LT(info.shmem, info.total);
   // Chrome unit tests are not run on actual Chrome OS hardware, so gem_objects
   // and gem_size cannot be tested here.
 #endif
 }
-#endif  // defined(OS_WIN) || (defined(OS_MACOSX) && !defined(OS_IOS)) ||
+#endif  // defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_IOS) ||
         // defined(OS_LINUX) || defined(OS_ANDROID)
 
 #if defined(OS_LINUX) || defined(OS_ANDROID)
 TEST(ProcessMetricsTest, ParseProcStatCPU) {
   // /proc/self/stat for a process running "top".
   const char kTopStat[] = "960 (top) S 16230 960 16230 34818 960 "
       "4202496 471 0 0 0 "
       "12 16 0 0 "  // <- These are the goods.
       "20 0 1 0 121946157 15077376 314 18446744073709551615 4194304 "
       "4246868 140733983044336 18446744073709551615 140244213071219 "
@@ skipping 106 matching lines @@
 
   std::unique_ptr<ProcessMetrics> metrics(
       ProcessMetrics::CreateProcessMetrics(child.Handle()));
   EXPECT_EQ(0, metrics->GetOpenFdCount());
   ASSERT_TRUE(child.Terminate(0, true));
 }
 #endif  // defined(OS_LINUX)
 
 }  // namespace debug
 }  // namespace base