http://codereview.chromium.org/8166017/diff/3001/src/heap.h File src/heap.h (right):
http://codereview.chromium.org/8166017/diff/3001/src/heap.h#newcode204 src/heap.h:204: "KeyedStoreElmMonoTransSmiObject") \ On 2011/10/07 09:16:38, danno wrote:
Use the full symbol name
Done. http://codereview.chromium.org/8166017/diff/3001/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1600 src/ic.cc:1600: if (ic_state == UNINITIALIZED || ic_state == PREMONOMORPHIC || On 2011/10/07 09:16:38, danno wrote:
How about just create a variable "use_monomorphic_stub" that gets set
if
ic_state == UNINITIALIZED || ic_state == PREMONOMORPHIC is true, or in
a
separate if for IsMonomorphicButMissingTransition, so that you don't
have to
re-test ic_state == MONOMORPHIC && stub_kind > STORE_NO_TRANSITION
Done. http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1603 src/ic.cc:1603: target()->ContainsMap(receiver->map()))) { On 2011/10/07 09:16:38, danno wrote:
Can you put these into a predicate or something that makes it clear
what the
test is actually trying to accomplish? Like
MonomorphicButMissingTransition.
Whatever.
Refactored this section. http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1604 src/ic.cc:1604: Map* existing_additional_map = NULL; On 2011/10/07 09:16:38, danno wrote:
smi_only_or_double_map? Or at least comment the name.
Done. http://codereview.chromium.org/8166017/diff/3001/src/ic.cc#newcode1759 src/ic.cc:1759: bool force_generic) { On 2011/10/07 09:16:38, danno wrote:
stub_kind?
No, as discussed. Two reasons: (1) The logic wouldn't become any easier, it would only be moved. (2) Having the logic here means we don't have to go through a miss first, but can generate a transition IC right away if we have to. http://codereview.chromium.org/8166017/diff/3001/src/ic.h File src/ic.h (right): http://codereview.chromium.org/8166017/diff/3001/src/ic.h#newcode345 src/ic.h:345: enum StubKind { LOAD, STORE_NO_TRANSITION, STORE_TRANSITION_SMI_TO_OBJECT, On 2011/10/07 09:16:38, danno wrote:
nit: list these vertically, they are much easier to read.
Done. http://codereview.chromium.org/8166017/diff/3001/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2223 src/objects.cc:2223: ASSERT(safe_to_add_transition); On 2011/10/07 09:16:38, danno wrote:
comment about this assert?
Done. http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2280 src/objects.cc:2280: if (current_map->elements_kind() == FAST_SMI_ONLY_ELEMENTS && On 2011/10/07 09:16:38, danno wrote:
can you put this in "from_kind" and rename the other one to "to_kind"
Done. http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode2346 src/objects.cc:2346: } On 2011/10/07 09:16:38, danno wrote:
I think you need a test that covers these code paths.
Done. http://codereview.chromium.org/8166017/diff/3001/src/objects.cc#newcode7536 src/objects.cc:7536: // TODO(jkummerow): Consider refactoring this to share some code. On 2011/10/07 09:16:38, danno wrote:
Do you still need this?
No. http://codereview.chromium.org/8166017/diff/3001/src/objects.h File src/objects.h (right): http://codereview.chromium.org/8166017/diff/3001/src/objects.h#newcode3748 src/objects.h:3748: bool ContainsMap(Map* map); On 2011/10/07 09:16:38, danno wrote:
Do you still need this?
No. http://codereview.chromium.org/8166017/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
