Add pointer cache field to external string for access in generated code.

Add pointer cache field to external string for access in generated code.

TEST=test/mjsunit/string-externalize.js

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

[Yang, 2011-11-16 14:19:59]

This replaces my previous CL to improve external string accesses.

The number of external strings (in Chromium) typically fluctuates around a few
hundreds to a few thousands, but rarely exceeds 10k, which is why adding another
field to cache the character array address is a small price to pay to enable
external strings for generated code. This speeds up things like
http://code.google.com/p/v8/issues/detail?id=1750 by a lot (330ms down to
100ms).

So far only ia32's string.charAt takes advantage of the pointer cache. Ports to
other string operations and architectures will follow if everything goes well.

[Lasse Reichstein, 2011-11-17 13:40:18]

LGTM.
Obviously it would be great to have it work in generated code  on X64 and ARM
too, but since this is a safe extension, the current code will work until the
optimization is ported. Nice!

http://codereview.chromium.org/8513010/diff/1012/src/code-stubs.h
File src/code-stubs.h (right):

http://codereview.chromium.org/8513010/diff/1012/src/code-stubs.h#newcode798
src/code-stubs.h:798: // result character.  We expect |index| to be not
smi-tagged.
"not smi-tagged" -> "untagged".

http://codereview.chromium.org/8513010/diff/1012/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/8513010/diff/1012/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:5156: __ Assert(zero, "external string expected, but
not found");
This test checks for "indirect string" but reports "not external". Seems to
ignore sequential strings. Make a comment here if we ruled those out earlier.

http://codereview.chromium.org/8513010/diff/1012/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:5164: __ cmp(FieldOperand(string,
HeapObject::kMapOffset),
Comparing against the same memory location twice in a row. Is there no register
free to load it?
Or, possibly, keep it alive since we first loaded it in line 5129?
But this'll probably not happen often enough to warrant a scratch register.

http://codereview.chromium.org/8513010/diff/1012/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/8513010/diff/1012/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:3377:
StringCharCodeAtGenerator::GenerateCharLoad(masm(),
If GenerateCharLoad is used from outside StringCharCodeAtGenerator, you might as
well make it a macro in macro-assembler-*.

http://codereview.chromium.org/8513010/diff/1012/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/8513010/diff/1012/src/objects-inl.h#newcode2321
src/objects-inl.h:2321: if (*data_field == NULL) *data_field =
resource()->data();
Alternatively, we could ensure that the data_field is always set (by setting it
when creating the string object). That allows us to avoid checking it on every
access.

http://codereview.chromium.org/8513010/diff/1012/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8513010/diff/1012/src/objects.cc#newcode6056
src/objects.cc:6056: return GetChars() + start;
Functions like this are so small, it would be great if they could be inlined,
i.e., if they were in objects-inl.h instead.

[Yang, 2011-11-17 17:06:23]

Landed in r10023.

http://codereview.chromium.org/8513010/diff/1012/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

http://codereview.chromium.org/8513010/diff/1012/src/ia32/code-stubs-ia32.cc#...
src/ia32/code-stubs-ia32.cc:5164: __ cmp(FieldOperand(string,
HeapObject::kMapOffset),
On 2011/11/17 13:40:18, Lasse Reichstein wrote:
> Comparing against the same memory location twice in a row. Is there no
register
> free to load it?
> Or, possibly, keep it alive since we first loaded it in line 5129?
> But this'll probably not happen often enough to warrant a scratch register.

|string| is used as a temp register, so I can use it as scratch at this point.

http://codereview.chromium.org/8513010/diff/1012/src/ia32/lithium-codegen-ia3...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/8513010/diff/1012/src/ia32/lithium-codegen-ia3...
src/ia32/lithium-codegen-ia32.cc:3377:
StringCharCodeAtGenerator::GenerateCharLoad(masm(),
On 2011/11/17 13:40:18, Lasse Reichstein wrote:
> If GenerateCharLoad is used from outside StringCharCodeAtGenerator, you might
as
> well make it a macro in macro-assembler-*.

Moved to codegen-*.cc instead. Big chunks of code that are shared only in a few
places belong there. Fast element transition code is put there already.

http://codereview.chromium.org/8513010/diff/1012/src/objects-inl.h
File src/objects-inl.h (right):

http://codereview.chromium.org/8513010/diff/1012/src/objects-inl.h#newcode2321
src/objects-inl.h:2321: if (*data_field == NULL) *data_field =
resource()->data();
On 2011/11/17 13:40:18, Lasse Reichstein wrote:
> Alternatively, we could ensure that the data_field is always set (by setting
it
> when creating the string object). That allows us to avoid checking it on every
> access.

I thought lazy init is nice. I'll check in another CL whether this has
advantages.

[fschneider, 2011-11-17 17:18:30]

Don't forget to update the corresponding issue in the issue tracker :)

On 2011/11/17 17:06:23, Yang wrote:
> Landed in r10023.
> 
> http://codereview.chromium.org/8513010/diff/1012/src/ia32/code-stubs-ia32.cc
> File src/ia32/code-stubs-ia32.cc (right):
> 
>
http://codereview.chromium.org/8513010/diff/1012/src/ia32/code-stubs-ia32.cc#...
> src/ia32/code-stubs-ia32.cc:5164: __ cmp(FieldOperand(string,
> HeapObject::kMapOffset),
> On 2011/11/17 13:40:18, Lasse Reichstein wrote:
> > Comparing against the same memory location twice in a row. Is there no
> register
> > free to load it?
> > Or, possibly, keep it alive since we first loaded it in line 5129?
> > But this'll probably not happen often enough to warrant a scratch register.
> 
> |string| is used as a temp register, so I can use it as scratch at this point.
> 
>
http://codereview.chromium.org/8513010/diff/1012/src/ia32/lithium-codegen-ia3...
> File src/ia32/lithium-codegen-ia32.cc (right):
> 
>
http://codereview.chromium.org/8513010/diff/1012/src/ia32/lithium-codegen-ia3...
> src/ia32/lithium-codegen-ia32.cc:3377:
> StringCharCodeAtGenerator::GenerateCharLoad(masm(),
> On 2011/11/17 13:40:18, Lasse Reichstein wrote:
> > If GenerateCharLoad is used from outside StringCharCodeAtGenerator, you
might
> as
> > well make it a macro in macro-assembler-*.
> 
> Moved to codegen-*.cc instead. Big chunks of code that are shared only in a
few
> places belong there. Fast element transition code is put there already.
> 
> http://codereview.chromium.org/8513010/diff/1012/src/objects-inl.h
> File src/objects-inl.h (right):
> 
> http://codereview.chromium.org/8513010/diff/1012/src/objects-inl.h#newcode2321
> src/objects-inl.h:2321: if (*data_field == NULL) *data_field =
> resource()->data();
> On 2011/11/17 13:40:18, Lasse Reichstein wrote:
> > Alternatively, we could ensure that the data_field is always set (by setting
> it
> > when creating the string object). That allows us to avoid checking it on
every
> > access.
> 
> I thought lazy init is nice. I'll check in another CL whether this has
> advantages.