Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624583002:
Remove redundant c_str() calls. (Closed)
|
Created:
3 years, 11 months ago by Andrey Kraynov Modified:
3 years, 8 months ago Reviewers:
Primiano Tucci (use gerrit), Bernhard Bauer, Devlin, Lei Zhang, dcheng, Charlie Harrison, Sorin Jianu, Nico CC:
chromium-reviews, extensions-reviews_chromium.org, vmpstr+watch_chromium.org, wfh+watch_chromium.org, donnd+watch_chromium.org, Randy Smith (Not in Mondays), loading-reviews_chromium.org, twellington+watch_chromium.org, pfeldman, devtools-reviews_chromium.org, tracing+reviews_chromium.org, chromium-apps-reviews_chromium.org, jshin+watch_chromium.org, Mark Larson Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove redundant c_str() calls.
It seems, there are lot of redundant calls to std::basic_string<>::c_str() or
base::StringPiece::c_str() when passing parameters to different functions.
When a result of |string.c_str()| expression is passed to a function that takes
a StringPiece, it will measure the length of the string using strlen() function.
This is useless operation because we know the length already.
Even worse, if target function takes a |std::string| then passing c_str() to it
will lead to copying the string and wasting memory.
No behaviour changes.
BUG=679479
Review-Url: https://codereview.chromium.org/2624583002
Cr-Commit-Position: refs/heads/master@{#460066}
Committed: https://chromium.googlesource.com/chromium/src/+/4b80ef168dc0dbeef5acf70771884678ce65fe00
Patch Set 1 #
Total comments: 6
Patch Set 2 : Replace host() with host_piece() call #Patch Set 3 : Rebase #Patch Set 4 : Revert some changes #
Messages
Total messages: 41 (17 generated)
Description was changed from ========== Remove redundant c_str() calls. BUG= ========== to ========== Remove redundant c_str() calls. It seems, there are lot of redundant calls to std::basic_string<>::c_str() or base::StringPiece::c_str() when passing parameters to different functions. That can lead to unwanted calls of strlen() or even worse, to making a copy of a string. BUG=679479 ==========
iceman@yandex-team.ru changed reviewers: + thestig@chromium.org
Hi! Here i tried to remove some redundant calls to c_str() function. Changes are pretty simple but they affect a lot of different files. I'm not sure, maybe I should split this CL into few separate CLs? Could you take a look, please? Thanks in advance! https://codereview.chromium.org/2624583002/diff/1/base/trace_event/trace_conf... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2624583002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:858: bool TraceConfig::IsCategoryEnabled(StringPiece category_name) const { Later |category_name| will be used as an argument for MatchPattern() function https://cs.chromium.org/chromium/src/base/trace_event/trace_config.cc?q=IsCat... It has following signature - |bool MatchPattern(const StringPiece&, const StringPiece&)| https://cs.chromium.org/chromium/src/base/strings/pattern.h?q=MatchPattern&sq...
iceman@yandex-team.ru changed reviewers: + thakis@chromium.org
primiano@chromium.org changed reviewers: + primiano@chromium.org
https://codereview.chromium.org/2624583002/diff/1/base/trace_event/trace_conf... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2624583002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:858: bool TraceConfig::IsCategoryEnabled(StringPiece category_name) const { On 2017/01/09 21:50:45, Andrey Kraynov wrote: > Later |category_name| will be used as an argument for MatchPattern() function > https://cs.chromium.org/chromium/src/base/trace_event/trace_config.cc?q=IsCat... > > It has following signature - |bool MatchPattern(const StringPiece&, const > StringPiece&)| > https://cs.chromium.org/chromium/src/base/strings/pattern.h?q=MatchPattern&sq... Please don't change this signature. This is used by very hot code in the tracing codebase which passes always const char ptr. Of you do this you will cause an implicit temporary string piece to be constructed.
csharrison@chromium.org changed reviewers: + csharrison@chromium.org
https://codereview.chromium.org/2624583002/diff/1/chrome/browser/loader/chrom... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2624583002/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:777: request->url().DomainIs(webstore_url.host())) { Drive-by: this does a string copy. You should use webstore_url.host_piece().
https://codereview.chromium.org/2624583002/diff/1/base/trace_event/trace_conf... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2624583002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:858: bool TraceConfig::IsCategoryEnabled(StringPiece category_name) const { On 2017/01/09 23:52:37, Primiano Tucci (back but slow) wrote: > On 2017/01/09 21:50:45, Andrey Kraynov wrote: > > Later |category_name| will be used as an argument for MatchPattern() function > > > https://cs.chromium.org/chromium/src/base/trace_event/trace_config.cc?q=IsCat... > > > > It has following signature - |bool MatchPattern(const StringPiece&, const > > StringPiece&)| > > > https://cs.chromium.org/chromium/src/base/strings/pattern.h?q=MatchPattern&sq... > > Please don't change this signature. This is used by very hot code in the tracing > codebase which passes always const char ptr. Of you do this you will cause an > implicit temporary string piece to be constructed. My point was that in case of the old signature of IsCategoryEnabled(), temporary StringPiece objects will be constructed every time MatchPattern is called from IsCategoryEnabled() function. MatchPattern() isn't an inline function so when passing a |const char*| to it, StringPiece should be created. Am I missing something?
https://codereview.chromium.org/2624583002/diff/1/base/trace_event/trace_conf... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2624583002/diff/1/base/trace_event/trace_conf... base/trace_event/trace_config.cc:858: bool TraceConfig::IsCategoryEnabled(StringPiece category_name) const { On 2017/01/10 06:24:53, Andrey Kraynov wrote: > On 2017/01/09 23:52:37, Primiano Tucci (back but slow) wrote: > > On 2017/01/09 21:50:45, Andrey Kraynov wrote: > > > Later |category_name| will be used as an argument for MatchPattern() > function > > > > > > https://cs.chromium.org/chromium/src/base/trace_event/trace_config.cc?q=IsCat... > > > > > > It has following signature - |bool MatchPattern(const StringPiece&, const > > > StringPiece&)| > > > > > > https://cs.chromium.org/chromium/src/base/strings/pattern.h?q=MatchPattern&sq... > > > > Please don't change this signature. This is used by very hot code in the > tracing > > codebase which passes always const char ptr. Of you do this you will cause an > > implicit temporary string piece to be constructed. > > My point was that in case of the old signature of IsCategoryEnabled(), temporary > StringPiece objects will be constructed every time MatchPattern is called from > IsCategoryEnabled() function. MatchPattern() isn't an inline function so when > passing a |const char*| to it, StringPiece should be created. > > Am I missing something? ahh sorry you are right, it would actually happen below. please ignore my comment.
https://codereview.chromium.org/2624583002/diff/1/chrome/browser/loader/chrom... File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right): https://codereview.chromium.org/2624583002/diff/1/chrome/browser/loader/chrom... chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:777: request->url().DomainIs(webstore_url.host())) { On 2017/01/10 00:20:35, Charlie Harrison wrote: > Drive-by: this does a string copy. You should use webstore_url.host_piece(). Thanks for pointing! Done.
chrome/browser/loader lgtm
base/trace_event LGTM
iceman@yandex-team.ru changed reviewers: + bauerb@chromium.org, mal@chromium.org, rdevlin.cronin@chromium.org
Added OWNERS reviewers for specific parts of this CL. Sorry if I did it wrong. PTAL.
android LGTM.
chrome/browser/extensions lgtm. Thanks for the patch!
mal@, would you take a look at changes in chrome/browser/component_updater/ ? thakis@, will you have a time to take a look at few lines in base/ ? Thanks in advance!
bauerb@chromium.org changed reviewers: + sorin@chromium.org - mal@chromium.org
On 2017/01/13 06:51:00, Andrey Kraynov wrote: > mal@, would you take a look at changes in chrome/browser/component_updater/ ? Mark is no longer actively working on Chrome. Sorin, could you take a look instead?
On 2017/01/13 17:40:39, Bernhard Bauer wrote: > On 2017/01/13 06:51:00, Andrey Kraynov wrote: > > mal@, would you take a look at changes in chrome/browser/component_updater/ ? > > Mark is no longer actively working on Chrome. Sorin, could you take a look > instead? Should Mark be removed from OWNERs files then, to remove needless roundtrips and stalls?
lgtm Thank you! component updater paths lgtm
lgtm Thank you! component updater paths lgtm
iceman@yandex-team.ru changed reviewers: + dcheng@chromium.org
I need a base/ OWNER review for the few lines of code in this CL. dcheng@, could you take a look, please? Thanks!
//base changes lgtm, but please update the CL description (and the bug) to explain more about why these c_str() calls are redundant. (I guess for //base, it's because the result is passed to StringPiece, which then needs to measure the length again, but it would be nice for thees details to be in the CL description itself)
On 2017/01/17 09:33:38, dcheng wrote: > //base changes lgtm, but please update the CL description (and the bug) to > explain more about why these c_str() calls are redundant. > > (I guess for //base, it's because the result is passed to StringPiece, which > then needs to measure the length again, but it would be nice for thees details > to be in the CL description itself) You are quite right, this is exactly the problem here. OK, I will update the description of this CL. (Not sure about the bug, I still don't have enough privileges to edit bugs. Can I ask someone about this?) Thank you for the review!
Description was changed from ========== Remove redundant c_str() calls. It seems, there are lot of redundant calls to std::basic_string<>::c_str() or base::StringPiece::c_str() when passing parameters to different functions. That can lead to unwanted calls of strlen() or even worse, to making a copy of a string. BUG=679479 ========== to ========== Remove redundant c_str() calls. It seems, there are lot of redundant calls to std::basic_string<>::c_str() or base::StringPiece::c_str() when passing parameters to different functions. When a result of |string.c_str()| expression is passed to a function that takes a StringPiece, it will measure the length of the string using strlen() function. This is useless operation because we know the length already. Even worse, if target function takes a |std::string| then passing c_str() to it will lead to copying the string and wasting memory. No behaviour changes. BUG=679479 ==========
Description was changed from ========== Remove redundant c_str() calls. It seems, there are lot of redundant calls to std::basic_string<>::c_str() or base::StringPiece::c_str() when passing parameters to different functions. When a result of |string.c_str()| expression is passed to a function that takes a StringPiece, it will measure the length of the string using strlen() function. This is useless operation because we know the length already. Even worse, if target function takes a |std::string| then passing c_str() to it will lead to copying the string and wasting memory. No behaviour changes. BUG=679479 ========== to ========== Remove redundant c_str() calls. It seems, there are lot of redundant calls to std::basic_string<>::c_str() or base::StringPiece::c_str() when passing parameters to different functions. When a result of |string.c_str()| expression is passed to a function that takes a StringPiece, it will measure the length of the string using strlen() function. This is useless operation because we know the length already. Even worse, if target function takes a |std::string| then passing c_str() to it will lead to copying the string and wasting memory. No behaviour changes. BUG=679479 ==========
The CQ bit was checked by iceman@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #3 (id:40001) has been deleted
On 2017/01/18 06:37:36, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) This CL has a lot of reviewers and approvals but looks like it is not enough to land the patch. I decided to rebase it and remove changes that require additional approvals. Then I can land this CL and make another CL to continue this work. Hope that no one will be against that.
The CQ bit was checked by iceman@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, bauerb@chromium.org, rdevlin.cronin@chromium.org, dcheng@chromium.org, csharrison@chromium.org, sorin@chromium.org Link to the patchset: https://codereview.chromium.org/2624583002/#ps80001 (title: "Revert some changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490699809355770,
"parent_rev": "a399db89e2ab252a851f074b437cf8972b86e0f0", "commit_rev":
"4b80ef168dc0dbeef5acf70771884678ce65fe00"}
Message was sent while issue was closed.
Description was changed from ========== Remove redundant c_str() calls. It seems, there are lot of redundant calls to std::basic_string<>::c_str() or base::StringPiece::c_str() when passing parameters to different functions. When a result of |string.c_str()| expression is passed to a function that takes a StringPiece, it will measure the length of the string using strlen() function. This is useless operation because we know the length already. Even worse, if target function takes a |std::string| then passing c_str() to it will lead to copying the string and wasting memory. No behaviour changes. BUG=679479 ========== to ========== Remove redundant c_str() calls. It seems, there are lot of redundant calls to std::basic_string<>::c_str() or base::StringPiece::c_str() when passing parameters to different functions. When a result of |string.c_str()| expression is passed to a function that takes a StringPiece, it will measure the length of the string using strlen() function. This is useless operation because we know the length already. Even worse, if target function takes a |std::string| then passing c_str() to it will lead to copying the string and wasting memory. No behaviour changes. BUG=679479 Review-Url: https://codereview.chromium.org/2624583002 Cr-Commit-Position: refs/heads/master@{#460066} Committed: https://chromium.googlesource.com/chromium/src/+/4b80ef168dc0dbeef5acf7077188... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/4b80ef168dc0dbeef5acf7077188... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
