Don't share function result caches between contexts.

Don't share function result caches between contexts.

A reference to the caches array was embedded directly into the builtin
code and this allowed sharing objects between contexts.

Unfortunately, clearing the cache on GC won't prevent sharing so we
either have to have per-context builtin code or load the cache
indirectly from the current context. This change implements the second
approach. The first approach may be interesting to consider in the
future for some perfomance critical functions, and the current
approach can still be improved by putting the caches directly into the
global context (or even global objects).

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

[Vitaly Repeshko, 2010-04-20 22:49:39]

Anton,

This is a quick fix I needed while debugging window.open memory leak reported by
Pavel (I don't know the bug number). In short, repeatedly opening/closing gmail
window using window.open/close leaks memory. I don't think the result cache is
the only thing to blame here, but fixing it helps with heap tracing. I'm sure
there're better ways to fix it.

I'm also working on adding debug-mode checks for this.


Thanks,
Vitaly

[Lasse Reichstein, 2010-04-21 07:33:46]

Drive-by comment: The cache should definitely be per-context (and I should have
noticed during review. My bad!). 
The result of a function that belongs to one context must not be returned by a
function in another context, since it might be using completely different
objects to build the result (in the current use-case, the two contexts have
different RegExp.prototype objects, so caching the result of the RegExp function
would give incorrect results).

[antonm, 2010-04-21 11:13:27]

Thanks a lot for spotting this and sorry for introducing the trouble.

Almost LGTM.

Am I missing something or src/bootstrapper.cc should be update as well to build
caches per context?

http://codereview.chromium.org/1731002/diff/1/2
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1731002/diff/1/2#newcode4179
src/arm/codegen-arm.cc:4179: __ ldr(r1, MemOperand(cp,
Context::SlotOffset(Context::GLOBAL_INDEX)));
there is CodeGenerator::ContextOperand that might be convenient here.

http://codereview.chromium.org/1731002/diff/1/2#newcode4182
src/arm/codegen-arm.cc:4182: Context::JSFUNCTION_RESULT_CACHES_INDEX)));
ditto.  Plus I'd rather indent is with C in Context at same column as r in r1

http://codereview.chromium.org/1731002/diff/1/3
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1731002/diff/1/3#newcode6561
src/ia32/codegen-ia32.cc:6561: __ mov(cache.reg(), Operand(esi,
Context::SlotOffset(Context::GLOBAL_INDEX)));
ditto for ContextOperand

http://codereview.chromium.org/1731002/diff/1/3#newcode6571
src/ia32/codegen-ia32.cc:6571: ASSERT(tmp.is_valid());
just curious.  tmp.reg() below has this assert as well.  What is our common
approach, to put such assert or not?

[Vitaly Repeshko, 2010-04-23 12:43:04]

http://codereview.chromium.org/1731002/diff/1/2
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/1731002/diff/1/2#newcode4179
src/arm/codegen-arm.cc:4179: __ ldr(r1, MemOperand(cp,
Context::SlotOffset(Context::GLOBAL_INDEX)));
On 2010/04/21 11:13:27, antonm wrote:
> there is CodeGenerator::ContextOperand that might be convenient here.

Done.

http://codereview.chromium.org/1731002/diff/1/2#newcode4182
src/arm/codegen-arm.cc:4182: Context::JSFUNCTION_RESULT_CACHES_INDEX)));
On 2010/04/21 11:13:27, antonm wrote:
> ditto.  Plus I'd rather indent is with C in Context at same column as r in r1

Done.

http://codereview.chromium.org/1731002/diff/1/3
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/1731002/diff/1/3#newcode6561
src/ia32/codegen-ia32.cc:6561: __ mov(cache.reg(), Operand(esi,
Context::SlotOffset(Context::GLOBAL_INDEX)));
On 2010/04/21 11:13:27, antonm wrote:
> ditto for ContextOperand

Done.

http://codereview.chromium.org/1731002/diff/1/3#newcode6571
src/ia32/codegen-ia32.cc:6571: ASSERT(tmp.is_valid());
On 2010/04/21 11:13:27, antonm wrote:
> just curious.  tmp.reg() below has this assert as well.  What is our common
> approach, to put such assert or not?

I can't comment for the whole team, but my preference is to check register
allocation results early (especially in inline functions like this one that
depend on the state of the current frame).

[Vitaly Repeshko, 2010-04-23 12:45:39]

On 2010/04/21 11:13:27, antonm wrote:
> Thanks a lot for spotting this and sorry for introducing the trouble.
> 
> Almost LGTM.
> 
> Am I missing something or src/bootstrapper.cc should be update as well to
build
> caches per context?

I think that code is already doing per-context initialization.

Please take another look.


Thanks,
Vitaly

[antonm, 2010-04-23 12:52:53]

LGTM and thanks