Allocate space in StringBuilder for m_string and the new bytes.

Allocate space in StringBuilder for m_string and the new bytes.

StringBuilder has an optimization to just store the first String object
you append to avoid a copy in cases where you only append one String
and then call toString(), for example if there's onlyone Text child
and you do element.textContent.

When appending the second string we then allocate a Vector buffer and
copy the first string (m_string) into it. We weren't allocating extra
space for the bytes we were about to append in the second string
though, which meant we always did an extra copy immediately following
the the buffer creation:

String first;
String second;

StringBuilder builder;
// No copy, just stores a pointer to first.
builder.append(first);
// 1. Creates a Vector
// 2. Copies first into it.
// 3. Appends second to the Vector which causes a Vector resize.
builder.append(second);

We solve this by calling reserveInitialCapacity with m_length + the
number of bytes we're about to append when we created the buffer. We
also inflate the number of bytes to be at least the inline capacity
size so that doing:

builder.append(first);
builder.append('1');
builder.append('2');

doesn't require an immediate resize for appending '2' by ensuring we
always overallocate the Vector by at least the inline capacity size so
even when appending a String you get 16 chars of extra space to insert
something else.

BUG=624642
Committed: https://crrev.com/464918ccc62d3a70a976b3c38b5ef6ce6e552f31
Cr-Commit-Position: refs/heads/master@{#403407}

[esprehn, 2016-06-30 20:03:48]

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

[commit-bot: I haz the power, 2016-06-30 20:04:53]

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, 2016-06-30 21:41:04]

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

[commit-bot: I haz the power, 2016-06-30 21:41:05]

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

[esprehn, 2016-06-30 23:23:14]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org, jbroman@chromium.org

[esprehn, 2016-06-30 23:23:15]

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

https://codereview.chromium.org/2106283004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/StringBuilder.h:209: static unsigned
initialBufferSize() { return kInlineBufferSize; }
You have to have a method since using kInlineBufferSize in std::max doesn't work
since it tries to take a reference... silly stl.

[esprehn, 2016-07-01 00:29:33]

Description was changed from

==========
Allocate space in StringBuilder for m_string and the new bytes.

StringBuilder has an optimization to just store the first String object
you append to avoid a copy in cases where you only append one String
and then call toString(), for example if there's onlyone Text child
and you do element.textContent.

When appending the second string we then allocate a Vector buffer and
copy the first string (m_string) into it. We weren't allocating extra
space for the bytes we were about to append in the second string
though, which meant we always did an extra copy immediately following
the the buffer creation:

String first;
String second;

StringBuilder builder;
// No copy, just stores a pointer to first.
builder.append(first);
// 1. Creates a Vector
// 2. Copies first into it.
// 3. Appends second to the Vector which causes a Vector resize.
builder.append(second);

We solve this by calling reserveInitialCapacity with m_length + the
number of bytes we're about to append when we created the buffer. We
also inflate the number of bytes to be at least the inline capacity
size so that doing:

builder.append(first);
builder.append('1');
builder.append('2');

doesn't require an immediate resize for appending '2' by ensuring we
always overallocate the Vector by at least the inline capacity size so
even when appending a String you get 16 chars of extra space to insert
something else.
==========

to

==========
Allocate space in StringBuilder for m_string and the new bytes.

StringBuilder has an optimization to just store the first String object
you append to avoid a copy in cases where you only append one String
and then call toString(), for example if there's onlyone Text child
and you do element.textContent.

When appending the second string we then allocate a Vector buffer and
copy the first string (m_string) into it. We weren't allocating extra
space for the bytes we were about to append in the second string
though, which meant we always did an extra copy immediately following
the the buffer creation:

String first;
String second;

StringBuilder builder;
// No copy, just stores a pointer to first.
builder.append(first);
// 1. Creates a Vector
// 2. Copies first into it.
// 3. Appends second to the Vector which causes a Vector resize.
builder.append(second);

We solve this by calling reserveInitialCapacity with m_length + the
number of bytes we're about to append when we created the buffer. We
also inflate the number of bytes to be at least the inline capacity
size so that doing:

builder.append(first);
builder.append('1');
builder.append('2');

doesn't require an immediate resize for appending '2' by ensuring we
always overallocate the Vector by at least the inline capacity size so
even when appending a String you get 16 chars of extra space to insert
something else.

BUG=624642
==========

[haraken, 2016-07-01 02:01:03]

LGTM

https://codereview.chromium.org/2106283004/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right):

https://codereview.chromium.org/2106283004/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/StringBuilder.cpp:138:
m_buffer8->reserveInitialCapacity(m_length + std::max(addedSize,
initialBufferSize()));

I don't quite see any reason you want to take std::max(addedSize,
initialBufferSize()). I guess you can just allocate |m_length + addedSize +
someSlack|.

