Refactor the way we collect the information for associating type-related infos

Refactor the way we collect the information for associating type-related infos
with AST IDs. Previously 3 different places had to match in how they handle a
given case, now we are down to 2, with an even simpler logic.

The downside is that due to this simpler logic the allocated dictionary could be
larger than before, but test have shown that this happens *very* rarely, because
its capacity is rounded to the next power of 2, anyway. Furthermore, the oracle
doesn't live long enough that we should really care.

The whole oracle is probably still a bit too tricky in its details, but this is
at least a step into the right direction.
Committed: http://code.google.com/p/v8/source/detail?r=8330

[Mads Ager (chromium), 2011-06-20 08:57:38]

LGTM if the extra size and extra traversal (you are also relocating the extra
entries) does not cost enough to be noticable.

http://codereview.chromium.org/7204003/diff/1/src/type-info.cc
File src/type-info.cc (right):

http://codereview.chromium.org/7204003/diff/1/src/type-info.cc#newcode421
src/type-info.cc:421: byte* old_start = (*code)->instruction_start();
You shouldn't need the explicit dereference here?

code->instruction_start()

http://codereview.chromium.org/7204003/diff/1/src/type-info.cc#newcode423
src/type-info.cc:423: byte* new_start = (*code)->instruction_start();
Ditto.

http://codereview.chromium.org/7204003/diff/1/src/type-info.cc#newcode477
src/type-info.cc:477: SetInfo(ast_id, target);
Didn't we have some filtering in the old code for generic states here (for
compare and binary op ics)? Have you removed that filtering on purpose to keep
this code simpler and because they are just ignored later?

[Søren Thygesen Gjesse, 2011-06-20 09:24:21]

LGTM

[Sven Panne, 2011-06-20 10:16:19]

http://codereview.chromium.org/7204003/diff/1/src/type-info.cc
File src/type-info.cc (right):

http://codereview.chromium.org/7204003/diff/1/src/type-info.cc#newcode421
src/type-info.cc:421: byte* old_start = (*code)->instruction_start();
On 2011/06/20 08:57:38, Mads Ager wrote:
> You shouldn't need the explicit dereference here?
> 
> code->instruction_start()

Done.

http://codereview.chromium.org/7204003/diff/1/src/type-info.cc#newcode423
src/type-info.cc:423: byte* new_start = (*code)->instruction_start();
On 2011/06/20 08:57:38, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/7204003/diff/1/src/type-info.cc#newcode477
src/type-info.cc:477: SetInfo(ast_id, target);
On 2011/06/20 08:57:38, Mads Ager wrote:
> Didn't we have some filtering in the old code for generic states here (for
> compare and binary op ics)? Have you removed that filtering on purpose to keep
> this code simpler and because they are just ignored later?

We *had* some filtering code here, but I removed it very deliberately. The
result of the corresponding oracle calls later are the same with or without them
(as is the performance), and "no code is good code". Let's not 'optimize' too
early, let's keep the initialization code as simple as possible instead. It
would be even better if ProcessTarget simply did a SetInfo(ast_id, target) for
*all* cases and all real logic was in the various oracle calls, but it's not
obvious if this would hurt performance or not, so I did this (intermediate) step
first.

[fschneider, 2011-06-20 10:28:54]

drive-by comment:

http://codereview.chromium.org/7204003/diff/5/src/type-info.cc
File src/type-info.cc (right):

http://codereview.chromium.org/7204003/diff/5/src/type-info.cc#newcode400
src/type-info.cc:400: List<RelocInfo> infos(16);
Maybe use a ZoneList here instead because List<> are malloc'ed.

[Sven Panne, 2011-06-20 11:41:37]

On 2011/06/20 10:28:54, fschneider wrote:
> drive-by comment:
> 
> http://codereview.chromium.org/7204003/diff/5/src/type-info.cc
> File src/type-info.cc (right):
> 
> http://codereview.chromium.org/7204003/diff/5/src/type-info.cc#newcode400
> src/type-info.cc:400: List<RelocInfo> infos(16);
> Maybe use a ZoneList here instead because List<> are malloc'ed.

The list of RelocInfos is malloc'ed for a very good reason: The RelocIterator
and the RelocInfos are not GC-safe. If we really want to get rid of the mallocs,
we would have to do another pass over the relocation infos to find out how big
the ZoneList has to be. I am not sure if we really want that (comments?), so I
propose to leave this out of the current change.

[Mads Ager (chromium), 2011-06-20 11:57:50]

On 2011/06/20 11:41:37, Sven wrote:
> On 2011/06/20 10:28:54, fschneider wrote:
> > drive-by comment:
> > 
> > http://codereview.chromium.org/7204003/diff/5/src/type-info.cc
> > File src/type-info.cc (right):
> > 
> > http://codereview.chromium.org/7204003/diff/5/src/type-info.cc#newcode400
> > src/type-info.cc:400: List<RelocInfo> infos(16);
> > Maybe use a ZoneList here instead because List<> are malloc'ed.
> 
> The list of RelocInfos is malloc'ed for a very good reason: The RelocIterator
> and the RelocInfos are not GC-safe. If we really want to get rid of the
mallocs,
> we would have to do another pass over the relocation infos to find out how big
> the ZoneList has to be. I am not sure if we really want that (comments?), so I
> propose to leave this out of the current change.

Zone memory is not in the heap either. It is just managed by us explicitly and
everything is freed up in one go at the end of a zone scope. So I think a
ZoneList would be fine here.

[Sven Panne, 2011-06-20 12:13:18]

On 2011/06/20 11:57:50, Mads Ager wrote:
> On 2011/06/20 11:41:37, Sven wrote:
> > On 2011/06/20 10:28:54, fschneider wrote:
> > > drive-by comment:
> > > 
> > > http://codereview.chromium.org/7204003/diff/5/src/type-info.cc
> > > File src/type-info.cc (right):
> > > 
> > > http://codereview.chromium.org/7204003/diff/5/src/type-info.cc#newcode400
> > > src/type-info.cc:400: List<RelocInfo> infos(16);
> > > Maybe use a ZoneList here instead because List<> are malloc'ed.
> > 
> > The list of RelocInfos is malloc'ed for a very good reason: The
RelocIterator
> > and the RelocInfos are not GC-safe. If we really want to get rid of the
> mallocs,
> > we would have to do another pass over the relocation infos to find out how
big
> > the ZoneList has to be. I am not sure if we really want that (comments?), so
I
> > propose to leave this out of the current change.
> 
> Zone memory is not in the heap either. It is just managed by us explicitly and
> everything is freed up in one go at the end of a zone scope. So I think a
> ZoneList would be fine here.

OK, I didn't know that, so I'll simply use ZoneList instead of List...