Adding a type vector to replace type cells.

Adding a type vector to replace type cells.

R=bmeurer@chromium.org

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

[mvstanton, 2014-01-23 17:01:50]

Hi Danno, hi Benedikt,
Here is the CL for type vector to replace type cells.

[Benedikt Meurer, 2014-01-24 11:29:54]

First round of comments, in addition to offline chatting.

https://codereview.chromium.org/137403009/diff/480001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/137403009/diff/480001/src/ast.cc#newcode727
src/ast.cc:727: if (CallFeedbackSlot() >= 0) {
!= kInvalidFeedbackSlot

https://codereview.chromium.org/137403009/diff/480001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/137403009/diff/480001/src/ast.h#newcode197
src/ast.h:197: maximum_feedback_slots_ += maximum;
Can we have an ASSERT here to make sure that  minimum_feedback_slots_ <=
maximum_feedback_slots_?

https://codereview.chromium.org/137403009/diff/480001/src/ast.h#newcode967
src/ast.h:967: first_feedback_slot_(0),
Initialize to -1 as below. Extra points if you add some
kInvalidFeedbackSlotIndex constant for this purpose.

https://codereview.chromium.org/137403009/diff/480001/src/ast.h#newcode1849
src/ast.h:1849: first_feedback_slot_(-1),
Remember the extra points...? ;-)

https://codereview.chromium.org/137403009/diff/480001/src/ast.h#newcode1909
src/ast.h:1909: first_feedback_slot_(0),
See above.

https://codereview.chromium.org/137403009/diff/480001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/137403009/diff/480001/src/full-codegen.cc#new...
src/full-codegen.cc:395: feedback_vector_ =
isolate->factory()->NewFixedArrayWithHoles(minimum_length,
As discussed offline: Please add an explanation here, that we have to do this
for now because of our parser.

https://codereview.chromium.org/137403009/diff/480001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/137403009/diff/480001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5814: 
Add an AssertIsSmi(edx).

https://codereview.chromium.org/137403009/diff/480001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5821: // TODO(mvstanton): change this message below.
Please fix this TODO.

[mvstanton, 2014-01-27 17:19:53]

Hi Benedikt, hi Danno,
I do still have some comments to address but here you can see the separate phase
for the work.
PTAL, thx,
--Michael

[danno, 2014-01-28 08:27:16]

Two top level comments:

- The new additional phase seems expensive and bolted-on. I really think the
initialization of slots indices should be folded into the construction of the
AST even though it conceptually could be considered separate.
- It seems that it would be possible to simplify how slots get allocated and
make the "client interface" to getting slots simpler (see comments below).

https://codereview.chromium.org/137403009/diff/690001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/137403009/diff/690001/src/ast.cc#newcode728
src/ast.cc:728: if (CallFeedbackSlot() >= 0) {
You probably want to turn this into a convenience predicate HasFeedbackSlot()

https://codereview.chromium.org/137403009/diff/690001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/137403009/diff/690001/src/ast.h#newcode217
src/ast.h:217: virtual int ConsumeFeedbackSlots(Isolate* isolate, int
current_slot) {
Is it really consuming? Isn't it more like earmarking or reserving? A comment
might be useful here, too, e.g. "By default, AST nodes don't store their type
information in the type vector and therefore don't reserve slots" or something
to that effect.

https://codereview.chromium.org/137403009/diff/690001/src/ast.h#newcode1864
src/ast.h:1864: virtual int ConsumeFeedbackSlots(Isolate* isolate, int
current_slot) {
Somehow, this seems a little clunky. It seems like you want to have a virtual
method that returns the number of nodes needed by the AST node, and that by
default return zero, and only have to implement this method if you want slots.
For examples, if you change the consumefeedbackslots logic in the ASTNode
baseclass to call SetFirstFeedbackSlot and GetFirstFeedbackSlot virtual methods
that do nothing/ASSERT by default and are only implemented for a new subclass of
Expression called ExpressionWithFeedback (which Call and ForIn derive from) you
can get the memory back and make adding slot support easy, just 1) subclass from
ExpressionWithFeedback and 2) Implement virtual int GetFeedbackSlotCount().

https://codereview.chromium.org/137403009/diff/690001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/137403009/diff/690001/src/compiler.cc#newcode64
src/compiler.cc:64: feedback_slots_(-1),
Don't you want to use kInvalidFeedbackSlot here and below?

https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.cc
File src/feedbackslots.cc (right):

https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.cc#ne...
src/feedbackslots.cc:39: #define RECURSE(call)                               \
Here and below, this seems like a lot of boilerplate to accomplish what you want
to do. Since we need the allocation slot information in common cases, maybe it
makes sense to fold it into building the AST? That approach is probably less
code and more efficient.

https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.h
File src/feedbackslots.h (right):

https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.h#new...
src/feedbackslots.h:44: explicit FeedbackSlotAllocator(Zone* zone) : used_(0) {
I worry that the additional pass over the AST might be expensive, especially for
very short-running programs. E.g. did you measure SunSpider performance?

https://codereview.chromium.org/137403009/diff/690001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/137403009/diff/690001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:2462: // ebx : Feedback vector
Boy, I wish we would generate all the code here and below in Hydrogen.

https://codereview.chromium.org/137403009/diff/690001/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/137403009/diff/690001/src/type-info.cc#newcode87
src/type-info.cc:87: !CanRetainOtherContext(JSFunction::cast(obj),
*native_context_))) {
nit: weird indentation

https://codereview.chromium.org/137403009/diff/690001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/137403009/diff/690001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2850: // TODO(mvstanton): fix this test.
Yes, please do :-)

[mvstanton, 2014-01-30 15:13:40]

Hi guys, addressed comments, PTAL. 3 notes:
1) You guys had different thoughts on the way feedback slots should be
allocated, Benedikt favoring an Ast graph traversal, and Danno favoring a work
list built up during parse, and later traversed after scope analysis. I've now
gone with Danno's approach. Danno, I also addressed your feedback to avoid
adding a virtual function to AstNode with the use of multiple inheritance and
template magic (including import of a file from the LOKI library).

