Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2220643002:
Initialize the TaskScheduler with Variation Parameters if Available (Closed)
DescriptionInitialize the TaskScheduler with Variation Parameters if Available
This connects the initialization of TaskScheduler to the availability
of an appropriate finch config.
Support via a command line and flags will be available in a forthcoming
CL.
BUG=553459
Committed: https://crrev.com/7af523f24d4487d4c58d439f29a956cbee7ebbcf
Cr-Commit-Position: refs/heads/master@{#410786}
Patch Set 1 #
Total comments: 24
Patch Set 2 : CR Feedback #
Total comments: 4
Patch Set 3 : Grouped All Predefined Parameters Together #
Total comments: 5
Patch Set 4 : CR Feedback #
Total comments: 12
Patch Set 5 : CR Feedback #
Total comments: 15
Patch Set 6 : CR Feedback #
Total comments: 2
Patch Set 7 : Add Const #Messages
Total messages: 57 (23 generated)
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:20: #include "base/command_line.h" #include "base/logging.h" https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:325: enum WorkerPoolType : size_t { enum class? https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:338: int threads; struct WorkerPoolVariationValues { int threads = 0; int64_t detach_milliseconds = 0; }; https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:342: // Converts |pool_descriptor| to a WorkerPoolVariationValues. Returns a default |pool_descriptor| is a string that follows the format "minimum number of threads,maximum number of threads,..." https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:343: // WorkerPoolVariationValues on failure. No need to document what the function does after NOTREACHED(); https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:346: std::vector<std::string> tokens = SplitString(pool_descriptor, ",", const https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:363: int threads = std::round<int>(num_of_cores * multiplier) + offset; const num_of_cores const threads https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:390: static const char kFieldTrialName[] = "BrowserScheduler"; constexpr https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:396: static const char* kWorkerPoolNames[] = { constexpr https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:411: NOTREACHED() << "Missing Worker Pool Configuration: " << current; https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Should a bad configuration from the server be handled gracefully (remove the NOTREACHED)? https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:421: " [" << pair->second << "]"; ditto https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:428: DCHECK_EQ(values.size(), WorkerPoolType::WORKER_POOL_COUNT); overkill DCHECK? I don't suggest that you remove it, I'm just curious about the rules that decide when a DCHECK is appropriate.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Comments applied. Thanks! https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:20: #include "base/command_line.h" On 2016/08/05 20:58:54, fdoray wrote: > #include "base/logging.h" Done. https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:325: enum WorkerPoolType : size_t { On 2016/08/05 20:58:54, fdoray wrote: > enum class? I kept this as an enum for the similar reason we did in the tests: It didn't seem worth it to throw static_casts everywhere since we're really using the underlying value of this enum. https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:338: int threads; On 2016/08/05 20:58:53, fdoray wrote: > struct WorkerPoolVariationValues { > int threads = 0; > int64_t detach_milliseconds = 0; > }; Done. I kept forgetting about inline initialization in C++! https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:342: // Converts |pool_descriptor| to a WorkerPoolVariationValues. Returns a default On 2016/08/05 20:58:53, fdoray wrote: > |pool_descriptor| is a string that follows the format "minimum number of > threads,maximum number of threads,..." Done. https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:343: // WorkerPoolVariationValues on failure. On 2016/08/05 20:58:54, fdoray wrote: > No need to document what the function does after NOTREACHED(); See below on the NOTREACHED discussion. Changed to a LOG(WARNING) to be resilient to failure. https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:346: std::vector<std::string> tokens = SplitString(pool_descriptor, ",", On 2016/08/05 20:58:53, fdoray wrote: > const Done. https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:363: int threads = std::round<int>(num_of_cores * multiplier) + offset; On 2016/08/05 20:58:54, fdoray wrote: > const num_of_cores > const threads Done. https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:390: static const char kFieldTrialName[] = "BrowserScheduler"; On 2016/08/05 20:58:54, fdoray wrote: > constexpr Done. https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:396: static const char* kWorkerPoolNames[] = { On 2016/08/05 20:58:54, fdoray wrote: > constexpr This triggers a compiler bug, so it's going to have to stay this way for now: http://stackoverflow.com/questions/35956033/vs-2015-internal-compiler-error-f... https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:411: NOTREACHED() << "Missing Worker Pool Configuration: " << current; On 2016/08/05 20:58:54, fdoray wrote: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > Should a bad configuration from the server be handled gracefully (remove the > NOTREACHED)? I guess these are coming in from the land of network so anything can happen. It would be an unfortunate DoS! Changed these to LOG(WARNING) along with the StringToWorkerPoolVariationValues above. https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:421: " [" << pair->second << "]"; On 2016/08/05 20:58:53, fdoray wrote: > ditto See above. https://codereview.chromium.org/2220643002/diff/1/chrome/browser/chrome_brows... chrome/browser/chrome_browser_main.cc:428: DCHECK_EQ(values.size(), WorkerPoolType::WORKER_POOL_COUNT); On 2016/08/05 20:58:54, fdoray wrote: > overkill DCHECK? > I don't suggest that you remove it, I'm just curious about the rules that decide > when a DCHECK is appropriate. I guess it's not intrinsic that the values.size() should equal WORKER_POOL_COUNT. If someone introduced a break in the for-loop rather than a return, then this DCHECK would trigger.
A suggestion. Code l-g-t-m if you don't like my suggestion. https://codereview.chromium.org/2220643002/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:408: static_assert(arraysize(kWorkerPoolNames) == WORKER_POOL_COUNT, What do you think of this? struct SchedulerWorkerPoolPredefinedParams { const char* name, ThreadPriority priority_hint, IORestriction io_restriction }; std::vector<SchedulerWorkerPoolPredefinedParams> predefined_params_vector; DCHECK_EQ(predefined_params_vector.size(), BACKGROUND_WORKER_POOL); predefined_params_vector.emplace_back( "Background", ThreadPriority::BACKGROUND, IORestriction::DISALLOWED); ... for (const auto& predefined_params : predefined_params_vector) { // Get extra params from |variation_params|. // Add SchedulerWorkerPoolParams to |params_vector|. } I like that the name and the priority hint/io restriction are together + I'm pretty sure that it will require less lines of code. https://codereview.chromium.org/2220643002/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:412: for (size_t i = 0; i < arraysize(kWorkerPoolNames); i++) { for (const char* worker_pool_name : kWorkerPoolNames)
https://codereview.chromium.org/2220643002/diff/60001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:408: static_assert(arraysize(kWorkerPoolNames) == WORKER_POOL_COUNT, On 2016/08/05 22:59:12, fdoray wrote: > What do you think of this? > > struct SchedulerWorkerPoolPredefinedParams { > const char* name, > ThreadPriority priority_hint, > IORestriction io_restriction > }; > > std::vector<SchedulerWorkerPoolPredefinedParams> predefined_params_vector; > > DCHECK_EQ(predefined_params_vector.size(), BACKGROUND_WORKER_POOL); > predefined_params_vector.emplace_back( > "Background", > ThreadPriority::BACKGROUND, > IORestriction::DISALLOWED); > ... > > for (const auto& predefined_params : predefined_params_vector) { > // Get extra params from |variation_params|. > // Add SchedulerWorkerPoolParams to |params_vector|. > } > > I like that the name and the priority hint/io restriction are together + I'm > pretty sure that it will require less lines of code. Went one further and just used a static const struct array. https://codereview.chromium.org/2220643002/diff/60001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:412: for (size_t i = 0; i < arraysize(kWorkerPoolNames); i++) { On 2016/08/05 22:59:12, fdoray wrote: > for (const char* worker_pool_name : kWorkerPoolNames) Done.
lgtm w/ comments https://codereview.chromium.org/2220643002/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:350: const base::StringPiece& pool_descriptor) { Prefer passing StringPiece by value https://cs.chromium.org/chromium/src/base/strings/string_piece.h?q=stringpiec... https://codereview.chromium.org/2220643002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:408: static const SchedulerWorkerPoolPredefinedParams kAllPredefinedParams[] = { static const struct { const char* name; ThreadPriority priority_hint; IORestriction io_restriction; } kAllPredefinedParams[] = { ... };
https://codereview.chromium.org/2220643002/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:350: const base::StringPiece& pool_descriptor) { On 2016/08/06 00:39:55, fdoray wrote: > Prefer passing StringPiece by value > https://cs.chromium.org/chromium/src/base/strings/string_piece.h?q=stringpiec... Done. https://codereview.chromium.org/2220643002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:408: static const SchedulerWorkerPoolPredefinedParams kAllPredefinedParams[] = { On 2016/08/06 00:39:55, fdoray wrote: > static const struct { > const char* name; > ThreadPriority priority_hint; > IORestriction io_restriction; > } kAllPredefinedParams[] = { > ... > }; Every time I've done this, the reviewer has suggested a split since you can see the definition and declaration separately. I'll keep this as is.
lgtm https://codereview.chromium.org/2220643002/diff/80001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/80001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:408: static const SchedulerWorkerPoolPredefinedParams kAllPredefinedParams[] = { On 2016/08/06 01:19:46, robliao wrote: > On 2016/08/06 00:39:55, fdoray wrote: > > static const struct { > > const char* name; > > ThreadPriority priority_hint; > > IORestriction io_restriction; > > } kAllPredefinedParams[] = { > > ... > > }; > > Every time I've done this, the reviewer has suggested a split since you can see > the definition and declaration separately. I'll keep this as is. Acknowledged.
lgtm w/ comments and I'd change CL description/title to: "Initialize the TaskScheduler with variation parameters if available." (i.e. the condition is on the variations parameters' presence) Also, mention in description what is expected to follow-up from this CL. As-is to test this locally you need a Finch config (or an *extremely long* command-line). Essentially I'm wondering what your plan is for about://flags and to enable local testing. https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:368: const int threads = std::round<int>(num_of_cores * multiplier) + offset; std::ceil? So that say a multiplier below 1 can't cause 0 on single core machines? Or is that the purpose of offset? https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:370: values.threads = std::min(maximum, std::max(minimum, threads)); std::clamp https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:421: LOG(WARNING) << "Missing Worker Pool Configuration: " << I'd say change those from WARNING to DFATAL so that developers notice if this happens under normal circumstances and users get ERROR logs if it happens in the wild.
Description was changed from ========== Conditionally Initialize the Task Scheduler With Variation Parameters BUG=553459 ========== to ========== Initialize the TaskScheduler with Variation Parameters if Available This connects the initialization of TaskScheduler to the availability of an appropriate finch config. Support via a command line and flags will be available in a forthcoming CL. BUG=553459 ==========
https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:368: const int threads = std::round<int>(num_of_cores * multiplier) + offset; On 2016/08/06 21:25:50, gab wrote: > std::ceil? So that say a multiplier below 1 can't cause 0 on single core > machines? Or is that the purpose of offset? std::ceil is indeed better. The offset allows us some flexibility like we want 5 threads + a scaled amount. https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:370: values.threads = std::min(maximum, std::max(minimum, threads)); On 2016/08/06 21:25:50, gab wrote: > std::clamp std::clamp isn't availble until C++17. https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:421: LOG(WARNING) << "Missing Worker Pool Configuration: " << On 2016/08/06 21:25:50, gab wrote: > I'd say change those from WARNING to DFATAL so that developers notice if this > happens under normal circumstances and users get ERROR logs if it happens in the > wild. We had these as DCHECKs before. What's the advantage of logging to DFATAL over DCHECK? It still breaks in the debugger.
> lgtm w/ comments and I'd change CL description/title to: > > "Initialize the TaskScheduler with variation parameters if available." > > (i.e. the condition is on the variations parameters' presence) > > Also, mention in description what is expected to follow-up from this CL. As-is > to test this locally you need a Finch config (or an *extremely long* > command-line). > > Essentially I'm wondering what your plan is for about://flags and to enable > local testing. Done. Because there are a lot of config parameters, we're going to need to expose some prebaked command line flags for simplicity. Chrome flags has the capability to allow a single set of options within the flags interface. We just need to determine what set we want.
PS: We don't typically capitalize CL titles AFAIK (it reads weird IMO). https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:368: const int threads = std::round<int>(num_of_cores * multiplier) + offset; On 2016/08/08 17:26:25, robliao wrote: > On 2016/08/06 21:25:50, gab wrote: > > std::ceil? So that say a multiplier below 1 can't cause 0 on single core > > machines? Or is that the purpose of offset? > > std::ceil is indeed better. The offset allows us some flexibility like we want 5 > threads + a scaled amount. Missing PS upload? https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:370: values.threads = std::min(maximum, std::max(minimum, threads)); On 2016/08/08 17:26:25, robliao wrote: > On 2016/08/06 21:25:50, gab wrote: > > std::clamp > > std::clamp isn't availble until C++17. Ah :-), figured there had to be a std::algorithm for that, found it, but hadn't realized it was C++17..! https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:421: LOG(WARNING) << "Missing Worker Pool Configuration: " << On 2016/08/08 17:26:25, robliao wrote: > On 2016/08/06 21:25:50, gab wrote: > > I'd say change those from WARNING to DFATAL so that developers notice if this > > happens under normal circumstances and users get ERROR logs if it happens in > the > > wild. > > We had these as DCHECKs before. What's the advantage of logging to DFATAL over > DCHECK? It still breaks in the debugger. Thought you wanted to use LOG so that we would also generate non-Debug logs (i.e. not DLOG). Now that I think about it non-debug logs are usually frowned upon (strings in Release builds). I realize now that DCHECK is perhaps a bit too strong (i.e. don't want to crash every developer's build if we make a typo in config). How about DLOG(ERROR) then? Would be mostly for our sake.
Patchset #5 (id:120001) has been deleted
> PS: We don't typically capitalize CL titles AFAIK (it reads weird IMO). I've generally been doing this since the first line is treated as a title in SCMs and CR tools :-). https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:368: const int threads = std::round<int>(num_of_cores * multiplier) + offset; On 2016/08/08 17:42:09, gab wrote: > On 2016/08/08 17:26:25, robliao wrote: > > On 2016/08/06 21:25:50, gab wrote: > > > std::ceil? So that say a multiplier below 1 can't cause 0 on single core > > > machines? Or is that the purpose of offset? > > > > std::ceil is indeed better. The offset allows us some flexibility like we want > 5 > > threads + a scaled amount. > > Missing PS upload? Indeed! I must have dreamed that I uploaded it and saw the diff. https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:370: values.threads = std::min(maximum, std::max(minimum, threads)); On 2016/08/08 17:42:09, gab wrote: > On 2016/08/08 17:26:25, robliao wrote: > > On 2016/08/06 21:25:50, gab wrote: > > > std::clamp > > > > std::clamp isn't availble until C++17. > > Ah :-), figured there had to be a std::algorithm for that, found it, but hadn't > realized it was C++17..! I wanted to use it on an earlier changelist and found out the hard way :-) https://codereview.chromium.org/2220643002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:421: LOG(WARNING) << "Missing Worker Pool Configuration: " << On 2016/08/08 17:42:09, gab wrote: > On 2016/08/08 17:26:25, robliao wrote: > > On 2016/08/06 21:25:50, gab wrote: > > > I'd say change those from WARNING to DFATAL so that developers notice if > this > > > happens under normal circumstances and users get ERROR logs if it happens in > > the > > > wild. > > > > We had these as DCHECKs before. What's the advantage of logging to DFATAL over > > DCHECK? It still breaks in the debugger. > > Thought you wanted to use LOG so that we would also generate non-Debug logs > (i.e. not DLOG). > > Now that I think about it non-debug logs are usually frowned upon (strings in > Release builds). > > I realize now that DCHECK is perhaps a bit too strong (i.e. don't want to crash > every developer's build if we make a typo in config). > > How about DLOG(ERROR) then? Would be mostly for our sake. Done.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
robliao@chromium.org changed reviewers: + thestig@chromium.org
thestig@chromium.org: Please review changes in chrome_browser_main.cc. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Is there any place outside of chrome/browser/chrome_browser_main.cc that would make sense for this code to live? I would prefer if chrome_browser_main.cc does not keep growing. It's already at ~2000 lines. BTW, it would be nice if the CL description is wrapped at 72 columns, since that's what goes into the git commit and thus what others see in git log. gab: ^ Same for your CLs.
Description was changed from ========== Initialize the TaskScheduler with Variation Parameters if Available This connects the initialization of TaskScheduler to the availability of an appropriate finch config. Support via a command line and flags will be available in a forthcoming CL. BUG=553459 ========== to ========== Initialize the TaskScheduler with Variation Parameters if Available This connects the initialization of TaskScheduler to the availability of an appropriate finch config. Support via a command line and flags will be available in a forthcoming CL. BUG=553459 ==========
On 2016/08/08 23:24:58, Lei Zhang wrote: > Is there any place outside of chrome/browser/chrome_browser_main.cc that would > make sense for this code to live? I would prefer if chrome_browser_main.cc does > not keep growing. It's already at ~2000 lines. > > BTW, it would be nice if the CL description is wrapped at 72 columns, since > that's what goes into the git commit and thus what others see in git log. > > gab: ^ Same for your CLs. Adjusted the linebreaks for the CL description. On the location, I think this is the best place. We need to be initialized as soon as the field-trials and variations are ready and before anyone posts any tasks. Technically that's during field-trial initialization but before field-trial posts its task to get the seed. We'll be looking into splitting this up in a later CL.
https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:331: WORKER_POOL_COUNT, subtle nit: Usually we leave a comma at the end so we can do: FOO, BAR, +QUX, instead of: FOO, -BAR +BAR, +QUX but since the COUNT is always the last item, you can leave off the trailing comma, and perhaps put in its place: // Always last. https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:368: const int threads = std::ceil<int>(num_of_cores * multiplier) + offset; Who controls the input? Do we care if we integer overflow? https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:375: DLOG(ERROR) << "Invalid Worker Pool Descriptor: " << pool_descriptor; Are the DLOG(ERROR)s reachable? Just wondering if they should be NOTREACHED() instead - in which case they would crash a build with DCHECKs, instead of just outputting an error. https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:388: return traits.priority() == base::TaskPriority::BACKGROUND Does it feel better if we add: bool is_background = traits.priority() == base::TaskPriority::BACKGROUND; instead of explicitly checking it twice? https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:419: const auto pair = variation_params.find(predefined_params.name); Isn't |pair| really an iterator? Variable name is slightly misleading. https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:421: DLOG(ERROR) << "Missing Worker Pool Configuration: " << I bet "git cl format" will rewrite this as: LOG(ERROR) << FOO << BAR; https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:443: DCHECK_EQ(params_vector.size(), WORKER_POOL_COUNT); I like to write these in the same order as EXPECT_EQ(expected, actual), so the arguments should be flipped around.
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:331: WORKER_POOL_COUNT, On 2016/08/08 23:37:44, Lei Zhang wrote: > subtle nit: Usually we leave a comma at the end so we can do: > > FOO, > BAR, > +QUX, > > instead of: > > FOO, > -BAR > +BAR, > +QUX > > but since the COUNT is always the last item, you can leave off the trailing > comma, and perhaps put in its place: // Always last. Agreed. Done. https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:368: const int threads = std::ceil<int>(num_of_cores * multiplier) + offset; On 2016/08/08 23:37:44, Lei Zhang wrote: > Who controls the input? Do we care if we integer overflow? Finch controls the input, and that comes from network. If the integer overflows, then things could go negative. I've changed the validity check below to make sure the thread count is always greater than 1. https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:375: DLOG(ERROR) << "Invalid Worker Pool Descriptor: " << pool_descriptor; On 2016/08/08 23:37:44, Lei Zhang wrote: > Are the DLOG(ERROR)s reachable? Just wondering if they should be NOTREACHED() > instead - in which case they would crash a build with DCHECKs, instead of just > outputting an error. These were originally NOTREACHED(), but since these values are coming from network, it seemed safer to just log errors and fail creating the task scheduler. https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:388: return traits.priority() == base::TaskPriority::BACKGROUND On 2016/08/08 23:37:44, Lei Zhang wrote: > Does it feel better if we add: bool is_background = traits.priority() == > base::TaskPriority::BACKGROUND; instead of explicitly checking it twice? Makes wrapping better. Change applied. https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:419: const auto pair = variation_params.find(predefined_params.name); On 2016/08/08 23:37:44, Lei Zhang wrote: > Isn't |pair| really an iterator? Variable name is slightly misleading. The iterator points at a value type which is an alias of pair<const key_type, mapped_type). https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:421: DLOG(ERROR) << "Missing Worker Pool Configuration: " << On 2016/08/08 23:37:44, Lei Zhang wrote: > I bet "git cl format" will rewrite this as: > LOG(ERROR) << FOO > << BAR; Indeed. It's oddly inconsistent with operators at the end of lines. https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:443: DCHECK_EQ(params_vector.size(), WORKER_POOL_COUNT); On 2016/08/08 23:37:44, Lei Zhang wrote: > I like to write these in the same order as EXPECT_EQ(expected, actual), so the > arguments should be flipped around. Done.
lgtm https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/140001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:368: const int threads = std::ceil<int>(num_of_cores * multiplier) + offset; On 2016/08/09 00:20:44, robliao wrote: > On 2016/08/08 23:37:44, Lei Zhang wrote: > > Who controls the input? Do we care if we integer overflow? > > Finch controls the input, and that comes from network. If the integer overflows, > then things could go negative. I've changed the validity check below to make > sure the thread count is always greater than 1. Ok. I imagine Finch won't give bad input, but UBSan + ClusterFuzz may complain.
Still lgtm w/ micro-nit https://codereview.chromium.org/2220643002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:383: bool is_background = traits.priority() == base::TaskPriority::BACKGROUND; const ?
On 2016/08/08 23:24:58, Lei Zhang wrote: > Is there any place outside of chrome/browser/chrome_browser_main.cc that would > make sense for this code to live? I would prefer if chrome_browser_main.cc does > not keep growing. It's already at ~2000 lines. > > BTW, it would be nice if the CL description is wrapped at 72 columns, since > that's what goes into the git commit and thus what others see in git log. > > gab: ^ Same for your CLs. I keep hearing this but I don't quite understand, when I do git log in my terminal (cmd.exe on Windows) I see the full lines (wrapped to terminal's column size), e.g. I see all characters in say http://crrev.com/a77902b8a9ed16436143e6da5025b6ddd8bea643
On 2016/08/09 15:56:57, gab wrote: > I keep hearing this but I don't quite understand, when I do git log in my > terminal (cmd.exe on Windows) I see the full lines (wrapped to terminal's column > size), e.g. I see all characters in say > http://crrev.com/a77902b8a9ed16436143e6da5025b6ddd8bea643 Yes, we see all the characters, bu t they look cut off like this, whi chi is annoying to some. I understand bots do it and sometimes URLs are long, but it would be nice to do 72 columns for commit messages.
https://codereview.chromium.org/2220643002/diff/200001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2220643002/diff/200001/chrome/browser/chrome_... chrome/browser/chrome_browser_main.cc:383: bool is_background = traits.priority() == base::TaskPriority::BACKGROUND; On 2016/08/09 15:54:56, gab wrote: > const ? Done.
The CQ bit was checked by robliao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/09 18:54:09, Lei Zhang wrote: > On 2016/08/09 15:56:57, gab wrote: > > I keep hearing this but I don't quite understand, when I do git log in my > > terminal (cmd.exe on Windows) I see the full lines (wrapped to terminal's > column > > size), e.g. I see all characters in say > > http://crrev.com/a77902b8a9ed16436143e6da5025b6ddd8bea643 > > Yes, we see all the characters, bu > t they look cut off like this, whi > chi is annoying to some. In my terminal they only wrap at terminal width, not at 72 chars..? And even if it does that in other terminals, how often does one really need to have high readability from git log..? If I need details I just hit the codereview link anyways. So I'm not convinced it's worth optimizing that use case. On the flip side, putting the burden on all devs to manually format all CL description to 72 characters is a definitive pain IMO. If we want this to be the case, smart wrapping should be done by the tool landing the CL, not by devs. Today the only "tooling" help for devs on this AFAIK is a comment highlighting the visual length of 72 chars during the first git cl upload for a CL... > > I understand bots do it and sometimes URLs are long, but it would be nice to do > 72 columns for commit messages.
On 2016/08/09 19:14:37, robliao wrote: > https://codereview.chromium.org/2220643002/diff/200001/chrome/browser/chrome_... > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/2220643002/diff/200001/chrome/browser/chrome_... > chrome/browser/chrome_browser_main.cc:383: bool is_background = > traits.priority() == base::TaskPriority::BACKGROUND; > On 2016/08/09 15:54:56, gab wrote: > > const ? > > Done. lgtm++
On 2016/08/09 19:16:12, gab wrote: > On 2016/08/09 18:54:09, Lei Zhang wrote: > > On 2016/08/09 15:56:57, gab wrote: > > > I keep hearing this but I don't quite understand, when I do git log in my > > > terminal (cmd.exe on Windows) I see the full lines (wrapped to terminal's > > column > > > size), e.g. I see all characters in say > > > http://crrev.com/a77902b8a9ed16436143e6da5025b6ddd8bea643 > > > > Yes, we see all the characters, bu > > t they look cut off like this, whi > > chi is annoying to some. > > In my terminal they only wrap at terminal width, not at 72 chars..? > > And even if it does that in other terminals, how often does one really need to > have high readability from git log..? If I need details I just hit the > codereview link anyways. > > So I'm not convinced it's worth optimizing that use case. On the flip side, > putting the burden on all devs to manually format all CL description to 72 > characters is a definitive pain IMO. If we want this to be the case, smart > wrapping should be done by the tool landing the CL, not by devs. Today the only > "tooling" help for devs on this AFAIK is a comment highlighting the visual > length of 72 chars during the first git cl upload for a CL... > > > > > I understand bots do it and sometimes URLs are long, but it would be nice to > do > > 72 columns for commit messages. My terminal shows the full lengths and gitiles does as well. Maybe the bots should be adjusted to do the same? I suspect more people target 80 (I do for longer commit messages).
On 2016/08/09 19:16:12, gab wrote: > On 2016/08/09 18:54:09, Lei Zhang wrote: > > On 2016/08/09 15:56:57, gab wrote: > > > I keep hearing this but I don't quite understand, when I do git log in my > > > terminal (cmd.exe on Windows) I see the full lines (wrapped to terminal's > > column > > > size), e.g. I see all characters in say > > > http://crrev.com/a77902b8a9ed16436143e6da5025b6ddd8bea643 > > > > Yes, we see all the characters, bu > > t they look cut off like this, whi > > chi is annoying to some. > > In my terminal they only wrap at terminal width, not at 72 chars..? > > And even if it does that in other terminals, how often does one really need to > have high readability from git log..? If I need details I just hit the > codereview link anyways. > > So I'm not convinced it's worth optimizing that use case. On the flip side, > putting the burden on all devs to manually format all CL description to 72 > characters is a definitive pain IMO. If we want this to be the case, smart > wrapping should be done by the tool landing the CL, not by devs. Today the only > "tooling" help for devs on this AFAIK is a comment highlighting the visual > length of 72 chars during the first git cl upload for a CL... > > > > > I understand bots do it and sometimes URLs are long, but it would be nice to > do > > 72 columns for commit messages. My terminal shows the full lengths and gitiles does as well. Maybe the bots should be adjusted to do the same? I suspect more people target 80 (I do for longer commit messages).
On 2016/08/09 19:16:12, gab wrote: > On 2016/08/09 18:54:09, Lei Zhang wrote: > > On 2016/08/09 15:56:57, gab wrote: > > > I keep hearing this but I don't quite understand, when I do git log in my > > > terminal (cmd.exe on Windows) I see the full lines (wrapped to terminal's > > column > > > size), e.g. I see all characters in say > > > http://crrev.com/a77902b8a9ed16436143e6da5025b6ddd8bea643 > > > > Yes, we see all the characters, bu > > t they look cut off like this, whi > > chi is annoying to some. > > In my terminal they only wrap at terminal width, not at 72 chars..? git log indents 8 spaces, so for the commit message to not wrap as viewed via git log in a standard 80 col terminal, it should be 72 chars. > And even if it does that in other terminals, how often does one really need to > have high readability from git log..? If I need details I just hit the > codereview link anyways. > > So I'm not convinced it's worth optimizing that use case. On the flip side, > putting the burden on all devs to manually format all CL description to 72 > characters is a definitive pain IMO. If we want this to be the case, smart > wrapping should be done by the tool landing the CL, not by devs. Today the only > "tooling" help for devs on this AFAIK is a comment highlighting the visual > length of 72 chars during the first git cl upload for a CL... Not sure how you are writing commit messages, but on Linux, when I git commit, vi is already set up to auto-wrap at 72 cols. So when I git cl upload, it reuses the git commit message, which is already nicely wrapped. If your workflow can be easily adjusted to auto-wrapper better, that would be helpful for those who like to read git log output. I agree this is a tooling problem, and that's why I say to everyone "it would be nice" and not something firmer. So if it is too much of a pain, then don't stress out and carry on.
On 2016/08/09 15:56:57, gab wrote: > I keep hearing this but I don't quite understand, when I do git log in my > terminal (cmd.exe on Windows) I see the full lines (wrapped to terminal's column > size), e.g. I see all characters in say > http://crrev.com/a77902b8a9ed16436143e6da5025b6ddd8bea643 BTW, I hope it's not just me who repeatedly tell you this. :) The reason you keep hearing it is that it's a git commit style that some go by. Obviously it's not enforced for Chromium, but some like it that way. If you search for git commit style, you'll see lots of recommendations to wrap at 72.
On 2016/08/09 19:27:32, Lei Zhang wrote: > On 2016/08/09 19:16:12, gab wrote: > > On 2016/08/09 18:54:09, Lei Zhang wrote: > > > On 2016/08/09 15:56:57, gab wrote: > > > > I keep hearing this but I don't quite understand, when I do git log in my > > > > terminal (cmd.exe on Windows) I see the full lines (wrapped to terminal's > > > column > > > > size), e.g. I see all characters in say > > > > http://crrev.com/a77902b8a9ed16436143e6da5025b6ddd8bea643 > > > > > > Yes, we see all the characters, bu > > > t they look cut off like this, whi > > > chi is annoying to some. > > > > In my terminal they only wrap at terminal width, not at 72 chars..? > > git log indents 8 spaces, so for the commit message to not wrap as viewed via > git log in a standard 80 col terminal, it should be 72 chars. Ah I see, so it's not that "git commits that are wrapped at 72 chars", it's just that git adds 8 spaces and that it thus wraps at 72 in 80 cols terminals (my terminal is much wider because most tools -- e.g. git grep -- don't attempt to be nice about 80 cols...). > > > And even if it does that in other terminals, how often does one really need to > > have high readability from git log..? If I need details I just hit the > > codereview link anyways. > > > > So I'm not convinced it's worth optimizing that use case. On the flip side, > > putting the burden on all devs to manually format all CL description to 72 > > characters is a definitive pain IMO. If we want this to be the case, smart > > wrapping should be done by the tool landing the CL, not by devs. Today the > only > > "tooling" help for devs on this AFAIK is a comment highlighting the visual > > length of 72 chars during the first git cl upload for a CL... > > Not sure how you are writing commit messages, but on Linux, when I git commit, > vi is already set up to auto-wrap at 72 cols. So when I git cl upload, it reuses > the git commit message, which is already nicely wrapped. If your workflow can be > easily adjusted to auto-wrapper better, that would be helpful for those who like > to read git log output. When I git commit / git cl upload, I get a vi prompt (depot_tools\git-2.8.3-64_bin\usr\bin\vim.exe), but it's not my main editor and thus isn't configured to do anything fancy like this. > > I agree this is a tooling problem, and that's why I say to everyone "it would be > nice" and not something firmer. So if it is too much of a pain, then don't > stress out and carry on. I try to keep wrapping to a "reasonable" width, but without tooling I'm not going to measure 72 chars (especially since git's vim on Windows is lame so I tend to just upload with a title and then Edit Issue to write the full CL description from Rietveld).
On 2016/08/09 19:35:47, Lei Zhang wrote: > On 2016/08/09 15:56:57, gab wrote: > > I keep hearing this but I don't quite understand, when I do git log in my > > terminal (cmd.exe on Windows) I see the full lines (wrapped to terminal's > column > > size), e.g. I see all characters in say > > http://crrev.com/a77902b8a9ed16436143e6da5025b6ddd8bea643 > > BTW, I hope it's not just me who repeatedly tell you this. :) > > The reason you keep hearing it is that it's a git commit style that some go by. > Obviously it's not enforced for Chromium, but some like it that way. If you > search for git commit style, you'll see lots of recommendations to wrap at 72. I don't recall exactly who (you and Nico maybe?)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2220643002/#ps220001 (title: "Add Const")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Initialize the TaskScheduler with Variation Parameters if Available This connects the initialization of TaskScheduler to the availability of an appropriate finch config. Support via a command line and flags will be available in a forthcoming CL. BUG=553459 ========== to ========== Initialize the TaskScheduler with Variation Parameters if Available This connects the initialization of TaskScheduler to the availability of an appropriate finch config. Support via a command line and flags will be available in a forthcoming CL. BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Initialize the TaskScheduler with Variation Parameters if Available This connects the initialization of TaskScheduler to the availability of an appropriate finch config. Support via a command line and flags will be available in a forthcoming CL. BUG=553459 ========== to ========== Initialize the TaskScheduler with Variation Parameters if Available This connects the initialization of TaskScheduler to the availability of an appropriate finch config. Support via a command line and flags will be available in a forthcoming CL. BUG=553459 Committed: https://crrev.com/7af523f24d4487d4c58d439f29a956cbee7ebbcf Cr-Commit-Position: refs/heads/master@{#410786} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7af523f24d4487d4c58d439f29a956cbee7ebbcf Cr-Commit-Position: refs/heads/master@{#410786} |
