ArcLowMemoryKillerMonitor:

chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc

Index: chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc
diff --git a/chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc b/chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc
new file mode 100644
index 0000000000000000000000000000000000000000..47ac33463e62a7a479229f82dc588e2a4371c72d
--- /dev/null
+++ b/chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.cc
@@ -0,0 +1,93 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.

[Yusuke Sato, 2016/03/12 00:24:29]

high level question: what's the point for having this in arc/? I don't see any
ARC specific feature in the files.

[cylee1, 2016/03/14 13:03:35]

It's a good question. Though it didn't ARC specific feature, it reports ARC-only
metrics (namely ARC memory management like app kills). Also it would only be
used by ArcXXXService. So I guess it's fine to land here.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/arc/arc_lowmemorykiller_monitor.h"

[Yusuke Sato, 2016/03/12 00:24:29]

I believe the general mapping rule for class/file names is from FooBar to
foo_bar. Can you rename arc_low_memory_killer_monitor.h/cc to follow that?

[cylee1, 2016/03/14 13:03:35]

Done.

+
+#include <vector>
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "base/metrics/histogram_macros.h"
+#include "base/sequenced_task_runner.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/time/time.h"
+#include "third_party/re2/src/re2/re2.h"
+
+namespace arc {
+
+using base::SequencedWorkerPool;
+using base::TimeDelta;

[Yusuke Sato, 2016/03/12 00:24:29]

Can you sort them alphabetically?

[cylee1, 2016/03/14 13:03:35]

Done.

+using base::StringPiece;
+
+#define UMA_HISTOGRAM_LOWMEMORYKILL_TIMES(name, sample) \
+    UMA_HISTOGRAM_CUSTOM_TIMES( \
+        name, sample, base::TimeDelta::FromMilliseconds(1), \
+        base::TimeDelta::FromSeconds(30), 50)
+
+ArcLowMemoryKillerMonitor::ArcLowMemoryKillerMonitor()
+    : worker_pool_(new SequencedWorkerPool(1, "arc_lmk_monitor")) {}

[Yusuke Sato, 2016/03/12 00:24:29]

please spell that out. lmk -> low_memory_killer

https://google.github.io/styleguide/cppguide.html#General_Naming_Rules

[cylee1, 2016/03/14 13:03:35]

Done.

+
+ArcLowMemoryKillerMonitor::~ArcLowMemoryKillerMonitor() {
+  Stop();
+}
+
+void ArcLowMemoryKillerMonitor::Start() {
+  auto task_runner = worker_pool_->GetTaskRunnerWithShutdownBehavior(
+      SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
+  task_runner->PostTask(
+      FROM_HERE,
+      base::Bind(&ArcLowMemoryKillerMonitor::Run, base::Unretained(this)));

[Yusuke Sato, 2016/03/12 00:24:29]

* Is it always safe to use Unretained(this)? What's the lifetime of |this|
object? Any reason not to use a weak pointer here?
* Instead of having L.53, can't we bind a ref'ed worker_pool_ here and pass it
to Run() as a function argument?

[cylee1, 2016/03/14 13:03:35]

* The life cycle of |this| is meant to be the same as the main thread (e.g., the
same as ChromeBrowserMainPartsChromeos so I thought it's fine). Changed to weak
pointer for better safety anyway.
* Yes but it adds one more copy when calling Bind, or some other type of
overhead. I think it's more light weight this way? 

[Yusuke Sato, 2016/03/14 18:02:12]

Copying a smart pointer seems just fine. What I'm saying here is that there
might be a race. With the current code, what happens if |this| is destructed
after Run() is called and before worker_pool(worker_pool_); is executed? Such a
destruction will unlikely happen hopefully, but the code looks a bit scary to me
still. WDYT?

[cylee1, 2016/03/15 12:49:15]

I see. Changed.

+}
+
+void ArcLowMemoryKillerMonitor::Stop() {
+  worker_pool_->Shutdown();
+}
+
+void ArcLowMemoryKillerMonitor::Run() {
+  // Keep a reference to worker_pool_ in case |this| is deleted in
+  // shutdown process while this thread returns from a blocking read.
+  scoped_refptr<SequencedWorkerPool> worker_pool(worker_pool_);

[Yusuke Sato, 2016/03/12 00:24:29]

Is SequencedWorkerPool derived from RefCountedThreadSafe?

..ah yes it is.

[cylee1, 2016/03/14 13:03:35]

Acknowledged.

+
+  static const int kMaxBufSize = 512;
+
+  FILE* kmsg_fd = fopen("/dev/kmsg", "r");

[Yusuke Sato, 2016/03/12 00:24:29]

Using the raw stdio library seems error-prone. Can you use base/files/ instead?

[cylee1, 2016/03/14 13:03:35]

I didn't see class like FileLineReader in chrome codebase. Is there any library
in base/files which let me read in one line at a time?

+  char buf[kMaxBufSize];
+  int freed_size;
+  int64_t timestamp, last_timestamp = -1;
+  TimeDelta time_delta;

[Yusuke Sato, 2016/03/12 00:24:29]

Add a variable comment please (to explain what this is for.) I think you can
just copy and paste some part of the CL description if you want.

[cylee1, 2016/03/14 13:03:35]

Done.

+
+  while (fgets(buf, kMaxBufSize, kmsg_fd)) {
+    if (worker_pool->IsShutdownInProgress()) {

[Yusuke Sato, 2016/03/12 00:24:29]

Is it safe to use worker_pool_ from this thread?

[cylee1, 2016/03/14 13:03:35]

From the comments of IsShutdownInProcess
  // Check if Shutdown was called for given threading pool. This method is used
  // for aborting time consuming operation to avoid blocking shutdown.
  //
  // Can be called from any thread.
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq...

+      LOG(WARNING) << "Chrome is shutting down, exit now.";
+      break;
+    }
+    if (RE2::PartialMatch(buf,
+                          "lowmemorykiller: .* to free (\\d+)kB",

[Yusuke Sato, 2016/03/12 00:24:29]

* Please give 1-2 example lines of /dev/kmsg (as a code comment) which the code
tries to capture.
* Is the unit always 'kB'?

[cylee1, 2016/03/14 13:03:35]

Done.
Yes it's always in kB as reported from kernel.

+                          &freed_size)) {
+      std::vector<StringPiece> fields = SplitStringPiece(
+          buf, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+
+      time_delta = TimeDelta::Max();
+      // Timestamp is the third field in a line of /dev/kmsg.
+      if (fields.size() >= 3) {
+        base::StringToInt64(fields[2], &timestamp);
+        if (last_timestamp > 0) {
+          time_delta = TimeDelta::FromMicroseconds(timestamp - last_timestamp);
+        }
+        last_timestamp = timestamp;
+      }
+
+      UMA_HISTOGRAM_MEMORY_KB(
+          "ArcRuntime.LowMemoryKiller.FreedSize", freed_size);
+      UMA_HISTOGRAM_LOWMEMORYKILL_TIMES(

[Yusuke Sato, 2016/03/12 00:24:29]

Shouldn't we move this call to the 'if (fields.size() >= 3) {}' block? Do we
want to record TimeDelta::Max()?

[cylee1, 2016/03/14 13:03:35]

You're right that I should move it to the if clause.
But I may want to record TimeDelta::Max() in case of the first kill (comment
added at the variable description), since it gives us a clue that a sequence of
kill events occurs (serves as an approximation of #(memory pressure events),
where one memory pressure events triggers a sequence of kill events).

+          "ArcRuntime.LowMemoryKiller.TimeDelta", time_delta);
+    }
+  }
+  fclose(kmsg_fd);
+}
+
+}  // namespace arc