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#newcode4804
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#newcode4839
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#newcode5938
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.cc#newcode3427
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.cc#newcode3458
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.cc#newcode3472
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.cc#newcode3486
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.cc#newcode3492
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?
http://codereview.chromium.org/7477045/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
