Replace WTF::emptyString{16Bit}() with a static global

Replace WTF::emptyString{16Bit}() with a static global

This is an extension of [1] which applies this approach to StringImpl.
In that patch, a simple micro-benchmark testing:
  StringView("<some literal>") == StringView("<some literal>")
in a tight loop is improved by 30% with this approach, as StringViews
of literals are backed with empty StringImpls.

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

Also note: The trick to make static strings without static constructors 
technically uses undefined behavior and breaks strict aliasing, see 
DEFINE_GLOBAL for details.

[1] https://codereview.chromium.org/2655853003/

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2668903003
Cr-Commit-Position: refs/heads/master@{#447703}
Committed: https://chromium.googlesource.com/chromium/src/+/166d82710cf7ee3212dd839e46547a3ce83e35aa

[Charlie Harrison, 2017-02-01 06:38:58]

Description was changed from

==========
Replace WTF::emptyString{16Bit}() with a static global

BUG=684604
==========

to

==========
Replace WTF::emptyString{16Bit}() with a static global

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Charlie Harrison, 2017-02-01 06:39:02]

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

[commit-bot: I haz the power, 2017-02-01 06:39:16]

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

[Charlie Harrison, 2017-02-01 06:41:24]

Description was changed from

==========
Replace WTF::emptyString{16Bit}() with a static global

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Replace WTF::emptyString{16Bit}() with a static global

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Charlie Harrison, 2017-02-01 07:51:10]

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

[commit-bot: I haz the power, 2017-02-01 07:51:23]

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

[commit-bot: I haz the power, 2017-02-01 09:41:36]

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

[commit-bot: I haz the power, 2017-02-01 09:41:37]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-02-01 16:18:42]

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

[Charlie Harrison, 2017-02-01 16:18:43]

esprehn: PTAL. The only non-trivial changes (i.e. non-sed based changed) are in
WTFString and StringStatics.

Might as well do this at once rather than churn through a couple of CLs w/ an
intermediate inline method (imo). Feel free to disagree.

[esprehn, 2017-02-01 16:24:06]

lgtm

[haraken, 2017-02-01 16:32:37]

LGTM

[Charlie Harrison, 2017-02-01 17:01:38]

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

[Charlie Harrison, 2017-02-01 17:01:39]

Thanks, I think I need IPC folk for the mojo stuff, +dcheng?

[esprehn, 2017-02-01 17:14:16]

On 2017/02/01 at 17:01:39, csharrison wrote:
> Thanks, I think I need IPC folk for the mojo stuff, +dcheng?

Which stuff do you mean? My LGTM should cover all the files you touched here.

[Charlie Harrison, 2017-02-01 17:20:58]

On 2017/02/01 17:14:16, esprehn wrote:
> On 2017/02/01 at 17:01:39, csharrison wrote:
> > Thanks, I think I need IPC folk for the mojo stuff, +dcheng?
> 
> Which stuff do you mean? My LGTM should cover all the files you touched here.
KURLStructTraits.h has setnoparent, with owner IPC_OWNERS

[dcheng, 2017-02-01 17:30:56]

Is the gain here because we don't have to bounce through a function-local
static? It might be worth noting where the gain actually comes from.

I can't say I'm hugely enthusiastic about this, since it looks very much like a
global which would generate a static initializer. It's also unclear to me that
the clever magic it uses to not generate the static initializer isn't undefined
behavior, per the strict aliasing rule.

[Charlie Harrison, 2017-02-01 17:56:14]

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

[Charlie Harrison, 2017-02-01 17:56:16]

Hm, nameStorage has type void*, and we dereference
*reinterpret_cast<String*>(&nameStorage) which is dereferences &nameStorage as a
String* instead of as a void**.

That does indeed look problematic (I think type punning void* is okay though?)

In any case, it is bad if this is wrong because we've been using this trick
since 2006 afaict.

+Nico who might know.

[dcheng, 2017-02-01 17:58:33]

On 2017/02/01 17:56:16, Charlie Harrison-slow til Feb3 wrote:
> Hm, nameStorage has type void*, and we dereference
> *reinterpret_cast<String*>(&nameStorage) which is dereferences &nameStorage as
a
> String* instead of as a void**.
> 
> That does indeed look problematic (I think type punning void* is okay though?)

My understanding is that the guarantee is that if you go T* -> void* -> T* you
get back the original pointer, and nothing more.

> 
> In any case, it is bad if this is wrong because we've been using this trick
> since 2006 afaict.
> 
> +Nico who might know.

To be clear, I think it's "technically undefined" but "generally works". It
might be good to formally document this officially though.

[esprehn, 2017-02-01 20:32:45]

This is the same thing we do for all the fooAtom constants and tag names like
HTMLNames::aTag we've used for years. Using it avoids the function call overhead
and also the initialzied branch for the static local.

[dcheng, 2017-02-02 05:30:35]

LGTM

As discussed, let's include the benchmark numbers in the CL description and
mentions that this technically invokes undefined behavior.

[Charlie Harrison, 2017-02-02 05:51:21]

Description was changed from

==========
Replace WTF::emptyString{16Bit}() with a static global

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Replace WTF::emptyString{16Bit}() with a static global

This is an extension of [1] which applies this approach to StringImpl.
In that patch, a simple micro-benchmark testing:
  StringView("<some literal>") == StringView("<some literal>")
in a tight loop is improved by 30% with this approach, as StringViews
of literals are backed with StringImpls.

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

