Release the buffer in StringBuilder::toString() and toAtomicString().

Release the buffer in StringBuilder::toString() and toAtomicString().

Various places in the code like FetchDataLoader and WorkerScriptLoader
keep a StringBuilder as a data member to buffer data and then call
toString() on it later. By clearing the buffer in the string conversion
methods we avoid having two copies of the values in memory persistently.

This restores the behavior from before I switched it to a Vector in
https://codereview.chromium.org/2046353002 where toString() and
toAtomicString() would always clear the buffer when converting to a
string because they called reifyString.

While I haven't been able to confirm this is the cause, this seems a
possible candidate for the small regression on system health benchmarks.
It's also just generally better behavior since it avoids an accidental
2x memory increase from a StringBuilder member.

This also makes appending individual characters cheaper by removing the
branch from resetting m_string every time.

BUG=633197
Committed: https://crrev.com/e5dd14ac89435d384867f678106374f471dd414a
Cr-Commit-Position: refs/heads/master@{#418437}

[esprehn, 2016-09-13 20:34:29]

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

[commit-bot: I haz the power, 2016-09-13 20:34: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-09-13 22:19:41]

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

[commit-bot: I haz the power, 2016-09-13 22:19:42]

Dry run: This issue passed the CQ dry run.

[esprehn, 2016-09-13 22:37:47]

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

[esprehn, 2016-09-13 22:38:33]

Description was changed from

==========
Release the buffer in StringBuilder::toString() and toAtomicString().

Various places in the code like FetchDataLoader and WorkerScriptLoader
keep a StringBuilder as a data member to buffer data and then call
toString() on it later. By clearing the buffer in the string conversion
methods we avoid having two copies of the values in memory persistently.

This restores the behavior from before I switched it to a Vector in
https://codereview.chromium.org/2046353002 where toString() and
toAtomicString() would always clear the buffer when converting to a
string because they called reifyString.

While I haven't been able to confirm this is the cause, this seems a
possible candidate for the small regression on system health benchmarks.
It's also just generally better behavior since it avoids an accidental
2x memory increase from a StringBuilder member.

BUG=633197
==========

to

==========
Release the buffer in StringBuilder::toString() and toAtomicString().

Various places in the code like FetchDataLoader and WorkerScriptLoader
keep a StringBuilder as a data member to buffer data and then call
toString() on it later. By clearing the buffer in the string conversion
methods we avoid having two copies of the values in memory persistently.

This restores the behavior from before I switched it to a Vector in
https://codereview.chromium.org/2046353002 where toString() and
toAtomicString() would always clear the buffer when converting to a
string because they called reifyString.

While I haven't been able to confirm this is the cause, this seems a
possible candidate for the small regression on system health benchmarks.
It's also just generally better behavior since it avoids an accidental
2x memory increase from a StringBuilder member.

This also makes appending individual characters cheaper by removing the
branch from resetting m_string every time.

BUG=633197
==========

[haraken, 2016-09-14 00:05:41]

LGTM

https://codereview.chromium.org/2335193005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right):

https://codereview.chromium.org/2335193005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringBuilder.cpp:93: void
StringBuilder::clear()

I'm just curious but would you help me understand the difference between clear()
and clearBuffer()?

[esprehn, 2016-09-14 00:14:31]

https://codereview.chromium.org/2335193005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right):

https://codereview.chromium.org/2335193005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringBuilder.cpp:93: void
StringBuilder::clear()
On 2016/09/14 at 00:05:41, haraken wrote:
> I'm just curious but would you help me understand the difference between
clear() and clearBuffer()?

clear() resets the entire state of the builder, like it's brand new.

clearBuffer() just resets the buffer used to make the string, leaving behind
state like m_string, m_length and m_is8Bit. That's why clear() needs to call
clearBuffer() to reset all of the state.

We only want to reset the storage when you call toString(), not the state
related to the length or the char type so if you did append() after calling
toString() we allocate the right kind of buffer (LChar vs UChar), and so calling
length() or is8Bit() after calling toString() still works.

[esprehn, 2016-09-14 00:20:07]

The CQ bit was checked by esprehn@chromium.org

[commit-bot: I haz the power, 2016-09-14 00:20:32]

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

[haraken, 2016-09-14 00:21:06]

https://codereview.chromium.org/2335193005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right):

https://codereview.chromium.org/2335193005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringBuilder.cpp:93: void
StringBuilder::clear()
On 2016/09/14 00:14:31, esprehn wrote:
> On 2016/09/14 at 00:05:41, haraken wrote:
> > I'm just curious but would you help me understand the difference between
> clear() and clearBuffer()?
> 
> clear() resets the entire state of the builder, like it's brand new.
> 
> clearBuffer() just resets the buffer used to make the string, leaving behind
> state like m_string, m_length and m_is8Bit. That's why clear() needs to call
> clearBuffer() to reset all of the state.
> 
> We only want to reset the storage when you call toString(), not the state
> related to the length or the char type so if you did append() after calling
> toString() we allocate the right kind of buffer (LChar vs UChar), and so
calling
> length() or is8Bit() after calling toString() still works.

Ah, got it. Thanks.

(I was misunderstanding that this CL is making StringBuilder entirely useless
once StringBuilder::toString is called. Maybe this is something we want to
consider though.)

[esprehn, 2016-09-14 00:25:39]

https://codereview.chromium.org/2335193005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/text/StringBuilder.cpp (right):

https://codereview.chromium.org/2335193005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/text/StringBuilder.cpp:93: void
StringBuilder::clear()
On 2016/09/14 at 00:21:05, haraken wrote:
> ...
> 
> Ah, got it. Thanks.
> 
> (I was misunderstanding that this CL is making StringBuilder entirely useless
once StringBuilder::toString is called. Maybe this is something we want to
consider though.)

Yeah we could consider that. I don't understand the various code that keeps
StringBuilder members enough to know if that's doable. We should audit those. :)

[commit-bot: I haz the power, 2016-09-14 00:26:47]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-14 00:29:49]

