Handlify upper layers of KeyedLoadIC.

Handlify upper layers of KeyedLoadIC.


BUG=
TEST=

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

[ulan, 2011-10-19 12:04:19]

PTAL.

[ulan, 2011-10-19 13:47:47]

http://codereview.chromium.org/8352003/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1169
src/ic.cc:1169: set_target(*code);
Add CHECK(!code.is_null()) here and below.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1244
src/ic.cc:1244: Heap* heap = receiver->GetHeap();
Replace heap by isolate()->factory()

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

http://codereview.chromium.org/8352003/diff/1/src/objects.h#newcode4259
src/objects.h:4259: static void UpdateCodeCache(Isolate* isolate,
Drop the isolate argument.

[Kevin Millikin (Chromium), 2011-10-20 08:58:17]

LGTM if comments below are addressed.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1131
src/ic.cc:1131: isnan(HeapNumber::cast(*key)->value())) {
I slightly prefer Handle<HeapNumber>::cast(key)->value() in handle code.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1153
src/ic.cc:1153: Handle<Code> code =
You might consider indenting these lines like so:

Handle<Code> code = isolate()->stub_cache()->ComputeKeyedLoadStringLength(
    name, string);

Because (a) it seems to work better for long function names, and so (b) it can
be uniform, and (c) has the advantage of not wasting so vertical space for
functions with more than two arguments.

Then you could also remove the extra variable and write:

Handle<Code> code = isolate()->stub_cache()->ComputeKeyedLoadStringLength(
    name, Handle<String>::cast(object));

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1169
src/ic.cc:1169: set_target(*code);
On 2011/10/19 13:47:47, ulan wrote:
> Add CHECK(!code.is_null()) here and below.

OK but ASSERT, not CHECK.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1173
src/ic.cc:1173: return Handle<JSArray>::cast(object)->length();
Is this just

return array->length()?

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1179
src/ic.cc:1179: JSFunction::cast(*object)->should_have_prototype()) {
Another place we could do

Handle<JSFunction>::cast(object)->should_have_prototype()

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1203
src/ic.cc:1203: LookupForRead(object, name, &lookup);
We should check if the raw version of LookupForRead is still used.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1245
src/ic.cc:1245: Handle<Map> elements_map(receiver->elements()->map());
In the call code, we had handlified the non_strict_arguments_elements_map
instead.  It occurs to me that there is no great reason to have a handle for
either, because we just immediately dereference it and it has only that one use.

That is:

if (receiver->elements()->map() ==
    isolate()->heap()->non_strict_arguments_elements_map()) { ...

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1264
src/ic.cc:1264: }
The function wants two blank lines after it.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1266
src/ic.cc:1266: void KeyedLoadIC::UpdateCaches(LookupResult* lookup, State
state,
V8 style is one parameter per line if they don't all fit on the same line for
declarations and definitions.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1301
src/ic.cc:1301: Handle<AccessorInfo>
callback(AccessorInfo::cast(*callback_object));
This is better:

Handle<AccessorInfo> callback = Handle<AccessorInfo>::cast(callback_object);

http://codereview.chromium.org/8352003/diff/1/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/8352003/diff/1/src/ic.h#newcode359
src/ic.h:359: return Handle<Code>();
I prefer the static factory function rather than the constructor:

return Handle<Code>::null();

It's a little more verbose, but it's easier to spot due to the word 'null' and
doesn't require understanding what the default constructor does.

http://codereview.chromium.org/8352003/diff/1/src/stub-cache.cc
File src/stub-cache.cc (right):

http://codereview.chromium.org/8352003/diff/1/src/stub-cache.cc#newcode360
src/stub-cache.cc:360: compiler.CompileLoadConstant(name, receiver, holder,
value);
I guess this line is indented too far to the right.

[ulan, 2011-10-20 09:30:05]

Comments addressed, landing.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc
File src/ic.cc (right):

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1131
src/ic.cc:1131: isnan(HeapNumber::cast(*key)->value())) {
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> I slightly prefer Handle<HeapNumber>::cast(key)->value() in handle code.

Done.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1153
src/ic.cc:1153: Handle<Code> code =
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> You might consider indenting these lines like so:
> 
> Handle<Code> code = isolate()->stub_cache()->ComputeKeyedLoadStringLength(
>     name, string);
> 
> Because (a) it seems to work better for long function names, and so (b) it can
> be uniform, and (c) has the advantage of not wasting so vertical space for
> functions with more than two arguments.
> 
> Then you could also remove the extra variable and write:
> 
> Handle<Code> code = isolate()->stub_cache()->ComputeKeyedLoadStringLength(
>     name, Handle<String>::cast(object));

I prefer it too, but in this case 

Handle<Code> code = isolate()->stub_cache()->ComputeKeyedLoadStringLength(

is longer than 80 chars.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1169
src/ic.cc:1169: set_target(*code);
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> On 2011/10/19 13:47:47, ulan wrote:
> > Add CHECK(!code.is_null()) here and below.
> 
> OK but ASSERT, not CHECK.

Done.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1173
src/ic.cc:1173: return Handle<JSArray>::cast(object)->length();
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> Is this just
> 
> return array->length()?

Done.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1179
src/ic.cc:1179: JSFunction::cast(*object)->should_have_prototype()) {
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> Another place we could do
> 
> Handle<JSFunction>::cast(object)->should_have_prototype()

Done.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1203
src/ic.cc:1203: LookupForRead(object, name, &lookup);
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> We should check if the raw version of LookupForRead is still used.

Done.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1245
src/ic.cc:1245: Handle<Map> elements_map(receiver->elements()->map());
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> In the call code, we had handlified the non_strict_arguments_elements_map
> instead.  It occurs to me that there is no great reason to have a handle for
> either, because we just immediately dereference it and it has only that one
use.
> 
> That is:
> 
> if (receiver->elements()->map() ==
>     isolate()->heap()->non_strict_arguments_elements_map()) { ...

Done.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1264
src/ic.cc:1264: }
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> The function wants two blank lines after it.

Done.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1266
src/ic.cc:1266: void KeyedLoadIC::UpdateCaches(LookupResult* lookup, State
state,
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> V8 style is one parameter per line if they don't all fit on the same line for
> declarations and definitions.

Done.

http://codereview.chromium.org/8352003/diff/1/src/ic.cc#newcode1301
src/ic.cc:1301: Handle<AccessorInfo>
callback(AccessorInfo::cast(*callback_object));
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> This is better:
> 
> Handle<AccessorInfo> callback = Handle<AccessorInfo>::cast(callback_object);

Done.

http://codereview.chromium.org/8352003/diff/1/src/ic.h
File src/ic.h (right):

http://codereview.chromium.org/8352003/diff/1/src/ic.h#newcode359
src/ic.h:359: return Handle<Code>();
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> I prefer the static factory function rather than the constructor:
> 
> return Handle<Code>::null();
> 
> It's a little more verbose, but it's easier to spot due to the word 'null' and
> doesn't require understanding what the default constructor does.

Done.

http://codereview.chromium.org/8352003/diff/1/src/stub-cache.cc
File src/stub-cache.cc (right):

http://codereview.chromium.org/8352003/diff/1/src/stub-cache.cc#newcode360
src/stub-cache.cc:360: compiler.CompileLoadConstant(name, receiver, holder,
value);
On 2011/10/20 08:58:17, Kevin Millikin wrote:
> I guess this line is indented too far to the right.

Done.