Remove the different length string types...

Remove the different length string types

The different length string types was used to encode the string length and the hash in one field. This is now split into two fields one for length and one for hash. The hash field still encodes the array index of the string if it has one. If an array index is encoded in the hash field the string length is added to the top bits of the hash field to avoid a hash value of zero.

On 32-bit this causes an additional 4 bytes to be used for all string objects. On 64-bit this will be half on average dur to pointer alignment.

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

[Søren Thygesen Gjesse, 2009-11-23 08:32:42]

The change in test/cctest/test-alloc.cc is due to an unknown issue on the ARM
simulator. I will open a bug on this.

[Mads Ager (chromium), 2009-11-23 13:56:45]

LGTM with a couple of nits.

http://codereview.chromium.org/436001/diff/2001/2012
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/436001/diff/2001/2012#newcode316
src/ia32/ic-ia32.cc:316: // Array index string: If short enough use cache in
length/hash field (ebx).
We need to look for formulations like these and get rid of them.

http://codereview.chromium.org/436001/diff/2001/2012#newcode323
src/ia32/ic-ia32.cc:323: const int kLengthFieldLimit =
I'm getting confused by the naming here.  We are talking about the hash field
here and not the length field, right?

http://codereview.chromium.org/436001/diff/2001/2014
File src/objects.h (right):

http://codereview.chromium.org/436001/diff/2001/2014#newcode225
src/objects.h:225: V(SYMBOL_TYPE)                          \
Please align the '\' on the right as before.

