New issue
Advanced search Search tips

Issue 679479 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Feb 2018
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 3
Type: Bug



Sign in to add a comment

Redundant string::c_str() calls may be removed.

Project Member Reported by ice...@yandex-team.ru, Jan 9 2017

Issue description

Looks like 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.

I will try to make a CL with a patch for this bug.
 
Labels: TE-NeedsTriageHelp

Comment 3 by shrike@chromium.org, Jan 13 2017

Labels: -TE-NeedsTriageHelp
Status: Started (was: Unconfirmed)

Comment 4 Deleted

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b80ef168dc0dbeef5acf70771884678ce65fe00

commit 4b80ef168dc0dbeef5acf70771884678ce65fe00
Author: iceman <iceman@yandex-team.ru>
Date: Tue Mar 28 12:33:21 2017

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}

[modify] https://crrev.com/4b80ef168dc0dbeef5acf70771884678ce65fe00/base/environment.cc
[modify] https://crrev.com/4b80ef168dc0dbeef5acf70771884678ce65fe00/base/i18n/file_util_icu.cc
[modify] https://crrev.com/4b80ef168dc0dbeef5acf70771884678ce65fe00/chrome/browser/android/contextualsearch/contextual_search_manager.cc
[modify] https://crrev.com/4b80ef168dc0dbeef5acf70771884678ce65fe00/chrome/browser/android/contextualsearch/ctr_suppression.cc
[modify] https://crrev.com/4b80ef168dc0dbeef5acf70771884678ce65fe00/chrome/browser/android/history_report/delta_file_backend_leveldb.cc
[modify] https://crrev.com/4b80ef168dc0dbeef5acf70771884678ce65fe00/chrome/browser/android/preferences/website_preference_bridge.cc
[modify] https://crrev.com/4b80ef168dc0dbeef5acf70771884678ce65fe00/chrome/browser/component_updater/pnacl_component_installer.cc
[modify] https://crrev.com/4b80ef168dc0dbeef5acf70771884678ce65fe00/chrome/browser/component_updater/recovery_component_installer.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 28 2017

Project Member

Comment 8 by bugdroid1@chromium.org, Mar 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dbec06a1bfb0204b72ebd11fafd0885792197043

commit dbec06a1bfb0204b72ebd11fafd0885792197043
Author: imcheng <imcheng@chromium.org>
Date: Tue Mar 28 22:15:52 2017

