Clean JS function results cache on each major GC.

Clean JS function results cache on each major GC.

We don't want to retain cached objects for too long.

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

[antonm, 2010-04-23 17:03:36]

Lasse,

may you have a look?

And any ideas how to make guard in Heap::ClearJSFunction... prettier?

[Lasse Reichstein, 2010-04-26 10:04:55]

http://codereview.chromium.org/1780001/diff/1/3
File src/heap.cc (right):

http://codereview.chromium.org/1780001/diff/1/3#newcode548
src/heap.cc:548: if (context == NULL) return;
Are Result-caches per-context yet?
If so, isn't this only clearing the currently active context?
I am expecting to see an operation on the sleeping threads as well somewhere -
preferably in a gc_prologue like the other cache flushes.

http://codereview.chromium.org/1780001/diff/1/5
File src/objects-inl.h (right):

http://codereview.chromium.org/1780001/diff/1/5#newcode573
src/objects-inl.h:573: return IsFixedArray();
Also check that the length is correct (at least kEntriesIndex plus something
even).

[antonm, 2010-04-26 16:34:45]

Lasse,

may you have another look?

http://codereview.chromium.org/1780001/diff/1/3
File src/heap.cc (right):

http://codereview.chromium.org/1780001/diff/1/3#newcode548
src/heap.cc:548: if (context == NULL) return;
On 2010/04/26 10:04:55, Lasse Reichstein wrote:
> Are Result-caches per-context yet?

As of time when I uploaded this CL they were not in yet.  Now they are
submitted.  I am going to rebaseline and maybe start another review.


> If so, isn't this only clearing the currently active context?
> I am expecting to see an operation on the sleeping threads as well somewhere -
> preferably in a gc_prologue like the other cache flushes.

Ok.

http://codereview.chromium.org/1780001/diff/1/5
File src/objects-inl.h (right):

http://codereview.chromium.org/1780001/diff/1/5#newcode573
src/objects-inl.h:573: return IsFixedArray();
On 2010/04/26 10:04:55, Lasse Reichstein wrote:
> Also check that the length is correct (at least kEntriesIndex plus something
> even).
> 

Done

[Lasse Reichstein, 2010-04-29 07:14:18]

LGTM.
Consider adding a threaded test that repeatedly uses the cache from different
threads, causes occasional GC's, and checks that it gets the correct results.

http://codereview.chromium.org/1780001/diff/6001/7004
File src/objects-inl.h (right):

http://codereview.chromium.org/1780001/diff/6001/7004#newcode575
src/objects-inl.h:575: return ((self->length() -
JSFunctionResultCache::kEntriesIndex)
Would accept a zero or one length FixedArray (depending on whether kEntriesIndex
is even or odd).
Check that length is at least kEntriesIndex.
Perhaps add an #ifdef DEBUG block that does thorough testing (e.g., that both
size and finger are smis that point inside the array and that the function is a
function).

[antonm, 2010-04-29 18:37:04]

Lasse,

I've added a test as you suggested.  It's pretty weak---only checks if we don't
crash.

I could expose additional apt to query cache status, just let me know if you
think it's worth it.

Not submitting, waiting for another LGTM

http://codereview.chromium.org/1780001/diff/6001/7004
File src/objects-inl.h (right):

http://codereview.chromium.org/1780001/diff/6001/7004#newcode575
src/objects-inl.h:575: return ((self->length() -
JSFunctionResultCache::kEntriesIndex)
On 2010/04/29 07:14:19, Lasse Reichstein wrote:
> Would accept a zero or one length FixedArray (depending on whether
kEntriesIndex
> is even or odd).
> Check that length is at least kEntriesIndex.
> Perhaps add an #ifdef DEBUG block that does thorough testing (e.g., that both
> size and finger are smis that point inside the array and that the function is
a
> function).

Done.

Do you want to add Verify logic into HeapObject::HeapObjectVerify?

But to be honest, I am not sure it's a good idea--other classes extending
FixedArray do not do that and those checks are just heuristics, but you should
know better.

[Lasse Reichstein, 2010-05-04 13:40:39]

LGTM.

http://codereview.chromium.org/1780001/diff/6001/7004
File src/objects-inl.h (right):

http://codereview.chromium.org/1780001/diff/6001/7004#newcode575
src/objects-inl.h:575: return ((self->length() -
JSFunctionResultCache::kEntriesIndex)
I wouldn't add it to HeapObjectVerify. It's not a property of the heap-object
that can be checked independently of its use. Being a function cache is only a
role for a FixedArray, not an identity, so a heap iterator wouldn't be able to
use it for anything.

http://codereview.chromium.org/1780001/diff/11001/12006
File src/objects.h (right):

http://codereview.chromium.org/1780001/diff/11001/12006#newcode2336
src/objects.h:2336: void JSFunctionResultCacheVerfiy();
Typo: Verfiy->Verify.

[Lasse Reichstein, 2010-05-04 13:50:35]

Still LGTM.

http://codereview.chromium.org/1780001/diff/11001/12007
File test/cctest/test-threads.cc (right):

http://codereview.chromium.org/1780001/diff/11001/12007#newcode65
test/cctest/test-threads.cc:65: first_section_->Lock();
I'm not sure how Mutex'es work. Can you lock them in one thread and release them
in another? If so, this seems fine.

http://codereview.chromium.org/1780001/diff/11001/12007#newcode88
test/cctest/test-threads.cc:88: // Release acuired lock.
typo: acuired->acquired.

[antonm, 2010-05-04 16:17:30]

Thanks a lot for review, Lasse.

http://codereview.chromium.org/1780001/diff/6001/7004
File src/objects-inl.h (right):

http://codereview.chromium.org/1780001/diff/6001/7004#newcode575
src/objects-inl.h:575: return ((self->length() -
JSFunctionResultCache::kEntriesIndex)
On 2010/05/04 13:40:39, Lasse Reichstein wrote:
> I wouldn't add it to HeapObjectVerify. It's not a property of the heap-object
> that can be checked independently of its use. Being a function cache is only a
> role for a FixedArray, not an identity, so a heap iterator wouldn't be able to
> use it for anything.
> 

Thanks.

http://codereview.chromium.org/1780001/diff/11001/12006
File src/objects.h (right):

http://codereview.chromium.org/1780001/diff/11001/12006#newcode2336
src/objects.h:2336: void JSFunctionResultCacheVerfiy();
On 2010/05/04 13:40:40, Lasse Reichstein wrote:
> Typo: Verfiy->Verify.

Thanks, fixed.

http://codereview.chromium.org/1780001/diff/11001/12007
File test/cctest/test-threads.cc (right):

http://codereview.chromium.org/1780001/diff/11001/12007#newcode65
test/cctest/test-threads.cc:65: first_section_->Lock();
On 2010/05/04 13:50:35, Lasse Reichstein wrote:
> I'm not sure how Mutex'es work. Can you lock them in one thread and release
them
> in another? If so, this seems fine.

I think you noticed a real problem---the test doesn't run as I expect it.  I
reworked it.  I'll ask Erik to have a look and would appreciate if you have a
look as well, but for now I am going to submit this CL.

[Erik Corry, 2010-05-04 19:31:34]

New version of the threaded test LGTM.

You can't in general unlock a mutex from a different thread from the one it was
locked in.  That would defeat deadlock detection.  Semaphores don't have that
restriction and for the same reason you can't normally detect a semaphore
deadlock.   At least this is how I see it and Wikipedia seems to agree.