Serendipitously arrange the tags so that String.length() becomes a branch-fre...

Serendipitously arrange the tags so that String.length() becomes a branch-free
operation.  I have another version of this change that does not remove the
special inline caches for difference sized strings.  The other version is ever
so slightly faster, but the nice thing about this version is that it removes
253 lines of code.

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

[Kasper Lund, 2008-10-27 08:27:31]

I think you should let Mads take a look at the specifics of the length encoding.
I have more general comments:

1. Does this work on ARM?
2. Does it lint?

http://codereview.chromium.org/8187/diff/1/12
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/8187/diff/1/12#newcode2859
Line 2859: // The code assumes the tags are serendpitous.
Please choose a better word than serendpitous. Preferably one you can spell :-)

http://codereview.chromium.org/8187/diff/1/12#newcode2861
Line 2861: ASSERT(kMediumStringTag + String::kLongLengthShift ==
String::kMediumLengthShift);
Is this line too long?

http://codereview.chromium.org/8187/diff/1/12#newcode2862
Line 2862: ASSERT(kShortStringTag + String::kLongLengthShift ==
String::kShortLengthShift);
Ditto?

[Mads Ager (chromium), 2008-10-27 09:37:00]

LGTM.  

The new encoding is quite clever and it could use a comment in objects.h.

Let's hope we don't have to change the encoding much.

http://codereview.chromium.org/8187/diff/1/12
File src/codegen-ia32.cc (right):

http://codereview.chromium.org/8187/diff/1/12#newcode2868
Line 2868: __ shr(edx);  // Ecx is implicit operand.
Since Ecx is a register name, I probably would not capitalize it.

http://codereview.chromium.org/8187/diff/1/8
File src/objects.h (right):

http://codereview.chromium.org/8187/diff/1/8#newcode398
Line 398: // If bit 7 is clear, bits 5 and 6 are the string's size (short,
medium or long).
This is now bits 4 and 5?

Also, you should add a comment explaining that you use this encoding so that you
can perform the length shifts based on the tags in the instance type.

http://codereview.chromium.org/8187/diff/1/8#newcode404
Line 404: // If bit 7 is clear, bit 4 indicates that the string is a symbol (if
set) or
And this is now bit 5?

http://codereview.chromium.org/8187/diff/1/8#newcode417
Line 417: // If bit 7 is clear, the low-order 3 bits indicate the representation
3 -> 2 bits.

http://codereview.chromium.org/8187/diff/1/8#newcode3157
Line 3157: static const int kMaxMediumStringSize = 16383;
These reductions in the limits is the only thing that worries me.  It means that
a lot of strings could potentially have worse hash-codes.

http://codereview.chromium.org/8187/diff/1/8#newcode3183
Line 3183: static const int kShortLengthShift = 26;
Could we write these as kShortStringTag + kRepresentationTagSize and
kMediumStringTag + kRepresentationTagSize.  That would make it clearer to me
what the encoding is.

http://codereview.chromium.org/8187/diff/1/2
File src/stub-cache-ia32.cc (right):

http://codereview.chromium.org/8187/diff/1/2#newcode182
Line 182: // Ecx is also the receiver.
Ecx -> ecx since it is a register name?

http://codereview.chromium.org/8187/diff/1/2#newcode184
Line 184: __ shr(eax);  // Ecx is implicit shift register.
Ditto?