Improve the memory consumption and performance of our string buffer implementation.

Improve the memory consumption and performance of our string buffer implementation.

R=lrn@google.com
BUG=

Committed: https://code.google.com/p/dart/source/detail?r=27698

[Lasse Reichstein Nielsen, 2013-09-20 11:40:17]

LGTM, STV, HUZZAH, etc., but get a VM guys comments as well.

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:64: // '$obj', but apparently this is too
slow on the Dart VM.
Is this still true?

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:107: // allow me to pass in a grownable
array directly, but for
Should be fairly simple to do.

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:133: _bufferPosition =
_bufferCodeUnitMagnitude = 0;
Consider growing the buffer eventually?

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:164: _partsCompactionArray = new
_ObjectArray(_PARTS_TO_COMPACT);
Consider whether reusing the array is worth it - it increases the risk that the
parts array goes to old-space and you store a lot of new-space pointers in it. A
quick new-space array that is thrown away afterwards might be better. 
That's guessing, so try both.

https://codereview.chromium.org/24251012/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/code_buffer.dart (left):

https://codereview.chromium.org/24251012/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/code_buffer.dart:84: return write(new
String.fromCharCodes([charCode]));
Ouch. How much would you gain from just this change?

https://codereview.chromium.org/24251012/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/code_buffer.dart (right):

https://codereview.chromium.org/24251012/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/code_buffer.dart:15: CodeBuffer();
You can even remove the constructor entirely.
Probably too confusing.

https://codereview.chromium.org/24251012/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/code_buffer.dart:85: buffer = new
StringBuffer();
Per my earlier comments, this is probably a good thing, but you could also do
buffer.clear();

[kasperl, 2013-09-20 12:04:27]

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:107: // allow me to pass in a grownable
array directly, but for
On 2013/09/20 11:40:17, Lasse Reichstein Nielsen wrote:
> Should be fairly simple to do.

I guess I really want something like a _concatAllNative(array, start, end)
method that allows passing in a growable array.

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:164: _partsCompactionArray = new
_ObjectArray(_PARTS_TO_COMPACT);
On 2013/09/20 11:40:17, Lasse Reichstein Nielsen wrote:
> Consider whether reusing the array is worth it - it increases the risk that
the
> parts array goes to old-space and you store a lot of new-space pointers in it.
A
> quick new-space array that is thrown away afterwards might be better. 
> That's guessing, so try both.

I've tried both and reusing the array seems preferable. If we could change the
_concatAllNative to allow taking a slice of a growable array instead of
insisting on getting a fixed-size array then we wouldn't have this problem at
all.

[kasperl, 2013-09-20 12:07:22]

As this solves a memory consumption issue we're facing in dart2js in our string
buffer based emitter, I'm going to submit this now.

I'd be happy to address any concerns from the VM team in a follow up CL.

[kasperl, 2013-09-20 12:07:53]

Committed patchset #2 manually as r27698 (presubmit successful).

[srdjan, 2013-09-25 20:56:39]

lgtm

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:107: // allow me to pass in a grownable
array directly, but for
s/grownable/growable/

[kasperl, 2013-09-26 05:30:28]

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:107: // allow me to pass in a grownable
array directly, but for
On 2013/09/25 20:56:40, srdjan wrote:
> s/grownable/growable/

:-)

Maybe you could help me fix the TODO? If concatAllNative was a little bit more
lenient, I might be able to pass in a _parts unmodified here and do something
ala:

String compacted = _StringBase._concatAllNative(
    _parts, 
    _partsCompactionIndex,                    // Start
    _partsCompactionIndex + PARTS_TO_COMPACT  // End
);

in the _compact method. That should help by copying and allocating less.

[srdjan, 2013-09-26 13:22:45]

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/24251012/diff/4001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:107: // allow me to pass in a grownable
array directly, but for
On 2013/09/26 05:30:29, kasperl wrote:
> On 2013/09/25 20:56:40, srdjan wrote:
> > s/grownable/growable/
> 
> :-)
> 
> Maybe you could help me fix the TODO? If concatAllNative was a little bit more
> lenient, I might be able to pass in a _parts unmodified here and do something
> ala:
> 
> String compacted = _StringBase._concatAllNative(
>     _parts, 
>     _partsCompactionIndex,                    // Start
>     _partsCompactionIndex + PARTS_TO_COMPACT  // End
> );
> 
> in the _compact method. That should help by copying and allocating less.

Yes, I will do that.