Tentative implementation of string slices (hidden under the flag --string-slices).

Tentative implementation of string slices (hidden under the flag --string-slices).
TEST=test/mjsunit/string-slices.js

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

[Yang, 2011-07-27 11:51:57]

PTAL.

It's probably best to start with objects.h for the definition of SlicedString
and go from there.

I haven't implemented generated code for creating substrings yet. Slices can
only be created in the runtime system for now. I just adapted a lot of other
things so that slices works with existing stuff.

http://codereview.chromium.org/7477045/diff/1/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7477045/diff/1/src/arm/code-stubs-arm.cc#newco...
src/arm/code-stubs-arm.cc:5395: 
Do not create string slice unless experimental flag is set.

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

http://codereview.chromium.org/7477045/diff/1/src/ia32/code-stubs-ia32.cc#new...
src/ia32/code-stubs-ia32.cc:5628: 
Do not create string slice unless experimental flag is set.

http://codereview.chromium.org/7477045/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7477045/diff/1/src/x64/code-stubs-x64.cc#newco...
src/x64/code-stubs-x64.cc:4545: 
Do not create string slice unless experimental flag is set.

http://codereview.chromium.org/7477045/diff/1/test/mjsunit/string-slices.js
File test/mjsunit/string-slices.js (right):

http://codereview.chromium.org/7477045/diff/1/test/mjsunit/string-slices.js#n...
test/mjsunit/string-slices.js:27: 
Mostly a copy of test/mjsunit/substr.js, but using --string-slices flag and a
few additional tests.

[antonm, 2011-07-27 12:16:39]

+VItalyr who did a lot of string work.

First round of comments.

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

http://codereview.chromium.org/7477045/diff/1/src/heap.cc#newcode2687
src/heap.cc:2687: WriteBarrierMode mode =
sliced_string->GetWriteBarrierMode(no_gc);
sliced_string should be allocated in new space, so do we need a write barrier at
all?

http://codereview.chromium.org/7477045/diff/1/src/heap.cc#newcode2692
src/heap.cc:2692: sliced_string->set_parent(cons->first(), mode);
why first?  what if I slice in the second part?

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

http://codereview.chromium.org/7477045/diff/1/src/objects-inl.h#newcode190
src/objects-inl.h:190: return (type & (kIsNotStringMask |
kStringRepresentationMask)) ==
probably is worth refactoring.

http://codereview.chromium.org/7477045/diff/1/src/objects-inl.h#newcode2269
src/objects-inl.h:2269: WRITE_FIELD(this, kParentOffset, parent);
maybe add an assert that parent is never a sliced string/external string.

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

http://codereview.chromium.org/7477045/diff/1/src/objects.h#newcode494
src/objects.h:494: const uint32_t kIsIndirectStringTag = 0x1;
maybe worth adding some documentation why it works.  And would probably added a
helper function right here which checks if type is indirect.

http://codereview.chromium.org/7477045/diff/1/test/mjsunit/string-slices.js
File test/mjsunit/string-slices.js (right):

http://codereview.chromium.org/7477045/diff/1/test/mjsunit/string-slices.js#n...
test/mjsunit/string-slices.js:155: //Test charAt for different strings.
nit: missing space before Test.

http://codereview.chromium.org/7477045/diff/1/test/mjsunit/string-slices.js#n...
test/mjsunit/string-slices.js:159: for ( var i = 0; i < 50; i++) {
nit: unnecessary space after ( (here and below)

[Yang, 2011-07-27 12:47:24]

Made a few changes according to Anton's suggestions.

http://codereview.chromium.org/7477045/diff/6002/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/6002/src/heap.cc#newcode2691
src/heap.cc:2691: sliced_string->set_parent(cons->first());
There is no second part. I assert in line 2674 that the string is flat, meaning
that it is either sequential, sliced or a flattened cons. Flattened cons strings
only have a first(), which is a sequential string. Its second() is empty and has
length 0.

[antonm, 2011-07-27 14:04:49]

http://codereview.chromium.org/7477045/diff/6002/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7477045/diff/6002/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4804: __ tst(result_,
Operand(kStringRepresentationMask));
Do we have a spare register here?  If yes, we can probably and_ into it to save
and_ below.

http://codereview.chromium.org/7477045/diff/6002/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4839: __ ldrb(result_, FieldMemOperand(result_,
Map::kInstanceTypeOffset));
maybe add an assert that result_ is always a flat string.

http://codereview.chromium.org/7477045/diff/6002/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:5938: STATIC_ASSERT(SlicedString::kMinLength >=
String::kMinNonFlatLength);
again, might be worth adding a generated code check, something like if
(FLAG_debug_code) { ... }

http://codereview.chromium.org/7477045/diff/6002/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7477045/diff/6002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:3427: Register offset =
ToRegister(instr->TempAt(0));
do you need offset, cannot you live only with scratch?

http://codereview.chromium.org/7477045/diff/6002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:3458: __ tst(result,
Operand(kStringRepresentationMask));
here you have another scratch---offset.  so optimization should apply

http://codereview.chromium.org/7477045/diff/6002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:3472: __ mov(offset, Operand(result, LSR,
kSmiTagSize));
do you want to untag here?  if you defer it to lookup, it apparently will take
less instructions.

http://codereview.chromium.org/7477045/diff/6002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:3486: Operand(SeqTwoByteString::kHeaderSize -
kHeapObjectTag));
you probably may encode const_index into this constant.

http://codereview.chromium.org/7477045/diff/6002/src/arm/lithium-codegen-arm....
src/arm/lithium-codegen-arm.cc:3492: Operand(SeqAsciiString::kHeaderSize -
kHeapObjectTag));
ditto