[esprehn, 2016-07-01 02:11:34]

On 2016/07/01 at 02:01:03, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/2106283004/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right):
> 
>
https://codereview.chromium.org/2106283004/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/wtf/text/StringBuilder.cpp:138:
m_buffer8->reserveInitialCapacity(m_length + std::max(addedSize,
initialBufferSize()));
> 
> I don't quite see any reason you want to take std::max(addedSize,
initialBufferSize()). I guess you can just allocate |m_length + addedSize +
someSlack|.

That would defeat the inline capacity optimization by forcing a malloc
immediately unless someSlack was small. By taking the max of either the size of
the inline capacity or the addedSize we avoid the malloc when appending the
first byte.

[esprehn, 2016-07-01 02:15:40]

> > ...
> > I don't quite see any reason you want to take std::max(addedSize,
initialBufferSize()). I guess you can just allocate |m_length + addedSize +
someSlack|.
> 
> That would defeat the inline capacity optimization by forcing a malloc
immediately unless someSlack was small. By taking the max of either the size of
the inline capacity or the addedSize we avoid the malloc when appending the
first byte.

That is we avoid it while taking the largest amount of slack possible. I guess
this does fall down if you append 16 bytes right away and then one more by
resulting in an immediate resize on the extra byte, but that was always true
with the inline capacity even if m_string was null before this optimization.

[haraken, 2016-07-01 02:16:56]

On 2016/07/01 02:15:40, esprehn wrote:
> > > ...
> > > I don't quite see any reason you want to take std::max(addedSize,
> initialBufferSize()). I guess you can just allocate |m_length + addedSize +
> someSlack|.
> > 
> > That would defeat the inline capacity optimization by forcing a malloc
> immediately unless someSlack was small. By taking the max of either the size
of
> the inline capacity or the addedSize we avoid the malloc when appending the
> first byte.
> 
> That is we avoid it while taking the largest amount of slack possible. I guess
> this does fall down if you append 16 bytes right away and then one more by
> resulting in an immediate resize on the extra byte, but that was always true
> with the inline capacity even if m_string was null before this optimization.

Ah, thanks! Makes sense.

[esprehn, 2016-07-01 02:38:56]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2016-07-01 02:39:20]

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, 2016-07-01 04:00:48]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-01 04:02:49]

Description was changed from

==========
Allocate space in StringBuilder for m_string and the new bytes.

StringBuilder has an optimization to just store the first String object
you append to avoid a copy in cases where you only append one String
and then call toString(), for example if there's onlyone Text child
and you do element.textContent.

When appending the second string we then allocate a Vector buffer and
copy the first string (m_string) into it. We weren't allocating extra
space for the bytes we were about to append in the second string
though, which meant we always did an extra copy immediately following
the the buffer creation:

String first;
String second;

StringBuilder builder;
// No copy, just stores a pointer to first.
builder.append(first);
// 1. Creates a Vector
// 2. Copies first into it.
// 3. Appends second to the Vector which causes a Vector resize.
builder.append(second);

We solve this by calling reserveInitialCapacity with m_length + the
number of bytes we're about to append when we created the buffer. We
also inflate the number of bytes to be at least the inline capacity
size so that doing:

builder.append(first);
builder.append('1');
builder.append('2');

doesn't require an immediate resize for appending '2' by ensuring we
always overallocate the Vector by at least the inline capacity size so
even when appending a String you get 16 chars of extra space to insert
something else.

BUG=624642
==========

to

==========
Allocate space in StringBuilder for m_string and the new bytes.

StringBuilder has an optimization to just store the first String object
you append to avoid a copy in cases where you only append one String
and then call toString(), for example if there's onlyone Text child
and you do element.textContent.

When appending the second string we then allocate a Vector buffer and
copy the first string (m_string) into it. We weren't allocating extra
space for the bytes we were about to append in the second string
though, which meant we always did an extra copy immediately following
the the buffer creation:

String first;
String second;

StringBuilder builder;
// No copy, just stores a pointer to first.
builder.append(first);
// 1. Creates a Vector
// 2. Copies first into it.
// 3. Appends second to the Vector which causes a Vector resize.
builder.append(second);

We solve this by calling reserveInitialCapacity with m_length + the
number of bytes we're about to append when we created the buffer. We
also inflate the number of bytes to be at least the inline capacity
size so that doing:

builder.append(first);
builder.append('1');
builder.append('2');

doesn't require an immediate resize for appending '2' by ensuring we
always overallocate the Vector by at least the inline capacity size so
even when appending a String you get 16 chars of extra space to insert
something else.

BUG=624642

Committed: https://crrev.com/464918ccc62d3a70a976b3c38b5ef6ce6e552f31
Cr-Commit-Position: refs/heads/master@{#403407}
==========

[commit-bot: I haz the power, 2016-07-01 04:02:51]

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