Implement target cache for constructor calls.

Implement target cache for constructor calls.

This caches call targets of constructor calls by associating one element
caches with call sites. The type feedback oracle can use the recorded
valued to gather type information for monomorphic constructor call sites.

R=kmillikin@chromium.org,vegorov@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=10531

[Michael Starzinger, 2011-12-13 11:00:24]

This only works on IA32 so far, but I wanted to get a first round of reviews
before I start porting it to other architectures.

[Vyacheslav Egorov (Chromium), 2012-01-23 10:39:52]

approach seems fine to me (not counting chicken and egg problem with stubs and
builtins).

there is also some hand-waving in builtins.cc Builtins::SetUp that tries to
avoid GC for no apparent reason I am a bit concerned about it (though I am
pretty sure that just generating one additional small stub will not cause GC).

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:421: CallConstructStub stub(NO_CALL_FUNCTION_FLAGS);
according to isolate initialization sequence stub cache is initialized after
builtins so I sense an ordering problem here.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (left):

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4576: void CallFunctionStub::FinishCode(Handle<Code>
code) {
Did it disappear? I can't find it.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4600: Label initialize, call;
I would rename call to done.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4604: __ mov(ebx, Operand(ebx, 1));  // 1 ~ sizeof
'test eax' opcode in bytes.
you can generate assertion into instruction stream that verifies that next
instruction is test eax, ...

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/full-codegen-i...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2332: __ call(stub.GetCode(),
RelocInfo::CODE_TARGET, expr->id());
RelocInfo::CONSTRUCT_CALL

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2335: if (record_call_target) {
I am curious if you can introduce helper method (e.g. on macro assembler?) which
both generates call to stub _and_ emits test eax instruction if necessary. This
way nobody will forget it.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/type-info.cc
File src/type-info.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/1/src/type-info.cc#newcod...
src/type-info.cc:571: if (CallStub::HasCache(target)) {
It's a bit confusing that HasCache also verifies whether target is a CallStub.
Maybe call it: CallStub::IsCallStubWithCache?

[Michael Starzinger, 2012-01-25 11:42:28]

PTAL.

This turned into a larger CL, so here are the major changes to look for:
- Switched to an external array (i.e. cache_cells) which holds pairs of AST ids
and global property cells used for caching call targets (used by
MarkCompactCollector and TypeFeedbackOracle).
- Passing reference to the cache cell as a third register argument to the
CallConstructStub now.
- Adapted debugger to above change.
- Ported to x64 and ARM architectures.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:421: CallConstructStub stub(NO_CALL_FUNCTION_FLAGS);
On 2012/01/23 10:39:52, Vyacheslav Egorov wrote:
> according to isolate initialization sequence stub cache is initialized after
> builtins so I sense an ordering problem here.

The CodeStubs are not stored in the stub cache but in a dictionary reachable
through heap()->code_stubs(), that is part of the root set. So I don't see any
ordering problem here. Please let me know if I am wrong on this one.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (left):

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4576: void CallFunctionStub::FinishCode(Handle<Code>
code) {
On 2012/01/23 10:39:52, Vyacheslav Egorov wrote:
> Did it disappear? I can't find it.

Done. It was still there on line 4633. No longer needed with new approach.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4600: Label initialize, call;
On 2012/01/23 10:39:52, Vyacheslav Egorov wrote:
> I would rename call to done. 

Done.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:4604: __ mov(ebx, Operand(ebx, 1));  // 1 ~ sizeof
'test eax' opcode in bytes.
On 2012/01/23 10:39:52, Vyacheslav Egorov wrote:
> you can generate assertion into instruction stream that verifies that next
> instruction is test eax, ... 

Done. No longer needed with new approach.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/full-codegen-i...
File src/ia32/full-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2332: __ call(stub.GetCode(),
RelocInfo::CODE_TARGET, expr->id());
On 2012/01/23 10:39:52, Vyacheslav Egorov wrote:
> RelocInfo::CONSTRUCT_CALL

Done. Required some changes in the debugger.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/ia32/full-codegen-i...
src/ia32/full-codegen-ia32.cc:2335: if (record_call_target) {
On 2012/01/23 10:39:52, Vyacheslav Egorov wrote:
> I am curious if you can introduce helper method (e.g. on macro assembler?)
which
> both generates call to stub _and_ emits test eax instruction if necessary.
This
> way nobody will forget it.

Done. No longer needed with new approach.

https://chromiumcodereview.appspot.com/8932004/diff/1/src/type-info.cc
File src/type-info.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/1/src/type-info.cc#newcod...
src/type-info.cc:571: if (CallStub::HasCache(target)) {
On 2012/01/23 10:39:52, Vyacheslav Egorov wrote:
> It's a bit confusing that HasCache also verifies whether target is a CallStub.
> Maybe call it: CallStub::IsCallStubWithCache?

Done. No longer needed with new approach.

[Vyacheslav Egorov (Chromium), 2012-01-25 15:51:54]

LGTM!

https://chromiumcodereview.appspot.com/8932004/diff/6001/src/full-codegen.h
File src/full-codegen.h (right):

https://chromiumcodereview.appspot.com/8932004/diff/6001/src/full-codegen.h#n...
src/full-codegen.h:581: unsigned id;
id -> ast_id ?

[Michael Starzinger, 2012-01-27 13:03:50]

Added new patch set. Landed.

https://chromiumcodereview.appspot.com/8932004/diff/6001/src/full-codegen.h
File src/full-codegen.h (right):

https://chromiumcodereview.appspot.com/8932004/diff/6001/src/full-codegen.h#n...
src/full-codegen.h:581: unsigned id;
On 2012/01/25 15:51:55, Vyacheslav Egorov wrote:
> id -> ast_id ?

Done.

https://chromiumcodereview.appspot.com/8932004/diff/6001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/8932004/diff/6001/src/objects.h#newcod...
src/objects.h:3952: class CacheCells: public FixedArray {
As discussed offline, this was renamed to "TypeFeedbackCells".

[Erik Corry, 2012-01-30 15:01:11]

https://chromiumcodereview.appspot.com/8932004/diff/9001/src/type-info.cc
File src/type-info.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/9001/src/type-info.cc#new...
src/type-info.cc:550: ProcessTypeFeedbackCells(code);
This call uses number dictionaries on the heap, and thus it can cause
allocations, but it is inside an AssertNoAllocation scope.

[Vyacheslav Egorov (Chromium), 2012-01-30 15:10:03]

https://chromiumcodereview.appspot.com/8932004/diff/9001/src/type-info.cc
File src/type-info.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/9001/src/type-info.cc#new...
src/type-info.cc:550: ProcessTypeFeedbackCells(code);
On 2012/01/30 15:01:11, Erik Corry wrote:
> This call uses number dictionaries on the heap, and thus it can cause
> allocations, but it is inside an AssertNoAllocation scope.

Good catch! Yeah, CreateDictionary should add length of typefeedbackcells to the
infos length when allocating a dictionary.

[Michael Starzinger, 2012-01-30 15:22:58]

https://chromiumcodereview.appspot.com/8932004/diff/9001/src/type-info.cc
File src/type-info.cc (right):

https://chromiumcodereview.appspot.com/8932004/diff/9001/src/type-info.cc#new...
src/type-info.cc:550: ProcessTypeFeedbackCells(code);
On 2012/01/30 15:01:11, Erik Corry wrote:
> This call uses number dictionaries on the heap, and thus it can cause
> allocations, but it is inside an AssertNoAllocation scope.

Nice catch! Fix: https://chromiumcodereview.appspot.com/9293005/