http://codereview.chromium.org/7477045/diff/6002/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/6002/src/heap.cc#newcode2691
src/heap.cc:2691: sliced_string->set_parent(cons->first());
On 2011/07/27 12:47:24, Yang wrote:
> There is no second part. I assert in line 2674 that the string is flat,
meaning
> that it is either sequential, sliced or a flattened cons. Flattened cons
strings
> only have a first(), which is a sequential string. Its second() is empty and
has
> length 0.

Thanks a lot for explanation!  Maybe worth asserting that the second part is
empty here too to ease reading and maintaince.

http://codereview.chromium.org/7477045/diff/6002/src/heap.cc#newcode2691
src/heap.cc:2691: sliced_string->set_parent(cons->first());
just curious:  is it possible to get:

 cons string {
    first: const string;
    second: empty string;
}

I think it should be doable, but it will break your invariants.

May you add a test?

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

http://codereview.chromium.org/7477045/diff/6002/src/objects-inl.h#newcode190
src/objects-inl.h:190: uint32_t representation =
if you don't want to introduce helper function which can be reused by both
IsConstStrng and IsSlicedString, I think they should be as similar as possible.

http://codereview.chromium.org/7477045/diff/6002/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7477045/diff/6002/src/objects.cc#newcode5250
src/objects.cc:5250: string_tag = StringShape(string).representation_tag();
not related to your change, but I am curious what will happen if have the
following structure:

cons string
  first -> const string
  second -> empty

http://codereview.chromium.org/7477045/diff/6002/src/objects.cc#newcode5253
src/objects.cc:5253: // Note that a sliced string cannot contain an external
string.
maybe be more elaborate why it's needed.

http://codereview.chromium.org/7477045/diff/6002/src/objects.cc#newcode5964
src/objects.cc:5964: *offset_ptr += offset;
looks sketchy, but I am not sure I am aware of better solution.

http://codereview.chromium.org/7477045/diff/6002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7477045/diff/6002/src/objects.h#newcode498
src/objects.h:498: (kSlicedStringTag & kIsIndirectStringMask) ==
kIsIndirectStringTag);
sorry, up to you, but I would code it like:

bool is_indirect_string(SRT tag) {
  ASSERT((tag & kIISM == kIIST) == ((tag == kConstString) || (tag ==
kSlicedString));
  return tag & kIISM == kIIST;
}

And reuse it in heap.h

http://codereview.chromium.org/7477045/diff/6002/src/objects.h#newcode524
src/objects.h:524: SLICED_SYMBOL_TYPE = kTwoByteStringTag | kSymbolTag |
kSlicedStringTag,
Do we want sliced symbols?

[Vitaly Repeshko, 2011-07-27 15:27:24]

There's an issue with string externalization. We can't rely on parent staying a
sequential string. It may be externalized and change its encoding. This makes
slices similar to conses, we have to check the parent's type separately.

I don't think we want to prohibit externalization of parents. In case they are
huge strings we risk wasting lots of memory when passing them from V8 to the
embedder.

[Rico, 2011-07-28 06:49:50]

http://codereview.chromium.org/7477045/diff/6002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7477045/diff/6002/src/objects.h#newcode524
src/objects.h:524: SLICED_SYMBOL_TYPE = kTwoByteStringTag | kSymbolTag |
kSlicedStringTag,
On 2011/07/27 14:04:49, antonm wrote:
> Do we want sliced symbols?
I am a little concerned about this as well, this could potentially keep strings
alive for a lot longer since these are often identifier names, e.g., if json (or
eval for that sake) is used for loading dynamic content we will keep the string
alive where the identifiers are first described potentially long after the
original objects died.

[Yang, 2011-07-28 15:43:06]

I addressed a few suggestions by Anton and removed the constants for symbols.
I'm still thinking about how to take care of externalizing strings.

http://codereview.chromium.org/7477045/diff/6002/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7477045/diff/6002/src/arm/code-stubs-arm.cc#ne...
src/arm/code-stubs-arm.cc:4804: __ tst(result_,
Operand(kStringRepresentationMask));
Unfortunately, I don't see anything here. index_ is read only.

On 2011/07/27 14:04:49, antonm wrote:
> Do we have a spare register here?  If yes, we can probably and_ into it to
save
> and_ below.

http://codereview.chromium.org/7477045/diff/6002/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/6002/src/heap.cc#newcode2691
src/heap.cc:2691: sliced_string->set_parent(cons->first());
There is no way to create such a cons string. The only way to get an empty
second string would to flatten the cons string so that the first string is a seq
string and the cons string itself is merely a wrapper.

On 2011/07/27 14:04:49, antonm wrote:
> just curious:  is it possible to get:
> 
>  cons string {
>     first: const string;
>     second: empty string;
> }
> 
> I think it should be doable, but it will break your invariants.
> 
> May you add a test?

http://codereview.chromium.org/7477045/diff/6002/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7477045/diff/6002/src/objects.cc#newcode5250
src/objects.cc:5250: string_tag = StringShape(string).representation_tag();
Ditto. Said structure is impossible to get. I'll add an assert here.

On 2011/07/27 14:04:49, antonm wrote:
> not related to your change, but I am curious what will happen if have the
> following structure:
> 
> cons string
>   first -> const string
>   second -> empty

http://codereview.chromium.org/7477045/diff/6002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7477045/diff/6002/src/objects.h#newcode498
src/objects.h:498: (kSlicedStringTag & kIsIndirectStringMask) ==
kIsIndirectStringTag);
There is only one place where this tag is actually used. I put the assertion
there.

