In general, definitely going in the right direction, see my comments below I
think you might be able to split this CL in a fewer smaller pieces.
https://codereview.chromium.org/35413006/diff/200001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):
https://codereview.chromium.org/35413006/diff/200001/src/ia32/ic-ia32.cc#newcode738
src/ia32/ic-ia32.cc:738: // We have to go to the runtime if the current
value is undefined because
nit: the hole, not undefined. It could be some other value that we get
from the prototype, all we know here is that it's the hole.
https://codereview.chromium.org/35413006/diff/200001/src/ia32/macro-assembler-ia32.cc
File src/ia32/macro-assembler-ia32.cc (right):
https://codereview.chromium.org/35413006/diff/200001/src/ia32/macro-assembler-ia32.cc#newcode3561
src/ia32/macro-assembler-ia32.cc:3561: // ebx contained elements
pointer.
nit: comment is wrong above (no explicit register need to be named).
https://codereview.chromium.org/35413006/diff/200001/src/ia32/macro-assembler-ia32.cc#newcode3562
src/ia32/macro-assembler-ia32.cc:3562: mov(scratch, FieldOperand(object,
HeapObject::kMapOffset));
If you move object into scratch before entering the loop and add a bind
of a label "loop_again" then at the end of the loop you can just use:
cmp(scratch, Immediate(factory->null_value()));
j(not_equal, &loop_again);
and get rid of the
mov(scratch, FieldOperand(scratch, HeapObject::kMapOffset));
jmp(&loop);
https://codereview.chromium.org/35413006/diff/200001/src/ic.cc
File src/ic.cc (right):
https://codereview.chromium.org/35413006/diff/200001/src/ic.cc#newcode1980
src/ic.cc:1980: if (object->IsJSObject()) {
You should only have to check for JSObject once at this logic below
MISS_FORCE_GENERIC, and then only do the logic below:
if (receiver->map()->MayHaveIndexedCallbacksInPrototypeChain()) {
...
}
Might make things a little cleaner.
https://codereview.chromium.org/35413006/diff/200001/src/ic.cc#newcode1992
src/ic.cc:1992: TRACE_GENERIC_IC(isolate(), "KeyedStoreIC", "key not a
number");
Remove the TRACE_GENERIC_IC here and below, it's covered by Hannes' new
code on line 2007.
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode2958
src/objects.cc:2958: for (Object* pt = prototype();
don't use the abbreviation, use the variable name prototype and call the
function by specifying this->prototype()
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode4525
src/objects.cc:4525: // The final fields to look at are flag fields 3
and 4. Field 3
Is the logic needed? I thought the conclusion was other than the
transitions array, everything in this slow assert logic is OK. In that
case, it's probably better/safer to just use the original memcmp, since
that will automatically catch problems with new fields that are added to
the Map without having to explicitly add them in the code here.
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode4751
src/objects.cc:4751: FixedArrayBase* array = GetElements();
I'd call this GetElementBackingStore(), and you might want to find the
places in the code that can use it (look for "get(1)" in this file).
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode5625
src/objects.cc:5625: ASSERT(!transition_map->is_extensible());
nit: remove stray whitespace change.
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode6292
src/objects.cc:6292: Handle<SeededNumberDictionary>
new_element_dictionary;
I think NormalizeElements will do the right thing without all of this
extra logic, including gcreating a transition from the fast elements
kind map to the new dictionary map if you need one, or just leaving the
object in dictionary mode and not changing the map if you are already
here.
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode6301
src/objects.cc:6301: old_map->LookupTransition(*object,
It looks like JSObject::SetObserve does almost exactly the same logic as
you have here. Maybe it makes sense to have a SetHasElementsCallback or
PrepareForSetElementCallback that works just like SetObserve (including
the logic to to ignore external array elements, e.g. TypedArrays).
Sorry, I should have found that code for your sooner.
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode6324
src/objects.cc:6324: // I *could* look at the maps and only reset the
poly/mono ones that
nit: we try to avoid using "I" or "we" in comments. Passive voice is
your enemy in writing a novel, but your friend here :-p
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode7029
src/objects.cc:7029: MaybeObject* Map::CopyAsElementCallbacksTransition(
I think with a little work, you can 100% share this code with
CopyForObserved, refactoring to call it CopyForTransitionedMap, and just
pustting the logic to set either "set_is_observed" or
"set_has_elements_callback" either in wrapper functions
CopyForObserved/CopyForElementsCallback, or just directly in
SetObserved/SetElementCallback.
https://codereview.chromium.org/35413006/diff/200001/src/objects.h
File src/objects.h (right):
https://codereview.chromium.org/35413006/diff/200001/src/objects.h#newcode2110
src/objects.h:2110: inline FixedArrayBase* GetElements();
Why is elements() not sufficient here?
https://codereview.chromium.org/35413006/diff/200001/src/objects.h#newcode5314
src/objects.h:5314: void ClearInlineCaches(Kind* kind);
Shouldn't this one be private?
https://codereview.chromium.org/35413006/
--
--
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/groups/opt_out.
