Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11823028:
Use better group names for zram field trial (Closed)
|
Created:
7 years, 11 months ago by Luigi Semenzato Modified:
7 years, 11 months ago CC:
chromium-reviews, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, James Cook, Sonny Base URL:
https://chromium.googlesource.com/chromium/src.git@zram2 Visibility:
Public. |
DescriptionUse better group names for zram field trial
This will save some trouble to those who want to interpret
the field trial results.
BUG=chromium-os:37583
TEST=manually tested
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175944
Patch Set 1 #Patch Set 2 : Use better group names for zram field trial #
Total comments: 6
Patch Set 3 : Use better group names for zram field trial #
Total comments: 1
Patch Set 4 : Use better group names for zram field trial #
Total comments: 2
Patch Set 5 : Use better group names for zram field trial #Patch Set 6 : Use better group names for zram field trial #Messages
Total messages: 18 (0 generated)
Please let me know if this is better. I am not familiar with the way the experiment results are presented.
I'll defer to others on the trial name style. https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:777: if (!file_util::ReadFileToString(kZramGroupPath, &zram_file_content)) Does this need a check for zram_file_content.empty() ? I think the line zram_file_content[0] will crash if the file is empty.
Thanks James, I fixed that case. https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:777: if (!file_util::ReadFileToString(kZramGroupPath, &zram_file_content)) On 2013/01/09 18:17:17, James Cook (Chromium) wrote: > Does this need a check for zram_file_content.empty() ? I think the line > zram_file_content[0] will crash if the file is empty. Ah, I was assuming empty file == null-terminated empty string, but this is a std::string... Interestingly, this code works: std::string s(""); std::cout << s[0]; but the reference doesn't say anything about this, so better play it safe.
LGTM with optional nit https://codereview.chromium.org/11823028/diff/5001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11823028/diff/5001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:783: if (zram_file_content.length() == 0) { nit: Chrome-style for std::string is to use zram_file_content.empty()
Thanks. I will wait for feedback regarding the group names.
https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:783: char zram_group = zram_file_content[0]; Can you check that !zram_file_content.empty() somewhere before this? https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:790: LOG(WARNING) << "zram field trial: group " << zram_group; field_trial.cc has VLOGs that can tell you this already, so you don't need a separate one (unless vlog doesn't work for you).
https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:783: char zram_group = zram_file_content[0]; On 2013/01/09 19:00:36, Alexei Svitkine wrote: > Can you check that !zram_file_content.empty() somewhere before this? Sorry, I wrote this before you addressed James' comment.
Thanks Alexei. Are the group names OK? https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11823028/diff/2001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:790: LOG(WARNING) << "zram field trial: group " << zram_group; On 2013/01/09 19:00:36, Alexei Svitkine wrote: > field_trial.cc has VLOGs that can tell you this already, so you don't need a > separate one (unless vlog doesn't work for you). We discussed this in the previous review. That's a DVLOG(1) but for ease of analyzing crashes etc. we want to always have this information in the Chrome OS logs, since it's only one line per Chrome start. This only affects Chrome OS, not even the chromeos linux tests.
https://codereview.chromium.org/11823028/diff/8001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11823028/diff/8001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:801: trial->AppendGroup("2GB RAM, no swap", zram_group == '0' ? 1 : 0); Can you make the group names not have spaces and commas (and other special chars)? You can use underscores for example. (I'm not sure if it's a problem in practice, but no other trials use them I think, so it's safer not to).
Thanks! https://codereview.chromium.org/11823028/diff/8001/chrome/browser/chromeos/ch... File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right): https://codereview.chromium.org/11823028/diff/8001/chrome/browser/chromeos/ch... chrome/browser/chromeos/chrome_browser_main_chromeos.cc:801: trial->AppendGroup("2GB RAM, no swap", zram_group == '0' ? 1 : 0); On 2013/01/09 19:22:06, Alexei Svitkine wrote: > Can you make the group names not have spaces and commas (and other special > chars)? You can use underscores for example. > > (I'm not sure if it's a problem in practice, but no other trials use them I > think, so it's safer not to). Done.
LGTM, thanks.
Fixed compilation.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/semenzato@chromium.org/11823028/10003
Presubmit check for 11823028-10003 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/chromeos
Hi Steven, I changed this a bit to have better group names for the experiment. Thanks!
That seems better, cheers. LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/semenzato@chromium.org/11823028/10003
Message was sent while issue was closed.
Change committed as 175944 |
