Here comes bunch of comments.
http://codereview.chromium.org/3141022/diff/14003/53001 File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/3141022/diff/14003/53001#newcode5635 src/arm/codegen-arm.cc:5635: Register tmp = frame_->scratch0(); tmp is not used? http://codereview.chromium.org/3141022/diff/14003/53001#newcode5637 src/arm/codegen-arm.cc:5637: ASSERT(String::kHashShift >= kSmiTagSize); assert also that kSmiTag == 0 http://codereview.chromium.org/3141022/diff/14003/53001#newcode5638 src/arm/codegen-arm.cc:5638: __ and_(value, value, Operand(String::kArrayIndexValueMask)); There is GenerateIndexFromHash in ic-arm.cc. It seems to provide better instruction sequence based on Ubfx [kArrayIndexValue mask is too big for ARM so and_ will produce two instruction: load mask from memory to ip, then and_(value, value, ip)]. Turn GenerateIndexFromHash into method of MacroAssembler and use it here. http://codereview.chromium.org/3141022/diff/14003/53003 File src/arm/full-codegen-arm.cc (right): http://codereview.chromium.org/3141022/diff/14003/53003#newcode2674 src/arm/full-codegen-arm.cc:2674: ASSERT(String::kHashShift >= kSmiTagSize); assert kSmiTag == 0 http://codereview.chromium.org/3141022/diff/14003/53003#newcode2675 src/arm/full-codegen-arm.cc:2675: __ and_(r0, r0, Operand(String::kArrayIndexValueMask)); use GenerateIndexFromHash (see comment in codegen-arm.cc). http://codereview.chromium.org/3141022/diff/14003/53005 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/3141022/diff/14003/53005#newcode8076 src/ia32/codegen-ia32.cc:8076: string.ToRegister(); should not we string.Unuse() somewhere later? http://codereview.chromium.org/3141022/diff/14003/53005#newcode8081 src/ia32/codegen-ia32.cc:8081: ASSERT(String::kHashShift > kSmiTagSize);
=
kSmiTag == 0 http://codereview.chromium.org/3141022/diff/14003/53005#newcode8082 src/ia32/codegen-ia32.cc:8082: __ and_(number.reg(), String::kArrayIndexValueMask); to avoid code duplication use GenerateIndexFromHash (make it method of MacroAssembler). http://codereview.chromium.org/3141022/diff/14003/53007 File src/ia32/full-codegen-ia32.cc (right): http://codereview.chromium.org/3141022/diff/14003/53007#newcode2773 src/ia32/full-codegen-ia32.cc:2773: ASSERT(String::kHashShift >= kSmiTagSize); kSmiTag == 0 http://codereview.chromium.org/3141022/diff/14003/53007#newcode2774 src/ia32/full-codegen-ia32.cc:2774: __ and_(eax, String::kArrayIndexValueMask); to avoid code duplication use GenerateIndexFromHash (make it method of MacroAssembler). http://codereview.chromium.org/3141022/diff/14003/53008 File src/runtime.cc (right): http://codereview.chromium.org/3141022/diff/14003/53008#newcode4849 src/runtime.cc:4849: } else if ((subject->hash_field() & String::kHashNotComputedMask) != 0 && This condition looks really complicated to me: 1) don't check String::kHashNotComputedMask directly. there is a method for that. 2) I do not understand (len == 1 || data[0] != '0') check. Are you trying to guard against octal literals? I don't think it is necessary here. At least ParseDecimalInteger is not taking them into account. 3) Even if len >= String::kMaxCachedArrayIndexLength it worth setting hash any way http://codereview.chromium.org/3141022/diff/14003/53008#newcode4854 src/runtime.cc:4854: uint32_t hash = (d << String::kHashShift) | Do not set hash directly. I am pretty sure there is a static function somewhere for that. Make it a static method of String and use it. This will remove code duplication. http://codereview.chromium.org/3141022/diff/14003/53009 File src/runtime.js (right): http://codereview.chromium.org/3141022/diff/14003/53009#newcode506 src/runtime.js:506: return %_IsCachedArrayIndex(x) ? %_CachedArrayIndexToNumber(x) IsCachedArrayIndex/CachedArrayIndexToNumber are strange names. Maybe HasCachedArrayIndex/GetCachedArrayIndex? Or something like that. http://codereview.chromium.org/3141022/diff/14003/53010 File src/v8natives.js (right): http://codereview.chromium.org/3141022/diff/14003/53010#newcode114 src/v8natives.js:114: if (!IS_STRING(string)) string = NonStringToString(string); we have TO_STRING_INLINE macros http://codereview.chromium.org/3141022/diff/14003/53010#newcode125 src/v8natives.js:125: if (!IS_STRING(string)) string = NonStringToString(string); we have TO_STRING_INLINE macros http://codereview.chromium.org/3141022/diff/14003/53011 File src/x64/codegen-x64.cc (right): http://codereview.chromium.org/3141022/diff/14003/53011#newcode7332 src/x64/codegen-x64.cc:7332: should not we string.Unuse() somewhere? http://codereview.chromium.org/3141022/diff/14003/53011#newcode7336 src/x64/codegen-x64.cc:7336: __ andl(number.reg(), Immediate(String::kArrayIndexValueMask)); to avoid codeduplication use GenerateIndexFromHash http://codereview.chromium.org/3141022/diff/14003/53013 File src/x64/full-codegen-x64.cc (right): http://codereview.chromium.org/3141022/diff/14003/53013#newcode2773 src/x64/full-codegen-x64.cc:2773: __ Integer32ToSmi(rax, rax); GenerateIndexFromHash http://codereview.chromium.org/3141022/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