[1] https://codereview.chromium.org/2655853003/

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Charlie Harrison, 2017-02-02 05:51:44]

Description was changed from

==========
Replace WTF::emptyString{16Bit}() with a static global

This is an extension of [1] which applies this approach to StringImpl.
In that patch, a simple micro-benchmark testing:
  StringView("<some literal>") == StringView("<some literal>")
in a tight loop is improved by 30% with this approach, as StringViews
of literals are backed with StringImpls.

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

[1] https://codereview.chromium.org/2655853003/

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Replace WTF::emptyString{16Bit}() with a static global

This is an extension of [1] which applies this approach to StringImpl.
In that patch, a simple micro-benchmark testing:
  StringView("<some literal>") == StringView("<some literal>")
in a tight loop is improved by 30% with this approach, as StringViews
of literals are backed with empty StringImpls.

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

[1] https://codereview.chromium.org/2655853003/

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Charlie Harrison, 2017-02-02 05:53:59]

Description was changed from

==========
Replace WTF::emptyString{16Bit}() with a static global

This is an extension of [1] which applies this approach to StringImpl.
In that patch, a simple micro-benchmark testing:
  StringView("<some literal>") == StringView("<some literal>")
in a tight loop is improved by 30% with this approach, as StringViews
of literals are backed with empty StringImpls.

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

[1] https://codereview.chromium.org/2655853003/

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Replace WTF::emptyString{16Bit}() with a static global

This is an extension of [1] which applies this approach to StringImpl.
In that patch, a simple micro-benchmark testing:
  StringView("<some literal>") == StringView("<some literal>")
in a tight loop is improved by 30% with this approach, as StringViews
of literals are backed with empty StringImpls.

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

Also note: The trick to make static strings without static constructors uses
undefined behavior and breaks strict aliasing, see DEFINE_GLOBAL.

[1] https://codereview.chromium.org/2655853003/

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Charlie Harrison, 2017-02-02 05:54:34]

Description was changed from

==========
Replace WTF::emptyString{16Bit}() with a static global

This is an extension of [1] which applies this approach to StringImpl.
In that patch, a simple micro-benchmark testing:
  StringView("<some literal>") == StringView("<some literal>")
in a tight loop is improved by 30% with this approach, as StringViews
of literals are backed with empty StringImpls.

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

Also note: The trick to make static strings without static constructors uses
undefined behavior and breaks strict aliasing, see DEFINE_GLOBAL.

[1] https://codereview.chromium.org/2655853003/

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Replace WTF::emptyString{16Bit}() with a static global

This is an extension of [1] which applies this approach to StringImpl.
In that patch, a simple micro-benchmark testing:
  StringView("<some literal>") == StringView("<some literal>")
in a tight loop is improved by 30% with this approach, as StringViews
of literals are backed with empty StringImpls.

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

Also note: The trick to make static strings without static constructors 
technically uses undefined behavior and breaks strict aliasing, see 
DEFINE_GLOBAL for details.

[1] https://codereview.chromium.org/2655853003/

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Charlie Harrison, 2017-02-02 05:56:00]

Alright, I've updated the description. Thanks for the review

[Charlie Harrison, 2017-02-02 05:56:13]

The CQ bit was checked by csharrison@chromium.org

[commit-bot: I haz the power, 2017-02-02 05:56:35]

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

[commit-bot: I haz the power, 2017-02-02 06:05:11]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486014973000960,
"parent_rev": "5fdae6fa2582b407ad6281c152e5cb6f7b95a391", "commit_rev":
"166d82710cf7ee3212dd839e46547a3ce83e35aa"}

[commit-bot: I haz the power, 2017-02-02 06:09:52]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1486014973000960,
"parent_rev": "5fdae6fa2582b407ad6281c152e5cb6f7b95a391", "commit_rev":
"166d82710cf7ee3212dd839e46547a3ce83e35aa"}

[commit-bot: I haz the power, 2017-02-02 06:10:19]

Description was changed from

==========
Replace WTF::emptyString{16Bit}() with a static global

This is an extension of [1] which applies this approach to StringImpl.
In that patch, a simple micro-benchmark testing:
  StringView("<some literal>") == StringView("<some literal>")
in a tight loop is improved by 30% with this approach, as StringViews
of literals are backed with empty StringImpls.

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

Also note: The trick to make static strings without static constructors 
technically uses undefined behavior and breaks strict aliasing, see 
DEFINE_GLOBAL for details.

[1] https://codereview.chromium.org/2655853003/

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Replace WTF::emptyString{16Bit}() with a static global

This is an extension of [1] which applies this approach to StringImpl.
In that patch, a simple micro-benchmark testing:
  StringView("<some literal>") == StringView("<some literal>")
in a tight loop is improved by 30% with this approach, as StringViews
of literals are backed with empty StringImpls.

To fix callers I ran:
git grep -l "emptyString()" | xargs sed -i 's/emptyString()/emptyString/g'

with 16 bit variants in third_party/WebKit/Source.

Also note: The trick to make static strings without static constructors 
technically uses undefined behavior and breaks strict aliasing, see 
DEFINE_GLOBAL for details.

[1] https://codereview.chromium.org/2655853003/

BUG=684604
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2668903003
Cr-Commit-Position: refs/heads/master@{#447703}
Committed:
https://chromium.googlesource.com/chromium/src/+/166d82710cf7ee3212dd839e4654...
==========

[commit-bot: I haz the power, 2017-02-02 06:10:22]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/166d82710cf7ee3212dd839e4654...