Use direct copy and templates to speed up flattening of strings.

Use direct copy and templates to speed up flattening of strings.

Committed: http://code.google.com/p/v8/source/detail?r=549

[Christian Plesner Hansen, 2008-10-21 14:16:22]

Lgtm with a whole bunch of comments.

http://codereview.chromium.org/8011/diff/1/2
File src/heap.cc (right):

http://codereview.chromium.org/8011/diff/1/2#newcode1359
Line 1359: byte* dest = SeqAsciiString::cast(result)->GetCharsAddress();
There is some confusion about whether to use char or byte for ascii characters. 
We should either use char consistently or introduce a new type (say uc8).

http://codereview.chromium.org/8011/diff/1/7
File src/objects-inl.h (right):

http://codereview.chromium.org/8011/diff/1/7#newcode1372
Line 1372: int String::full_representation_tag() {
We have a lot of these methods with lower-case names that do nontrivial work. 
We have to be sure they're really fast, otherwise we should rename them to upper
camel case names.

http://codereview.chromium.org/8011/diff/1/3
File src/objects.cc (right):

http://codereview.chromium.org/8011/diff/1/3#newcode547
Line 547: SeqTwoByteString::cast(result)->GetCharsAddress());
Maybe add an accessor on seq two byte string that returns its chars viewed under
the right type.  I expect we could use that in other places too.

http://codereview.chromium.org/8011/diff/1/3#newcode2914
Line 2914: const byte* start = reinterpret_cast<const
byte*>(ext->resource()->data());
Bleh.  Why not stick with char?

http://codereview.chromium.org/8011/diff/1/3#newcode3591
Line 3591: memcpy(
Could this be simplified by using a template-ified memcpy with a specialization
for each character type?

http://codereview.chromium.org/8011/diff/1/3#newcode3606
Line 3606: case kTwoByteStringTag + kExternalStringTag: {
If | gives the same result as + we should use that instead -- but maybe it
doesn't?

http://codereview.chromium.org/8011/diff/1/3#newcode3616
Line 3616: const byte* src =
Isn't this the branch where we can memcpy?  Source is a sequential ascii string
and sizeof(sinkchar) is 1.

http://codereview.chromium.org/8011/diff/1/3#newcode3623
Line 3623: Cpy168(
Conversely, this looks to me like we're memcpying from a seq ascii string into
an uc16 array.

http://codereview.chromium.org/8011/diff/1/3#newcode3652
Line 3652: if (boundary < 64 || to - boundary >= boundary - from) {
Magic number.  Please define a constant.

http://codereview.chromium.org/8011/diff/1/3#newcode3748
Line 3748: VectorIterator<const byte> ib(b->ToAsciiVector());
There's no need to explicitly add 'const' to a Vector iterator.  A
VectorIterator<T> holds a Vector<const T>.

http://codereview.chromium.org/8011/diff/1/6
File src/utils.h (right):

http://codereview.chromium.org/8011/diff/1/6#newcode451
Line 451: static inline void Cpy168(sinkchar* dest, const unsigned char* src,
int chars) {
You may consider copying the first parts of the string in int-size blocks.

http://codereview.chromium.org/8011/diff/1/6#newcode452
Line 452: ASSERT(sizeof(sinkchar) == 2);
You can use STATIC_CHECK here to catch this at compiletime.

[Erik Corry, 2008-10-22 11:59:48]

http://codereview.chromium.org/8011/diff/1/2
File src/heap.cc (right):

http://codereview.chromium.org/8011/diff/1/2#newcode1359
Line 1359: byte* dest = SeqAsciiString::cast(result)->GetCharsAddress();
I've tried to use char consistently, since the name fits the use and the
problems with signed vs. unsigned char shouldn't matter for ASCII.

http://codereview.chromium.org/8011/diff/1/7
File src/objects-inl.h (right):

http://codereview.chromium.org/8011/diff/1/7#newcode1372
Line 1372: int String::full_representation_tag() {
This one is certainly trivial.

http://codereview.chromium.org/8011/diff/1/3
File src/objects.cc (right):

http://codereview.chromium.org/8011/diff/1/3#newcode547
Line 547: SeqTwoByteString::cast(result)->GetCharsAddress());
Good idea.

http://codereview.chromium.org/8011/diff/1/3#newcode2914
Line 2914: const byte* start = reinterpret_cast<const
byte*>(ext->resource()->data());
Done

http://codereview.chromium.org/8011/diff/1/3#newcode3591
Line 3591: memcpy(
Done.

http://codereview.chromium.org/8011/diff/1/3#newcode3606
Line 3606: case kTwoByteStringTag + kExternalStringTag: {
Done

http://codereview.chromium.org/8011/diff/1/3#newcode3616
Line 3616: const byte* src =
The non-memcpy may be marginally faster, but this is removed in the new version.

http://codereview.chromium.org/8011/diff/1/3#newcode3623
Line 3623: Cpy168(
Gone in new version.

http://codereview.chromium.org/8011/diff/1/3#newcode3652
Line 3652: if (boundary < 64 || to - boundary >= boundary - from) {
I got rid of this optimization because I found a test failure and the fix would
probably deoptimize the optimization.

http://codereview.chromium.org/8011/diff/1/3#newcode3748
Line 3748: VectorIterator<const byte> ib(b->ToAsciiVector());
OK

http://codereview.chromium.org/8011/diff/1/6
File src/utils.h (right):

http://codereview.chromium.org/8011/diff/1/6#newcode451
Line 451: static inline void Cpy168(sinkchar* dest, const unsigned char* src,
int chars) {
Can't do this because of alignment issues.