Speed up FastAsciiArrayJoin on ia32 by improving hand-written assembly code.

Speed up FastAsciiArrayJoin on ia32 by improving hand-written assembly code.

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

[Lasse Reichstein, 2011-01-13 09:20:32]

LGTM.
Have you considered doing the copying using rep movsb and compare the
speed/size?

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3359: loop_3b, loop_3b_condition;
That's a lot of loops. Could they be given more telling names? Or give an
explanation of the algorithm here, so we can see which loops do what.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3398: 1 << Map::kHasFastElements);
Will Copy-on-Write arrays have fast elements if queried this way?

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3422: // Check that all array elements are
sequential ascii strings, and
ascii -> ASCII.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3423: // accumulate the sum of their lengths.
Say that the sum is maintained as a Smi value.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3440: __ mov_b(scratch, FieldOperand(scratch,
Map::kInstanceTypeOffset));
Use movzx_b to avoid depending on the remaining bits, or use and_b and cmp_b
below.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3472: // Check that the separator is a flat ascii
string.
ASCII.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3487: __ sub(current_string_length,
Operand(scratch));
Perhaps worth noting that this can (and may) give a negative result.

[William Hesse, 2011-01-13 16:25:59]

Comments addressed, CopyBytes function factored out, and optimized.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3359: loop_3b, loop_3b_condition;
On 2011/01/13 09:20:32, Lasse Reichstein wrote:
> That's a lot of loops. Could they be given more telling names? Or give an
> explanation of the algorithm here, so we can see which loops do what.

Loops eliminated by making the string copy a macro-assembler function.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3398: 1 << Map::kHasFastElements);
On 2011/01/13 09:20:32, Lasse Reichstein wrote:
> Will Copy-on-Write arrays have fast elements if queried this way?

Yes, they will (I checked).

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3422: // Check that all array elements are
sequential ascii strings, and
On 2011/01/13 09:20:32, Lasse Reichstein wrote:
> ascii -> ASCII.

Both forms occur in our comments, about 1/3 lowercase, 2/3 uppercase.
I prefer lowercase.
But done.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3423: // accumulate the sum of their lengths.
On 2011/01/13 09:20:32, Lasse Reichstein wrote:
> Say that the sum is maintained as a Smi value.

Done.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3440: __ mov_b(scratch, FieldOperand(scratch,
Map::kInstanceTypeOffset));
On 2011/01/13 09:20:32, Lasse Reichstein wrote:
> Use movzx_b to avoid depending on the remaining bits, or use and_b and cmp_b
> below.

done.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3472: // Check that the separator is a flat ascii
string.
On 2011/01/13 09:20:32, Lasse Reichstein wrote:
> ASCII.

Done.

http://codereview.chromium.org/6148007/diff/6001/src/ia32/full-codegen-ia32.c...
src/ia32/full-codegen-ia32.cc:3487: __ sub(current_string_length,
Operand(scratch));
On 2011/01/13 09:20:32, Lasse Reichstein wrote:
> Perhaps worth noting that this can (and may) give a negative result.

Done.

[Lasse Reichstein, 2011-01-14 10:47:37]

LGTM

http://codereview.chromium.org/6148007/diff/11001/src/ia32/macro-assembler-ia...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/6148007/diff/11001/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:902: // Because destination is 4-byte aligned,
we keep it aligned for movs.
How do we know that destination is 4-byte aligned?
Do you mean "If destination is 4-byte aligned"?

http://codereview.chromium.org/6148007/diff/11001/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:906: shr(ecx, 2);
If length was divisible by four, you will copy the last word twice.
Subtract one from ecx before shifting, to copy one less word if (length & 3) ==
0, but not otherwise.

http://codereview.chromium.org/6148007/diff/11001/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:921: dec(length);
This won't be faster if you do:

 add(source, length);
 add(destination, length);
 neg(length);
 bind(&short_loop);
 mov_b(scratch, Operand(source, ecx));
 mov_b(Operand(destination, ecx), scratch);
 inc(ecx);
 j(not_zero, &short_loop);

(It's quite possible that it won't. There's probably lots of latency available
during reading and writing for updating all three registers, and it's actually
one instruction longer in total).

[William Hesse, 2011-01-14 10:54:44]

http://codereview.chromium.org/6148007/diff/11001/src/ia32/macro-assembler-ia...
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/6148007/diff/11001/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:902: // Because destination is 4-byte aligned,
we keep it aligned for movs.
On 2011/01/14 10:47:37, Lasse Reichstein wrote:
> How do we know that destination is 4-byte aligned?
> Do you mean "If destination is 4-byte aligned"?

In our uses, source is 4-byte aligned.
Changed comment.

http://codereview.chromium.org/6148007/diff/11001/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:906: shr(ecx, 2);
Long rep.movs averages much less than a cycle per word.  This would save one
word only 1/4 of the time.  Not worth the extra code, I think.

On 2011/01/14 10:47:37, Lasse Reichstein wrote:
> If length was divisible by four, you will copy the last word twice.
> Subtract one from ecx before shifting, to copy one less word if (length & 3)
==
> 0, but not otherwise.

http://codereview.chromium.org/6148007/diff/11001/src/ia32/macro-assembler-ia...
src/ia32/macro-assembler-ia32.cc:921: dec(length);
I tried that - it was slower. On 2011/01/14 10:47:37, Lasse Reichstein wrote:
> This won't be faster if you do:
> 
>  add(source, length);
>  add(destination, length);
>  neg(length);
>  bind(&short_loop);
>  mov_b(scratch, Operand(source, ecx));
>  mov_b(Operand(destination, ecx), scratch);
>  inc(ecx);
>  j(not_zero, &short_loop);
> 
> (It's quite possible that it won't. There's probably lots of latency available
> during reading and writing for updating all three registers, and it's actually
> one instruction longer in total).