[wip] Add LiteralStringPiece which templatizes based on character length

Add LiteralStringPiece which templatizes based on character length

StringPiece is often constructed out of string literals, which
entails a call to strlen() or equivalent. This is unnecessary, as
we know the length of a literal at compile time.

BUG=673376

[Charlie Harrison, 2016-12-14 21:40:15]

The CQ bit was checked by csharrison@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 21:40:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2016-12-14 21:42:16]

Description was changed from

==========
Add LiteralStringPiece which templatizes based on character length

BUG=
==========

to

==========
Add LiteralStringPiece which templatizes based on character length

StringPiece is often constructed out of string literals, which
entails a call to strlen() or equivalent. This is unnecessary, as
we know the length of a literal at compile time.

BUG=
==========

[Charlie Harrison, 2016-12-14 21:57:02]

csharrison@chromium.org changed reviewers:
+ thakis@chromium.org

[Charlie Harrison, 2016-12-14 21:57:02]

Nico: are you a good reviewer for this? There are places in the codebase that do
a lot of these comparisons, and strlen() has been showing up in profiles. I have
a few questions though:

1. Will templatizing off of length bloat the binary significantly? I imagine we
could have a reasonable amount of these between 1-20 chars long.

2. Is there a trick to make this a first class constructor? I tried adding

template<size_t size>
BasicStringPiece(const value_type(&array)[size])
 : ptr_(array), length_(size) {}

but string literals would use the const value_type* constructor instead (I think
they prefer non-template constructors).

[Nico, 2016-12-14 21:58:54]

i'm super behind, so i won't get to this super soon at lesat.

yes, this will bloat size. (easy to measure though, so just do that.)

On Wed, Dec 14, 2016 at 4:57 PM, <csharrison@chromium.org> wrote:

