De-bloat SVGPathStringBuilder

De-bloat SVGPathStringBuilder

For a class this meager, it was a bit of a heavy-weight:

SVGPathStringBuilder::arcTo() clocked in at a whopping 7891 bytes
(Linux, x86-64, gcc-4.8.2), but even the much simpler lineToVertical()
used > .5k of text-space.
Sacrifice the StringOperators-based pattern and use a cascade of functions
instead.
Besides reducing footprint, this also eliminates some of the copying that
would occur between various temporary String objects.
With the change in place, arcTo() dropped to 640 bytes (and lineToVertical
got cut in half.)

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

[fs, 2014-03-26 16:29:30]

Some days I look at footprint/symbol sizes and notice odd things... This was one
of these days.

@cevans: PTAL at WTF/StringBuilder.

[abarth-chromium, 2014-03-26 16:52:30]

https://codereview.chromium.org/212603003/diff/1/Source/wtf/text/StringBuilde...
File Source/wtf/text/StringBuilder.cpp (right):

https://codereview.chromium.org/212603003/diff/1/Source/wtf/text/StringBuilde...
Source/wtf/text/StringBuilder.cpp:353:
append(numberToFixedPrecisionString(number, precision, buffer,
trailingZerosTruncatingPolicy == TruncateTrailingZeros));
This can be made more efficient.  You're creating a temporary string that isn't
needed....

[fs, 2014-03-27 14:11:13]

On 2014/03/26 16:52:30, abarth wrote:
>
https://codereview.chromium.org/212603003/diff/1/Source/wtf/text/StringBuilde...
> File Source/wtf/text/StringBuilder.cpp (right):
> 
>
https://codereview.chromium.org/212603003/diff/1/Source/wtf/text/StringBuilde...
> Source/wtf/text/StringBuilder.cpp:353:
> append(numberToFixedPrecisionString(number, precision, buffer,
> trailingZerosTruncatingPolicy == TruncateTrailingZeros));
> This can be made more efficient.  You're creating a temporary string that
isn't
> needed....

Personally I didn't worry too much about the 96 byte stack buffer - the stack is
mostly cache-hot, and allocation is ~1 instruction. Anyway, I changed it to use
appendUninitialized, assuming that's what you meant.

[Erik Dahlström (inactive), 2014-03-27 14:42:34]

https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
File Source/wtf/text/StringBuilderTest.cpp (right):

https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
Source/wtf/text/StringBuilderTest.cpp:322: const double someNumber = 1.2345;
String::number() uses precision=6 per default, and appendNumber too, but would
this really catch a change in one of the places?

Why not use a const for setting the default value to the same in both places?

Would it make sense to have that as a test too?

[fs, 2014-03-27 15:40:44]

On 2014/03/27 14:42:34, Erik Dahlström wrote:
>
https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
> File Source/wtf/text/StringBuilderTest.cpp (right):
> 
>
https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
> Source/wtf/text/StringBuilderTest.cpp:322: const double someNumber = 1.2345;
> String::number() uses precision=6 per default, and appendNumber too, but would
> this really catch a change in one of the places?
> 
> Why not use a const for setting the default value to the same in both places?
> 
> Would it make sense to have that as a test too?

This test was mainly intended to test that the UTF-16 code-path does "the right
thing" when converting from LChar -> UChar. I guess I could add additional
digits to trigger the precision, but that's not what this test was expected to
test.

[fs, 2014-03-28 14:59:09]

abarth/others, PTAL

[abarth-chromium, 2014-03-28 17:41:43]

Ok, you might have made this too clever, but I like it.  LGTM

https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
File Source/wtf/text/StringBuilder.cpp (right):

https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
Source/wtf/text/StringBuilder.cpp:356: ASSERT(current == buffer);
This ASSERT seems unnecessary.  That's the only way to exit the loop.

https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
Source/wtf/text/StringBuilder.cpp:371: expandLCharToUCharInplace(dest,
numberLength);
Woah

[fs, 2014-03-31 07:41:56]

https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
File Source/wtf/text/StringBuilder.cpp (right):

https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
Source/wtf/text/StringBuilder.cpp:356: ASSERT(current == buffer);
On 2014/03/28 17:41:43, abarth wrote:
> This ASSERT seems unnecessary.  That's the only way to exit the loop.

Dropped.

[pdr., 2014-03-31 16:19:31]

On 2014/03/31 07:41:56, fs wrote:
>
https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
> File Source/wtf/text/StringBuilder.cpp (right):
> 
>
https://codereview.chromium.org/212603003/diff/20001/Source/wtf/text/StringBu...
> Source/wtf/text/StringBuilder.cpp:356: ASSERT(current == buffer);
> On 2014/03/28 17:41:43, abarth wrote:
> > This ASSERT seems unnecessary.  That's the only way to exit the loop.
> 
> Dropped.

LGTM2

[fs, 2014-04-01 06:52:42]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-04-01 06:52:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fs@opera.com/212603003/40001

[commit-bot: I haz the power, 2014-04-01 07:20:12]

Change committed as 170523