createPlainText(): avoid a 32k buffer allocation for empty ranges.

createPlainText(): avoid a 32k buffer allocation for empty ranges.

The pre-allocation of the StringBuilder is redundant if the range is
empty, hence avoid.

While here, simplify empty string builder testing also.

R=yosin
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201212

[sof, 2015-08-25 14:49:33]

sigbjornf@opera.com changed reviewers:
+ tkent@chromium.org, yosin@chromium.org

[sof, 2015-08-25 14:49:33]

please take a look.

[yosin_UTC9, 2015-08-26 01:23:21]

I can read your intention, e.g. not to use redundant memory.
However, it is better describe your intention and what
this patch improves explicitly in the description.

Q: I read old webkit bug, do we still need about size of
memory reservation. Do we still need to reserve memory
in new partition allocator?

Also, TextIterator and (HTML markup) Serializer don't need
to have fancy StringBuilder feature, such as substring(),
characters(). It it better to have simplified StringBuilder
which keeps collected characters in list of chunks to avoid
copying characters at internal buffer expansion? 

It seems there are other clients fit to simplified StringBuilder,
e.g. Range.toString(), TEXTAREA.toString(), and so on, they are
potentially yield large string.

https://codereview.chromium.org/1317593003/diff/1/Source/core/editing/iterato...
File Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/1317593003/diff/1/Source/core/editing/iterato...
Source/core/editing/iterators/TextIterator.cpp:1099: if (!it.atEnd()) {
Can we use early return pattern?

if (it.atEnd())
    return emptyString();

for (...) {
 ...
}

https://codereview.chromium.org/1317593003/diff/1/Source/core/editing/iterato...
Source/core/editing/iterators/TextIterator.cpp:1107: bufferLength +=
it.length();
Optional: Do we need to sum result length up?
It seems we should change an implementation of StringBuilder::toString()
in another patch.

Background:
If we want to create new empty string by StringBuilder::toString(),
we should change an implementation of StringBuilder::toString().
It seems it always create new empty string.

Current:
 String toString()
    {
        shrinkToFit();
        if (m_string.isNull())
            reifyString();
        return m_string;
    }

Suggested:
String toString()
{
  if (isEmpty())
    return emptyString();
  ... copy of original ...
}

[sof, 2015-08-26 06:32:21]

(+Cc: haraken@ who wrote the perf improving patch referred to.)

On 2015/08/26 01:23:21, Yosi_UTC9 wrote:
>
...
> Q: I read old webkit bug, do we still need about size of memory reservation.
Do we still need to reserve memory in new partition allocator?

The need for pre-allocation shouldn't fall away with a new/different allocator,
but you could argue the size might be reconsidered and re-tuned (up to 1 << 16?)
for it. But looking at the perf test on that older bug, it looks rather
microbenchmark-like.

https://codereview.chromium.org/1317593003/diff/1/Source/core/editing/iterato...
File Source/core/editing/iterators/TextIterator.cpp (right):

https://codereview.chromium.org/1317593003/diff/1/Source/core/editing/iterato...
Source/core/editing/iterators/TextIterator.cpp:1099: if (!it.atEnd()) {
On 2015/08/26 01:23:21, Yosi_UTC9 wrote:
> Can we use early return pattern?
> 
> if (it.atEnd())
>     return emptyString();
> 
> for (...) {
>  ...
> }

That looks better, switched to an early return.

https://codereview.chromium.org/1317593003/diff/1/Source/core/editing/iterato...
Source/core/editing/iterators/TextIterator.cpp:1107: bufferLength +=
it.length();
On 2015/08/26 01:23:21, Yosi_UTC9 wrote:
> Optional: Do we need to sum result length up?
> It seems we should change an implementation of StringBuilder::toString()
> in another patch.
> 
> Background:
> If we want to create new empty string by StringBuilder::toString(),
> we should change an implementation of StringBuilder::toString().
> It seems it always create new empty string.
> 
> Current:
>  String toString()
>     {
>         shrinkToFit();
>         if (m_string.isNull())
>             reifyString();
>         return m_string;
>     }
> 
> Suggested:
> String toString()
> {
>   if (isEmpty())
>     return emptyString();
>   ... copy of original ...
> }

Just testing for builder.isEmpty() here would suffice rather than reworking
toString(), I'd say.

[sof, 2015-08-26 06:39:07]

actually Cc: haraken@

[haraken, 2015-08-26 06:55:21]

LGTM

That 32 KB pre-allocation is important to avoid unnecessary reallocations &
copyings while appending Strings. Also the pre-allocation doesn't add any memory
overhead because the pre-allocation happens on stack. I'd propose keeping it
unless it is causing any actual problem.

[yosin_UTC9, 2015-08-26 07:02:33]

LGTM

Thanks for cleanup TextIterator.cpp!

[sof, 2015-08-26 07:06:15]

On 2015/08/26 06:55:21, haraken wrote:
> LGTM
> 
> That 32 KB pre-allocation is important to avoid unnecessary reallocations &
> copyings while appending Strings. Also the pre-allocation doesn't add any
memory
> overhead because the pre-allocation happens on stack. I'd propose keeping it
> unless it is causing any actual problem.

The allocation is short-lived, but StringBuilder::allocateBuffer() uses the
heap, not stack (somehow)? (i.e., seeing the larger buffer heap allocation was
how I spotted this.)

[yosin_UTC9, 2015-08-26 07:09:29]

The CQ bit was checked by yosin@chromium.org

[commit-bot: I haz the power, 2015-08-26 07:09:36]

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

[commit-bot: I haz the power, 2015-08-26 08:07:29]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201212