Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11744024:
Add zram field trial (Closed)
|
Created:
7 years, 11 months ago by Luigi Semenzato Modified:
7 years, 11 months ago Reviewers:
James Cook, Ilya Sherman, stevenjb, Alexei Svitkine (slow), Greg Spencer (Chromium), Sonny CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, jar (doing other things), Alexei Svitkine (slow), SteveT Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd zram field trial.
This adds a field trial for compressed swap. Chrome OS picks the trial
group to be used, based on the HW address of the wifi controller. Chrome
assigns a 100% probability to the group picked by Chrome OS.
BUG=chromium-os:37583
TEST=manually tested
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175704
Patch Set 1 #Patch Set 2 : Add zram field trial. #Patch Set 3 : add logging #
Total comments: 37
Patch Set 4 : address most comments by reviewers #
Total comments: 12
Patch Set 5 : fix compilation #Patch Set 6 : remove isRunningOnChromeOS check #Patch Set 7 : address more review comments #
Total comments: 8
Patch Set 8 : fix compilation again, sigh #
Messages
Total messages: 22 (0 generated)
Hi James, Greg, Ilya, it would be great if you can review this. Thanks!
https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:767: // The dice for this experiment has been thrown at boot. The selected group nit: either "has" -> "have" or "dice" -> "die" https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:784: const char *groups[] = { "0", "1", "2", "3", "4", "5", "6", "7", "8" }; nit: "const char* const kGroups[] = ..." https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:784: const char *groups[] = { "0", "1", "2", "3", "4", "5", "6", "7", "8" }; Is 9 intentionally omitted? What do each of these groups mean? https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:785: for (unsigned int i = 0; i < sizeof(groups) / sizeof(groups[0]); i++) { nit: Please use the arraysize macro here. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:785: for (unsigned int i = 0; i < sizeof(groups) / sizeof(groups[0]); i++) { nit: "unsigned int" -> "size_t" https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:785: for (unsigned int i = 0; i < sizeof(groups) / sizeof(groups[0]); i++) { nit: "i++" -> "++i" https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:786: bool match = StartsWithASCII(zram_group, groups[i], true); Why are you checking for a prefix, rather than direct equality? https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:787: (void) trial->AppendGroup(groups[i], match ? 1 : 0); nit: Remove the "(void)" https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:789: LOG(WARNING) << "zram field trial: group " << groups[i]; nit: Can this be a DVLOG(1)? https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:790: } nit: No need for curly braces since this is a 1-line if-stmt.
Overall approach seems good, either Greg or I should try patching this in to a linux_chromeos Debug build to see how it runs there. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:769: const FilePath kZramGroupPath("/home/chronos/.swap_exp_enrolled"); 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. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:774: if (!file_util::ReadFileToString(kZramGroupPath, &zram_group) || 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. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:789: LOG(WARNING) << "zram field trial: group " << groups[i]; On 2013/01/08 02:19:04, Ilya Sherman wrote: > nit: Can this be a DVLOG(1)? 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.
Thanks for the review! Before I upload the next changes, would you please take a look at my replies to your comments? There are a couple of places where I am still not sure which way it should go. Thanks! Also, for further information, please look at https://gerrit.chromium.org/gerrit/#/c/40354/ https://gerrit.chromium.org/gerrit/#/c/40356/ https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:767: // The dice for this experiment has been thrown at boot. The selected group On 2013/01/08 02:19:04, Ilya Sherman wrote: > nit: either "has" -> "have" or "dice" -> "die" 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. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:769: const FilePath kZramGroupPath("/home/chronos/.swap_exp_enrolled"); On 2013/01/08 05:07:43, James Cook (Chromium) wrote: > 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. 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? https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:774: if (!file_util::ReadFileToString(kZramGroupPath, &zram_group) || On 2013/01/08 05:07:43, James Cook (Chromium) wrote: > 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. OK, thanks. I don't even know if the file thread exists at this point. Is this an acceptable exception to the rule? https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:784: const char *groups[] = { "0", "1", "2", "3", "4", "5", "6", "7", "8" }; On 2013/01/08 02:19:04, Ilya Sherman wrote: > nit: "const char* const kGroups[] = ..." OK https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:784: const char *groups[] = { "0", "1", "2", "3", "4", "5", "6", "7", "8" }; On 2013/01/08 02:19:04, Ilya Sherman wrote: > Is 9 intentionally omitted? What do each of these groups mean? 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. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:785: for (unsigned int i = 0; i < sizeof(groups) / sizeof(groups[0]); i++) { On 2013/01/08 02:19:04, Ilya Sherman wrote: > nit: Please use the arraysize macro here. OK https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:785: for (unsigned int i = 0; i < sizeof(groups) / sizeof(groups[0]); i++) { On 2013/01/08 02:19:04, Ilya Sherman wrote: > nit: "unsigned int" -> "size_t" Are you sure? A size divided by a size is a dimensionless number. Size_t is a size in bytes. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:785: for (unsigned int i = 0; i < sizeof(groups) / sizeof(groups[0]); i++) { On 2013/01/08 02:19:04, Ilya Sherman wrote: > nit: "i++" -> "++i" OK https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:786: bool match = StartsWithASCII(zram_group, groups[i], true); On 2013/01/08 02:19:04, Ilya Sherman wrote: > Why are you checking for a prefix, rather than direct equality? Because I don't want to have to worry whether the file contains a newline after the group name. (Right now it does.) https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:787: (void) trial->AppendGroup(groups[i], match ? 1 : 0); On 2013/01/08 02:19:04, Ilya Sherman wrote: > nit: Remove the "(void)" OK. I thought it may help to indicate I am aware I am discarding the return value. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:789: LOG(WARNING) << "zram field trial: group " << groups[i]; On 2013/01/08 05:07:43, James Cook (Chromium) wrote: > On 2013/01/08 02:19:04, Ilya Sherman wrote: > > nit: Can this be a DVLOG(1)? > > 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. Yes, it's only one line and I would prefer to always have it. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:790: } On 2013/01/08 02:19:04, Ilya Sherman wrote: > nit: No need for curly braces since this is a 1-line if-stmt. OK
Comments below. Do you want me to patch this in and run in Debug on linux_chromeos? Or wait for another patch set? https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:769: const FilePath kZramGroupPath("/home/chronos/.swap_exp_enrolled"); On 2013/01/08 18:17:57, Luigi Semenzato wrote: > On 2013/01/08 05:07:43, James Cook (Chromium) wrote: > > 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. > > 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? 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. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:774: if (!file_util::ReadFileToString(kZramGroupPath, &zram_group) || On 2013/01/08 18:17:57, Luigi Semenzato wrote: > On 2013/01/08 05:07:43, James Cook (Chromium) wrote: > > 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. > > OK, thanks. > > I don't even know if the file thread exists at this point. Is this an > acceptable exception to the rule? 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. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:787: (void) trial->AppendGroup(groups[i], match ? 1 : 0); On 2013/01/08 18:17:57, Luigi Semenzato wrote: > On 2013/01/08 02:19:04, Ilya Sherman wrote: > > nit: Remove the "(void)" > > OK. I thought it may help to indicate I am aware I am discarding the return > value. I think Chrome-style is to leave it out.
Thanks James, can you try this patch? I will also try it on my chromebook as soon as I can get the chrome build to work again.
+cc others familiar with FieldTrial code, just in cases LGTM with nits: https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:784: const char *groups[] = { "0", "1", "2", "3", "4", "5", "6", "7", "8" }; On 2013/01/08 18:17:57, Luigi Semenzato wrote: > On 2013/01/08 02:19:04, Ilya Sherman wrote: > > Is 9 intentionally omitted? What do each of these groups mean? > > 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. 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. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:785: for (unsigned int i = 0; i < sizeof(groups) / sizeof(groups[0]); i++) { On 2013/01/08 18:17:57, Luigi Semenzato wrote: > On 2013/01/08 02:19:04, Ilya Sherman wrote: > > nit: "unsigned int" -> "size_t" > > Are you sure? A size divided by a size is a dimensionless number. Size_t is a > size in bytes. 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. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:786: bool match = StartsWithASCII(zram_group, groups[i], true); On 2013/01/08 18:17:57, Luigi Semenzato wrote: > On 2013/01/08 02:19:04, Ilya Sherman wrote: > > Why are you checking for a prefix, rather than direct equality? > > Because I don't want to have to worry whether the file contains a newline after > the group name. (Right now it does.) IMO it would be better to trim off any whitespace, and then compare for exact equality -- the code would be less brittle that way. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:789: LOG(WARNING) << "zram field trial: group " << groups[i]; On 2013/01/08 05:07:43, James Cook (Chromium) wrote: > On 2013/01/08 02:19:04, Ilya Sherman wrote: > > nit: Can this be a DVLOG(1)? > > 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. FWIW, the list of running field trials is also available at chrome://chrome. https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:723: SetupZramFieldTrial(); nit: Include curly braces for this if-stmt since it spans two textual lines with the comment. https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:792: for (unsigned int i = 0; i < arraysize(kGroups); i++) { nit: Still ++i ;)
https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:787: base::FieldTrialList::FactoryGetFieldTrial( Nit: Indent 4. https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:788: "ZRAM", kDivisor, "default", 2013, 12, 31, NULL); Nit: Also indent 4 from line above. https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:793: bool match = StartsWithASCII(zram_group, kGroups[i], true); I don't know the details of how that file is created, but is StartsWith() the right thing to use here? What if it the file contains "10"? Or is that impossible?
https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:791: const char* const kGroups[] = { "0", "1", "2", "3", "4", "5", "6", "7", "8" }; Note: I saw your earlier concern about whether these are good group names to use. They are for the following reason. We have a script that parses source code to extract names used in field trials so that they can be made available to the Finch dashboard. It doesn't work for group names created in a loop, such as here. However, we special case simple numeric values, so that they are always available. So while the script won't be able to parse the loop, the values you chose should still work with the dashboard without additional intervention. https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:793: bool match = StartsWithASCII(zram_group, kGroups[i], true); On 2013/01/08 20:30:47, Alexei Svitkine wrote: > I don't know the details of how that file is created, but is StartsWith() the > right thing to use here? What if it the file contains "10"? Or is that > impossible? I notice this was somewhat covered by earlier discussion. I would suggest adding a DCHECK() to ensure the file contents are as expected. (Do we run with DCHECKs enabled on actual ChromeOS hardware? e.g. on dev channel?)
https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:793: bool match = StartsWithASCII(zram_group, kGroups[i], true); On 2013/01/08 20:38:02, Alexei Svitkine wrote: > (Do we run with DCHECKs > enabled on actual ChromeOS hardware? e.g. on dev channel?) No, we don't run Debug builds on any channels: the resulting build doesn't fit on the partition. AFAIK, Debug builds are pretty much only run by developers when debugging.
LGTM assuming comments from other reviewers addressed. I ran this in Debug on linux_chromeos both with and without checks for IsRunningOnChromeOS and there are no assertions about file IO occurring on the main thread. So this should be fine.
https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:793: bool match = StartsWithASCII(zram_group, kGroups[i], true); On 2013/01/08 20:41:03, Greg Spencer (Chromium) wrote: > On 2013/01/08 20:38:02, Alexei Svitkine wrote: > > (Do we run with DCHECKs > > enabled on actual ChromeOS hardware? e.g. on dev channel?) > > No, we don't run Debug builds on any channels: the resulting build doesn't fit > on the partition. > > AFAIK, Debug builds are pretty much only run by developers when debugging. DCHECKs can be enabled in release builds though, with via dcheck_always_on=1 gyp flag. I was wondering if that was on for e.g. dev channel.
Thanks to all and sorry in advance if I missed anything. Adding stevenjb as OWNER. Hi Steven, thanks for looking at this. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:769: const FilePath kZramGroupPath("/home/chronos/.swap_exp_enrolled"); On 2013/01/08 19:03:44, James Cook (Chromium) wrote: > On 2013/01/08 18:17:57, Luigi Semenzato wrote: > > On 2013/01/08 05:07:43, James Cook (Chromium) wrote: > > > 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. > > > > 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? > > 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. 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. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:774: if (!file_util::ReadFileToString(kZramGroupPath, &zram_group) || On 2013/01/08 19:03:44, James Cook (Chromium) wrote: > On 2013/01/08 18:17:57, Luigi Semenzato wrote: > > On 2013/01/08 05:07:43, James Cook (Chromium) wrote: > > > 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. > > > > OK, thanks. > > > > I don't even know if the file thread exists at this point. Is this an > > acceptable exception to the rule? > > 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. James tested this and no assertion is produced. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:784: const char *groups[] = { "0", "1", "2", "3", "4", "5", "6", "7", "8" }; On 2013/01/08 20:16:16, Ilya Sherman wrote: > On 2013/01/08 18:17:57, Luigi Semenzato wrote: > > On 2013/01/08 02:19:04, Ilya Sherman wrote: > > > Is 9 intentionally omitted? What do each of these groups mean? > > > > 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. > > 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. OK. I was about to change the names, but then asvitkine saved me at the last minute. Will add comments. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:785: for (unsigned int i = 0; i < sizeof(groups) / sizeof(groups[0]); i++) { On 2013/01/08 20:16:16, Ilya Sherman wrote: > On 2013/01/08 18:17:57, Luigi Semenzato wrote: > > On 2013/01/08 02:19:04, Ilya Sherman wrote: > > > nit: "unsigned int" -> "size_t" > > > > Are you sure? A size divided by a size is a dimensionless number. Size_t is > a > > size in bytes. > > 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. Thank you for setting me straight on this. https://codereview.chromium.org/11744024/diff/4001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:789: LOG(WARNING) << "zram field trial: group " << groups[i]; On 2013/01/08 20:16:16, Ilya Sherman wrote: > On 2013/01/08 05:07:43, James Cook (Chromium) wrote: > > On 2013/01/08 02:19:04, Ilya Sherman wrote: > > > nit: Can this be a DVLOG(1)? > > > > 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. > > FWIW, the list of running field trials is also available at chrome://chrome. Good to know, thanks, but if you don't mind I'd leave this in because I am not sure we log that. https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:723: SetupZramFieldTrial(); On 2013/01/08 20:16:16, Ilya Sherman wrote: > nit: Include curly braces for this if-stmt since it spans two textual lines with > the comment. Thanks. The if has been removed. https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:791: const char* const kGroups[] = { "0", "1", "2", "3", "4", "5", "6", "7", "8" }; On 2013/01/08 20:38:02, Alexei Svitkine wrote: > Note: I saw your earlier concern about whether these are good group names to > use. They are for the following reason. We have a script that parses source code > to extract names used in field trials so that they can be made available to the > Finch dashboard. > > It doesn't work for group names created in a loop, such as here. However, we > special case simple numeric values, so that they are always available. So while > the script won't be able to parse the loop, the values you chose should still > work with the dashboard without additional intervention. Good to know, thanks. https://codereview.chromium.org/11744024/diff/13001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:792: for (unsigned int i = 0; i < arraysize(kGroups); i++) { On 2013/01/08 20:16:16, Ilya Sherman wrote: > nit: Still ++i ;) Aw!
https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:799: LOG(WARNING) << "zram field trial: group " << kGroups[i]; nit: We should avoid WARNING spam unless it's truly important. DVLOG(1) would be better. Also, can't we break here, since the compare can only succeed once? Or better yet, couldn't we just return here and eliminate the need for |matched| entirely, warning if we complete the loop?
https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:799: LOG(WARNING) << "zram field trial: group " << kGroups[i]; On 2013/01/08 21:36:27, stevenjb (chromium) wrote: > nit: We should avoid WARNING spam unless it's truly important. DVLOG(1) would be > better. > Also, can't we break here, since the compare can only succeed once? Or better > yet, couldn't we just return here and eliminate the need for |matched| entirely, > warning if we complete the loop? > I mentioned on an earlier pass that I would like to keep the warning. It's only one line once per start and it will really help when looking at out-of-memory feedback reports to tell if the user is in the trial or not.
https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:799: LOG(WARNING) << "zram field trial: group " << kGroups[i]; On 2013/01/08 21:38:47, James Cook (Chromium) wrote: > On 2013/01/08 21:36:27, stevenjb (chromium) wrote: > > nit: We should avoid WARNING spam unless it's truly important. DVLOG(1) would > be > > better. > > Also, can't we break here, since the compare can only succeed once? Or better > > yet, couldn't we just return here and eliminate the need for |matched| > entirely, > > warning if we complete the loop? > > > > I mentioned on an earlier pass that I would like to keep the warning. It's only > one line once per start and it will really help when looking at out-of-memory > feedback reports to tell if the user is in the trial or not. Will it show up on tests? We already have quite a few "one line" WARNINGs that show up on every test run that I would like to get rid of.
Thanks! Comments in line. https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:799: LOG(WARNING) << "zram field trial: group " << kGroups[i]; On 2013/01/08 21:38:47, James Cook (Chromium) wrote: > On 2013/01/08 21:36:27, stevenjb (chromium) wrote: > > nit: We should avoid WARNING spam unless it's truly important. DVLOG(1) would > be > > better. > > Also, can't we break here, since the compare can only succeed once? Or better > > yet, couldn't we just return here and eliminate the need for |matched| > entirely, > > warning if we complete the loop? > > > > I mentioned on an earlier pass that I would like to keep the warning. It's only > one line once per start and it will really help when looking at out-of-memory > feedback reports to tell if the user is in the trial or not. I would also prefer to log all the time for exactly the same reasons. Regarding early loop termination: I might get into trouble if I do that, as the trial object would have a variable number of groups. The AddGroup method both adds a group and decides if that group should be selected or not. The API is not sufficiently clear to diverge from existing usage. (I almost got into trouble because the AddGroup calls are in a loop...)
Answering Steve's question. https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:799: LOG(WARNING) << "zram field trial: group " << kGroups[i]; On 2013/01/08 21:53:15, stevenjb (chromium) wrote: > On 2013/01/08 21:38:47, James Cook (Chromium) wrote: > > On 2013/01/08 21:36:27, stevenjb (chromium) wrote: > > > nit: We should avoid WARNING spam unless it's truly important. DVLOG(1) > would > > be > > > better. > > > Also, can't we break here, since the compare can only succeed once? Or > better > > > yet, couldn't we just return here and eliminate the need for |matched| > > entirely, > > > warning if we complete the loop? > > > > > > > I mentioned on an earlier pass that I would like to keep the warning. It's > only > > one line once per start and it will really help when looking at out-of-memory > > feedback reports to tell if the user is in the trial or not. > > Will it show up on tests? We already have quite a few "one line" WARNINGs that > show up on every test run that I would like to get rid of. > No, it won't show up on tests, unless the file /home/chronos/.swap_exp_enrolled exists and is readable in your test environment.
https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:799: LOG(WARNING) << "zram field trial: group " << kGroups[i]; I ran it and can confirm it doesn't log in browser_tests.
LGTM https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:799: LOG(WARNING) << "zram field trial: group " << kGroups[i]; Ah, I see, that makes sense. It might be slightly more clear as: int probability = (zram_group.compare(kGroups[i]) == 0) ? 1 : 0; trial->AppendGroup(kGroups[i], probability); But it's fine as is. https://codereview.chromium.org/11744024/diff/24001/chrome/browser/chromeos/c... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:799: LOG(WARNING) << "zram field trial: group " << kGroups[i]; On 2013/01/08 22:07:18, James Cook (Chromium) wrote: > I ran it and can confirm it doesn't log in browser_tests. OK, cool, then that's fine. I wish we had a LOG(ALWAYS) instead of using WARNING for things like this. Maybe we should revive INFO, remove any remaining spammy uses, and repurpose it? Not in this CL of course.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/semenzato@chromium.org/11744024/28001
Message was sent while issue was closed.
Change committed as 175704 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
