Avoid GC when compiling CallIC stubs....

Avoid GC when compiling CallIC stubs.

In rare cases GC could be called from ComputeCallMiss function thus
breaking CallIC::LoadFunction.


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

[Vitaly Repeshko, 2010-07-30 13:33:34]

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

http://codereview.chromium.org/3047027/diff/1/2#newcode1292
src/arm/stub-cache-arm.cc:1292: Object* branch = GenerateMissBranch();
Well, the generator function doesn't return the branch code (in fact, it really
can't since the branch is part of the current code object). The contract here is
that if it returned a failure, it must be propagated. Something like this:
Object* obj = GenerateMissBranch();
if (obj->IsFailure()) return obj;

[antonm, 2010-07-30 13:48:25]

LGTM.

As discussed offline, we probably need to add guards in ..IC::Load/Store..
methods.

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

http://codereview.chromium.org/3047027/diff/1/2#newcode1258
src/arm/stub-cache-arm.cc:1258: __ Jump(Handle<Code>(Code::cast(code)),
RelocInfo::CODE_TARGET);
stylistic nit: maybe negative check:

if (!code->IsCode) return code;

or even if (code->IsFailure()) return code;

?

And maybe code should be renamed into something like result.

Feel free to ignore.

[Vladislav Kaznacheev, 2010-07-30 15:25:32]

Addressed the comments.
Added AssertNoGC class to prohibit GC while the stubs are being compiled.

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

http://codereview.chromium.org/3047027/diff/1/2#newcode1258
src/arm/stub-cache-arm.cc:1258: __ Jump(Handle<Code>(Code::cast(code)),
RelocInfo::CODE_TARGET);
On 2010/07/30 13:48:25, antonm wrote:
> stylistic nit: maybe negative check:
> 
> if (!code->IsCode) return code;
> 
> or even if (code->IsFailure()) return code;
> 
> ?
> 
> And maybe code should be renamed into something like result.
> 
> Feel free to ignore.

Done.

http://codereview.chromium.org/3047027/diff/1/2#newcode1292
src/arm/stub-cache-arm.cc:1292: Object* branch = GenerateMissBranch();
On 2010/07/30 13:33:34, Vitaly wrote:
> Well, the generator function doesn't return the branch code (in fact, it
really
> can't since the branch is part of the current code object). The contract here
is
> that if it returned a failure, it must be propagated. Something like this:
> Object* obj = GenerateMissBranch();
> if (obj->IsFailure()) return obj;

Done.

[Vitaly Repeshko, 2010-07-30 16:59:53]

http://codereview.chromium.org/3047027/diff/16001/17006
File src/ic.cc (right):

http://codereview.chromium.org/3047027/diff/16001/17006#newcode574
src/ic.cc:574: AssertNoGC nogc;  // GC could invalidate the pointers held in
lookup.
The comment implies that the nogc object should live as long as the lookup
object. Can we introduce local scopes containing both these objects in the
functions that call UpdateCaches?

[antonm, 2010-07-30 17:18:57]

http://codereview.chromium.org/3047027/diff/16001/17003
File src/heap.cc (right):

http://codereview.chromium.org/3047027/diff/16001/17003#newcode328
src/heap.cc:328: ASSERT(gc_allowed_);
maybe at the very start of the method?

http://codereview.chromium.org/3047027/diff/16001/17006
File src/ic.cc (right):

http://codereview.chromium.org/3047027/diff/16001/17006#newcode574
src/ic.cc:574: AssertNoGC nogc;  // GC could invalidate the pointers held in
lookup.
I agree with Vitaly here (if I understand him correctly): why not at the very
start of the function?  Same for all other functions.

http://codereview.chromium.org/3047027/diff/16001/17006#newcode1483
src/ic.cc:1483: AssertNoGC nogc;  // GC could invalidate the pointers held in
lookup.
why not change ::Load/Store methods as well like:

LookupResult lookup;

{
  AssertNoGc nogc;
  LookupForRead(...);
 
  ...
}

PropertyAttributes attr;
Object* result = object->GetProperty(...)

?

That would allow us to catch additional problems.

http://codereview.chromium.org/3047027/diff/16001/17008
File src/stub-cache.h (right):

http://codereview.chromium.org/3047027/diff/16001/17008#newcode688
src/stub-cache.h:688: // exist and cannot be created without causing GC.
nit: maybe don't overspecify: returns Failure if jump to miss stub cannot be
generated?

by no means insisting.

