I stand corrected, thanks a lot for explanations, Slava.
Probably StringHasher::MakeCachedArrayIndex should be renamed to
MakeArrayIndexHash as it processes cases when we won't get cached
array index.
Apparently we could change it slightly as well:
Index: src/objects.cc
===================================================================
--- src/objects.cc (revision 5415)
+++ src/objects.cc (working copy)
@@ -4997,7 +4997,7 @@
ASSERT(TenToThe(String::kMaxCachedArrayIndexLength) <
(1 << String::kArrayIndexValueBits));
ASSERT(String::kMaxArrayIndexSize < (1 << String::kArrayIndexValueBits));
- value &= ~String::kIsNotArrayIndexMask;
+ ASSERT((value & String::kIsNotArrayIndexMask) == 0);
value |= length << String::kArrayIndexHashLengthShift;
return value;
}
And move value <<= after ASSERTs.
If those changes look sane, I'd make a CL out of them unless someone
else (e.g. Sergey in this CL) is going to do it.
yours,
anton.
On Mon, Sep 6, 2010 at 11:54 PM, Vyacheslav Egorov <[email protected]> wrote:
> I think
>
> ASSERT(String::kMaxArrayIndexSize < (1 << String::kArrayIndexValueBits));
>
> was originally intended to be
>
> ASSERT(String::kMaxArrayIndexSize < (1 << String::kArrayIndexLengthBits));
>
> I think I mistyped this when I was refactoring this place (r5363). In
> any case this should be better turned into STATIC_ASSERT and moved to
> right into class String.
>
>>> But if understand what goes on here correctly, ASSERT(length <=
>>> String::kMaxArrayIndexSize); is fishy as well.
>
> It is fishy but it is correct: we encode length into hash field even
> if it bigger than kMaxCachedArrayIndexLength so we can later do a fast
> check through kContainsCachedArrayIndexMask.
>
> Instead of removing assertion ASSERT((hash &
> String::kContainsCachedArrayIndexMask) == 0); I recommend transforming
> it and moving into MakeCachedArrayIndex:
>
> ASSERT((length > kMaxCachedArrayIndexLength) || (hash &
> String::kContainsCachedArrayIndexMask) == 0);
>
> This will check that mask correctly detects cases of length >
> kMaxCachedArrayIndexLength.
>
> --
> Vyacheslav Egorov
>
>
> On Mon, Sep 6, 2010 at 9:10 PM, <[email protected]> wrote:
>> Drive by comments.
>>
>> I agree with Andrei (caseq@) that check in MakeCachedArrayIndex seems
>> strange.
>> But if understand what goes on here correctly, ASSERT(length <=
>> String::kMaxArrayIndexSize); is fishy as well.
>>
>> If I am right, we want something like:
>>
>> ASSERT(length > 0);
>> ASSERT(length <= String::kMax[Cached]ArrayIndex[Length]);
>> ASSERT(TenToThe(String::kMaxCachedArrayIndexLength) <
>> (1 << String::kArrayIndexValueBits));
>> ASSERT(String::kMaxArrayIndexSize >= String::kMaxCachedArrayIndex);
>>
>> But the last check only checks we don't spend unnecessary stuff.
>>
>> Plus probably most of those checks should be turned into STATIC_ASSERT and
>> moved
>> into String class itself.
>>
>>
>> http://codereview.chromium.org/3295017/diff/1/2
>> File src/runtime.cc (left):
>>
>> http://codereview.chromium.org/3295017/diff/1/2#oldcode4858
>> src/runtime.cc:4858: len <= String::kMaxArrayIndexSize &&
>> I think a mistake is here: it should be len <=
>> String::kMaxCachedArrayIndexLength----we only have 24 bits for an index
>> which only allows room for 10 ** 7 (but my knowledge of this part of v8
>> is poor).
>>
>> If I am right, we need to go over doc and fix it.
>>
>> http://codereview.chromium.org/3295017/show
>>
>> --
>> v8-dev mailing list
>> [email protected]
>> http://groups.google.com/group/v8-dev
>>
>
> --
> v8-dev mailing list
> [email protected]
> http://groups.google.com/group/v8-dev
>
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