Revert of Avoid calling strlen() where possible in base/trace_config (patchset #2 id:20001 of https://codereview.chromium.org/2777203004/ )

Reason for revert:
Broke WebKit Win x64 Builder (dbg)
blink_platform_unittests:

WorkQueueTest.BlockedByFenceNewFenceUnblocks
WorkQueueTest.TakeTaskFromWorkQueue
WorkQueueTest.InsertFenceAfterEnqueueingNonBlocking
WorkQueueTest.RemoveFence
WorkQueueTest.TakeTaskFromWorkQueue_HitFence
WorkQueueTest.InsertNewFence
WorkQueueTest.BlockedByFencePop
WorkQueueTest.BlockedByFencePopBecomesEmpty

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit_Win_x64_Builder__dbg_%2F106650%2F%2B%2Frecipes%2Fsteps%2Fblink_platform_unittests%2F0%2Fstdout

Original issue's description:
> The main target of this CL is to avoid calling strlen() where possible.
> It can happen if a raw |char*| literal or result of |c_str()| function is used
> to construct a StringPiece or std::string objects.
> In addition, StringPiece API like SplitStringPiece was used to avoid
> unnecessary string copying and allocations.
>
> BUG= 679479 
>
> Review-Url: https://codereview.chromium.org/2777203004
> Cr-Commit-Position: refs/heads/master@{#460203}
> Committed: https://chromium.googlesource.com/chromium/src/+/b5d44bb49fd4dc5e544f4312de6b00b1a1eb625c

TBR=primiano@chromium.org,yusukes@chromium.org,iceman@yandex-team.ru
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 679479 

Review-Url: https://codereview.chromium.org/2776393004
Cr-Commit-Position: refs/heads/master@{#460223}

[modify] https://crrev.com/dbec06a1bfb0204b72ebd11fafd0885792197043/base/trace_event/trace_config.cc
[modify] https://crrev.com/dbec06a1bfb0204b72ebd11fafd0885792197043/base/trace_event/trace_config.h
[modify] https://crrev.com/dbec06a1bfb0204b72ebd11fafd0885792197043/base/trace_event/trace_config_category_filter.cc
[modify] https://crrev.com/dbec06a1bfb0204b72ebd11fafd0885792197043/base/trace_event/trace_config_category_filter.h
[modify] https://crrev.com/dbec06a1bfb0204b72ebd11fafd0885792197043/base/trace_event/trace_event_etw_export_win.cc
[modify] https://crrev.com/dbec06a1bfb0204b72ebd11fafd0885792197043/base/trace_event/trace_event_etw_export_win.h
[modify] https://crrev.com/dbec06a1bfb0204b72ebd11fafd0885792197043/chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4179f17c901660139200edfcc751c2a7d9b5d03e

commit 4179f17c901660139200edfcc751c2a7d9b5d03e
Author: iceman <iceman@yandex-team.ru>
Date: Wed Mar 29 19:50:09 2017

Continue to clean c_str() calls.

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.

No behaviour changes.

BUG= 679479 

Review-Url: https://codereview.chromium.org/2779893005
Cr-Commit-Position: refs/heads/master@{#460501}

[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/browser/prefs/pref_metrics_service.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/browser/profiles/profile_avatar_icon_util.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/browser/supervised_user/experimental/safe_search_url_reporter.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/browser/ui/browser_window_state.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/browser/ui/chrome_pages.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/browser/ui/webui/instant_ui.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/common/pepper_flash.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/common/trace_event_args_whitelist.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/installer/util/google_chrome_distribution.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/installer/util/user_experiment.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/renderer/chrome_content_renderer_client.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/chrome/test/chromedriver/element_util.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/content_settings/core/browser/host_content_settings_map.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/dom_distiller/content/browser/dom_distiller_viewer_source.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/nacl/zygote/nacl_fork_delegate_linux.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/omnibox/browser/builtin_provider.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/omnibox/browser/omnibox_edit_model.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/policy/core/common/cloud/cloud_policy_core.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/prefs/pref_service.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/signin/core/browser/about_signin_internals.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/sync_preferences/pref_model_associator.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/sync_preferences/synced_pref_change_registrar.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/translate/core/browser/translate_prefs.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/components/webcrypto/algorithms/test_helpers.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/content/browser/devtools/devtools_http_handler.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/content/browser/gpu/compositor_util.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/printing/backend/print_backend_chromeos.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/printing/backend/print_backend_cups.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/remoting/client/jni/jni_client.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/remoting/host/register_support_host_request.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/remoting/protocol/jingle_session.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/sandbox/linux/suid/client/setuid_sandbox_host.cc
[modify] https://crrev.com/4179f17c901660139200edfcc751c2a7d9b5d03e/ui/base/clipboard/clipboard_android.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Mar 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e8d28b598fa533bd3c0503e59e037d63913fbde

commit 5e8d28b598fa533bd3c0503e59e037d63913fbde
Author: iceman <iceman@yandex-team.ru>
Date: Thu Mar 30 11:21:59 2017

Reland: Avoid calling strlen() where possible in base/trace_config

Fix for  crbug.com/706416  was landed and this patch should work fine.

The main target of this CL is to avoid calling strlen() where possible.
It can happen if a raw |char*| literal or result of |c_str()| function is used
to construct a StringPiece or std::string objects.

In addition, StringPiece API like SplitStringPiece was used to avoid
unnecessary string copying and allocations.

BUG= 679479 
TBR=primiano@chromium.org,yusukes@chromium.org

Review-Url: https://codereview.chromium.org/2780983003
Cr-Commit-Position: refs/heads/master@{#460720}

[modify] https://crrev.com/5e8d28b598fa533bd3c0503e59e037d63913fbde/base/trace_event/trace_config.cc
[modify] https://crrev.com/5e8d28b598fa533bd3c0503e59e037d63913fbde/base/trace_event/trace_config.h
[modify] https://crrev.com/5e8d28b598fa533bd3c0503e59e037d63913fbde/base/trace_event/trace_config_category_filter.cc
[modify] https://crrev.com/5e8d28b598fa533bd3c0503e59e037d63913fbde/base/trace_event/trace_config_category_filter.h
[modify] https://crrev.com/5e8d28b598fa533bd3c0503e59e037d63913fbde/base/trace_event/trace_event_etw_export_win.cc
[modify] https://crrev.com/5e8d28b598fa533bd3c0503e59e037d63913fbde/base/trace_event/trace_event_etw_export_win.h
[modify] https://crrev.com/5e8d28b598fa533bd3c0503e59e037d63913fbde/chrome/browser/chromeos/arc/tracing/arc_tracing_bridge.cc

Status: Fixed (was: Started)

Sign in to add a comment