Change VM's string-buffer patch to use a Uin16Array as backing buffer.

Change VM's string-buffer patch to use a Uin16Array as backing buffer.

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

[floitsch, 2013-03-05 14:28:55]

LGTM.

[Vyacheslav Egorov (Google), 2013-03-05 14:44:32]

I'd like to see that benchmark we are trying to optimize here.

https://codereview.chromium.org/12421002/diff/1/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/12421002/diff/1/runtime/lib/string.cc#newcode214
runtime/lib/string.cc:214: return String::FromUTF16(data_position, length,
Heap::kNew);
This is not GC safe and is against VM guidelines. 

String::FromUTF16 first allocates new string and then copies content into it. 

Which means your data might get moved to some other place under your feet, if GC
happens when new string is allocated and you will end up copying garbage.

In VM we only pass handles into functions (with a few exceptions), raw pointers
into the middle are not allowed.

[Lasse Reichstein Nielsen, 2013-03-05 15:34:58]

Thanks for the comments. I hope I have made it GC-safe now.

(The perfect solution would be converting the Uint16Array directly into a
String, but that's definitely out of my reach).

[srdjan, 2013-03-05 16:02:21]

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string.cc
File runtime/lib/string.cc (left):

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string.cc#oldc...
runtime/lib/string.cc:52: 
Why remove the line?

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string.cc#newc...
runtime/lib/string.cc:199:
DEFINE_NATIVE_ENTRY(StringBuffer_createStringFromUint16Array, 2) {
Check if you can create OneByteString instead of TwoByteString. The former are
much faster than the later ate the moment, and use much less memory.

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string.cc#newc...
runtime/lib/string.cc:204: if (length < 0 || length > array_len) {
Add parenthesis (length < 0) || (length > array_len)

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

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:89: static _Uint16Array _newBuffer(int
length) native "Uint16Array_new";
Why not use "new Uint16List" instead of _newBuffer? We can optimize on the
former, while the later is unrecognized and slow.

[Lasse Reichstein Nielsen, 2013-03-05 16:56:56]

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string.cc
File runtime/lib/string.cc (left):

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string.cc#oldc...
runtime/lib/string.cc:52: 
That was where I originally had the new method. Guess I cut one line too many
when moving it below. Will restore.

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string.cc#newc...
runtime/lib/string.cc:199:
DEFINE_NATIVE_ENTRY(StringBuffer_createStringFromUint16Array, 2) {
Will do.

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string.cc#newc...
runtime/lib/string.cc:204: if (length < 0 || length > array_len) {
Will do.

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

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:89: static _Uint16Array _newBuffer(int
length) native "Uint16Array_new";
It would require dart:core to import dart:scalarlist, increasing the footprint
of core. If that's acceptable, I'm all for it. :)

[Ivan Posva, 2013-03-05 17:50:55]

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

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:89: static _Uint16Array _newBuffer(int
length) native "Uint16Array_new";
On 2013/03/05 16:56:56, Lasse Reichstein Nielsen wrote:
> It would require dart:core to import dart:scalarlist, increasing the footprint
> of core. If that's acceptable, I'm all for it. :)

It would not increase the footprint of the VM as both dart:core and
dart:typeddata are loaded always.

[srdjan, 2013-03-05 17:53:20]

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

https://codereview.chromium.org/12421002/diff/6001/runtime/lib/string_buffer_...
runtime/lib/string_buffer_patch.dart:81: static String _create(List<int> buffer,
int length)
Also specify here Uint16List as well as the type of the _buffer.

[Lasse Reichstein Nielsen, 2013-03-07 09:15:20]

PTAL

[Vyacheslav Egorov (Google), 2013-03-07 13:50:52]

lgtm

please wait until Srdjan's comments.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:200: GET_NON_NULL_NATIVE_ARGUMENT(Uint16Array, a,
arguments->NativeArgAt(0));
I'd give a longer name to a.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:201: GET_NON_NULL_NATIVE_ARGUMENT(Smi, smiLength,
arguments->NativeArgAt(1));
smiLength -> length

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:204: int32_t length = smiLength.Value();
length -> length_value

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:214: uint16_t* data_position =
reinterpret_cast<uint16_t*>(
consider a.ByteAddr(0)

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:79: return _create(_buffer, _length,
_code_magnitude <= 0xFF);
consider:

final isLatin1 =  _code_magnitude <= 0xFF;
_create(..., isLatin1)

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:85: if (requiredCapacity <= _buffer.length)
return;
consider

  {
return;
}

and more vertical white space for readability.

[srdjan, 2013-03-07 17:58:34]

Addional comments

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc
File runtime/lib/string.cc (left):

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#old...
runtime/lib/string.cc:52: 
Please restore line

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:204: int32_t length = smiLength.Value();
intr32_t -> intptr_t; on x64 Smi is greater than int32_t.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:17: int _code_magnitude = 0;
s/_code_magnitude/_codeMagnitude/

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:82: /** Ensures that the buffer has enough
capacity to contain n code units. */
s/contain/add/

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:85: if (requiredCapacity <= _buffer.length)
return;
I would split the rare case out:
if (requiredCapacity > buffer.length) {
  _grow(requiredCapacity);
}

That way there is a larger chance that ensureCapacity gets inlined.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:88: newCapacity *= 2;
Optional: This growth may be too aggressive. Opinions?

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:98: static String _create(List<int> buffer,
int length, bool isLatin1)
s/List<int>/Uint16List/

[Lasse Reichstein Nielsen, 2013-03-08 07:18:44]

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc
File runtime/lib/string.cc (left):

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#old...
runtime/lib/string.cc:52: 
Done. *And* saving the buffer this time.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:200: GET_NON_NULL_NATIVE_ARGUMENT(Uint16Array, a,
arguments->NativeArgAt(0));
On 2013/03/07 13:50:52, Vyacheslav Egorov (Google) wrote:
> I'd give a longer name to a.

Done.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:201: GET_NON_NULL_NATIVE_ARGUMENT(Smi, smiLength,
arguments->NativeArgAt(1));
On 2013/03/07 13:50:52, Vyacheslav Egorov (Google) wrote:
> smiLength -> length

Done.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:204: int32_t length = smiLength.Value();
On 2013/03/07 13:50:52, Vyacheslav Egorov (Google) wrote:
> length -> length_value

Done.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:214: uint16_t* data_position =
reinterpret_cast<uint16_t*>(
I already tried that, but it seems to be private on Uint16Array. Could I somehow
cast it to a ByteArray, where ByteAddr is public?

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
File runtime/lib/string_buffer_patch.dart (right):

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:17: int _code_magnitude = 0;
On 2013/03/07 17:58:34, srdjan wrote:
> s/_code_magnitude/_codeMagnitude/

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:17: int _code_magnitude = 0;
_codeUnitMagnitude even.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:79: return _create(_buffer, _length,
_code_magnitude <= 0xFF);
On 2013/03/07 13:50:52, Vyacheslav Egorov (Google) wrote:
> consider:
> 
> final isLatin1 =  _code_magnitude <= 0xFF;
> _create(..., isLatin1)

Done.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:82: /** Ensures that the buffer has enough
capacity to contain n code units. */
On 2013/03/07 17:58:34, srdjan wrote:
> s/contain/add/

Done.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:85: if (requiredCapacity <= _buffer.length)
return;
On 2013/03/07 17:58:34, srdjan wrote:
> I would split the rare case out:
> if (requiredCapacity > buffer.length) {
>   _grow(requiredCapacity);
> }
> 
> That way there is a larger chance that ensureCapacity gets inlined.

Done.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:88: newCapacity *= 2;
I was wondering if it was too small, and considering *4 or more for the first
steps (16, 64, 256, 1024, maybe even skipping 64). It obviously depends on
usage, and I have no real data. For string literals in the V8 parser, growing
fast in the beginning was an advantage, but string literals are artificially
limited by line length, so they tended to be either short, ~72 chars or very
long.

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string_buffer...
runtime/lib/string_buffer_patch.dart:98: static String _create(List<int> buffer,
int length, bool isLatin1)
On 2013/03/07 17:58:34, srdjan wrote:
> s/List<int>/Uint16List/

Done.

[Ivan Posva, 2013-03-08 09:15:41]

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:214: uint16_t* data_position =
reinterpret_cast<uint16_t*>(
On 2013/03/08 07:18:44, Lasse Reichstein Nielsen wrote:
> I already tried that, but it seems to be private on Uint16Array. Could I
somehow
> cast it to a ByteArray, where ByteAddr is public?

I told Lasse to make the ByteAddr public. This is all changing with TypedData
anyway, we will have to properly deal with it then.

[Lasse Reichstein Nielsen, 2013-03-08 10:19:37]

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc
File runtime/lib/string.cc (right):

https://codereview.chromium.org/12421002/diff/14001/runtime/lib/string.cc#new...
runtime/lib/string.cc:214: uint16_t* data_position =
reinterpret_cast<uint16_t*>(
And it works with a public ByteAddr.

[Lasse Reichstein Nielsen, 2013-03-08 10:22:07]

Committed patchset #5 manually as r19679 (presubmit successful).

[srdjan, 2013-03-08 19:05:01]

lgtm