vector-based ICs did not update type feedback counts correctly.

vector-based ICs did not update type feedback counts correctly.

BUG=v8:3605
LOG=N
R=jkummerow@chromium.org, ulan@chromium.org

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

[mvstanton, 2014-10-13 15:16:37]

Patchset #2 (id:280001) has been deleted

[mvstanton, 2014-10-14 09:54:07]

Patchset #1 (id:1) has been deleted

[mvstanton, 2014-10-14 09:54:15]

Patchset #1 (id:390001) has been deleted

[mvstanton, 2014-10-14 09:58:30]

mvstanton@chromium.org changed reviewers:
+ jkummerow@chromium.org, ulan@chromium.org

[mvstanton, 2014-10-14 09:58:31]

Hi Ulan, hi Jakob,
This is an involved fix, I needed to:

* Distinguish between two kinds of feedback vector slots, here named "ic slots"
and "slots."
* Make vector-based ICs (here, only call ic) correctly update statistics in the
vector rather than the TypeFeedbackInfo in the host.
* Increase the capabilities of the TypeFeedbackVector to manage and hide
information about the slots and the statistics in the vector.
* Pass the vector to the IC clearing mechanism.
* Tests!

I spoke with Ulan the most about the implementation, but Jakob you are familiar
with runtime profiling and may have some comments about that and ICs
specifically.
Thanks,
--Michael

[Jakob Kummerow, 2014-10-14 15:27:21]

