Introduce StringImpl::createPreallocatedStatic

Introduce StringImpl::createPreallocatedStatic

This CL adds StringImpl::createPreallocatedStatic and replaces all of
existing usage of StringImpl::createStatic with the new one.

In blink initialization phase, CoreInitializer::init() creates more than
500 of StringImpl with StringImpl::createStatic, which allocates new
buffers and copy string literals into them.
That costs around 3.1ms of renderer startup time on Nexus 4.

OTOH, StringImpl::createPreallocatedStatic reuses the static storage for
string literals without allocating extra buffer. That reduces
CoreInitializer::init() time to 1.8ms.

BUG=

[tzik, 2015-10-15 11:44:28]

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

[commit-bot: I haz the power, 2015-10-15 11:44:32]

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

[tzik, 2015-10-15 11:45:27]

tzik@chromium.org changed reviewers:
+ kouhei@chromium.org

[commit-bot: I haz the power, 2015-10-15 12:26:30]

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

[commit-bot: I haz the power, 2015-10-15 12:26:31]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[tzik, 2015-10-15 13:39:54]

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

[commit-bot: I haz the power, 2015-10-15 13:40:05]

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

[commit-bot: I haz the power, 2015-10-15 15:15:03]

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

[commit-bot: I haz the power, 2015-10-15 15:15:03]

Dry run: This issue passed the CQ dry run.

[kouhei (in TOK), 2015-10-21 01:08:16]

kouhei@chromium.org changed reviewers:
+ tkent@chromium.org

[kouhei (in TOK), 2015-10-21 01:08:17]

non-OWNER lgtm

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.cpp:357: return it->value;
Are we ever going to reach here? If not, ASSERT?

[tkent, 2015-10-21 03:19:34]

Can we remove StringImpl::createStatic?

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl
(right):

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl:32: char
string_impl_storage[sizeof(StringImpl)];
* Need a comment that the layout should be matched StringImpl, and an assertion
that |name| is next to |string_impl_storrage|.