On 2011/07/27 14:04:49, antonm wrote:
> sorry, up to you, but I would code it like:
> 
> bool is_indirect_string(SRT tag) {
>   ASSERT((tag & kIISM == kIIST) == ((tag == kConstString) || (tag ==
> kSlicedString));
>   return tag & kIISM == kIIST;
> }
> 
> And reuse it in heap.h

http://codereview.chromium.org/7477045/diff/11001/test/mjsunit/string-slices.js
File test/mjsunit/string-slices.js (right):

http://codereview.chromium.org/7477045/diff/11001/test/mjsunit/string-slices....
test/mjsunit/string-slices.js:191: /*
Those tests fail, as pointed out by Vitaly. I'll figure out something for the
next update of this CL.

[Yang, 2011-08-04 09:19:53]

Added a patch for RegExp when handling sliced strings, a few tests and a set of
truncate-methods to do to slices what flatten-methods do to cons.

I'd like to commit this afterwards. The usage of sliced strings is hidden under
a flag anyways.

Things to do in following CLs will be: 
- substring created by generated code.
- patches to deal with externalized string that have slices (will have to do it
the hard way).
- garbage collect parent strings that are only referenced by slices.

http://codereview.chromium.org/7477045/diff/20075/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/7477045/diff/20075/src/arm/lithium-arm.cc#newc...
src/arm/lithium-arm.cc:2013: }
Legacy bug. The register 'string' is being altered in the generated code.

http://codereview.chromium.org/7477045/diff/20075/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/7477045/diff/20075/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:2067: }
Ditto.

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

http://codereview.chromium.org/7477045/diff/20075/src/objects-inl.h#newcode297
src/objects-inl.h:297: }
Being 'indirect' means either cons or slice. Accessing the first part of a
flattened cons or the parent of a truncated slice goes to the same offset, so
there is no point making a distinction here.

http://codereview.chromium.org/7477045/diff/20075/src/x64/lithium-x64.cc
File src/x64/lithium-x64.cc (right):

http://codereview.chromium.org/7477045/diff/20075/src/x64/lithium-x64.cc#newc...
src/x64/lithium-x64.cc:1997: }
Ditto.

[antonm, 2011-08-04 12:18:48]

LGTM

Might be a good idea to add verification for sliced strings.

http://codereview.chromium.org/7477045/diff/20075/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7477045/diff/20075/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:5431: __ nop(0);  // Jumping as first instruction
would crash the code generation.
nit: shouldn't nop go under if as well?

http://codereview.chromium.org/7477045/diff/20075/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7477045/diff/20075/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3484: Operand(SeqTwoByteString::kHeaderSize -
kHeapObjectTag
nit: apparently v8 style is not to start continuation line with operators.

http://codereview.chromium.org/7477045/diff/20075/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/7477045/diff/20075/src/heap.h#newcode95
src/heap.h:95: V(Map, sliced_symbol_map, SlicedSymbolMap)                       
           \
do we need these symbol maps?

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

http://codereview.chromium.org/7477045/diff/20075/src/objects-inl.h#newcode295
src/objects-inl.h:295: || ((type_ & kStringRepresentationMask) ==
kSlicedStringTag)));
Isn't it similar to change to Heap::TargetSpaceId?

http://codereview.chromium.org/7477045/diff/20075/test/cctest/test-strings.cc
File test/cctest/test-strings.cc (right):

http://codereview.chromium.org/7477045/diff/20075/test/cctest/test-strings.cc...
test/cctest/test-strings.cc:522: result =
v8::Script::Compile(v8::String::New(check))->Run();
I think we have something like CompileAndRun

[Yang, 2011-08-04 15:09:42]

Made changes suggested by Anton.

[Vitaly Repeshko, 2011-08-05 12:14:14]

http://codereview.chromium.org/7477045/diff/19020/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/19020/src/heap.cc#newcode2698
src/heap.cc:2698: ASSERT(sliced_string->parent()->IsSeqString());
I think we can have flat cons with external first. It will pass the
IsExternalString check above but will hit this assert.

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

http://codereview.chromium.org/7477045/diff/19020/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3413: Label cons_string, encoding;
encoding -> check_encoding

http://codereview.chromium.org/7477045/diff/19020/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7477045/diff/19020/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:3177: __ and_(result,
kStringRepresentationMask);
Too much code now. Either put it behind the flag or restructure to avoid two
copies of the code that checks encoding and loads chars. If the index is updated
relatively early the latter should be doable.

http://codereview.chromium.org/7477045/diff/19020/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/7477045/diff/19020/src/jsregexp.cc#newcode237
src/jsregexp.cc:237: if (!subject->IsFlatAndTruncated())
FlattenOrTruncateString(subject);
This doesn't seem necessary. This code can be modified to work with slices.

I don't think we need to expose the Truncate family of functions. Given that
slices only contain flat strings it's simply a matter of taking one more offset
into account at each use site. Truncating doesn't make us more efficient. That
said we might want to introduce GC-time truncating later, but it likely won't
use the handlified versions.

http://codereview.chromium.org/7477045/diff/19020/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7477045/diff/19020/src/objects.h#newcode6275
src/objects.h:6275: class SlicedString: public String {
Please document classes. Explain that only slices of flat strings are allowed
and that nested slices are automatically simplified.

[Yang, 2011-08-11 13:23:43]

I removed the family of methods related to truncate. The one I will implement in
the future that will kick in at GC time will look different.

I also changed parts of RegExp to support sliced strings without the need to
truncate them first. This took longer than I thought.

Other than that, I'm basically still on the same page, features to come:
 - taking externalized parent strings into account (same content, different
encoding).
 - generated code for substring allocation.
 - GC time truncation.

So far the dromaeo string benchmark scores 639us as opposed to 867us without
slices. The biggest item of performance improvement is unsurprisingly the slice
function, scoring 2123us instead of 83260us. There is still room for more
improvement though.

http://codereview.chromium.org/7477045/diff/19020/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/19020/src/heap.cc#newcode2698
src/heap.cc:2698: ASSERT(sliced_string->parent()->IsSeqString());
Actually a flat cons with external first can't exist since flat cons are only
created by SlowTryFlatten, which creates a new SeqString by WriteToFlat. On the
other hand, if you externalize a flat cons, it is morphed into an external
string.

On 2011/08/05 12:14:14, Vitaly Repeshko wrote:
> I think we can have flat cons with external first. It will pass the
> IsExternalString check above but will hit this assert.

[Vitaly Repeshko, 2011-08-12 19:00:59]

I only looked on architecture-independent parts and on ia32 code. Since most of
the new generated code is not behind the flag we should make sure it's in a good
shape. Let's discuss the slightly simplified charCodeAt code. After that I'll
need one more pass.


-- Vitaly

http://codereview.chromium.org/7477045/diff/19020/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/19020/src/heap.cc#newcode2698
src/heap.cc:2698: ASSERT(sliced_string->parent()->IsSeqString());
On 2011/08/11 13:23:43, Yang wrote:
> On 2011/08/05 12:14:14, Vitaly Repeshko wrote:
> > I think we can have flat cons with external first. It will pass the
> > IsExternalString check above but will hit this assert.
> 
> Actually a flat cons with external first can't exist since flat cons are only
> created by SlowTryFlatten, which creates a new SeqString by WriteToFlat. On
the
> other hand, if you externalize a flat cons, it is morphed into an external
> string.

Possible scenario:
1. a cons is flattened (its first component is a flat ASCII string now)
2. the first component is extracted (we do it in some places to improve
performance, could be hard to guard against)
3. the first component is externalized

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

http://codereview.chromium.org/7477045/diff/31001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3393: #define NOT_SLICED -1
Ugh, use a constant instead.

http://codereview.chromium.org/7477045/diff/31001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3411: // Check for flat cons string or truncated
sliced string.
Update the comment.

http://codereview.chromium.org/7477045/diff/31001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3472: // edi: encoding of subject string (1 if
ascii, 0 if two_byte);
Update the comment.

http://codereview.chromium.org/7477045/diff/31001/src/ia32/lithium-codegen-ia...
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/7477045/diff/31001/src/ia32/lithium-codegen-ia...
src/ia32/lithium-codegen-ia32.cc:3140: Register string =
ToRegister(instr->string());
Simplified version using three (writable) registers (untested):

  Register string = ToRegister(instr->string());
  Register index = ToRegister(instr->index);
  Register result = ToRegister(instr->result());

  DeferredStringCharCodeAt* deferred =
      new DeferredStringCharCodeAt(this, instr);

  // Fetch the instance type of the receiver into result register.
  __ mov(result, FieldOperand(string, HeapObject::kMapOffset));
  __ movzx_b(result, FieldOperand(result, Map::kInstanceTypeOffset));

  // We need special handling for indirect strings.
  Label check_sequential;
  __ test(result, Immediate(kIsIndirectStringMask));
  __ j(zero, &check_sequential, Label::kNear);

  // Dispatch on the indirect string shape: slice or cons.
  Label cons_string;
  uint32_t sliced_not_cons_mask = kSlicedStringTag & ~kConsStringTag;
  ASSERT(IsPowerOf2(sliced_not_cons_mask) && sliced_not_cons_mask != 0);
  __ test(result, Immediate(sliced_not_cons_mask));
  __ j(zero, &cons_string, Label::kNear);

  // Handle slices.
  Label indirect_string_loaded;
  __ mov(result, FieldOperand(string, SlicedString::kOffsetOffset));
  __ SmiUntag(result);
  __ add(index, Operand(result));
  __ mov(string, FieldOperand(string, SlicedString::kParentOffset));
  __ jmp(&indirect_string_loaded, Label::kNear);

  // Handle conses.
  // Check whether the right hand side is the empty string (i.e. if
  // this is really a flat string in a cons string). If that is not
  // the case we would rather go to the runtime system now to flatten
  // the string.
  __ bind(&cons_string);
  __ cmp(FieldOperand(string, ConsString::kSecondOffset),
         Immediate(factory()->empty_string()));
  __ j(not_equal, deferred->entry());
  __ mov(string, FieldOperand(string, ConsString::kFirstOffset));

  __ bind(&indirect_string_loaded);
  __ mov(result, FieldOperand(string, HeapObject::kMapOffset));
  __ movzx_b(result, FieldOperand(result, Map::kInstanceTypeOffset));

  // Check whether the string is sequential. The only non-sequential
  // shapes we support have just been unwrapped above.
  __ bind(&check_sequential);
  STATIC_ASSERT(kSeqStringTag == 0);
  __ test(result, Immediate(kStringRepresentationMask));
  __ j(not_zero, deferred->entry());

  // Dispatch on the encoding: ASCII or two-byte.
  Label ascii_string;
  STATIC_ASSERT(kAsciiStringTag != 0);
  __ test(result, Immediate(kStringEncodingMask));
  __ j(not_zero, &ascii_string, Label::kNear);

  // Two-byte string.
  // Load the two-byte character code into the result register.
  Label done;
  STATIC_ASSERT(kSmiTag == 0 && kSmiTagSize == 1);
  __ movzx_w(result, FieldOperand(string,
                                  index,
                                  times_2,
                                  SeqTwoByteString::kHeaderSize));
  __ jmp(&done, Label::kNear);

  // ASCII string.
  // Load the byte into the result register.
  __ bind(&ascii_string);
  __ movzx_b(result, FieldOperand(string,
                                  index,
                                  times_1,
                                  SeqAsciiString::kHeaderSize));
  __ bind(&done);
  __ bind(deferred->exit());

http://codereview.chromium.org/7477045/diff/31001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/7477045/diff/31001/src/ia32/lithium-ia32.cc#ne...
src/ia32/lithium-ia32.cc:2060: LOperand* string =
UseTempRegister(instr->string());
We can use 3 instead of 4 registers if we give up on special casing the const
index:
LOperand* string = UseTempRegister(instr->string());
LOperand* index = UseTempRegister(instr->string());
LOperand* context = UseAny(instr->context());
LStringCharCodeAt* result = new LStringCharCodeAt(context, string, index);

http://codereview.chromium.org/7477045/diff/31001/src/ia32/regexp-macro-assem...
File src/ia32/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/7477045/diff/31001/src/ia32/regexp-macro-assem...
src/ia32/regexp-macro-assembler-ia32.cc:1073: String* subject_ptr = *subject;
Should be put in a handle.

http://codereview.chromium.org/7477045/diff/31001/src/regexp-macro-assembler.cc
File src/regexp-macro-assembler.cc (right):

http://codereview.chromium.org/7477045/diff/31001/src/regexp-macro-assembler....
src/regexp-macro-assembler.cc:135: ASSERT(slice->parent()->IsSeqString());
Should this be here? Looks like we can handle external strings fine below.

[Yang, 2011-08-17 09:34:37]

I did some corrections and replaced the CharCodeAt stuff with simpler code on
all three platforms. As it turned out, special handling for constant indexes
doesn't really result in any better performance.

http://codereview.chromium.org/7477045/diff/39001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/39001/src/heap.cc#newcode2648
src/heap.cc:2648: !ConsString::cast(buffer)->first()->IsSeqString())) ||
A little ugly for now, but this will be removed once I dealt with the
externalized string issue. For now externalized strings won't be sliced, but
copied.

[Vitaly Repeshko, 2011-08-17 19:20:23]

Another bunch of comments. Once again I only looked on architecture-independent
parts and on ia32 code.

http://codereview.chromium.org/7477045/diff/39001/src/handles.h
File src/handles.h (right):

http://codereview.chromium.org/7477045/diff/39001/src/handles.h#newcode188
src/handles.h:188: 
nit: Undo blank line insertion.

http://codereview.chromium.org/7477045/diff/39001/src/heap-inl.h
File src/heap-inl.h (right):

http://codereview.chromium.org/7477045/diff/39001/src/heap-inl.h#newcode324
src/heap-inl.h:324: ASSERT(((type & kIsIndirectStringMask) ==
kIsIndirectStringTag)
The assert is extremely hard to read. Maybe we can drop it as an indirect string
is supposed to have a pointer to a "direct" string, so it shouldn't matter what
exact types of indirect strings there are.

http://codereview.chromium.org/7477045/diff/39001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/39001/src/heap.cc#newcode2552
src/heap.cc:2552: // Note that both parts cannot be string slices since:
"Note that neither of the two inputs can be a slice because:"

http://codereview.chromium.org/7477045/diff/39001/src/heap.cc#newcode2648
src/heap.cc:2648: !ConsString::cast(buffer)->first()->IsSeqString())) ||
On 2011/08/17 09:34:37, Yang wrote:
> A little ugly for now, but this will be removed once I dealt with the
> externalized string issue. For now externalized strings won't be sliced, but
> copied.

OK, but the indentation looks weird. Also please mark places that require more
work with TODO(slices bug number).

http://codereview.chromium.org/7477045/diff/39001/src/heap.cc#newcode2653
src/heap.cc:2653: ? AllocateRawAsciiString(length, pretenure )
nit: Extra space before ')'.

http://codereview.chromium.org/7477045/diff/39001/src/heap.cc#newcode2681
src/heap.cc:2681: MaybeObject* maybe_result = Allocate(map, NEW_SPACE);
Consider adding Heap::Allocate{Ascii,TwoByte}SlicedString.

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

http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3405: __ j(zero, &seq_two_byte_string);
Looks like this code was never updated to use near labels...

http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3432: __ mov(ebx, FieldOperand(eax,
ConsString::kSecondOffset));
These two instructions can be combined into a single memory-immediate compare.

http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3524: __ cmp(edi, kNotAStringSlice);
Will this still work if we initialize edi to 0 or to the slice offset and
unconditionally add it to ebx and the length loaded from the original string?

http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5665: if (FLAG_string_slices) {
Whooops.

http://codereview.chromium.org/7477045/diff/39001/src/ia32/regexp-macro-assem...
File src/ia32/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/7477045/diff/39001/src/ia32/regexp-macro-assem...
src/ia32/regexp-macro-assembler-ia32.cc:1068: HandleScope handles;
While you're here, please pass isolate to the scope constructor to avoid a TLS
load.

http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc
File src/jsregexp.cc (left):

http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc#oldcode487
src/jsregexp.cc:487: subject =
Handle<String>(ConsString::cast(*subject)->first(), isolate);
This unwrapping didn't help the code called below because it can handle flat
conses, right? What about interpreted regexps? I think the encoding can now be
incorrect for them.

http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc#newcode239
src/jsregexp.cc:239: // Extract flattened substrings of cons strings before
determining asciiness.
Update the comment.

http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc#newcode255
src/jsregexp.cc:255: subject->ToAsciiVector(),
To{Ascii,UC16}Vector won't work in case an indirect string encoding disagrees
with its underlying string encoding.

http://codereview.chromium.org/7477045/diff/39001/src/objects-debug.cc
File src/objects-debug.cc (right):

http://codereview.chromium.org/7477045/diff/39001/src/objects-debug.cc#newcod...
src/objects-debug.cc:374: CHECK(!this->parent()->IsSlicedString());
Add a min length check?

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

http://codereview.chromium.org/7477045/diff/39001/src/objects-inl.h#newcode2196
src/objects-inl.h:2196: return ConsString::cast(this)->second() ==
GetHeap()->empty_string();
Length check is probably a tiny bit faster as GetHeap is not cheap.

http://codereview.chromium.org/7477045/diff/39001/src/objects-inl.h#newcode2206
src/objects-inl.h:2206: return
String::cast(reinterpret_cast<ConsString*>(this)->first());
Doesn't work as advertised in the .h file. It should also support slices.

http://codereview.chromium.org/7477045/diff/39001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7477045/diff/39001/src/objects.cc#newcode744
src/objects.cc:744: ConsString* cs = ConsString::cast(this);
If there are no intended semantic changes here, please revert and keep the old
version.

http://codereview.chromium.org/7477045/diff/39001/src/objects.cc#newcode5248
src/objects.cc:5248: // Note that the parent of a slice cannot be a cons.
... or a slice.

http://codereview.chromium.org/7477045/diff/39001/src/objects.cc#newcode5251
src/objects.cc:5251: string = slice->parent();
string_tag also should be updated. Either add a TODO or fix now.

http://codereview.chromium.org/7477045/diff/39001/src/objects.cc#newcode5281
src/objects.cc:5281: // Note that the parent of a slice cannot be a cons.
See above.

http://codereview.chromium.org/7477045/diff/39001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7477045/diff/39001/src/objects.h#newcode5817
src/objects.h:5817: 
nit: Remove one blank line.

http://codereview.chromium.org/7477045/diff/39001/src/objects.h#newcode5820
src/objects.h:5820: inline String* GetIndirect();
GetUnderlying? GetIndirect sounds as if it should return an indirect string.

[Yang, 2011-08-18 12:17:32]

Implemented most of the changes you (Vitaly) suggested. I also tested this with
regexp=interpreted.

In order to check the encoding of a string I introduced the methods
Is{Ascii,TwoByte}RepresentationUnderneath() and I think my implementation is
reasonably fast for the non-indirect case. I haven't replaced
Is{Ascii,TwoByte}Representation() in all the places where it should be necessary
yet, as there are no test cases for that right now. I'll do that in another CL
when dealing with externalized strings. Since cons strings may suffer from the
same problem of wrong encoding tag and already has to be unpacked to check for
encoding in some places, using those two new methods should not have a big
overall performance impact. It mostly concerns places when choosing between
ToAsciiVector() and ToUC16Vector().

Cheers,

Yang

http://codereview.chromium.org/7477045/diff/39001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/39001/src/heap.cc#newcode2681
src/heap.cc:2681: MaybeObject* maybe_result = Allocate(map, NEW_SPACE);
On 2011/08/17 19:20:23, Vitaly Repeshko wrote:
> Consider adding Heap::Allocate{Ascii,TwoByte}SlicedString.

I decided not to, at least for now, since this is not used anywhere else and
there are no analogous methods for cons strings either.

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

http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3405: __ j(zero, &seq_two_byte_string);
On 2011/08/17 19:20:23, Vitaly Repeshko wrote:
> Looks like this code was never updated to use near labels...

I updated the part of code that I touched. I'll make a CL to update all the
jumps in the ia32 code in the future.

http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3524: __ cmp(edi, kNotAStringSlice);
On 2011/08/17 19:20:23, Vitaly Repeshko wrote:
> Will this still work if we initialize edi to 0 or to the slice offset and
> unconditionally add it to ebx and the length loaded from the original string?

It would. However I also use edi as indicator to whether we are dealing with a
sliced string as subject (whenever edi != kNotAStringSlice). If that is the
case, the slice has to be unpacked and also the length of the slice has to be
used instead of the length of the parent string (stored into esi). Using
kNotAStringSlice = 0 does not work as indicator since there are slices that
start at offset 0 but have a shorter length than the parent string.

http://codereview.chromium.org/7477045/diff/39001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:5665: if (FLAG_string_slices) {
On 2011/08/17 19:20:23, Vitaly Repeshko wrote:
> Whooops.

This is intended. I'm currently working on generated code for creating
substrings. Until then, whenever string slices are used, we resort to the
runtime system to create substrings. This method here doesn't create slices,
yet.

http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/7477045/diff/39001/src/jsregexp.cc#newcode255
src/jsregexp.cc:255: subject->ToAsciiVector(),
On 2011/08/17 19:20:23, Vitaly Repeshko wrote:
> To{Ascii,UC16}Vector won't work in case an indirect string encoding disagrees
> with its underlying string encoding.

I solved this by changing the assertion in both ToAsciiVector and ToUC16Vector
to consider indirect strings.

[Vitaly Repeshko, 2011-08-19 16:27:40]

LGTM for architecture-independent parts.

Waiting for an updated generated code with better handling of kNotASlice (as we
discussed by IM).

http://codereview.chromium.org/7477045/diff/43002/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/7477045/diff/43002/src/jsregexp.cc#newcode240
src/jsregexp.cc:240: String* needle =
String::cast(re->DataAt(JSRegExp::kAtomPatternIndex));
We don't call needle->IsAsciiRepresentationUnderneath() because it's guaranteed
to be sequential, right? If yes, add an assert here.

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

http://codereview.chromium.org/7477045/diff/43002/src/objects-inl.h#newcode278
src/objects-inl.h:278: STATIC_ASSERT(kIsIndirectStringMask !=
kStringEncodingMask);
Maybe (kIsIndirectStringMask & kStringEncodingMask) == 0?

http://codereview.chromium.org/7477045/diff/43002/src/objects-inl.h#newcode294
src/objects-inl.h:294: STATIC_ASSERT(kIsIndirectStringMask !=
kStringEncodingMask);
Ditto.

http://codereview.chromium.org/7477045/diff/43002/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/7477045/diff/43002/src/objects.cc#newcode5239
src/objects.cc:5239: // Either the string is sequential of external, in which
case the encoding
of -> or

http://codereview.chromium.org/7477045/diff/43002/src/objects.cc#newcode5277
src/objects.cc:5277: // Either the string is sequential of external, in which
case the encoding
of -> or

http://codereview.chromium.org/7477045/diff/43002/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/7477045/diff/43002/src/objects.h#newcode5784
src/objects.h:5784: // encoding of the underlying string.  This is taken into
account here.
Add "Requires: this->IsFlat()".

http://codereview.chromium.org/7477045/diff/43002/src/objects.h#newcode5825
src/objects.h:5825: // Returns the parent of a sliced string or first part of a
cons string.
"... of a *flat* cons ..."

http://codereview.chromium.org/7477045/diff/43002/src/objects.h#newcode5826
src/objects.h:5826: inline String* GetUnderlying();
Add "Requires: StringShape(this).IsIndirect() && this->IsFlat()".

http://codereview.chromium.org/7477045/diff/43002/src/objects.h#newcode6080
src/objects.h:6080: MUST_USE_RESULT MaybeObject* SlowTryTruncate(PretenureFlag
pretenure);
Remove?

[Yang, 2011-08-22 10:32:24]

Changed default offset to 0 instead of kNotASlice. And some minor corrections.

[antonm, 2011-08-25 12:04:36]

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4343: __ mov(r9, Operand(0));
does it belong here?  maybe move it int

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4456: __ add(r8, subject,
Operand(SeqAsciiString::kHeaderSize - kHeapObjectTag));
Apparently if you do:

__ add(r8, subject, Operand(...));
__ add(r9, r8, Operand(r9, LSL, r3));

(of course, you need to swap with __ eor below)

you can simplify calculations below.

Start would be just

__ add(r2, r9, Operand(r1, LSL, r3));

(as before)

And you won't need to do __ add(r8, r8, Operand(...)) at line 4470 in the
patched code.

WDYT?

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4462: __ ldr(r0, MemOperand(fp, kSubjectOffset +
2*kPointerSize));
nit: spaces around * (and in comment)

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4462: __ ldr(r0, MemOperand(fp, kSubjectOffset +
2*kPointerSize));
this looks complicated to me.  Cannot you thread original subject in some other
register?  Just asking.

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4463: // If slice offset is not 0, load the length
from the original sliced string.
I don't see any conditional logic below.

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4863: __ ldrb(result_, FieldMemOperand(result_,
Map::kInstanceTypeOffset));
might be worth asserting that result_ is a flat string.

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:5965: STATIC_ASSERT(SlicedString::kMinLength >=
String::kMinNonFlatLength);
again, might be worth asserting.

http://codereview.chromium.org/7477045/diff/52001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (left):

http://codereview.chromium.org/7477045/diff/52001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3507: __ ldrb(result, FieldMemOperand(string,
why const operand optimization is gone away?

Yes, implementing it for sliced is more involved, but still it seems worth it.

http://codereview.chromium.org/7477045/diff/52001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7477045/diff/52001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3472: // shapes we support have just been
unwrapped above.
again, it might be a good idea to check it in debug mode.

Also, we can never have Cons w/ 1st sliced component and empty second component,
correct?

http://codereview.chromium.org/7477045/diff/52001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/52001/src/heap.cc#newcode2648
src/heap.cc:2648: if (!FLAG_string_slices ||
this condition is very hard to parse.

http://codereview.chromium.org/7477045/diff/52001/src/heap.cc#newcode2684
src/heap.cc:2684: MaybeObject* maybe_result = Allocate(map, NEW_SPACE);
why ignore pretenure flag here?

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

http://codereview.chromium.org/7477045/diff/52001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3433: // edx: map of first part of cons string or
map of parent of sliced string.
do you use edx below?  apparently not, but if yes, you doesn't set it as
promised for sliced strings.

http://codereview.chromium.org/7477045/diff/52001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3522: __ add(ebx, Operand(edi));  // Calculate input
start wrt offset.
please, document what's in ebx.

[Yang, 2011-08-25 13:40:03]

Thanks Anton. Here's another iteration. Please have a look.

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4343: __ mov(r9, Operand(0));
On 2011/08/25 12:04:36, antonm wrote:
> does it belong here?  maybe move it int

This is right before branching off depending on string type. If it is a
sequential string and goes directly to calling the RegExp, we need to make sure
r9 (offset) is already set to default (0).

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4462: __ ldr(r0, MemOperand(fp, kSubjectOffset +
2*kPointerSize));
On 2011/08/25 12:04:36, antonm wrote:
> this looks complicated to me.  Cannot you thread original subject in some
other
> register?  Just asking.
I tried, but r0 - r7 are used to pass arguments and r8 and r9 are used as
scratch...

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:4463: // If slice offset is not 0, load the length
from the original sliced string.
On 2011/08/25 12:04:36, antonm wrote:
> I don't see any conditional logic below.
The condition is saved into r3. r3==1 if twobyte and r3==0 if ascii. Therefore
we shift by 1 bit to the left if the encoding is twobyte and shift by 0 bit (no
shift at all) if the encoding is ascii.

http://codereview.chromium.org/7477045/diff/52001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:5965: STATIC_ASSERT(SlicedString::kMinLength >=
String::kMinNonFlatLength);
On 2011/08/25 12:04:36, antonm wrote:
> again, might be worth asserting.
Removing this because the previous three lines already makes sure that only
sequential strings are added here.

http://codereview.chromium.org/7477045/diff/52001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (left):

http://codereview.chromium.org/7477045/diff/52001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3507: __ ldrb(result, FieldMemOperand(string,
I discussed this with Vitaly and also benchmarked this. Apparently there is no
performance difference with or without constant index, that's why I removed it
entirely.

On 2011/08/25 12:04:36, antonm wrote:
> why const operand optimization is gone away?
> 
> Yes, implementing it for sliced is more involved, but still it seems worth it.

http://codereview.chromium.org/7477045/diff/52001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/7477045/diff/52001/src/arm/lithium-codegen-arm...
src/arm/lithium-codegen-arm.cc:3472: // shapes we support have just been
unwrapped above.
On 2011/08/25 12:04:36, antonm wrote:
> again, it might be a good idea to check it in debug mode.
> 
> Also, we can never have Cons w/ 1st sliced component and empty second
component,
> correct?

Correct. Empty second component can only be in flat cons, which can only be
created by flattening.

We already check for the requirement that the subject string is sequential in
the two lines below.

http://codereview.chromium.org/7477045/diff/52001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/52001/src/heap.cc#newcode2648
src/heap.cc:2648: if (!FLAG_string_slices ||
On 2011/08/25 12:04:36, antonm wrote:
> this condition is very hard to parse.

Yes. This is work in progress. I want to remove this in another CL to enable
slices of external strings. But that requires some more work elsewhere too. So
for now I have this rather ugly term, temporarily.

This part:
  (buffer->IsConsString() &&
  (!buffer->IsFlat() ||
  !ConsString::cast(buffer)->first()->IsSeqString()))
basically only deals with the case that a flat cons string has a first part that
has been externalized.

http://codereview.chromium.org/7477045/diff/52001/src/heap.cc#newcode2684
src/heap.cc:2684: MaybeObject* maybe_result = Allocate(map, NEW_SPACE);
On 2011/08/25 12:04:36, antonm wrote:
> why ignore pretenure flag here?

The idea is that (coming in a following CL) sliced strings only exist in new
space. Whenever a sliced string is promoted to old space, it is converted into a
sequential string.

But you are right. I added the case that the pretenure flag is set to TENURED so
that the substring is copied into old space into a sequential string directly.

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

http://codereview.chromium.org/7477045/diff/52001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3433: // edx: map of first part of cons string or
map of parent of sliced string.
On 2011/08/25 12:04:36, antonm wrote:
> do you use edx below?  apparently not, but if yes, you doesn't set it as
> promised for sliced strings.

It's supposed to mean ebx below. Fixed this.

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

http://codereview.chromium.org/7477045/diff/59001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:4918: STATIC_ASSERT(kSeqStringTag == 0);
I decided to do this seq-string check for both cons and slices because in
another CL I want to allow slice parent to be an external string, which will
have to be handled by runtime system.

[antonm, 2011-08-25 16:02:45]

http://codereview.chromium.org/7477045/diff/59001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/7477045/diff/59001/src/heap.cc#newcode2651
src/heap.cc:2651: !ConsString::cast(buffer)->first()->IsSeqString())) ||
nit: please, indent this line for proper grouping

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

http://codereview.chromium.org/7477045/diff/59001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3415: __ cmp(Operand(ebx),
Immediate(kConsStringTag));
technically you can save an instruction here (and most probably on other
platforms too): if you cmp vs. kExternalString tag, then on eq you bailout, if
greater you've got a splice and if lesser you've got a cons.

http://codereview.chromium.org/7477045/diff/59001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:4890: __ and_(result_, kStringRepresentationMask);
ditto here for shorter dispatch sequence

http://codereview.chromium.org/7477045/diff/59001/src/ia32/regexp-macro-assem...
File src/ia32/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/7477045/diff/59001/src/ia32/regexp-macro-assem...
src/ia32/regexp-macro-assembler-ia32.cc:1104: if
(subject_tmp->IsAsciiRepresentation() != is_ascii) {
if string might have changed, shouldn't you refetch subject_tmp?

http://codereview.chromium.org/7477045/diff/59001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7477045/diff/59001/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:2377: __ Set(r14, 0);
maybe use arg1?

[Yang, 2011-08-26 10:44:40]

Please take another look.

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

http://codereview.chromium.org/7477045/diff/59001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3415: __ cmp(Operand(ebx),
Immediate(kConsStringTag));
On 2011/08/25 16:02:45, antonm wrote:
> technically you can save an instruction here (and most probably on other
> platforms too): if you cmp vs. kExternalString tag, then on eq you bailout, if
> greater you've got a splice and if lesser you've got a cons.

Great observation. Thanks.

http://codereview.chromium.org/7477045/diff/59001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7477045/diff/59001/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:2377: __ Set(r14, 0);
On 2011/08/25 16:02:45, antonm wrote:
> maybe use arg1?

I don't quite follow. arg1 is not defined until way later.

[antonm, 2011-08-26 11:29:17]

LGTM

http://codereview.chromium.org/7477045/diff/59001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

http://codereview.chromium.org/7477045/diff/59001/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:2377: __ Set(r14, 0);
On 2011/08/26 10:44:40, Yang wrote:
> On 2011/08/25 16:02:45, antonm wrote:
> > maybe use arg1?
> 
> I don't quite follow. arg1 is not defined until way later.

Never mind---my fault.  Sorry.

[Vitaly Repeshko, 2011-08-26 22:29:05]

LGTM

http://codereview.chromium.org/7477045/diff/62001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

http://codereview.chromium.org/7477045/diff/62001/src/arm/code-stubs-arm.cc#n...
src/arm/code-stubs-arm.cc:5444: __ nop(0);  // Jumping as first instruction
would crash the code generation.
Is this a bug?

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

http://codereview.chromium.org/7477045/diff/62001/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:3415: STATIC_ASSERT((kConsStringTag <
kExternalStringTag));
Why do we need extra () here? If the assert macro requires them, let's fix the
macro.

[Yang, 2011-08-29 11:44:03]

On 2011/08/26 22:29:05, Vitaly Repeshko wrote:
> LGTM
> 
> http://codereview.chromium.org/7477045/diff/62001/src/arm/code-stubs-arm.cc
> File src/arm/code-stubs-arm.cc (right):
> 
>
http://codereview.chromium.org/7477045/diff/62001/src/arm/code-stubs-arm.cc#n...
> src/arm/code-stubs-arm.cc:5444: __ nop(0);  // Jumping as first instruction
> would crash the code generation.
> Is this a bug?
> 
> http://codereview.chromium.org/7477045/diff/62001/src/ia32/code-stubs-ia32.cc
> File src/ia32/code-stubs-ia32.cc (right):
> 
>
http://codereview.chromium.org/7477045/diff/62001/src/ia32/code-stubs-ia32.cc...
> src/ia32/code-stubs-ia32.cc:3415: STATIC_ASSERT((kConsStringTag <
> kExternalStringTag));
> Why do we need extra () here? If the assert macro requires them, let's fix the
> macro.

Those two issues are addressed by
http://code.google.com/p/v8/issues/detail?id=1644
and
http://codereview.chromium.org/7776007/