Store a lookup result when compiling interceptor ICs....

Compile precanned answers for the case of failed interceptor for some combinations.


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

[Mads Ager (chromium), 2009-06-23 09:00:19]

Hi Anton, this looks like a good start.  

Avoiding the extra lookups post interceptor looks like a good thing.

Kasper just realized that what we have now is actually not safe.  There is
nothing that prevents the interceptor from adding or removing properties from
the object itself or any object in the prototype chain.  Therefore, the cached
index (or with this change lookup result) might not be valid when we get back
from the interceptor.

The right fix for this seems to be to continue working on this patch and modify
it to:

1. check the maps to the interceptor holder

2. call the interceptor

3. recheck the entire map chain to the holder of the real property

4. call the runtime system to get the real property given the cached lookup
result (which is valid since we have rechecked all the maps after calling the
interceptor).

This way it will be safe and we will not add extra overhead for the initial
interceptor call and should still get a speedup for the subsequent lookup.

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

http://codereview.chromium.org/140069/diff/1/2#newcode324
Line 324: static JSObject* Holder(JSObject* holder, LookupResult* lookup) {
The argument here is the receiver, right?  Rename |holder| argument to
|receiver|.

http://codereview.chromium.org/140069/diff/1/2#newcode329
Line 329: Object* current = holder;
Can this be written in the style:

  for (Object* pt = current->GetPrototype();
       pt != Heap::null_value();
       pt = JSObject::cast(pt)->GetPrototype()) {
    ...
  }

This is what we normally do.  When does GetPrototype return undefined?

http://codereview.chromium.org/140069/diff/1/2#newcode352
Line 352: void Compile(JSObject* holder, MacroAssembler* masm) {
|holder| is unused?

How about renaming to something like PushLookupResult?

http://codereview.chromium.org/140069/diff/1/2#newcode374
Line 374: LookupResult* lookup,
rename |lookup| to something like |post_interceptor_lookup|?

http://codereview.chromium.org/140069/diff/1/2#newcode405
Line 405: if (lookup->IsValid()) {
We should be able to combine the push of the real holder with the push of the
rest of the lookup result in the helper class.

http://codereview.chromium.org/140069/diff/1/2#newcode409
Line 409: LoadInterceptorHelper c(lookup);
Rename c to helper?

http://codereview.chromium.org/140069/diff/1/2#newcode416
Line 416: __ TailCallRuntime(load_ic_property, c.GetNArguments());
How about '4 + helper.LookupSize()' so that the helper only has to take the
actual lookup result into account?

http://codereview.chromium.org/140069/diff/1/2#newcode1079
Line 1079: &lookup,
Pass in the name here and put the LoadInterceptorHelper in the generate method
as is done for CallIC?

http://codereview.chromium.org/140069/diff/1/2#newcode1207
Line 1207: &lookup,
Ditto.

[antonm, 2009-06-29 12:03:43]

Guys, may you have a look at another pass?

I think that no getter interceptor case should be lifted higher in the stack
(ic.cc) and this CL lacks tests and cleanups, but I'd like to get your LGTM on
stub part first.

tia and yours,
anton.

[Mads Ager (chromium), 2009-06-30 09:22:28]

Overall this looks good.  There is a lot of code for generating the IC stubs now
and as you write, we need to make sure that it is all tested.  I wonder if more
of it can be factored out?

Also, I would like to have some counters added so we can get information about
how often the different parts of the code are hit.  If some of the cases you are
now generating code for are almost never hit, maybe it would be better to just
call the runtime system and reduce the complexity of the code?

http://codereview.chromium.org/140069/diff/3001/3002
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/140069/diff/3001/3002#newcode429
Line 429: masm->CheckMaps(holder, eax, GetLastJSObjectProto(holder),
Can we modify CheckMaps to check to the last JSObject if we pass in
Heap::undefined_value()?

http://codereview.chromium.org/140069/diff/3001/3003
File src/ic.h (right):

http://codereview.chromium.org/140069/diff/3001/3003#newcode38
Line 38: #define IC_UTIL_LIST(ICU)          \
Align all the '\' at the position for the longest name.

http://codereview.chromium.org/140069/diff/3001/3004
File src/stub-cache.cc (right):

http://codereview.chromium.org/140069/diff/3001/3004#newcode720
Line 720: * Attempts to load a property with an interceptor (which must
present).
must present -> must be present.

http://codereview.chromium.org/140069/diff/3001/3004#newcode792
Line 792: StackFrameIterator it;
Can we refactor these methods into pairs of methods (one for call and one for
load ICs) that share a couple of helper methods instead? It would be nice to
avoid the need for using a StackFrameIterator to determine if it is a call IC -
we have that information at the call-site.

[Mads Ager (chromium), 2009-06-30 09:23:21]

http://codereview.chromium.org/140069/diff/3001/3002
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/140069/diff/3001/3002#newcode429
Line 429: masm->CheckMaps(holder, eax, GetLastJSObjectProto(holder),
On 2009/06/30 09:22:28, Mads Ager wrote:
> Can we modify CheckMaps to check to the last JSObject if we pass in
> Heap::undefined_value()?

And by Heap::undefined_value() I of course mean Heap::null_value().  :-)

[antonm, 2009-07-03 00:09:45]

Mads, I hope I've addressed all your comments.

Tnx a lot for suggestion to simplify logic based on usage: I sent a doc with
some stats to you and Kasper.

I hope this CL might be simplified another bit: I think ic.is_contextual (used
to determine if we should throw an exception or return failed value) is
accessible from stub compiler and thus the whole separation of ForLoad/ForCall
trampolines may go away (and stubs become more specialized).

Just for my eduation: what does this is_contextual thing means?

yours,
anton.

http://codereview.chromium.org/140069/diff/3001/3002
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/140069/diff/3001/3002#newcode429
Line 429: masm->CheckMaps(holder, eax, GetLastJSObjectProto(holder),
On 2009/06/30 09:23:21, Mads Ager wrote:
> On 2009/06/30 09:22:28, Mads Ager wrote:
> > Can we modify CheckMaps to check to the last JSObject if we pass in
> > Heap::undefined_value()?
> 
> And by Heap::undefined_value() I of course mean Heap::null_value().  :-)
> 

Not directly as holder should be JSObject*, but I use NULL for it, is it fine
with you?

http://codereview.chromium.org/140069/diff/3001/3003
File src/ic.h (right):

http://codereview.chromium.org/140069/diff/3001/3003#newcode38
Line 38: #define IC_UTIL_LIST(ICU)          \
On 2009/06/30 09:22:28, Mads Ager wrote:
> Align all the '\' at the position for the longest name.

Done.

http://codereview.chromium.org/140069/diff/3001/3004
File src/stub-cache.cc (right):

http://codereview.chromium.org/140069/diff/3001/3004#newcode720
Line 720: * Attempts to load a property with an interceptor (which must
present).
On 2009/06/30 09:22:28, Mads Ager wrote:
> must present -> must be present.

Done.

http://codereview.chromium.org/140069/diff/3001/3004#newcode792
Line 792: StackFrameIterator it;
On 2009/06/30 09:22:28, Mads Ager wrote:
> Can we refactor these methods into pairs of methods (one for call and one for
> load ICs) that share a couple of helper methods instead? It would be nice to
> avoid the need for using a StackFrameIterator to determine if it is a call IC
-
> we have that information at the call-site.

For sure

[Mads Ager (chromium), 2009-07-03 07:54:23]

Another batch of comments.  I think we are getting there.  Thanks for doing the
statistics and simplifications based on them.

IsContextual really means that we are performing a global load.  Global loads of
properties that are not there throws ReferenceErrors.  Loads of other properties
that are not there do not throw exceptions and just returns undefined.

http://codereview.chromium.org/140069/diff/6006/5020
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/140069/diff/6006/5020#newcode476
Line 476: Register MacroAssembler::CheckMaps(JSObject* object, Register
object_reg,
Could you add a comment in the header file explaining that a NULL holder will
generate map checks all the way to the top of the prototype chain?

http://codereview.chromium.org/140069/diff/6006/5020#newcode497
Line 497: Object* proto = object->GetPrototype();
This code is really part of the loop termination condition.

How about replacing the while with a for:

// Check all maps to the holder or to the top of the prototype chain.
for (Object* proto = object->GetPrototype();
     proto != Heap::null_value() && object != holder;
     proto = proto->GetPrototype()) {
  // Generate code
}

http://codereview.chromium.org/140069/diff/6006/5024
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/140069/diff/6006/5024#newcode398
Line 398: struct LoadInterceptorCompiler {
Make this a class with public and a private part.  Put the constructor before
the methods and have 'name' be a private field.

Since these are only supposed to be allocate on the stack, you should use 

class LoadInterceptorCompiler BASE_EMBEDDED {
  // ...  
}

http://codereview.chromium.org/140069/diff/6006/5024#newcode407
Line 407: // Prepare tail call to follow
Add period at the end of comment.

http://codereview.chromium.org/140069/diff/6006/5024#newcode443
Line 443: // Cleanup a garbage of prepared second call
How about just: "Pop lookup result."

Above, you could add a "Push lookup result." comment where you do the three
calls to push.

http://codereview.chromium.org/140069/diff/6006/5024#newcode446
Line 446: __ push(scratch2);
Do you mean scratch1 here?  Do we have a test case that gets here?

http://codereview.chromium.org/140069/diff/6006/5024#newcode471
Line 471: struct CallInterceptorCompiler {
Make this a class as well.

http://codereview.chromium.org/140069/diff/6006/5025
File src/stub-cache.cc (right):

http://codereview.chromium.org/140069/diff/6006/5025#newcode785
Line 785: * but doesn't look down if interceptor returns nothing.
'look down' -> 'search the prototype chain'?

http://codereview.chromium.org/140069/diff/6006/5026
File src/stub-cache.h (right):

http://codereview.chromium.org/140069/diff/6006/5026#newcode308
Line 308: Object* ThrowUndefinedException(Arguments args);
This one is unused now, isn't it?  If so, please remove.

[Mads Ager (chromium), 2009-07-03 07:58:11]

A couple of thinks I forgot:

I think I missed the place where you actually use CheckMaps with a NULL holder?

We need tests that cover all cases in the generated code.  I would recommend
putting in "__ int3();" in one branch at a time and make sure that you have a
test that hits that branch.

Thanks,   -- Mads

[antonm, 2009-07-03 14:41:01]

Mads, tnx a lot for review!

I've added more tests to cover all the cases of cacheable lookup (hopefully
regular one is tested enough already).

Regarding IsContextual.  Thanks a lot for explanations.  Is there a way to
access this information from StubCompiler?  In this case I'd able to compile
proper trampoline which won't check contextualness of load.

http://codereview.chromium.org/140069/diff/6006/5020
File src/ia32/macro-assembler-ia32.cc (right):

http://codereview.chromium.org/140069/diff/6006/5020#newcode476
Line 476: Register MacroAssembler::CheckMaps(JSObject* object, Register
object_reg,
On 2009/07/03 07:54:23, Mads Ager wrote:
> Could you add a comment in the header file explaining that a NULL holder will
> generate map checks all the way to the top of the prototype chain?

Now obsolete as you spotted that I don't need this functionality
anymore---thanks a lot for spotting this!

http://codereview.chromium.org/140069/diff/6006/5020#newcode497
Line 497: Object* proto = object->GetPrototype();
On 2009/07/03 07:54:23, Mads Ager wrote:
> This code is really part of the loop termination condition.
> 
> How about replacing the while with a for:
> 
> // Check all maps to the holder or to the top of the prototype chain.
> for (Object* proto = object->GetPrototype();
>      proto != Heap::null_value() && object != holder;
>      proto = proto->GetPrototype()) {
>   // Generate code
> }

Ditto

http://codereview.chromium.org/140069/diff/6006/5024
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/140069/diff/6006/5024#newcode398
Line 398: struct LoadInterceptorCompiler {
On 2009/07/03 07:54:23, Mads Ager wrote:
> Make this a class with public and a private part.  Put the constructor before
> the methods and have 'name' be a private field.
> 
> Since these are only supposed to be allocate on the stack, you should use 
> 
> class LoadInterceptorCompiler BASE_EMBEDDED {
>   // ...  
> }

Done.

http://codereview.chromium.org/140069/diff/6006/5024#newcode407
Line 407: // Prepare tail call to follow
On 2009/07/03 07:54:23, Mads Ager wrote:
> Add period at the end of comment.

Done.

http://codereview.chromium.org/140069/diff/6006/5024#newcode443
Line 443: // Cleanup a garbage of prepared second call
On 2009/07/03 07:54:23, Mads Ager wrote:
> How about just: "Pop lookup result."
> 
> Above, you could add a "Push lookup result." comment where you do the three
> calls to push.

This is not a lookup itself, rather data I needed to save to perform a second
call if interceptor fails.  I tried to reword it, may you have another look?

http://codereview.chromium.org/140069/diff/6006/5024#newcode446
Line 446: __ push(scratch2);
On 2009/07/03 07:54:23, Mads Ager wrote:
> Do you mean scratch1 here?  Do we have a test case that gets here?

Oh yes, tnx a lot for spotting this!  And probably we don't.  Adding it.

Actually, you've spotted ways more serious problem (and helped me to fix it
:)---I didn't restore ecx when going into miss handle

http://codereview.chromium.org/140069/diff/6006/5024#newcode471
Line 471: struct CallInterceptorCompiler {
On 2009/07/03 07:54:23, Mads Ager wrote:
> Make this a class as well.

Done.

http://codereview.chromium.org/140069/diff/6006/5025
File src/stub-cache.cc (right):

http://codereview.chromium.org/140069/diff/6006/5025#newcode785
Line 785: * but doesn't look down if interceptor returns nothing.
On 2009/07/03 07:54:23, Mads Ager wrote:
> 'look down' -> 'search the prototype chain'?

Done.

http://codereview.chromium.org/140069/diff/6006/5026
File src/stub-cache.h (right):

http://codereview.chromium.org/140069/diff/6006/5026#newcode308
Line 308: Object* ThrowUndefinedException(Arguments args);
On 2009/07/03 07:54:23, Mads Ager wrote:
> This one is unused now, isn't it?  If so, please remove.

Done.

[Mads Ager (chromium), 2009-07-07 12:16:56]

LGTM once you make it compile on ARM and x64.

http://codereview.chromium.org/140069/diff/5032/6015
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/140069/diff/5032/6015#newcode437
Line 437: // Do tail-call to LoadPropertyByLookupResult
End comment with period.

http://codereview.chromium.org/140069/diff/5032/6015#newcode492
Line 492: // Under EnterInternalFrame and push(holder) this refers to name
End comment with period.

http://codereview.chromium.org/140069/diff/5032/6013
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/140069/diff/5032/6013#newcode5059
Line 5059: // Now it should be (hopefully) ICed and keep a reference
Remove '(hopefully)'. :-)

http://codereview.chromium.org/140069/diff/5032/6013#newcode5205
Line 5205: // This test checks that if interceptor provides a function,
It looks like this is testing the case where you use the cached function from
the prototype chain?

[antonm, 2009-07-07 21:45:20]

Mads, thanks a lot for review!

I've reran tests and curiously release mode tests for ia32 failed.  I don't know
precise reason for failure yet (only threaded test fails), but that made me
think if my approach is sane.  For example consider the case of CallIC.  Imagine
that call to get property with interceptor only triggered GC, but didn't return
the value.  That might invalidate all the pointers.  I don't know what would
happen to receiver and name stored in previous stack frame (would they mended?),
but mostly for sure holder stored on new frame won't be adjusted (is that
correct) which seems problematic.

If my analysis is right, is there a simple way out?  Storing handles in stubs
didn't work when I first tried that (holder might come from new space, btw, why
we don't allow storing handles to objects in new space?)

I was considering compiling a function to check the rest (from holder to holder
of cached result) and passing it into a runtime code (to be invoked after
interceptor check), but that might be not the best solution.  Any ideas are most
appreciated.

http://codereview.chromium.org/140069/diff/5032/6015
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/140069/diff/5032/6015#newcode437
Line 437: // Do tail-call to LoadPropertyByLookupResult
On 2009/07/07 12:16:56, Mads Ager wrote:
> End comment with period.

Done.

http://codereview.chromium.org/140069/diff/5032/6015#newcode492
Line 492: // Under EnterInternalFrame and push(holder) this refers to name
On 2009/07/07 12:16:56, Mads Ager wrote:
> End comment with period.

Done.

http://codereview.chromium.org/140069/diff/5032/6013
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/140069/diff/5032/6013#newcode5059
Line 5059: // Now it should be (hopefully) ICed and keep a reference
On 2009/07/07 12:16:56, Mads Ager wrote:
> Remove '(hopefully)'. :-)

Done.

http://codereview.chromium.org/140069/diff/5032/6013#newcode5205
Line 5205: // This test checks that if interceptor provides a function,
On 2009/07/07 12:16:56, Mads Ager wrote:
> It looks like this is testing the case where you use the cached function from
> the prototype chain?

Ctrl-C/Ctrl-V (or, to more precise, y/p).  Sorry and thanks a lot for spotting
this.

[antonm, 2009-07-09 12:24:37]

Apparently it was a bad idea to push holder out of internal frame---with stuff
moved into the frame tests pass.

x64 and arm works fine.

May you have another look?

[Mads Ager (chromium), 2009-07-09 15:06:40]

LGTM

[Kasper Lund, 2009-07-15 07:01:35]

If you change the CheckMaps to CheckPrototypes and add a few tests of why that
is necessary, it LGTM.

http://codereview.chromium.org/140069/diff/6055/6057
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/140069/diff/6055/6057#newcode365
Line 365: if (lookup->type() == FIELD) {
I think you should a comment here that explains in words what kind of
intercepted loads you can optimize.

http://codereview.chromium.org/140069/diff/6055/6057#newcode371
Line 371: optimize = callback->getter() != 0;
0 -> NULL?

http://codereview.chromium.org/140069/diff/6055/6057#newcode380
Line 380: __ EnterInternalFrame();
Maybe explain why you need a frame here?

http://codereview.chromium.org/140069/diff/6055/6057#newcode409
Line 409: holder = masm->CheckMaps(holder_obj, holder, lookup->holder(),
CheckPrototypes.

http://codereview.chromium.org/140069/diff/6055/6057#newcode418
Line 418: ASSERT(callback);
callback != NULL

http://codereview.chromium.org/140069/diff/6055/6057#newcode419
Line 419: ASSERT(callback->getter());
callback->getter() != NULL

http://codereview.chromium.org/140069/diff/6055/6057#newcode426
Line 426: holder = masm->CheckMaps(holder_obj, holder, lookup->holder(),
CheckPrototypes.

http://codereview.chromium.org/140069/diff/6055/6057#newcode463
Line 463: explicit LoadInterceptorCompiler(Register name) : name(name) {}
Constructor should go before the methods.

http://codereview.chromium.org/140069/diff/6055/6057#newcode466
Line 466: Register name;
name_?

http://codereview.chromium.org/140069/diff/6055/6057#newcode485
Line 485: if (function->is_compiled() && !holder_obj->IsJSArray()) {
I think this deserves a comment. Why is it a problem if holder_obj is a JS
array?

http://codereview.chromium.org/140069/diff/6055/6057#newcode512
Line 512: masm->CheckMaps(holder_obj, receiver, lookup->holder(),
Please rewrite using CheckPrototypes.

http://codereview.chromium.org/140069/diff/6055/6057#newcode557
Line 557: explicit CallInterceptorCompiler(const ParameterCount& arguments)
Constructor before methods in definition.

http://codereview.chromium.org/140069/diff/6055/6057#newcode561
Line 561: const ParameterCount& arguments;
Any good reason not to call these arguments_ and argc_?

http://codereview.chromium.org/140069/diff/6055/6056
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/140069/diff/6055/6056#newcode5028
Line 5028: // a stub, but interceptor produced value on its own
Terminate comment with .

http://codereview.chromium.org/140069/diff/6055/6056#newcode5048
Line 5048: // a stub, but it got invalidated later on
on -> on.

http://codereview.chromium.org/140069/diff/6055/6056#newcode5140
Line 5140: // a stub, but interceptor produced value on its own
own -> own.

http://codereview.chromium.org/140069/diff/6055/6056#newcode5168
Line 5168: // a stub, but it got invalidated later on
Ditto.

http://codereview.chromium.org/140069/diff/6055/6056#newcode5286
Line 5286: // a value, we can fetch regular value
Ditto.

[antonm, 2009-07-15 10:21:14]

Kasper, tnx a lot for review!

Hopefully addressed everything.

I reworked a bit case with callbacks and invalidation as NULL setter make
property readonly and __proto__ games I used to play apparently invalidate maps
and thus don't allow to prove necessity of CheckPrototypes.

When you're fine with this CL, I'd cook up a simplified version which only
compiles const functions (if it'll give us another speed up).

yours,
anton.

http://codereview.chromium.org/140069/diff/6055/6057
File src/ia32/stub-cache-ia32.cc (right):

http://codereview.chromium.org/140069/diff/6055/6057#newcode365
Line 365: if (lookup->type() == FIELD) {
On 2009/07/15 07:01:35, Kasper Lund wrote:
> I think you should a comment here that explains in words what kind of
> intercepted loads you can optimize.

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode371
Line 371: optimize = callback->getter() != 0;
On 2009/07/15 07:01:35, Kasper Lund wrote:
> 0 -> NULL?

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode380
Line 380: __ EnterInternalFrame();
On 2009/07/15 07:01:35, Kasper Lund wrote:
> Maybe explain why you need a frame here?

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode409
Line 409: holder = masm->CheckMaps(holder_obj, holder, lookup->holder(),
On 2009/07/15 07:01:35, Kasper Lund wrote:
> CheckPrototypes.

Done with a test which failed before.

http://codereview.chromium.org/140069/diff/6055/6057#newcode418
Line 418: ASSERT(callback);
On 2009/07/15 07:01:35, Kasper Lund wrote:
> callback != NULL

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode419
Line 419: ASSERT(callback->getter());
On 2009/07/15 07:01:35, Kasper Lund wrote:
> callback->getter() != NULL

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode426
Line 426: holder = masm->CheckMaps(holder_obj, holder, lookup->holder(),
On 2009/07/15 07:01:35, Kasper Lund wrote:
> CheckPrototypes.

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode463
Line 463: explicit LoadInterceptorCompiler(Register name) : name(name) {}
On 2009/07/15 07:01:35, Kasper Lund wrote:
> Constructor should go before the methods.

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode466
Line 466: Register name;
On 2009/07/15 07:01:35, Kasper Lund wrote:
> name_?

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode485
Line 485: if (function->is_compiled() && !holder_obj->IsJSArray()) {
On 2009/07/15 07:01:35, Kasper Lund wrote:
> I think this deserves a comment. Why is it a problem if holder_obj is a JS
> array?

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode512
Line 512: masm->CheckMaps(holder_obj, receiver, lookup->holder(),
On 2009/07/15 07:01:35, Kasper Lund wrote:
> Please rewrite using CheckPrototypes.

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode557
Line 557: explicit CallInterceptorCompiler(const ParameterCount& arguments)
On 2009/07/15 07:01:35, Kasper Lund wrote:
> Constructor before methods in definition.

Done.

http://codereview.chromium.org/140069/diff/6055/6057#newcode561
Line 561: const ParameterCount& arguments;
On 2009/07/15 07:01:35, Kasper Lund wrote:
> Any good reason not to call these arguments_ and argc_?

Nope, historical artifacts.  Done.

http://codereview.chromium.org/140069/diff/6055/6056
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/140069/diff/6055/6056#newcode5028
Line 5028: // a stub, but interceptor produced value on its own
On 2009/07/15 07:01:35, Kasper Lund wrote:
> Terminate comment with .

Done.

http://codereview.chromium.org/140069/diff/6055/6056#newcode5048
Line 5048: // a stub, but it got invalidated later on
On 2009/07/15 07:01:35, Kasper Lund wrote:
> on -> on.

Done.

http://codereview.chromium.org/140069/diff/6055/6056#newcode5140
Line 5140: // a stub, but interceptor produced value on its own
On 2009/07/15 07:01:35, Kasper Lund wrote:
> own -> own.

Done.

http://codereview.chromium.org/140069/diff/6055/6056#newcode5168
Line 5168: // a stub, but it got invalidated later on
On 2009/07/15 07:01:35, Kasper Lund wrote:
> Ditto.

Done.

http://codereview.chromium.org/140069/diff/6055/6056#newcode5286
Line 5286: // a value, we can fetch regular value
On 2009/07/15 07:01:35, Kasper Lund wrote:
> Ditto.

Done.

[Kasper Lund, 2009-07-15 10:44:38]

LGTM.

[Kasper Lund, 2009-07-29 10:02:37]

You should change the none_value name. It's too generic. Other than that it
still LGTM.

http://codereview.chromium.org/140069/diff/8019/8028
File src/heap.h (right):

http://codereview.chromium.org/140069/diff/8019/8028#newcode113
Line 113: V(Object, none_value, NoneValue)                                      
      \
I think you should call this something else. This is too generic for my liking.
How about something like no_interceptor_result_sentinel?

[antonm, 2009-07-29 12:20:54]

Thanks a lot, everyone!

Submitting

http://codereview.chromium.org/140069/diff/8019/8028
File src/heap.h (right):

http://codereview.chromium.org/140069/diff/8019/8028#newcode113
Line 113: V(Object, none_value, NoneValue)                                      
      \
On 2009/07/29 10:02:37, Kasper Lund wrote:
> I think you should call this something else. This is too generic for my
liking.
> How about something like no_interceptor_result_sentinel?

Done.