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#newcode4343 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#newcode4462 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#newcode4463 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#newcode5965 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.cc#oldcode3507 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.cc#newcode3472 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#newcode3433 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#newcode4918 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. http://codereview.chromium.org/7477045/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