Description was changed from

==========
Release the buffer in StringBuilder::toString() and toAtomicString().

Various places in the code like FetchDataLoader and WorkerScriptLoader
keep a StringBuilder as a data member to buffer data and then call
toString() on it later. By clearing the buffer in the string conversion
methods we avoid having two copies of the values in memory persistently.

This restores the behavior from before I switched it to a Vector in
https://codereview.chromium.org/2046353002 where toString() and
toAtomicString() would always clear the buffer when converting to a
string because they called reifyString.

While I haven't been able to confirm this is the cause, this seems a
possible candidate for the small regression on system health benchmarks.
It's also just generally better behavior since it avoids an accidental
2x memory increase from a StringBuilder member.

This also makes appending individual characters cheaper by removing the
branch from resetting m_string every time.

BUG=633197
==========

to

==========
Release the buffer in StringBuilder::toString() and toAtomicString().

Various places in the code like FetchDataLoader and WorkerScriptLoader
keep a StringBuilder as a data member to buffer data and then call
toString() on it later. By clearing the buffer in the string conversion
methods we avoid having two copies of the values in memory persistently.

This restores the behavior from before I switched it to a Vector in
https://codereview.chromium.org/2046353002 where toString() and
toAtomicString() would always clear the buffer when converting to a
string because they called reifyString.

While I haven't been able to confirm this is the cause, this seems a
possible candidate for the small regression on system health benchmarks.
It's also just generally better behavior since it avoids an accidental
2x memory increase from a StringBuilder member.

This also makes appending individual characters cheaper by removing the
branch from resetting m_string every time.

BUG=633197

Committed: https://crrev.com/e5dd14ac89435d384867f678106374f471dd414a
Cr-Commit-Position: refs/heads/master@{#418437}
==========

[commit-bot: I haz the power, 2016-09-14 00:29:49]

Patchset 2 (id:??) landed as
https://crrev.com/e5dd14ac89435d384867f678106374f471dd414a
Cr-Commit-Position: refs/heads/master@{#418437}