Literal AtomicString construction can rely on strlen optimization.

Literal AtomicString construction can rely on strlen optimization.

A literal string should have strlen() on it optimized into the length. We should
be able to rely on that instead of using template and enum tagging crazy. This
seems like it should be safe since base::StringPiece<std::string> does this
too.

I also tested with a small test program compiled with clang++ and g++ and both
-O2 and -Os will turn strlen into the literal length.

This machinery was once part of the ASCIILiteral optimization where we would
share the read only string in the binary with the StringImpl to avoid making
a copy for literal strings. This turned out to not be worth much savings
and was removed in the process of adding the small string optimization to
StringImpl. See: https://codereview.chromium.org/21274008

Committed: https://crrev.com/95188e3e597532927640b169d6cb8e096bf50588
Cr-Commit-Position: refs/heads/master@{#384333}

[esprehn, 2016-03-31 00:27:59]

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

[commit-bot: I haz the power, 2016-03-31 00:29:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844223002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844223002/1

[esprehn, 2016-03-31 05:22:34]

Description was changed from

==========
Literal AtomicString construction can rely on strlen optimization.
==========

to

==========
Literal AtomicString construction can rely on strlen optimization.

A literal string should have strlen() on it optimized into the length. We should
be able to rely on that instead of using template and enum tagging crazy. This
seems like it should be safe since base::StringPiece<std::string> does this
too.
==========

[esprehn, 2016-03-31 05:22:34]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org, jyasskin@chromium.org, yutak@chromium.org

[esprehn, 2016-03-31 05:23:06]

This is much simpler, and seems like it should have identical performance, plus
it doesn't require that verbose and weird AtomicString::FromLiteral incantation.

[haraken, 2016-03-31 05:54:32]

LGTM, if you're confident that strlen is optimized as you expected :)

[dcheng, 2016-03-31 05:54:35]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-03-31 05:54:36]

It might be worth mentioning in the CL description that originally, the literal
optimization was meant to avoid copying the contents of literals. We lost that
when abarth ripped out the literal optimization for StringImpls in
https://codereview.chromium.org/21274008, and so there really is no point to
this optimization anymore.

https://codereview.chromium.org/1844223002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/AtomicString.h (right):

https://codereview.chromium.org/1844223002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/AtomicString.h:153: if (!characters)
null is a valid input to add()?! That sounds insane.

[Yuta Kitamura, 2016-03-31 06:12:07]

https://codereview.chromium.org/1844223002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/AtomicString.h (right):

https://codereview.chromium.org/1844223002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/AtomicString.h:151: ALWAYS_INLINE static
PassRefPtr<StringImpl> add(const LChar* characters)
String literals have a type of |const char[N]|, so if you take
a string with a template, I think you don't ever need strlen
either.

I expect something like this would work:

    template <size_t n>
    void f(const char(&characters)[n]) {
        // string length == n - 1
        ...
    }

[commit-bot: I haz the power, 2016-03-31 06:30:56]

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

[commit-bot: I haz the power, 2016-03-31 06:30:57]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[esprehn, 2016-03-31 09:20:56]

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

[commit-bot: I haz the power, 2016-03-31 09:21:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844223002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844223002/1

[commit-bot: I haz the power, 2016-03-31 12:41:03]

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

[commit-bot: I haz the power, 2016-03-31 12:41:04]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-03-31 18:03:35]

Description was changed from

==========
Literal AtomicString construction can rely on strlen optimization.

A literal string should have strlen() on it optimized into the length. We should
be able to rely on that instead of using template and enum tagging crazy. This
seems like it should be safe since base::StringPiece<std::string> does this
too.
==========

to

==========
Literal AtomicString construction can rely on strlen optimization.

A literal string should have strlen() on it optimized into the length. We should
be able to rely on that instead of using template and enum tagging crazy. This
seems like it should be safe since base::StringPiece<std::string> does this
too.

I also tested with a small test program compiled with clang++ and g++ and both
-O2 and -Os will turn strlen into the literal length.

This machinery was once part of the ASCIILiteral optimization where we would
share the read only string in the binary with the StringImpl to avoid making
a copy for literal strings. This turned out to not be worth much savings
and was removed in the process of adding the small string optimization to
StringImpl. See: https://codereview.chromium.org/21274008
==========

[esprehn, 2016-03-31 18:03:50]

On 2016/03/31 at 05:54:36, dcheng wrote:
> It might be worth mentioning in the CL description that originally, the
literal optimization was meant to avoid copying the contents of literals. We
lost that when abarth ripped out the literal optimization for StringImpls in
https://codereview.chromium.org/21274008, and so there really is no point to
this optimization anymore.
> 

Done.

[esprehn, 2016-03-31 18:04:12]

https://codereview.chromium.org/1844223002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/AtomicString.h (right):

https://codereview.chromium.org/1844223002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/AtomicString.h:151: ALWAYS_INLINE static
PassRefPtr<StringImpl> add(const LChar* characters)
On 2016/03/31 at 06:12:06, Yuta Kitamura wrote:
> String literals have a type of |const char[N]|, so if you take
> a string with a template, I think you don't ever need strlen
> either.
> 
> I expect something like this would work:
> 
>     template <size_t n>
>     void f(const char(&characters)[n]) {
>         // string length == n - 1
>         ...
>     }

Yeah you can do that, but that won't work for

const char* kConstant = "foo";
AtomicString string(kConstant);

but the strlen one should apply for both const char* and literal usage.

https://codereview.chromium.org/1844223002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/AtomicString.h:153: if (!characters)
On 2016/03/31 at 05:54:36, dcheng wrote:
> null is a valid input to add()?! That sounds insane.

I'm just doing what the old code did... I agree it's crazy. We should stomp it
out.

[esprehn, 2016-03-31 18:04:24]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2016-03-31 18:04:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1844223002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1844223002/1

[commit-bot: I haz the power, 2016-03-31 18:17:36]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-31 18:19:25]

Description was changed from

==========
Literal AtomicString construction can rely on strlen optimization.

A literal string should have strlen() on it optimized into the length. We should
be able to rely on that instead of using template and enum tagging crazy. This
seems like it should be safe since base::StringPiece<std::string> does this
too.

I also tested with a small test program compiled with clang++ and g++ and both
-O2 and -Os will turn strlen into the literal length.

This machinery was once part of the ASCIILiteral optimization where we would
share the read only string in the binary with the StringImpl to avoid making
a copy for literal strings. This turned out to not be worth much savings
and was removed in the process of adding the small string optimization to
StringImpl. See: https://codereview.chromium.org/21274008
==========

to

==========
Literal AtomicString construction can rely on strlen optimization.

A literal string should have strlen() on it optimized into the length. We should
be able to rely on that instead of using template and enum tagging crazy. This
seems like it should be safe since base::StringPiece<std::string> does this
too.

I also tested with a small test program compiled with clang++ and g++ and both
-O2 and -Os will turn strlen into the literal length.

This machinery was once part of the ASCIILiteral optimization where we would
share the read only string in the binary with the StringImpl to avoid making
a copy for literal strings. This turned out to not be worth much savings
and was removed in the process of adding the small string optimization to
StringImpl. See: https://codereview.chromium.org/21274008

Committed: https://crrev.com/95188e3e597532927640b169d6cb8e096bf50588
Cr-Commit-Position: refs/heads/master@{#384333}
==========

[commit-bot: I haz the power, 2016-03-31 18:19:27]

Patchset 1 (id:??) landed as
https://crrev.com/95188e3e597532927640b169d6cb8e096bf50588
Cr-Commit-Position: refs/heads/master@{#384333}