* style error: |string_impl_storage| should be |stringImplStorage| or something.

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl:38: static
NameEntry Names[] = {
You don't need to rename kNames to Names, which doesn't match to the coding
style.

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.cpp:369: "Benign race on the
reference counter of a static string created by StringImpl::createStatic");
createStatic -> createPreallocatedStatic?

[Daniel Bratell, 2015-10-21 09:51:24]

bratell@opera.com changed reviewers:
+ bratell@opera.com

[Daniel Bratell, 2015-10-21 09:51:25]

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl
(right):

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl:32: char
string_impl_storage[sizeof(StringImpl)];
This looks like it could make the Names array rather large in the binary. There
are quite a lot of NameEntry objects right. Not sure why the const char* name
pointer was bad?

Also, can you move the string_impl_storage array out of this object because
while NameEntry objects need to be setup statically, string_impl_storage is just
a preallocated memory buffer and much cheaper to setup. Mixing them brings the
worse of both together.

[Daniel Bratell, 2015-10-22 08:09:15]

On 2015/10/21 09:51:25, Daniel Bratell wrote:
>
https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl
> (right):
> 
>
https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl:32: char
> string_impl_storage[sizeof(StringImpl)];
> This looks like it could make the Names array rather large in the binary.
There
> are quite a lot of NameEntry objects right. Not sure why the const char* name
> pointer was bad?

Ran this through the tools/binary_size/explain_binary_size_delta tool and it
will indeed grow the binary (and possibly memory usage) by 50 KB. For instance 
blink::HTMLNames::init()::Names went from 7KB to 21KB,
blink::SVGNames::init()::Names from 5KB to 16KB and so on. Those symbols are
also of the "d" flavour ("initialized data section") which means that they have
to be copied to RAM and fixed for use so this size increase will slow startup
time as well.

[tzik, 2015-10-28 14:32:41]

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

[tzik, 2015-10-28 14:33:00]

Description was changed from

==========
Introduce StringImpl::createPreallocatedStatic

This CL adds StringImpl::createPreallocatedStatic and replaces most of
existing usage of StringImpl::createStatic with the new one.

In blink initialization phase, CoreInitializer::init() creates more than
500 of StringImpl with StringImpl::createStatic, which allocates new
buffers and copy string literals into them.
That costs around 3.1ms of renderer startup time on Nexus 4.

OTOH, StringImpl::createPreallocatedStatic reuses the static storage for
string literals without allocating extra buffer. That reduces
CoreInitializer::init() time to 1.8ms.

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

to

==========
Introduce StringImpl::createPreallocatedStatic

This CL adds StringImpl::createPreallocatedStatic and replaces all of
existing usage of StringImpl::createStatic with the new one.

In blink initialization phase, CoreInitializer::init() creates more than
500 of StringImpl with StringImpl::createStatic, which allocates new
buffers and copy string literals into them.
That costs around 3.1ms of renderer startup time on Nexus 4.

OTOH, StringImpl::createPreallocatedStatic reuses the static storage for
string literals without allocating extra buffer. That reduces
CoreInitializer::init() time to 1.8ms.

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

[commit-bot: I haz the power, 2015-10-28 14:33:01]

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

[tzik, 2015-10-28 14:33:31]

Updated. PTAL.

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl
(right):

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl:32: char
string_impl_storage[sizeof(StringImpl)];
On 2015/10/21 03:19:33, tkent wrote:
> * Need a comment that the layout should be matched StringImpl, and an
assertion
> that |name| is next to |string_impl_storrage|.
> 
> * style error: |string_impl_storage| should be |stringImplStorage| or
something.

Done.

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl:32: char
string_impl_storage[sizeof(StringImpl)];
On 2015/10/21 09:51:25, Daniel Bratell wrote:
> This looks like it could make the Names array rather large in the binary.
There
> are quite a lot of NameEntry objects right. Not sure why the const char* name
> pointer was bad?
> 
> Also, can you move the string_impl_storage array out of this object because
> while NameEntry objects need to be setup statically, string_impl_storage is
just
> a preallocated memory buffer and much cheaper to setup. Mixing them brings the
> worse of both together.

|name| needs to be an array instead of char* to match memory layout requirement
of StringImpl. And stringImplStorage also needs to here.
Though single kMaxNameLength makes some gap between strings, but that seems not
significant.

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl:38: static
NameEntry Names[] = {
On 2015/10/21 03:19:33, tkent wrote:
> You don't need to rename kNames to Names, which doesn't match to the coding
> style.

Done.

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringImpl.cpp (right):

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.cpp:357: return it->value;
On 2015/10/21 01:08:17, kouhei wrote:
> Are we ever going to reach here? If not, ASSERT?

We do actually reach here due to duplicated static strings, namely:
a, autocomplete, color, direction, end, height, href, image, lang, max,
media, method, min, name, onfocusin, onfocusout, pattern, readonly, role,
script, style, target, text, title, type, version, width, xmlns

https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringImpl.cpp:369: "Benign race on the
reference counter of a static string created by StringImpl::createStatic");
On 2015/10/21 03:19:34, tkent wrote:
> createStatic -> createPreallocatedStatic?

Done.

[tzik, 2015-10-28 14:34:06]

On 2015/10/22 08:09:15, Daniel Bratell wrote:
> On 2015/10/21 09:51:25, Daniel Bratell wrote:
> >
>
https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl
> > (right):
> > 
> >
>
https://codereview.chromium.org/1405573003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/build/scripts/templates/MakeNames.cpp.tmpl:32:
char
> > string_impl_storage[sizeof(StringImpl)];
> > This looks like it could make the Names array rather large in the binary.
> There
> > are quite a lot of NameEntry objects right. Not sure why the const char*
name
> > pointer was bad?
> 
> Ran this through the tools/binary_size/explain_binary_size_delta tool and it
> will indeed grow the binary (and possibly memory usage) by 50 KB. For instance

> blink::HTMLNames::init()::Names went from 7KB to 21KB,
> blink::SVGNames::init()::Names from 5KB to 16KB and so on. Those symbols are
> also of the "d" flavour ("initialized data section") which means that they
have
> to be copied to RAM and fixed for use so this size increase will slow startup
> time as well.

Which binary did you measure? For my local measurement, release build of
ChromePublic.apk grows just 150B.
In addition, existing implementation allocates buffers for each names and copies
these names into the buffer.
So I think this CL actually reduces runtime memory slightly.

[Daniel Bratell, 2015-10-28 14:47:37]

On 2015/10/28 14:34:06, tzik wrote:

> Which binary did you measure? For my local measurement, release build of
> ChromePublic.apk grows just 150B.
> In addition, existing implementation allocates buffers for each names and
copies
> these names into the buffer.
> So I think this CL actually reduces runtime memory slightly.

I'm not so sure but memory usage is not easy to predict. The new space that was
allocated here will be zeroes and apk files are compressed so it is reasonable
that apk size didn't increase much. The increase of 50 KB will be when the
symbol is expanded into memory during process startup. Then some of that memory
will be saved when we can avoid allocating more memory but I don't know how much
and if it ends as a win or a loss.

[commit-bot: I haz the power, 2015-10-28 15:20:08]

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

[commit-bot: I haz the power, 2015-10-28 15:20:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)