Hi Danno, thx for the comments, PTAL (again :p),
--Michael
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
On 2013/10/30 12:17:45, danno wrote:
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.
Done.
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.
On 2013/10/30 12:17:45, danno wrote:
nit: comment is wrong above (no explicit register need to be named).
Done.
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));
On 2013/10/30 12:17:45, danno wrote:
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);
Done.
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()) {
On 2013/10/30 12:17:45, danno wrote:
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.
Done.
https://codereview.chromium.org/35413006/diff/200001/src/ic.cc#newcode1992
src/ic.cc:1992: TRACE_GENERIC_IC(isolate(), "KeyedStoreIC", "key not a
number");
On 2013/10/30 12:17:45, danno wrote:
Remove the TRACE_GENERIC_IC here and below, it's covered by Hannes'
new code on
line 2007.
Okay, it is nice to see the reason for going generic, but I'll go for
simplicity now.
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();
On 2013/10/30 12:17:45, danno wrote:
don't use the abbreviation, use the variable name prototype and call
the
function by specifying this->prototype()
Done.
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode5625
src/objects.cc:5625: ASSERT(!transition_map->is_extensible());
On 2013/10/30 12:17:45, danno wrote:
nit: remove stray whitespace change.
Done.
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode6292
src/objects.cc:6292: Handle<SeededNumberDictionary>
new_element_dictionary;
On 2013/10/30 12:17:45, danno wrote:
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.
Yep, I thought combining the move to dictionary and setting the
HasElementCallbacks bit in one transition would be better, but it sure
does simplify things if I let go of that!
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode6301
src/objects.cc:6301: old_map->LookupTransition(*object,
On 2013/10/30 12:17:45, danno wrote:
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.
addressed.
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
On 2013/10/30 12:17:45, danno wrote:
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
:) done.
https://codereview.chromium.org/35413006/diff/200001/src/objects.cc#newcode7029
src/objects.cc:7029: MaybeObject* Map::CopyAsElementCallbacksTransition(
On 2013/10/30 12:17:45, danno wrote:
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.
I this this in a basic way, with a protected function that does a
"meaningless" transition, then the caller can set a map bit on the new
map. Let me know if it should be fancier (there could be a macro with a
code snippet, for example).
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#newcode5314
src/objects.h:5314: void ClearInlineCaches(Kind* kind);
On 2013/10/30 12:17:45, danno wrote:
Shouldn't this one be private?
Done.
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.