http://codereview.chromium.org/436001/diff/2001/2014#newcode298
src/objects.h:298: V(SYMBOL_TYPE,                                               
         \
Alignment again.

http://codereview.chromium.org/436001/diff/2001/2014#newcode3881
src/objects.h:3881: // For strings which are array indexes the hash value has
the
Complete comment please.  I got confused by the ArrayIndexHashLengthShift, but I
see from the implementation why it is there.  The comment should explain that.

[Erik Corry, 2009-11-24 08:26:37]

STV (with comments)

http://codereview.chromium.org/436001/diff/2001/2023
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/436001/diff/2001/2023#newcode113
src/arm/ic-arm.cc:113: __ add(t1, t1,
Operand(StringDictionary::GetProbeOffset(i)));
If you add the probe offset shifted left here then you can omit the LSR above
and instead put it in the and instruction below.

http://codereview.chromium.org/436001/diff/2001/2022
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/436001/diff/2001/2022#newcode232
src/arm/stub-cache-arm.cc:232: __ ldr(r0, FieldMemOperand(receiver,
String::kLengthOffset));
isn't this already smi tagged?

http://codereview.chromium.org/436001/diff/2001/2006
File src/heap.cc (right):

http://codereview.chromium.org/436001/diff/2001/2006#newcode1847
src/heap.cc:1847: cons_string->set_hash_field(0);
we should have a constant that means hash not yet calculated

http://codereview.chromium.org/436001/diff/2001/2006#newcode2618
src/heap.cc:2618: // Set length and hash field of the allocated string.
field -> fields

http://codereview.chromium.org/436001/diff/2001/2006#newcode2655
src/heap.cc:2655: String::cast(result)->set_hash_field(0);
And here

http://codereview.chromium.org/436001/diff/2001/2006#newcode2683
src/heap.cc:2683: String::cast(result)->set_hash_field(0);
And here

http://codereview.chromium.org/436001/diff/2001/2025
File src/heap.h (right):

http://codereview.chromium.org/436001/diff/2001/2025#newcode74
src/heap.h:74: V(Map, undetectable_string_map, UndetectableShortStringMap)      
           \
why are these still short?

http://codereview.chromium.org/436001/diff/2001/2016
File src/utils.h (right):

http://codereview.chromium.org/436001/diff/2001/2016#newcode588
src/utils.h:588: int TenToThe(int exponent);
Is this used with a non-constant exponent.  For constant exponents the 1e5
notation is more compact and simpler.

http://codereview.chromium.org/436001/diff/2001/2019
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/436001/diff/2001/2019#newcode3772
src/x64/codegen-x64.cc:3772: // Check for index out of range.
Looks like you can omit the movl and just put the field operand in the cmpl

[Søren Thygesen Gjesse, 2009-11-24 13:19:17]

http://codereview.chromium.org/436001/diff/2001/2023
File src/arm/ic-arm.cc (right):

http://codereview.chromium.org/436001/diff/2001/2023#newcode113
src/arm/ic-arm.cc:113: __ add(t1, t1,
Operand(StringDictionary::GetProbeOffset(i)));
On 2009/11/24 08:26:37, Erik Corry wrote:
> If you add the probe offset shifted left here then you can omit the LSR above
> and instead put it in the and instruction below.

Done.

http://codereview.chromium.org/436001/diff/2001/2022
File src/arm/stub-cache-arm.cc (right):

http://codereview.chromium.org/436001/diff/2001/2022#newcode232
src/arm/stub-cache-arm.cc:232: __ ldr(r0, FieldMemOperand(receiver,
String::kLengthOffset));
On 2009/11/24 08:26:37, Erik Corry wrote:
> isn't this already smi tagged?

No.

http://codereview.chromium.org/436001/diff/2001/2006
File src/heap.cc (right):

http://codereview.chromium.org/436001/diff/2001/2006#newcode1847
src/heap.cc:1847: cons_string->set_hash_field(0);
On 2009/11/24 08:26:37, Erik Corry wrote:
> we should have a constant that means hash not yet calculated

Done added String::kEmptyHashField.

http://codereview.chromium.org/436001/diff/2001/2006#newcode2618
src/heap.cc:2618: // Set length and hash field of the allocated string.
On 2009/11/24 08:26:37, Erik Corry wrote:
> field -> fields

Done.

http://codereview.chromium.org/436001/diff/2001/2006#newcode2655
src/heap.cc:2655: String::cast(result)->set_hash_field(0);
On 2009/11/24 08:26:37, Erik Corry wrote:
> And here

Done (and in three other places as well).

http://codereview.chromium.org/436001/diff/2001/2006#newcode2683
src/heap.cc:2683: String::cast(result)->set_hash_field(0);
On 2009/11/24 08:26:37, Erik Corry wrote:
> And here

Done.

http://codereview.chromium.org/436001/diff/2001/2025
File src/heap.h (right):

http://codereview.chromium.org/436001/diff/2001/2025#newcode74
src/heap.h:74: V(Map, undetectable_string_map, UndetectableShortStringMap)      
           \
On 2009/11/24 08:26:37, Erik Corry wrote:
> why are these still short?

No more they are.

http://codereview.chromium.org/436001/diff/2001/2012
File src/ia32/ic-ia32.cc (right):

http://codereview.chromium.org/436001/diff/2001/2012#newcode316
src/ia32/ic-ia32.cc:316: // Array index string: If short enough use cache in
length/hash field (ebx).
On 2009/11/23 13:56:46, Mads Ager wrote:
> We need to look for formulations like these and get rid of them.

Done, found the same in the x64 version.

http://codereview.chromium.org/436001/diff/2001/2012#newcode323
src/ia32/ic-ia32.cc:323: const int kLengthFieldLimit =
On 2009/11/23 13:56:46, Mads Ager wrote:
> I'm getting confused by the naming here.  We are talking about the hash field
> here and not the length field, right?

I removed the length field limit check which did not make any sense with the new
object layout and is not required anyway. Did the same in the x64 version. This
part of the keyed load ic is not implemented on ARM.

http://codereview.chromium.org/436001/diff/2001/2014
File src/objects.h (right):

http://codereview.chromium.org/436001/diff/2001/2014#newcode225
src/objects.h:225: V(SYMBOL_TYPE)                          \
On 2009/11/23 13:56:46, Mads Ager wrote:
> Please align the '\' on the right as before.

Done.

http://codereview.chromium.org/436001/diff/2001/2014#newcode298
src/objects.h:298: V(SYMBOL_TYPE,                                               
         \
On 2009/11/23 13:56:46, Mads Ager wrote:
> Alignment again.

Done.

http://codereview.chromium.org/436001/diff/2001/2014#newcode3881
src/objects.h:3881: // For strings which are array indexes the hash value has
the
On 2009/11/23 13:56:46, Mads Ager wrote:
> Complete comment please.  I got confused by the ArrayIndexHashLengthShift, but
I
> see from the implementation why it is there.  The comment should explain that.

Finished the comment, and changed the line to

static const int kArrayIndexHashLengthShift = 24 + kNofLengthBitFields;

to show that 24 bits are reserved for the array index value.

http://codereview.chromium.org/436001/diff/2001/2016
File src/utils.h (right):

http://codereview.chromium.org/436001/diff/2001/2016#newcode588
src/utils.h:588: int TenToThe(int exponent);
On 2009/11/24 08:26:37, Erik Corry wrote:
> Is this used with a non-constant exponent.  For constant exponents the 1e5
> notation is more compact and simpler.

It is used with String::kMaxCachedArrayIndexLength (which is currently 7)

http://codereview.chromium.org/436001/diff/2001/2019
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/436001/diff/2001/2019#newcode3772
src/x64/codegen-x64.cc:3772: // Check for index out of range.
On 2009/11/24 08:26:37, Erik Corry wrote:
> Looks like you can omit the movl and just put the field operand in the cmpl

Done.