Add zram field trial

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

Index: chrome/browser/chromeos/chrome_browser_main_chromeos.cc
diff --git a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
index e8d9f722e6612e0c842e5832d3a88c43211cd7b9..d47e9fcf27a8a197c39dad4bf21fd147f476f69a 100644
--- a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
+++ b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
@@ -12,6 +12,7 @@
 #include "base/callback.h"
 #include "base/chromeos/chromeos_version.h"
 #include "base/command_line.h"
+#include "base/file_util.h"
 #include "base/lazy_instance.h"
 #include "base/linux_util.h"
 #include "base/message_loop.h"
@@ -712,6 +713,7 @@ void ChromeBrowserMainPartsChromeos::PostMainMessageLoopRun() {
 
 void ChromeBrowserMainPartsChromeos::SetupPlatformFieldTrials() {
   SetupLowMemoryHeadroomFieldTrial();
+  SetupZramFieldTrial();
 }
 
 void ChromeBrowserMainPartsChromeos::SetupLowMemoryHeadroomFieldTrial() {
@@ -760,4 +762,33 @@ void ChromeBrowserMainPartsChromeos::SetupLowMemoryHeadroomFieldTrial() {
   }
 }
 
+
+void ChromeBrowserMainPartsChromeos::SetupZramFieldTrial() {
+  // The dice for this experiment has been thrown at boot.  The selected group

[Ilya Sherman, 2013/01/08 02:19:04]

nit: either "has" -> "have" or "dice" -> "die"

[Luigi Semenzato, 2013/01/08 18:17:57]

Both forms (die and dice) are used for the singular (as well as "dices" for the
plural, really!) and I can never figure out which one is more
acceptable/readable.  But OK, let's stick to the old-fashioned form.

+  // number is stored in a file.
+  const FilePath kZramGroupPath("/home/chronos/.swap_exp_enrolled");

[James Cook, 2013/01/08 05:07:43]

We frequently run a linux_chromeos build on Ubuntu workstations and this path
either won't exist or will be on the NFS filer.  We might want to consider
checking to see if we're running on an actual Chrome OS device and use two
different paths.  See calls to IsRunningOnChromeOS above.

[Luigi Semenzato, 2013/01/08 18:17:57]

If the file does not exist, the experiment is not started.  Isn't this the
desired behavior for linux_chromeos?  What path should I use instead when
!IsRunningOnChromeOS?  Would that be for testing?

[James Cook, 2013/01/08 19:03:44]

I'm afraid this might cause NFS to attempt to mount /home/chronos (which exists
for our network).  I would either just not run this function if
!IsRunningOnChromeOS or have it look for "/tmp/.swap_exp_enrolled" for
linux_chromeos.

[Luigi Semenzato, 2013/01/08 21:28:18]

We discussed this offline and decided to leave as is.  It turns out that
something else is also accessing /home/chronos, as it gets automounted for
chromeos_linux.

+  std::string zram_group;
+  // If the file does not exist, the experiment has not started.  If the file
+  // exists but contains an "x", the user has a non-standard swap size and has
+  // been opted out.
+  if (!file_util::ReadFileToString(kZramGroupPath, &zram_group) ||

[James Cook, 2013/01/08 05:07:43]

We should run this in a Debug build to make sure we don't hit an assert here.  I
don't know what thread this function runs on and the main thread is generally
not allowed to do file I/O.  I can do this with a linux_chromeos Debug build -
ping me and we can try it there.

[Luigi Semenzato, 2013/01/08 18:17:57]

OK, thanks.

I don't even know if the file thread exists at this point.  Is this an
acceptable exception to the rule?

[James Cook, 2013/01/08 19:03:44]

If the file thread isn't created yet, this should be fine.  If we hit an assert
we can explicitly allow the disk IO with a ScopedAllowIO object.

[Luigi Semenzato, 2013/01/08 21:28:18]

James tested this and no assertion is produced.

+      StartsWithASCII(zram_group, "x", true)) {
+    return;
+  }
+  const base::FieldTrial::Probability kDivisor = 1;  // on/off only
+  scoped_refptr<base::FieldTrial> trial =
+    base::FieldTrialList::FactoryGetFieldTrial(
+      "ZRAM", kDivisor, "default", 2013, 12, 31, NULL);
+  // Assign probability of 1 to the group Chrome OS has picked.  Assign 0 to
+  // all other choices.
+  const char *groups[] = { "0", "1", "2", "3", "4", "5", "6", "7", "8" };

[Ilya Sherman, 2013/01/08 02:19:04]

nit: "const char* const kGroups[] = ..."

[Ilya Sherman, 2013/01/08 02:19:04]

Is 9 intentionally omitted?  What do each of these groups mean?

[Luigi Semenzato, 2013/01/08 18:17:57]

OK

[Luigi Semenzato, 2013/01/08 18:17:57]

Yes, there are only 9 groups (0-8).

Should the groups have better names?  This code should not care.  It was
convenient to do it this way on the Chrome OS side.  I am adding you to those
reviews so you can see.

[Ilya Sherman, 2013/01/08 20:16:16]

It's always nice when names are self-documenting.  If these are substantially
easier to use, it would be helpful to either add comments documenting them, or
point to the other file where they are documented.

[Luigi Semenzato, 2013/01/08 21:28:18]

OK.  I was about to change the names, but then asvitkine saved me at the last
minute.  Will add comments.

+  for (unsigned int i = 0; i < sizeof(groups) / sizeof(groups[0]); i++) {

[Ilya Sherman, 2013/01/08 02:19:04]

nit: Please use the arraysize macro here.

[Ilya Sherman, 2013/01/08 02:19:04]

nit: "unsigned int" -> "size_t"

[Ilya Sherman, 2013/01/08 02:19:04]

nit: "i++" -> "++i"

[Luigi Semenzato, 2013/01/08 18:17:57]

OK

[Luigi Semenzato, 2013/01/08 18:17:57]

Are you sure?  A size divided by a size is a dimensionless number.  Size_t is a
size in bytes.

[Luigi Semenzato, 2013/01/08 18:17:57]

OK

[Ilya Sherman, 2013/01/08 20:16:16]

According to [ http://dev.chromium.org/developers/coding-style#TOC-Types ],
unsigned int is generally the wrong type, and size_t is generally the right
type.  I don't think that size_t is necessarily a size in bytes -- for example,
the std::vector::size() method returns a size_t.

[Luigi Semenzato, 2013/01/08 21:28:18]

Thank you for setting me straight on this.

+    bool match = StartsWithASCII(zram_group, groups[i], true);

[Ilya Sherman, 2013/01/08 02:19:04]

Why are you checking for a prefix, rather than direct equality?

[Luigi Semenzato, 2013/01/08 18:17:57]

Because I don't want to have to worry whether the file contains a newline after
the group name.  (Right now it does.)

[Ilya Sherman, 2013/01/08 20:16:16]

IMO it would be better to trim off any whitespace, and then compare for exact
equality -- the code would be less brittle that way.

+    (void) trial->AppendGroup(groups[i], match ? 1 : 0);

[Ilya Sherman, 2013/01/08 02:19:04]

nit: Remove the "(void)"

[Luigi Semenzato, 2013/01/08 18:17:57]

OK.  I thought it may help to indicate I am aware I am discarding the return
value.

[James Cook, 2013/01/08 19:03:44]

I think Chrome-style is to leave it out.

+    if (match) {
+      LOG(WARNING) << "zram field trial: group " << groups[i];

[Ilya Sherman, 2013/01/08 02:19:04]

nit: Can this be a DVLOG(1)?

[James Cook, 2013/01/08 05:07:43]

Actually, I'd be OK with leaving this in as a warning. While we don't normally
like to add logs to Chrome under normal operation, being part of a zram trial
might have really important implications for debugging a real-world
out-of-memory problem and it would be super-valuable to have that line in the
logs.

[Luigi Semenzato, 2013/01/08 18:17:57]

Yes, it's only one line and I would prefer to always have it.

[Ilya Sherman, 2013/01/08 20:16:16]

FWIW, the list of running field trials is also available at chrome://chrome.

[Luigi Semenzato, 2013/01/08 21:28:18]

Good to know, thanks, but if you don't mind I'd leave this in because I am not
sure we log that.

+    }

[Ilya Sherman, 2013/01/08 02:19:04]

nit: No need for curly braces since this is a 1-line if-stmt.

[Luigi Semenzato, 2013/01/08 18:17:57]

OK

+  }
+}
+
 }  //  namespace chromeos