I've looked at:
src/ic/*
src/objects*
src/runtime*
test/*

I liked what I saw, just a couple nits.

https://codereview.chromium.org/650073002/diff/500001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/650073002/diff/500001/src/objects-debug.cc#ne...
src/objects-debug.cc:358: feedback_vector()->IsTypeFeedbackVector();
CHECK(...)?

https://codereview.chromium.org/650073002/diff/500001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/650073002/diff/500001/src/objects-printer.cc#...
src/objects-printer.cc:485: os << "\n - ic_total_count: " << ic_total_count()
nit: no need to repeat "os", just keep streaming (like the lines below)

https://codereview.chromium.org/650073002/diff/500001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/650073002/diff/500001/src/objects.cc#newcode1...
src/objects.cc:10388: target->kind() == FUNCTION ? type_feedback_info() : NULL;
I think you're confused here (or is it a bad merge?). Per the check above,
|target| is an inline cache stub here, never a FUNCTION.

https://codereview.chromium.org/650073002/diff/500001/src/objects.cc#newcode1...
src/objects.cc:10390: feedback_info->IsTypeFeedbackInfo()
feedback_info is NULL here!

https://codereview.chromium.org/650073002/diff/500001/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

https://codereview.chromium.org/650073002/diff/500001/src/runtime-profiler.cc...
src/runtime-profiler.cc:72: // Harvest vector-ics as well
I think this should be moved out of the if-block (but maybe needs its own vector
presence check instead?).

https://codereview.chromium.org/650073002/diff/500001/src/type-info.h
File src/type-info.h (right):

https://codereview.chromium.org/650073002/diff/500001/src/type-info.h#newcode29
src/type-info.h:29: bool CallIsMonomorphic(TypeFeedbackId aid);
While you're here: this method doesn't seem to exist any more.

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
File test/cctest/test-feedback-vector.cc (right):

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:1: // Copyright 2011 the V8 project authors.
All rights reserved.
nit: 2014, and use the short license header please.

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:72: CHECK_EQ(index,
TypeFeedbackVector::kReservedIndexCount);
nit: order is "expected, actual".

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:89: // Empty vectors are the empty fixed
array.
comment out of place?

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:104:
CHECK_EQ(vector->Get(FeedbackVectorSlot(0)), Smi::FromInt(1));
nit: order is "expected, actual" (again below).

[ulan, 2014-10-15 10:17:06]

Looks good.

https://codereview.chromium.org/650073002/diff/500001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/650073002/diff/500001/src/ic/ic.cc#newcode1964
src/ic/ic.cc:1964: if (host->type_feedback_info()->IsTypeFeedbackInfo()) {
Can we have a vector without having type_feedback_info()?

DCHECK(vector == host->type_feedback_info()->feedback_vector());

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
File src/type-feedback-vector-inl.h (right):

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:44: int first_ic_slot = first_ic_slot_index();
If instead of -1, you would make first_ic_slot == length() when there are no
ic_slots, then functions here would be simpler:

Slots(): return Max(0, first_ic_slot_index() - kReservedIndexCount);
ICSlots(): return length() - first_ic_slot_index();

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:70: DCHECK(first_ic_slot >= 0);
DCHECK(slot.ToInt() < ICSlots()) would be more precise.

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:76: DCHECK(index >= kReservedIndexCount);
also DCHECK(index < first_ic_slot_index() || first_ic_slot_index() == -1)?

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:82: DCHECK(index >= kReservedIndexCount);
DCHECK(index >= first_ic_slot_index) would be more precise.
also DCHECK(index < length())

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:89: return get(kReservedIndexCount +
slot.ToInt());
Why not use GetIndex(slot) here and below? :)

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vector.h
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector.h:34: // In a TypeFeedbackVector for ICs, the first two
indexes are reserved
Comment seems out-of-date.

[mvstanton, 2014-10-16 10:45:21]

Patchset #5 (id:540001) has been deleted

[mvstanton, 2014-10-16 10:54:16]

Hi guys, thanks for the look, I've addressed your comments, and dealt with:

1) perf issue: it's important to keep the MONO->MEGA transition in the stub and
not miss to the runtime for the CallIC. I simply need to update statistics in
the vector.
2) perf/design issue: As discussed in the last days, it's problematic to encode
a vector ic slot in the RelocInfo. Strange perf issues occur, and it breaks
logic we rely on that a RelocInfo ID *is* a TypeFeedbackInfo. I'm also unhappy
with the new field TypeFeedbackInfo::feedback_vector, which permutes gc behavior
more than I would like by reliably pointing to the vector from the code object
when code flushing may have tried to sever that relationship. It can keep a
function alive for one more gc, which just doesn't feel clean.

Therefore, in answer to 2), I've removed this field and set things up so that
vector-based ICs are cleared when the rest of the vector slots are cleared:
during a visit to the SharedFunctionInfo. This eliminates the problem with
RelocInfo.

For now, with this approach overhead is minimal, but it does mean a tagging with
Code::Kind will have to occur for IC slots. I've spoken with Toon about that at
length, and am sure this capability can be added to the vector with a minimum of
overhead. With this CL here, we've begun to add abstractions around the vector
internals that make this easier.

Performance is looking good. Thanks!
--Michael

ps - Jakob, I didn't yet address your feedback vis-a-vis
type-feedback-vector-inl.h, wanting to keep churn to a minimum, but I will
address it after this round (Ulan, the idea is too much use of inl...better to
just inline the most important functions directly in .h).

https://codereview.chromium.org/650073002/diff/500001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/650073002/diff/500001/src/ic/ic.cc#newcode1964
src/ic/ic.cc:1964: if (host->type_feedback_info()->IsTypeFeedbackInfo()) {
On 2014/10/15 10:17:05, ulan wrote:
> Can we have a vector without having type_feedback_info()?
> 
> DCHECK(vector == host->type_feedback_info()->feedback_vector());

Indeed, it looks like we can count on this because we are always a FUNCTION.

https://codereview.chromium.org/650073002/diff/500001/src/objects-debug.cc
File src/objects-debug.cc (right):

https://codereview.chromium.org/650073002/diff/500001/src/objects-debug.cc#ne...
src/objects-debug.cc:358: feedback_vector()->IsTypeFeedbackVector();
On 2014/10/14 15:27:20, Jakob wrote:
> CHECK(...)?

:D. Done.

https://codereview.chromium.org/650073002/diff/500001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/650073002/diff/500001/src/objects-printer.cc#...
src/objects-printer.cc:485: os << "\n - ic_total_count: " << ic_total_count()
On 2014/10/14 15:27:20, Jakob wrote:
> nit: no need to repeat "os", just keep streaming (like the lines below)

Done.

https://codereview.chromium.org/650073002/diff/500001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/650073002/diff/500001/src/objects.cc#newcode1...
src/objects.cc:10388: target->kind() == FUNCTION ? type_feedback_info() : NULL;
On 2014/10/14 15:27:20, Jakob wrote:
> I think you're confused here (or is it a bad merge?). Per the check above,
> |target| is an inline cache stub here, never a FUNCTION.

Yes, this is hilarious and sad. Sorry 'bout that!

https://codereview.chromium.org/650073002/diff/500001/src/objects.cc#newcode1...
src/objects.cc:10390: feedback_info->IsTypeFeedbackInfo()
On 2014/10/14 15:27:20, Jakob wrote:
> feedback_info is NULL here!

The logic was revisited.

https://codereview.chromium.org/650073002/diff/500001/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

https://codereview.chromium.org/650073002/diff/500001/src/runtime-profiler.cc...
src/runtime-profiler.cc:72: // Harvest vector-ics as well
On 2014/10/14 15:27:20, Jakob wrote:
> I think this should be moved out of the if-block (but maybe needs its own
vector
> presence check instead?).

Hmm, I don't get it. If we have a TypeFeedbackInfo then it'll have a valid
feedback_vector in it, so what would moving it out of the if block help?

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
File src/type-feedback-vector-inl.h (right):

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:44: int first_ic_slot = first_ic_slot_index();
On 2014/10/15 10:17:06, ulan wrote:
> If instead of -1, you would make first_ic_slot == length() when there are no
> ic_slots, then functions here would be simpler:
> 
> Slots(): return Max(0, first_ic_slot_index() - kReservedIndexCount);
> ICSlots(): return length() - first_ic_slot_index();

Done.

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:70: DCHECK(first_ic_slot >= 0);
On 2014/10/15 10:17:05, ulan wrote:
> DCHECK(slot.ToInt() < ICSlots()) would be more precise.

Done.

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:76: DCHECK(index >= kReservedIndexCount);
On 2014/10/15 10:17:06, ulan wrote:
> also DCHECK(index < first_ic_slot_index() || first_ic_slot_index() == -1)?

Done.

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:82: DCHECK(index >= kReservedIndexCount);
On 2014/10/15 10:17:05, ulan wrote:
> DCHECK(index >= first_ic_slot_index) would be more precise.
> also DCHECK(index < length())

Done.

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:89: return get(kReservedIndexCount +
slot.ToInt());
On 2014/10/15 10:17:06, ulan wrote:
> Why not use GetIndex(slot) here and below? :)

Oh brother, great idea, thx!

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vector.h
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vecto...
src/type-feedback-vector.h:34: // In a TypeFeedbackVector for ICs, the first two
indexes are reserved
On 2014/10/15 10:17:06, ulan wrote:
> Comment seems out-of-date.

Thanks. I removed it, as it seems pretty clear from the code what is happenin'.

https://codereview.chromium.org/650073002/diff/500001/src/type-info.h
File src/type-info.h (right):

https://codereview.chromium.org/650073002/diff/500001/src/type-info.h#newcode29
src/type-info.h:29: bool CallIsMonomorphic(TypeFeedbackId aid);
On 2014/10/14 15:27:20, Jakob wrote:
> While you're here: this method doesn't seem to exist any more.

Done.

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
File test/cctest/test-feedback-vector.cc (right):

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:1: // Copyright 2011 the V8 project authors.
All rights reserved.
On 2014/10/14 15:27:20, Jakob wrote:
> nit: 2014, and use the short license header please.

Done.

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:72: CHECK_EQ(index,
TypeFeedbackVector::kReservedIndexCount);
On 2014/10/14 15:27:20, Jakob wrote:
> nit: order is "expected, actual".

Done.

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:89: // Empty vectors are the empty fixed
array.
On 2014/10/14 15:27:20, Jakob wrote:
> comment out of place?

Done.

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:104:
CHECK_EQ(vector->Get(FeedbackVectorSlot(0)), Smi::FromInt(1));
On 2014/10/14 15:27:20, Jakob wrote:
> nit: order is "expected, actual" (again below).

Done.

https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedba...
test/cctest/test-feedback-vector.cc:104:
CHECK_EQ(vector->Get(FeedbackVectorSlot(0)), Smi::FromInt(1));
On 2014/10/14 15:27:20, Jakob wrote:
> nit: order is "expected, actual" (again below).

Done.

[ulan, 2014-10-17 13:55:43]

LGTM

[mvstanton, 2014-10-20 07:57:44]

Thanks Ulan! Jakob, I did some adjustment with type-feedback-vector-inl.h can
you have a look?
Thanks yall,
--Michael

https://codereview.chromium.org/650073002/diff/560001/src/type-feedback-vecto...
File src/type-feedback-vector-inl.h (right):

https://codereview.chromium.org/650073002/diff/560001/src/type-feedback-vecto...
src/type-feedback-vector-inl.h:97: }
I've moved these out of -inl.h back to .h. They don't pollute the namespace,
dealing only with smis, slots, constants, etc.

[Jakob Kummerow, 2014-10-20 10:29:43]

LGTM

https://codereview.chromium.org/650073002/diff/580001/src/type-feedback-vecto...
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/650073002/diff/580001/src/type-feedback-vecto...
src/type-feedback-vector.cc:71: // Fall through...
Uhm, what? Either "break;" or "// Fall through.", but not both!

(I'd just use an if/else, but if you like your switch statement, you can keep
it.)

https://codereview.chromium.org/650073002/diff/580001/src/type-feedback-vecto...
src/type-feedback-vector.cc:78: // Now clear vector-based ICs. They are all
CallICs.
Whoa. The comment in the header said "leaves vector ic slots alone".

Is there a DCHECK we could guard the "They're all CallICs" invariant with?

https://codereview.chromium.org/650073002/diff/580001/src/type-feedback-vector.h
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/650073002/diff/580001/src/type-feedback-vecto...
src/type-feedback-vector.h:18: // 0: first_ic_slot_index (-1 if no ic slots
present)
the "-1" part is outdated, right?

[mvstanton, 2014-10-20 11:01:57]

Patchset #7 (id:600001) has been deleted

[mvstanton, 2014-10-20 11:09:22]

Addressed comments. Thx again, submitting...
--Michael

https://codereview.chromium.org/650073002/diff/580001/src/type-feedback-vecto...
File src/type-feedback-vector.cc (right):

https://codereview.chromium.org/650073002/diff/580001/src/type-feedback-vecto...
src/type-feedback-vector.cc:71: // Fall through...
On 2014/10/20 10:29:43, Jakob wrote:
> Uhm, what? Either "break;" or "// Fall through.", but not both!
> 
> (I'd just use an if/else, but if you like your switch statement, you can keep
> it.)

You are right, it's unwieldy...changing to if()...

https://codereview.chromium.org/650073002/diff/580001/src/type-feedback-vecto...
src/type-feedback-vector.cc:78: // Now clear vector-based ICs. They are all
CallICs.
On 2014/10/20 10:29:43, Jakob wrote:
> Whoa. The comment in the header said "leaves vector ic slots alone".
> 
> Is there a DCHECK we could guard the "They're all CallICs" invariant with?

Nice catch, I'll fix that outdated comment in the header...it's an artifact of
the first try at this, where vector IC clearing was done during RelocInfo
marking.

Hmm, the DCHECK won't work because there isn't currently a "metadata" section in
the TypeFeedbackVector that can be checked to verify that each IC slot is a
CallIC. But my *next* CL already has such a structure, and it's usage will be
checked. The loop below will end up casing the handling of each IC slot on
something like 

Code::KIND kind(FeedbackVectorICSlot slot);

https://codereview.chromium.org/650073002/diff/580001/src/type-feedback-vector.h
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/650073002/diff/580001/src/type-feedback-vecto...
src/type-feedback-vector.h:18: // 0: first_ic_slot_index (-1 if no ic slots
present)
On 2014/10/20 10:29:43, Jakob wrote:
> the "-1" part is outdated, right?

Indeed, thanks!

[mvstanton, 2014-10-20 11:43:35]

Committed patchset #8 (id:640001) manually as 24732 (presubmit successful).