2) It was hard to pin down a chokepoint where the processing of the deferred
list should be done. A weakness here is the addition of a "magic" parameter to
BuildFunctionInfo(). I'll work to understand that better.

3) Golem heap/space size influenced the decision in
DeferredFeedbackSlotProcessor to go ahead and process nodes right away if they
know the number of slots they want at parse time. We get positive results if we
do that, negative otherwise. The difference is (needlessly) enqueuing all
CallNew and ForInStatement AstNodes for the sake of uniform processing. Happily,
this decision is contained entirely within DeferredFeedbackSlotProcessor, and
hopefully doesn't mar the design beyond existing accomodations.

Thanks!
--Michael

https://codereview.chromium.org/137403009/diff/480001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/137403009/diff/480001/src/ast.cc#newcode727
src/ast.cc:727: if (CallFeedbackSlot() >= 0) {
On 2014/01/24 11:29:54, Benedikt Meurer wrote:
> != kInvalidFeedbackSlot

Done.

https://codereview.chromium.org/137403009/diff/480001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/137403009/diff/480001/src/ast.h#newcode197
src/ast.h:197: maximum_feedback_slots_ += maximum;
On 2014/01/24 11:29:54, Benedikt Meurer wrote:
> Can we have an ASSERT here to make sure that  minimum_feedback_slots_ <=
> maximum_feedback_slots_?

Changed the approach totally...

https://codereview.chromium.org/137403009/diff/480001/src/full-codegen.cc
File src/full-codegen.cc (right):

https://codereview.chromium.org/137403009/diff/480001/src/full-codegen.cc#new...
src/full-codegen.cc:395: feedback_vector_ =
isolate->factory()->NewFixedArrayWithHoles(minimum_length,
On 2014/01/24 11:29:54, Benedikt Meurer wrote:
> As discussed offline: Please add an explanation here, that we have to do this
> for now because of our parser.

Changed to a new approach.

https://codereview.chromium.org/137403009/diff/480001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/137403009/diff/480001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5814: 
On 2014/01/24 11:29:54, Benedikt Meurer wrote:
> Add an AssertIsSmi(edx).

Done.

https://codereview.chromium.org/137403009/diff/480001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:5821: // TODO(mvstanton): change this message below.
On 2014/01/24 11:29:54, Benedikt Meurer wrote:
> Please fix this TODO.

Done.

https://codereview.chromium.org/137403009/diff/690001/src/ast.cc
File src/ast.cc (right):

https://codereview.chromium.org/137403009/diff/690001/src/ast.cc#newcode728
src/ast.cc:728: if (CallFeedbackSlot() >= 0) {
On 2014/01/28 08:27:17, danno wrote:
> You probably want to turn this into a convenience predicate HasFeedbackSlot()

Done.

https://codereview.chromium.org/137403009/diff/690001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/137403009/diff/690001/src/ast.h#newcode217
src/ast.h:217: virtual int ConsumeFeedbackSlots(Isolate* isolate, int
current_slot) {
On 2014/01/28 08:27:17, danno wrote:
> Is it really consuming? Isn't it more like earmarking or reserving? A comment
> might be useful here, too, e.g. "By default, AST nodes don't store their type
> information in the type vector and therefore don't reserve slots" or something
> to that effect.

Changed the approach quite a bit.

https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.cc
File src/feedbackslots.cc (right):

https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.cc#ne...
src/feedbackslots.cc:39: #define RECURSE(call)                               \
On 2014/01/28 08:27:17, danno wrote:
> Here and below, this seems like a lot of boilerplate to accomplish what you
want
> to do. Since we need the allocation slot information in common cases, maybe it
> makes sense to fold it into building the AST? That approach is probably less
> code and more efficient.

Done.

https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.h
File src/feedbackslots.h (right):

https://codereview.chromium.org/137403009/diff/690001/src/feedbackslots.h#new...
src/feedbackslots.h:44: explicit FeedbackSlotAllocator(Zone* zone) : used_(0) {
On 2014/01/28 08:27:17, danno wrote:
> I worry that the additional pass over the AST might be expensive, especially
for
> very short-running programs. E.g. did you measure SunSpider performance? 

Although it didn't appear to cost much (nodes in the cache, most likely, because
it's done after the scope analysis pass), it may be that walking over the AST is
too much. So I followed your advice elsewhere and created a list of deferred
nodes to process later.

https://codereview.chromium.org/137403009/diff/690001/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/137403009/diff/690001/src/type-info.cc#newcode87
src/type-info.cc:87: !CanRetainOtherContext(JSFunction::cast(obj),
*native_context_))) {
On 2014/01/28 08:27:17, danno wrote:
> nit: weird indentation

Addressed, but have another look.

https://codereview.chromium.org/137403009/diff/690001/test/cctest/test-heap.cc
File test/cctest/test-heap.cc (right):

https://codereview.chromium.org/137403009/diff/690001/test/cctest/test-heap.c...
test/cctest/test-heap.cc:2850: // TODO(mvstanton): fix this test.
On 2014/01/28 08:27:17, danno wrote:
> Yes, please do :-)

Done.

https://codereview.chromium.org/137403009/diff/750001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/137403009/diff/750001/src/ast.h#newcode2970
src/ast.h:2970: : dont_optimize_reason_(kNoReason),
pass the deferred worker here.

https://codereview.chromium.org/137403009/diff/750001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/137403009/diff/750001/src/compiler.cc#newcode248
src/compiler.cc:248: if (function()->slot_count() >= 0) {
turn this to an assedrt.

https://codereview.chromium.org/137403009/diff/750001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/137403009/diff/750001/src/compiler.h#newcode184
src/compiler.h:184: ProcessFeedbackSlots();
pull this out.

[mvstanton, 2014-01-30 17:37:41]

PORTS uploaded. Also, I confirmed with Daniel Berlin that it's okay to use this
file from the LOKI library.
thx,
--Michael

https://codereview.chromium.org/137403009/diff/750001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/137403009/diff/750001/src/ast.h#newcode2970
src/ast.h:2970: : dont_optimize_reason_(kNoReason),
On 2014/01/30 15:13:41, mvstanton wrote:
> pass the deferred worker here.

As it turns out, the AstConstructionVisitor can create/hold the deferred worker
just as it does for AstProperties.

https://codereview.chromium.org/137403009/diff/750001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/137403009/diff/750001/src/compiler.cc#newcode248
src/compiler.cc:248: if (function()->slot_count() >= 0) {
On 2014/01/30 15:13:41, mvstanton wrote:
> turn this to an assedrt.

Done.

https://codereview.chromium.org/137403009/diff/750001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/137403009/diff/750001/src/compiler.h#newcode184
src/compiler.h:184: ProcessFeedbackSlots();
On 2014/01/30 15:13:41, mvstanton wrote:
> pull this out.

Done.

[Benedikt Meurer, 2014-02-04 08:53:50]

https://codereview.chromium.org/137403009/diff/820001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/137403009/diff/820001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:3020: // Cache the called function in a global
property cell.  Cache states
Nit: global property cell -> feedback vector slot.

https://codereview.chromium.org/137403009/diff/820001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:3080: // Create an AllocationSite if we don't already
have it, store it in the cell
Nit: cell -> slot.

https://codereview.chromium.org/137403009/diff/820001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/137403009/diff/820001/src/ast.h#newcode918
src/ast.h:918: class ForInStatement V8_FINAL : public ForEachStatement,
You mentioned offline that you got rid of this multiple inheritance. Why is it
still around?

https://codereview.chromium.org/137403009/diff/820001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/137403009/diff/820001/src/compiler.cc#newcode246
src/compiler.cc:246: void CompilationInfo::ProcessFeedbackSlots() {
Why is this in CompilationInfo? And why do we need three different call sites
for this in the pipeline?

https://codereview.chromium.org/137403009/diff/820001/src/compiler.cc#newcode247
src/compiler.cc:247: // Scope analysis must have been done.
Can we ASSERT that?

https://codereview.chromium.org/137403009/diff/820001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/137403009/diff/820001/src/compiler.h#newcode641
src/compiler.h:641: bool process_feedback = true);
This bool parameter is really confusing. If we really need it here to make
things work, we should consider fixing our pipeline first, i.e. split this
method into two methods.

https://codereview.chromium.org/137403009/diff/820001/src/feedback_slots.h
File src/feedback_slots.h (right):

https://codereview.chromium.org/137403009/diff/820001/src/feedback_slots.h#ne...
src/feedback_slots.h:38: class FeedbackSlotInterface {
Hm, do we really need a new header file for this interface?

https://codereview.chromium.org/137403009/diff/820001/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/137403009/diff/820001/src/type-info.cc#newcode84
src/type-info.cc:84: if (obj->IsSmi() || obj->IsAllocationSite() ||
Please remove this left over hacks, and use something less obscure, i.e.:

if (!obj->IsJSFunction() || !CanRetainOtherContext(JSFunction::cast(obj),
*native_context_)))

Extra points if you generalize CanRetainOtherContext() to take an Object* and do
the IsJSFunction() check itself.

[mvstanton, 2014-02-04 13:03:27]

Hi Benedikt, hi Danno,

I've responded per our several conversations. Thanks for the help!
--Michael

https://codereview.chromium.org/137403009/diff/820001/src/arm/code-stubs-arm.cc
File src/arm/code-stubs-arm.cc (right):

https://codereview.chromium.org/137403009/diff/820001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:3020: // Cache the called function in a global
property cell.  Cache states
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
> Nit: global property cell -> feedback vector slot.

Done.

https://codereview.chromium.org/137403009/diff/820001/src/arm/code-stubs-arm....
src/arm/code-stubs-arm.cc:3080: // Create an AllocationSite if we don't already
have it, store it in the cell
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
> Nit: cell -> slot.

Done.

https://codereview.chromium.org/137403009/diff/820001/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/137403009/diff/820001/src/ast.h#newcode918
src/ast.h:918: class ForInStatement V8_FINAL : public ForEachStatement,
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
> You mentioned offline that you got rid of this multiple inheritance. Why is it
> still around?

Per our conversation, I'll keep the interface inheritance instead of adding
virtual functions to AstNode. One benefit of this approach is that we won't end
up wondering later if our new AstNode type has to implement those functions.

https://codereview.chromium.org/137403009/diff/820001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/137403009/diff/820001/src/compiler.cc#newcode246
src/compiler.cc:246: void CompilationInfo::ProcessFeedbackSlots() {
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
> Why is this in CompilationInfo? And why do we need three different call sites
> for this in the pipeline?

I managed to reduce this to a single chokepoint, SetScope(), now renamed
InitializeScope().

https://codereview.chromium.org/137403009/diff/820001/src/compiler.cc#newcode247
src/compiler.cc:247: // Scope analysis must have been done.
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
> Can we ASSERT that?

It's enough to do the work in InitializeScope() now, after the scope has been
set. I couldn't find a good handle on Scope to make sure it's complete. There is
a predicate, already_resolved(), but it doesn't appear to do exactly what I need
(private to Scope).

https://codereview.chromium.org/137403009/diff/820001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/137403009/diff/820001/src/compiler.h#newcode641
src/compiler.h:641: bool process_feedback = true);
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
> This bool parameter is really confusing. If we really need it here to make
> things work, we should consider fixing our pipeline first, i.e. split this
> method into two methods.

Took some effort but I finally figured out another way.
The real chokepoint for this work is
CompilationInfo::SetScope(), because when the scope is applied, the deferred
work can be performed. However because this is a pile of work that has to be
done, Danno noticed this was bad because "SetScope" implies a simple setter
rather than a method that does work. So I renamed the method to
InitializeScope(), indicating with a comment that some deferred work may be
performed.

In this way I don't need to even mention feedback slots in the public interface
of CompilationInfo.

https://codereview.chromium.org/137403009/diff/820001/src/feedback_slots.h
File src/feedback_slots.h (right):

https://codereview.chromium.org/137403009/diff/820001/src/feedback_slots.h#ne...
src/feedback_slots.h:38: class FeedbackSlotInterface {
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
> Hm, do we really need a new header file for this interface?

Since deferred slot processor is rather involved, ast.h is already 3400 lines,
and feedback slots are a feature used by a distinct subset of nodes only, it
seems like good style to keep this activity out of the main path.

https://codereview.chromium.org/137403009/diff/820001/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/137403009/diff/820001/src/type-info.cc#newcode84
src/type-info.cc:84: if (obj->IsSmi() || obj->IsAllocationSite() ||
On 2014/02/04 08:53:50, Benedikt Meurer wrote:
> Please remove this left over hacks, and use something less obscure, i.e.:
> 
> if (!obj->IsJSFunction() || !CanRetainOtherContext(JSFunction::cast(obj),
> *native_context_)))
> 
> Extra points if you generalize CanRetainOtherContext() to take an Object* and
do
> the IsJSFunction() check itself.


Thx, I removed the smi and allocationsite check. I reformulated the rest as you
described, but I didn't generalize for the reason that there is also a
CanRetainOtherContext(Map*, ...) method and if I provide a method that takes
Object* then the natural implication is that it should be able to call the Map*
as well. So it'll still be a little bit wordy, but this way
CanRetainOtherContext() remains very sharply defined as being able to operate on
a Map or a JSFunction.

[mvstanton, 2014-02-06 08:42:30]

ping. :D Let me know your thoughts, thanks,
--Michael

[Benedikt Meurer, 2014-02-07 09:29:25]

LGTM if question and nit below is addressed.

https://codereview.chromium.org/137403009/diff/1000001/src/feedback_slots.h
File src/feedback_slots.h (right):

https://codereview.chromium.org/137403009/diff/1000001/src/feedback_slots.h#n...
src/feedback_slots.h:52: virtual int GetFeedbackSlotCount(Isolate* isolate) = 0;
Nit: Rename to ComputeFeedbackSlotCount().

https://codereview.chromium.org/137403009/diff/1000001/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/137403009/diff/1000001/src/type-info.cc#newco...
src/type-info.cc:88: return
Handle<Object>::cast(isolate()->factory()->undefined_value());
Hm, now we return uninitialized if there's no concrete value in the slot, but
undefined if it's a JSFunction that can retain another context. Is that
intended? If so we should document this.

[mvstanton, 2014-02-07 10:39:22]

Thx guys, I addressed your comments Benedikt and your in-person comment, Danno.
--Michael

https://codereview.chromium.org/137403009/diff/1000001/src/feedback_slots.h
File src/feedback_slots.h (right):

https://codereview.chromium.org/137403009/diff/1000001/src/feedback_slots.h#n...
src/feedback_slots.h:52: virtual int GetFeedbackSlotCount(Isolate* isolate) = 0;
On 2014/02/07 09:29:25, Benedikt Meurer wrote:
> Nit: Rename to ComputeFeedbackSlotCount().

excellent idea, thx.

https://codereview.chromium.org/137403009/diff/1000001/src/type-info.cc
File src/type-info.cc (right):

https://codereview.chromium.org/137403009/diff/1000001/src/type-info.cc#newco...
src/type-info.cc:88: return
Handle<Object>::cast(isolate()->factory()->undefined_value());
On 2014/02/07 09:29:25, Benedikt Meurer wrote:
> Hm, now we return uninitialized if there's no concrete value in the slot, but
> undefined if it's a JSFunction that can retain another context. Is that
> intended? If so we should document this.

It's actually not new, it's derived from 

Handle<Object> TypeFeedbackOracle::GetInfo(TypeFeedbackId)

which did the same thing. And this is commented in the header file, for
GetInfo(TypeFeedbackId) and GetInfo(int slot).

[Benedikt Meurer, 2014-02-10 06:30:45]

>
https://codereview.chromium.org/137403009/diff/1000001/src/type-info.cc#newco...
> src/type-info.cc:88: return
> Handle<Object>::cast(isolate()->factory()->undefined_value());
> On 2014/02/07 09:29:25, Benedikt Meurer wrote:
> > Hm, now we return uninitialized if there's no concrete value in the slot,
but
> > undefined if it's a JSFunction that can retain another context. Is that
> > intended? If so we should document this.
> 
> It's actually not new, it's derived from 
> 
> Handle<Object> TypeFeedbackOracle::GetInfo(TypeFeedbackId)
> 
> which did the same thing. And this is commented in the header file, for
> GetInfo(TypeFeedbackId) and GetInfo(int slot).

Ah ok, I wasn't aware of that.

[mvstanton, 2014-02-10 21:27:09]

I'll go ahead and check in, and address additional feedback in a follow-on CL. I
think the CL at this point matches the expectations of my reviewers. Thanks
again guys!
--Michael

[mvstanton, 2014-02-10 21:38:40]

Committed patchset #15 manually as r19244 (presubmit successful).