[Vladislav Kaznacheev, 2010-08-04 14:29:20]

Addressed the comments and rebased. I do not quite like what I have done to
ic.cc, ideas on how to make it less ugly are welcome.

http://codereview.chromium.org/3047027/diff/16001/17003
File src/heap.cc (right):

http://codereview.chromium.org/3047027/diff/16001/17003#newcode328
src/heap.cc:328: ASSERT(gc_allowed_);
Cannot see much difference.

On 2010/07/30 17:18:57, antonm wrote:
> maybe at the very start of the method?

http://codereview.chromium.org/3047027/diff/16001/17006
File src/ic.cc (right):

http://codereview.chromium.org/3047027/diff/16001/17006#newcode574
src/ic.cc:574: AssertNoGC nogc;  // GC could invalidate the pointers held in
lookup.
I have done what I could. Cannot say I like what I see:)
On 2010/07/30 16:59:53, Vitaly wrote:
> The comment implies that the nogc object should live as long as the lookup
> object. Can we introduce local scopes containing both these objects in the
> functions that call UpdateCaches?

http://codereview.chromium.org/3047027/diff/16001/17006#newcode574
src/ic.cc:574: AssertNoGC nogc;  // GC could invalidate the pointers held in
lookup.
On 2010/07/30 17:18:57, antonm wrote:
> I agree with Vitaly here (if I understand him correctly): why not at the very
> start of the function?  Same for all other functions.

Done.

http://codereview.chromium.org/3047027/diff/16001/17006#newcode1483
src/ic.cc:1483: AssertNoGC nogc;  // GC could invalidate the pointers held in
lookup.
On 2010/07/30 17:18:57, antonm wrote:
> why not change ::Load/Store methods as well like:
> 
> LookupResult lookup;
> 
> {
>   AssertNoGc nogc;
>   LookupForRead(...);
>  
>   ...
> }
> 
> PropertyAttributes attr;
> Object* result = object->GetProperty(...)
> 
> ?
> 
> That would allow us to catch additional problems.

Done.

http://codereview.chromium.org/3047027/diff/16001/17008
File src/stub-cache.h (right):

http://codereview.chromium.org/3047027/diff/16001/17008#newcode688
src/stub-cache.h:688: // exist and cannot be created without causing GC.
On 2010/07/30 17:18:57, antonm wrote:
> nit: maybe don't overspecify: returns Failure if jump to miss stub cannot be
> generated?
> 
> by no means insisting.

Done.

[antonm, 2010-08-04 14:39:36]

LGTM.

http://codereview.chromium.org/3047027/diff/16001/17003
File src/heap.cc (right):

http://codereview.chromium.org/3047027/diff/16001/17003#newcode328
src/heap.cc:328: ASSERT(gc_allowed_);
On 2010/08/04 14:29:20, Vladislav Kaznacheev wrote:
> Cannot see much difference.
> 
> On 2010/07/30 17:18:57, antonm wrote:
> > maybe at the very start of the method?
> 
> 

You won't clean any caches.  The mantra is to catch error as early as possible. 
Not insisting.

[Vitaly Repeshko, 2010-08-04 14:45:37]

LGTM

I think now all calls to UpdateCaches are protected by AssertNoGC, so having
AssertNoGC is UpdateCaches is unnecessary. Since UpdateCaches gets the lookup
object from the calling function it's the responsibility of the caller to
protect it.

[Vladislav Kaznacheev, 2010-08-05 08:25:31]

Agree. Done.
On 2010/08/04 14:45:37, Vitaly wrote:
> LGTM
> 
> I think now all calls to UpdateCaches are protected by AssertNoGC, so having
> AssertNoGC is UpdateCaches is unnecessary. Since UpdateCaches gets the lookup
> object from the calling function it's the responsibility of the caller to
> protect it.

[Vladislav Kaznacheev, 2010-08-05 08:25:40]

http://codereview.chromium.org/3047027/diff/16001/17003
File src/heap.cc (right):

http://codereview.chromium.org/3047027/diff/16001/17003#newcode328
src/heap.cc:328: ASSERT(gc_allowed_);
On 2010/08/04 14:39:37, antonm wrote:
> On 2010/08/04 14:29:20, Vladislav Kaznacheev wrote:
> > Cannot see much difference.
> > 
> > On 2010/07/30 17:18:57, antonm wrote:
> > > maybe at the very start of the method?
> > 
> > 
> 
> You won't clean any caches.  The mantra is to catch error as early as
possible. 
> Not insisting.

Done.