> Reviewers: Nico
> CL: https://codereview.chromium.org/2579473002/
>
> Message:
> Nico: are you a good reviewer for this? There are places in the codebase
> that do
> a lot of these comparisons, and strlen() has been showing up in profiles.
> I have
> a few questions though:
>
> 1. Will templatizing off of length bloat the binary significantly? I
> imagine we
> could have a reasonable amount of these between 1-20 chars long.
>
> 2. Is there a trick to make this a first class constructor? I tried adding
>
> template<size_t size>
> BasicStringPiece(const value_type(&array)[size])
> : ptr_(array), length_(size) {}
>
> but string literals would use the const value_type* constructor instead (I
> think
> they prefer non-template constructors).
>
>
>
> Description:
> Add LiteralStringPiece which templatizes based on character length
>
> StringPiece is often constructed out of string literals, which
> entails a call to strlen() or equivalent. This is unnecessary, as
> we know the length of a literal at compile time.
>
> BUG=
>
> Affected files (+20, -7 lines):
> M base/strings/string_piece.h
> M base/strings/string_piece_unittest.cc
> M extensions/browser/api/web_request/web_request_permissions.cc
>
>
> Index: base/strings/string_piece.h
> diff --git a/base/strings/string_piece.h b/base/strings/string_piece.h
> index eaec14de5da788ebab2137df35015b9998ec892e..
> dccd109457895aa2c930cfcd8bcc1429e1a02f6f 100644
> --- a/base/strings/string_piece.h
> +++ b/base/strings/string_piece.h
> @@ -367,6 +367,11 @@ const typename BasicStringPiece<STRING_TYPE>:
> :size_type
> BasicStringPiece<STRING_TYPE>::npos =
> typename BasicStringPiece<STRING_TYPE>::size_type(-1);
>
> +template <size_t size>
> +inline StringPiece LiteralStringPiece(const char (&array)[size]) {
> + return StringPiece(array, size - 1);
> +}
> +
> // MSVC doesn't like complex extern templates and DLLs.
> #if !defined(COMPILER_MSVC)
> extern template class BASE_EXPORT BasicStringPiece<std::string>;
> Index: base/strings/string_piece_unittest.cc
> diff --git a/base/strings/string_piece_unittest.cc
> b/base/strings/string_piece_unittest.cc
> index f05aa152b508be69b9f1e035371d4080f448f924..
> 3e40178827b9a4e3a9ac43390931e0829fdb6dfb 100644
> --- a/base/strings/string_piece_unittest.cc
> +++ b/base/strings/string_piece_unittest.cc
> @@ -688,4 +688,11 @@ TYPED_TEST(CommonStringPieceTest, CheckConstructors)
> {
> ASSERT_TRUE(empty == BasicStringPiece<TypeParam>(empty.begin(),
> empty.end()));
> }
>
> +TEST(CommonStringPieceTest, LiteralStringPiece) {
> + StringPiece test_piece("test");
> + EXPECT_EQ(test_piece, LiteralStringPiece("test"));
> + const char test_ptr[] = "test";
> + EXPECT_EQ(test_piece, LiteralStringPiece(test_ptr));
> +}
> +
> } // namespace base
> Index: extensions/browser/api/web_request/web_request_permissions.cc
> diff --git a/extensions/browser/api/web_request/web_request_permissions.cc
> b/extensions/browser/api/web_request/web_request_permissions.cc
> index 11e8c9d1054ae421e7a14d0b209c8610959e574f..
> d241f515e76d81c8a01fccf97f5bd9ef7898b2fe 100644
> --- a/extensions/browser/api/web_request/web_request_permissions.cc
> +++ b/extensions/browser/api/web_request/web_request_permissions.cc
> @@ -50,7 +50,7 @@ bool IsSensitiveURL(const GURL& url) {
> const base::StringPiece& host = url.host_piece();
> const char kGoogleCom[] = "google.com";
> const char kClient[] = "clients";
> - if (url.DomainIs(kGoogleCom)) {
> + if (url.DomainIs(base::LiteralStringPiece(kGoogleCom))) {
> // Check for "clients[0-9]*.google.com" hosts.
> // This protects requests to several internal services such as sync,
> // extension update pings, captive portal detection, fraudulent certificate
> @@ -70,12 +70,13 @@ bool IsSensitiveURL(const GURL& url) {
> }
> // This protects requests to safe browsing, link doctor, and possibly
> // others.
> - sensitive_chrome_url = sensitive_chrome_url ||
> - url.DomainIs("clients.google.com") ||
> - url.DomainIs("sb-ssl.google.com") ||
> - (url.DomainIs("chrome.google.com") &&
> - base::StartsWith(url.path_piece(), "/webstore",
> - base::CompareCase::SENSITIVE));
> + sensitive_chrome_url =
> + sensitive_chrome_url ||
> + url.DomainIs(base::LiteralStringPiece("clients.google.com")) ||
> + url.DomainIs(base::LiteralStringPiece("sb-ssl.google.com")) ||
> + (url.DomainIs(base::LiteralStringPiece("chrome.google.com")) &&
> + base::StartsWith(url.path_piece(), "/webstore",
> + base::CompareCase::SENSITIVE));
> }
> return sensitive_chrome_url || extension_urls::IsWebstoreUpdateUrl(url) ||
> extension_urls::IsBlacklistUpdateUrl(url);
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2016-12-14 22:25:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 22:25:25]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[Charlie Harrison, 2016-12-14 23:14:53]

I tested two builds, one using the templatized function, and one removing the
template param, and simply forwarding to the const char* constructor. There
wasn't any difference in the build size. To instantiate all of the templates, I
used a series of CHECK statements like:

CHECK_EQ(base::LiteralStringPiece("aaaa"), "aaaa")
for 100 different lengths.

Both versions of the function used "inline", which might be the reason for this.

In any case, no rush on this CL it is very low priority. I think I'm missing a
BASE_EXPORT as well.

[esprehn, 2016-12-25 21:40:11]

The optimizer is actually removing all of these calls to strlen. We don't need
this literal class, I even removed the equivalent one in blink after verifying
the assembly output.

[esprehn, 2016-12-25 22:05:46]

On 2016/12/25 at 21:40:11, esprehn wrote:
> The optimizer is actually removing all of these calls to strlen. We don't need
this literal class, I even removed the equivalent one in blink after verifying
the assembly output.

That is if you're seeing this on profiles I'd love to see it and the associated
callstacks. :)

[Charlie Harrison, 2017-01-03 14:53:49]

Description was changed from

==========
Add LiteralStringPiece which templatizes based on character length

StringPiece is often constructed out of string literals, which
entails a call to strlen() or equivalent. This is unnecessary, as
we know the length of a literal at compile time.

BUG=
==========

to

==========
Add LiteralStringPiece which templatizes based on character length

StringPiece is often constructed out of string literals, which
entails a call to strlen() or equivalent. This is unnecessary, as
we know the length of a literal at compile time.

BUG=673376
==========