Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10541019:
Adding a field trial for testing --force-compositing-mode on 50% of (Closed)
|
Created:
8 years, 6 months ago by Vangelis Kokkevis Modified:
8 years, 6 months ago CC:
chromium-reviews, jar (doing other things), MAD, Alexei Svitkine (slow), jwd, SteveT Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionAdding a field trial for testing --force-compositing-mode on 50% of
canary users.
BUG=114549
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141164
Patch Set 1 #Patch Set 2 : #
Total comments: 7
Patch Set 3 : #
Total comments: 5
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #Patch Set 6 : #
Total comments: 3
Patch Set 7 : #Patch Set 8 : #
Messages
Total messages: 21 (0 generated)
Ready for review. Ilya, would you mind checking to see if there are any issues with my use of the FieldTrial? John, you get the OWNERS honors. Thanks!
owners check in browser isn't enforced yet (parent dir still has *), so you don't need me
James, would you mind also taking a look at this? Thanks!
The stuff I know about seems fine, but you should have someone familiar with field trials look at it (are we the first people to use this)?
On 2012/06/06 00:52:04, jamesr wrote: > The stuff I know about seems fine, but you should have someone familiar with > field trials look at it (are we the first people to use this)? Thanks for looking at this. Ilya will hopefully check the FieldTrial usage. There are a number of examples of FieldTrial's in the code, mostly initialized in chrome_browser_main.cc . That spot isn't particularly convenient for us as we need to use the result of the trial at the spot where we pass the flags down to the Renderer.
+cc Jim, Finches FieldTrial usage generally lg, though I have a couple of comments/questions inline. I would also recommend that you go ahead and tweak some of the existing histograms to be explicitly dependent on this field trial, a la [1]. You can currently use Dremel to query the FieldTrial's effect on any histogram even without this; but there aren't yet more convenient dashboards that support this without explicitly tagged histograms. [1] http://code.google.com/searchframe#OAMlx_jo-ck/src/base/metrics/field_trial.h... On 2012/06/06 01:00:31, vangelis wrote: > There are a number of examples of FieldTrial's in the code, mostly initialized > in chrome_browser_main.cc . That spot isn't particularly convenient for us as we > need to use the result of the trial at the spot where we pass the flags down to > the Renderer. Why does this make chrome_browser_main.cc inconvenient for initializing the FieldTrial? It should be fine to initialize the field trial in one place, and then query its state in another. The nice thing about initializing the FieldTrial in chrome_browser_main.cc is that the sequencing of events is guaranteed to work out correctly: one time randomization will be set up, then the field trial will be enabled, then any code that depends on the field trial will be able to query its state. The not so nice thing about initializing it in chrome_browser_main.cc is that the file is already ginormous... so as long you're confident that the sequencing will still work correctly when the trial is initialized elsewhere, that should be fine too. https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/brow... File chrome/browser/browser_trial.h (right): https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/brow... chrome/browser/browser_trial.h:23: static const char* kForceCompositingModeTrial; nit: Jim might correct me, but I don't think this file is actually meant to be used as a central repository anymore. If there's another natural place where you would have put this constant had this file not existed, that might be a more appropriate choice. If not, I guess this location should be fine. https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/chro... File chrome/browser/chrome_content_browser_client.cc (right): https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/chro... chrome/browser/chrome_content_browser_client.cc:344: UMA_HISTOGRAM_ENUMERATION("GPU.InForceCompositingModeFieldTrial", nit: UMA_HISTOGRAM_BOOLEAN? https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/gpu_... File chrome/browser/gpu_util.cc (right): https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/gpu_... chrome/browser/gpu_util.cc:212: return false; Can this be a DCHECK rather than an if-stmt? (See comment below for motivation.) https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/gpu_... chrome/browser/gpu_util.cc:213: if (trial->group_name() != std::string("enable")) nit: return trial->group_name() == "enable"; https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/gpu_... chrome/browser/gpu_util.cc:332: InForceCompositingFieldTrial())) Will the field trial always be set up by the time this code is reached? If not, is it ok that this function might return different results depending on when it is called?
lgtm with quick drive by nit :) https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/gpu_... File chrome/browser/gpu_util.cc (right): https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/gpu_... chrome/browser/gpu_util.cc:208: bool InForceCompositingFieldTrial() { nit: Indent is 4 here instead of 2.
https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/brow... File chrome/browser/browser_trial.h (right): https://chromiumcodereview.appspot.com/10541019/diff/6001/chrome/browser/brow... chrome/browser/browser_trial.h:23: static const char* kForceCompositingModeTrial; +1 I had to look back at the history... and this was last used in 2008.... and it should probably be swept away (as it has not had a demonstrable reason to exist since then). Note that this header won't be visible in renderer processes... so it has especially limited (potential) use. Reading the logs, it looks like this was intended to grow into a central area kindred to what Finch is currently chasing after (with UMA remote control over field trials). I think Finch has its own plan... which further deprecates this file's usage :-/. Sorry for the confusion caused by its presence. On 2012/06/06 09:50:21, Ilya Sherman wrote: > nit: Jim might correct me, but I don't think this file is actually meant to be > used as a central repository anymore. If there's another natural place where > you would have put this constant had this file not existed, that might be a more > appropriate choice. If not, I guess this location should be fine.
Thank you for your comments. I reworked the patch to move the FT creation to chrome_browser_main alongside the other trials and reverted changes to browser_trial . As for histograms, the effects of the trial primarily affect code in WebKit and we already have some histograms there to track the performance with and without accelerated compositing. I'll look into adding more once we figure out exactly what else we need to track. PTAL
https://chromiumcodereview.appspot.com/10541019/diff/5002/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10541019/diff/5002/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:1026: // Don't run the trial on windows XP. nit: "windows" -> "Windows" https://chromiumcodereview.appspot.com/10541019/diff/5002/chrome/browser/chro... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10541019/diff/5002/chrome/browser/chro... chrome/browser/chrome_browser_main.h:57: } Do we really need to cache this, rather than querying it from FieldTrialList? I suspect that we shouldn't be adding any new methods to the public interface for ChromeBrowserMainParts...
One more code adjustment. https://chromiumcodereview.appspot.com/10541019/diff/5002/chrome/browser/chro... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10541019/diff/5002/chrome/browser/chro... chrome/browser/chrome_browser_main.cc:1026: // Don't run the trial on windows XP. On 2012/06/06 22:31:51, Ilya Sherman wrote: > nit: "windows" -> "Windows" Done. https://chromiumcodereview.appspot.com/10541019/diff/5002/chrome/browser/chro... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10541019/diff/5002/chrome/browser/chro... chrome/browser/chrome_browser_main.h:57: } On 2012/06/06 22:31:51, Ilya Sherman wrote: > Do we really need to cache this, rather than querying it from FieldTrialList? I > suspect that we shouldn't be adding any new methods to the public interface for > ChromeBrowserMainParts... I removed the public method and replaced it with a global.. This is inline with what some of the other field trials are actually doing. Regarding caching, I only see two options: 1. Cache the group value 2. Expose the name of the trial and the enable group and query every time I need the value. Either way we end up with some global data that needs to be exposed across files. #1 eliminates unnecessary dictionary searches so it seems better. I'm open to other suggestions though.
https://chromiumcodereview.appspot.com/10541019/diff/5002/chrome/browser/chro... File chrome/browser/chrome_browser_main.h (right): https://chromiumcodereview.appspot.com/10541019/diff/5002/chrome/browser/chro... chrome/browser/chrome_browser_main.h:57: } On 2012/06/06 23:44:44, vangelis wrote: > On 2012/06/06 22:31:51, Ilya Sherman wrote: > > Do we really need to cache this, rather than querying it from FieldTrialList? > I > > suspect that we shouldn't be adding any new methods to the public interface > for > > ChromeBrowserMainParts... > > I removed the public method and replaced it with a global.. This is inline with > what some of the other field trials are actually doing. Regarding caching, I > only see two options: > > 1. Cache the group value > 2. Expose the name of the trial and the enable group and query every time I need > the value. > > Either way we end up with some global data that needs to be exposed across > files. #1 eliminates unnecessary dictionary searches so it seems better. I'd prefer #2, as it only requires global /constants/, which are a different animal altogether from global variables. If you strongly prefer a cached global, though, I would at least recommend defining it in a separate header -- I don't think this CL ought to change any publicly exported state in chrome_browser_main.h. > I'm open to other suggestions though.
Initial comments below. https://chromiumcodereview.appspot.com/10541019/diff/13002/chrome/browser/chr... File chrome/browser/chrome_browser_main.cc (right): https://chromiumcodereview.appspot.com/10541019/diff/13002/chrome/browser/chr... chrome/browser/chrome_browser_main.cc:519: bool g_in_force_compositing_mode_trial = false; Rather than using a global, which then puts a need to include the header for this file.... The field_trial.h defines the generic test function: base::FieldTrialList::Find(...) Which can be used to answer the question from anywhere (never a problem with #include dependency issues). *IF* you really find you need to often ask the question, you can certainly cache the result locally, assuming you understand the initialization order. In this case, you're correctly calling for this to be set up before we even go multi-threaded, so you are assured this will be done in advance of most any uses. https://chromiumcodereview.appspot.com/10541019/diff/13002/chrome/browser/chr... chrome/browser/chrome_browser_main.cc:1146: ForceCompositingModeFieldTrial(); nit: I think you should call this a "CompositingMode" field trial. When I read the function name, it looks like "Force" the use of the "CompositingMode" field trial.
Thanks for the comments, folks. I moved the field-trial related code into gpu_utils so that it sits next to other related code. I also removed caching of the field trial group result and do a fresh check every time. PTAL
LGTM. Thanks for your patience on this one :)
LGTM I noticed an indent nit in the CL... and then added a few other nits since I thought the code was a little harder to read (and balance braces) than need be in that area. They are not critical... but would be helpful. https://chromiumcodereview.appspot.com/10541019/diff/6003/chrome/browser/gpu_... File chrome/browser/gpu_util.cc (right): https://chromiumcodereview.appspot.com/10541019/diff/6003/chrome/browser/gpu_... chrome/browser/gpu_util.cc:347: if (kGpuFeatureInfo[i].fallback_to_software) nit: you could have made this an else if on line 347, and then avoided the more complex indents on lines 348-351. https://chromiumcodereview.appspot.com/10541019/diff/6003/chrome/browser/gpu_... chrome/browser/gpu_util.cc:355: gpu_access_blocked) { nit: unwrap this line https://chromiumcodereview.appspot.com/10541019/diff/6003/chrome/browser/gpu_... chrome/browser/gpu_util.cc:374: InForceCompositingModeTrial())) nit: indent: It is hard to say how to fix this up.... but 373 and 374 shouldn't be aligned IMO. Better might be to indent 373 by 4 more spaces (two levels of parens were entered). These lines are hard to read, and would probably be better broken up: if (kGpuFeatureInfo[i].name == "compositing") { more ifs; } else if (kGpuFeatureInfo[i].name == "css_animation") { // line 379 more ifs; } It was also a little surprising that you checked for equality with "composting" in two of these clauses (see line 376) which almost made me wonder if there was a typo.
Thank you all. I tried to improve the readability of the gpu_utils code a bit as per Jim's comments. I'm running it through the try servers once and I'll submit.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vangelis@chromium.org/10541019/11
Try job failure for 10541019-11 (retry) on mac_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vangelis@chromium.org/10541019/9005
Change committed as 141164 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
