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#newcode358
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#newcode485
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#newcode10388
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#newcode10390
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#newcode72
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-vector-inl.h
File src/type-feedback-vector-inl.h (right):
https://codereview.chromium.org/650073002/diff/500001/src/type-feedback-vector-inl.h#newcode44
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-vector-inl.h#newcode70
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-vector-inl.h#newcode76
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-vector-inl.h#newcode82
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-vector-inl.h#newcode89
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-vector.h#newcode34
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-feedback-vector.cc
File test/cctest/test-feedback-vector.cc (right):
https://codereview.chromium.org/650073002/diff/500001/test/cctest/test-feedback-vector.cc#newcode1
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-feedback-vector.cc#newcode72
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-feedback-vector.cc#newcode89
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-feedback-vector.cc#newcode104
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-feedback-vector.cc#newcode104
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/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
