Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1603063005:
Adding support for parsing extended gpu config parameters in nanobench. (Closed)
|
Created:
4 years, 11 months ago by Sami Väisänen Modified:
4 years, 10 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdding support for parsing extended gpu config parameters in nanobench
BUG=skia:2992
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1603063005
Committed: https://skia.googlesource.com/skia/+/c47635e0ed52970fa40a16ffb280071e6b838a52
Patch Set 1 #
Total comments: 42
Patch Set 2 : Adding support for parsing gpu config parameters in nanobench. #
Total comments: 4
Patch Set 3 : Adding support for parsing gpu config parameters in nanobench #Patch Set 4 : Adding support for parsing gpu config parameters in nanobench #Messages
Total messages: 23 (10 generated)
Description was changed from ========== Adding support for parsing extended gpu config parameters in nanobench. BUG=skia: ========== to ========== Adding support for parsing extended gpu config parameters in nanobench. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Adding support for parsing extended gpu config parameters in nanobench. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Adding support for parsing extended gpu config parameters in nanobench. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
svaisanen@nvidia.com changed reviewers: + kkinnunen@nvidia.com
Bear with me here :) I'm not a proper Skia reviewer and some of the rules are not skia rules. Still, I think they're good to follow. Take a look at the suggestions. Alternatively, you can structure code so that you don't have that create_gpu_config, rather you move the if -statement and the payload of that function as the first item in create_config(..) function. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp File bench/nanobench.cpp (left): https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#oldcode417 bench/nanobench.cpp:417: if (is_cpu_config_allowed(#name)) { \ here you would have if (is_cpu_config_allowed(#name, https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode383 bench/nanobench.cpp:383: static bool is_cpu_config_allowed(const char* name) { this would be static bool is_cpu_config_allowed(const char* name, const SkCommandLineConfig* config) https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode393 bench/nanobench.cpp:393: static bool is_gpu_config_allowed(const char* name, GrContextFactory::GLContextType ctxType, likewise here. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode414 bench/nanobench.cpp:414: // create a config from a specific 'gpu(...)' parameter pack. This comment does not add any information, if you change the name of the function, and so you can remove the comment. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode415 bench/nanobench.cpp:415: static void create_commandline_config(SkTArray<Config, false>& nanoConfigs, const SkCommandLineConfig& conf) { So this function could be called static void create_gpu_config(const SkCommandLineGpuConfig* config, SkTArray<Config>* configs) * the create_commandline_config is too vague for what the code actually does, especially with other related functions being present * don't use abbreviation conf. You save 2 letters. * don't use arbitrary, unrelated prefix nano * out parameters go last * out parameters as pointers https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode417 bench/nanobench.cpp:417: return; this would be removed. Instead, you actually need to guard all GPU-related lines with #if SK_SUPPORT_GPU ... #endif and you need to test the compile with: SKIA_OUT=out-no-gpu GYP_DEFINES=skia_gpu=0 ./gyp_skia && ninja -C out-no-gpu/Debug most https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode420 bench/nanobench.cpp:420: const auto* gpuConf = conf.asConfigGpu(); This would go to the parent function. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode427 bench/nanobench.cpp:427: if (tag.isEmpty()) This is wrong. The intention is that you check conf.asConfigGpu(). https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode429 bench/nanobench.cpp:429: you would need to call also the is_gpu_config_allowed. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode430 bench/nanobench.cpp:430: Config target; So here you handle all GPU related configs. Thus you need to remove the redundant impl in (1). https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode431 bench/nanobench.cpp:431: target.name = tag; Since the original used object initializer syntax, you could stay with that if possible. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode446 bench/nanobench.cpp:446: // that were specified using --config 'gpu(foo=bar,...)' parameters The comment doesn't add that much value. It's rather obvious what the function does? (Comments should start with capital letter, form normal english sentences and end with period.) https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode447 bench/nanobench.cpp:447: static void gather_commandline_configs(SkTArray<Config, false>& nanoConfigs) { So this could be called static void create_configs(SkTDArray<Config>* configs) This way it would not be confusing to assess the responsibilities of the new and old functions. This could go below the next function. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode450 bench/nanobench.cpp:450: for (int i=0; i<configs.count(); ++i) { Here whitespace between operators. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode451 bench/nanobench.cpp:451: const auto& config = *configs[i]; so here you should have code like #if SK_SUPPORT_GPU if (SkCommandLineGpuConfig* gpuConfig = configs[i]->asGpuConfig()) { create_gpu_config(gpuConfig, outConfigs); } else #endif { create_config(config[i], outConfigs); } https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode457 bench/nanobench.cpp:457: static void create_configs(SkTArray<Config, false>* configs) { This would be called: static void create_config(const SkCommandLineConfig* config, SkTArray<Config>* configs) https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode459 bench/nanobench.cpp:459: if (is_cpu_config_allowed(#name)) { \ so this would call if (is_cpu_config_allowed(#name, config)) https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode463 bench/nanobench.cpp:463: configs->push_back(config); \ in the macro, you would add "return", so that the function handles one config and returns. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode474 bench/nanobench.cpp:474: if (is_gpu_config_allowed(#name, GrContextFactory::ctxType, \ (1) GPU configs are already handled in the function you added. So you need to remove this hunk altogether.. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode519 bench/nanobench.cpp:519: #endif Not sure about this, but here you could propose to add a useful warning about not using the config. You could use same kind message wording as in DM.cpp, to stay consistent with it. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode... bench/nanobench.cpp:1135: SkTArray<Config, false> configs; TArray MEM_COPY defaults to false, so it's maybe clearer to leave it out. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode... bench/nanobench.cpp:1137: gather_commandline_configs(configs); and since create_configs would be your toplevel iteration loop, you would only need to call that here.
https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode485 bench/nanobench.cpp:485: configs->push_back(config); \ Also make sure that macro line continuations match consistently.
Also, the commit subject line should not need to end in period Personally, I try to write something in the commit message body. E.g. commit message should have a title and a body. Though this is probably not according to Skia standards. If nothing else, I have tried to include a rationale or a selling point on why I am doing this, what does the change enable or what concrete use-case the change fixes. You could look up the skia bug number in the commit that added the extended config form, and add that to the BUG=skia:NUMBER. You can git blame some of the lines related to SkCommandLineConfig, that should be the commit.
https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode383 bench/nanobench.cpp:383: static bool is_cpu_config_allowed(const char* name) { On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > this would be > static bool is_cpu_config_allowed(const char* name, const SkCommandLineConfig* > config) Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode393 bench/nanobench.cpp:393: static bool is_gpu_config_allowed(const char* name, GrContextFactory::GLContextType ctxType, On 2016/01/20 18:25:56, Kimmo Kinnunen wrote: > likewise here. Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode414 bench/nanobench.cpp:414: // create a config from a specific 'gpu(...)' parameter pack. On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > This comment does not add any information, if you change the name of the > function, and so you can remove the comment. Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode415 bench/nanobench.cpp:415: static void create_commandline_config(SkTArray<Config, false>& nanoConfigs, const SkCommandLineConfig& conf) { On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > So this function could be called > static void create_gpu_config(const SkCommandLineGpuConfig* config, > SkTArray<Config>* configs) > > * the create_commandline_config is too vague for what the code actually does, > especially with other related functions being present > * don't use abbreviation conf. You save 2 letters. > * don't use arbitrary, unrelated prefix nano > * out parameters go last > * out parameters as pointers Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode417 bench/nanobench.cpp:417: return; On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > this would be removed. > Instead, you actually need to guard all GPU-related lines with > #if SK_SUPPORT_GPU > ... > #endif > > and you need to test the compile with: > SKIA_OUT=out-no-gpu GYP_DEFINES=skia_gpu=0 ./gyp_skia && ninja -C > out-no-gpu/Debug most Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode429 bench/nanobench.cpp:429: On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > you would need to call also the is_gpu_config_allowed. Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode430 bench/nanobench.cpp:430: Config target; On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > So here you handle all GPU related configs. Thus you need to remove the > redundant impl in (1). Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode431 bench/nanobench.cpp:431: target.name = tag; On 2016/01/20 18:25:56, Kimmo Kinnunen wrote: > Since the original used object initializer syntax, you could stay with that if > possible. Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode446 bench/nanobench.cpp:446: // that were specified using --config 'gpu(foo=bar,...)' parameters On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > The comment doesn't add that much value. It's rather obvious what the function > does? > (Comments should start with capital letter, form normal english sentences and > end with period.) Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode447 bench/nanobench.cpp:447: static void gather_commandline_configs(SkTArray<Config, false>& nanoConfigs) { On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > So this could be called > static void create_configs(SkTDArray<Config>* configs) > > This way it would not be confusing to assess the responsibilities of the new and > old functions. > > This could go below the next function. Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode450 bench/nanobench.cpp:450: for (int i=0; i<configs.count(); ++i) { On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > Here whitespace between operators. Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode451 bench/nanobench.cpp:451: const auto& config = *configs[i]; On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > so here you should have code like > #if SK_SUPPORT_GPU > if (SkCommandLineGpuConfig* gpuConfig = configs[i]->asGpuConfig()) { > create_gpu_config(gpuConfig, outConfigs); > } else > #endif > { > create_config(config[i], outConfigs); > } Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode457 bench/nanobench.cpp:457: static void create_configs(SkTArray<Config, false>* configs) { On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > This would be called: > static void create_config(const SkCommandLineConfig* config, SkTArray<Config>* > configs) Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode459 bench/nanobench.cpp:459: if (is_cpu_config_allowed(#name)) { \ On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > so this would call > if (is_cpu_config_allowed(#name, config)) Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode474 bench/nanobench.cpp:474: if (is_gpu_config_allowed(#name, GrContextFactory::ctxType, \ On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > (1) GPU configs are already handled in the function you added. So you need to > remove this hunk altogether.. Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode485 bench/nanobench.cpp:485: configs->push_back(config); \ On 2016/01/20 18:30:46, Kimmo Kinnunen wrote: > Also make sure that macro line continuations match consistently. Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode519 bench/nanobench.cpp:519: #endif On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > Not sure about this, but here you could propose to add a useful warning about > not using the config. > You could use same kind message wording as in DM.cpp, to stay consistent with > it. Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode... bench/nanobench.cpp:1135: SkTArray<Config, false> configs; On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > TArray MEM_COPY defaults to false, so it's maybe clearer to leave it out. Done. https://codereview.chromium.org/1603063005/diff/1/bench/nanobench.cpp#newcode... bench/nanobench.cpp:1137: gather_commandline_configs(configs); On 2016/01/20 18:25:55, Kimmo Kinnunen wrote: > and since create_configs would be your toplevel iteration loop, you would only > need to call that here. Done.
Great. Small nits, then you could post it to mtklein@google.com for review.. https://chromiumcodereview.appspot.com/1603063005/diff/20001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://chromiumcodereview.appspot.com/1603063005/diff/20001/bench/nanobench.... bench/nanobench.cpp:375: static bool is_cpu_config_allowed(const char* name, const SkCommandLineConfig* config) { I guess this could be removed now, if it doesn't do the to_lower. https://chromiumcodereview.appspot.com/1603063005/diff/20001/bench/nanobench.... bench/nanobench.cpp:396: GrContextFactory::kNone_GLContextOptions; Sometimes I've gotten the request to align this part and ':' with '?' E.g. a ? b : c
Description was changed from ========== Adding support for parsing extended gpu config parameters in nanobench. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Adding support for parsing extended gpu config parameters in nanobench BUG=skia:2992 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
svaisanen@nvidia.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1603063005/diff/20001/bench/nanobench.cpp File bench/nanobench.cpp (right): https://codereview.chromium.org/1603063005/diff/20001/bench/nanobench.cpp#new... bench/nanobench.cpp:375: static bool is_cpu_config_allowed(const char* name, const SkCommandLineConfig* config) { On 2016/01/21 12:50:01, Kimmo Kinnunen wrote: > I guess this could be removed now, if it doesn't do the to_lower. Done. https://codereview.chromium.org/1603063005/diff/20001/bench/nanobench.cpp#new... bench/nanobench.cpp:396: GrContextFactory::kNone_GLContextOptions; On 2016/01/21 12:50:01, Kimmo Kinnunen wrote: > Sometimes I've gotten the request to align this part and ':' with '?' E.g. > > a ? b > : c > > Done.
kkinnunen@nvidia.com changed reviewers: + bsalomon@google.com
Ping Mike/Brian, PTAL if you have time.
lgtm lgtm
The CQ bit was checked by svaisanen@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1603063005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1603063005/60001
The CQ bit was unchecked by commit-bot@chromium.org
The author svaisanen@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was checked by svaisanen@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1603063005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1603063005/60001
Message was sent while issue was closed.
Description was changed from ========== Adding support for parsing extended gpu config parameters in nanobench BUG=skia:2992 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Adding support for parsing extended gpu config parameters in nanobench BUG=skia:2992 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/c47635e0ed52970fa40a16ffb280071e6b838a52 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/c47635e0ed52970fa40a16ffb280071e6b838a52 |
