Improved version of LookupForRead (tnx to Kasper) + some faster paths....

Improved version of LookupForRead (tnx to Kasper) + some faster paths.

1) add no GC check;
2) do not use recursion;


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

[Kasper Lund, 2009-07-08 06:55:06]

LGTM.

http://codereview.chromium.org/155141/diff/1003/1004
File src/ic.cc (right):

http://codereview.chromium.org/155141/diff/1003/1004#newcode282
Line 282: if (lookup->IsNotFound() || lookup->type() != INTERCEPTOR ||
Why did you add the !IsCacheable test here? Is it mostly a quick bailout check,
so we can avoid an expensive computation of the LookupResult when we know we'll
not really need it? I think it deserves a comment.

http://codereview.chromium.org/155141/diff/1003/1004#newcode293
Line 293: if (lookup->IsValid()) {
Do you want a non-cacheable bailout check here too?

[antonm, 2009-07-09 12:41:44]

Thanks a lot for review, Kasper!

http://codereview.chromium.org/155141/diff/1003/1004
File src/ic.cc (right):

http://codereview.chromium.org/155141/diff/1003/1004#newcode282
Line 282: if (lookup->IsNotFound() || lookup->type() != INTERCEPTOR ||
On 2009/07/08 06:55:06, Kasper Lund wrote:
> Why did you add the !IsCacheable test here? Is it mostly a quick bailout
check,
> so we can avoid an expensive computation of the LookupResult when we know
we'll
> not really need it? I think it deserves a comment.

Done.

http://codereview.chromium.org/155141/diff/1003/1004#newcode293
Line 293: if (lookup->IsValid()) {
On 2009/07/08 06:55:06, Kasper Lund wrote:
> Do you want a non-cacheable bailout check here too?

Well, I was somewhat uneasy here.  If lookup is valid, we'll quit anyway,
however if it's invalid and for some reason (that shouldn't happen now to the
best of my knowledge) is not cacheable we would wrongly inform caller that
lookup failed while it might be satisfied down the prototype chain.