Correct handling of arrays with callbacks in the prototype chain.

Correct handling of arrays with callbacks in the prototype chain.

Our generic KeyedStoreIC doesn't handle the case when a callback is
set on array elements in the prototype chain of the object, nor do
we recognize that we need to avoid the monomorphic case if these
callbacks exist.

This CL addresses the issue by looking for dictionary elements in
the prototype chain on IC misses and crankshaft element store
instructions. When found, the generic IC is used. The generic IC is
changed to go to the runtime in this case too.

In general, keyed loads are immune from this problem because they
won't return the hole: discovery of the hole goes to the runtime where
the callback will be found in the prototype chain. Double array loads
in crankshaft can return the hole but only if the prototype chain is
unaltered (we will catch such alterations).

Includes the following patch as well (already reviewed by bmeurer):
Performance regression found in test regress-2185-2.js. The problem was
that the bailout method for TransitionAndStoreStub was not performing
the appropriate transition.

(Review URL for the ElementsTransitionAndStoreIC_Miss change:
https://codereview.chromium.org/26911007)

R=danno@chromium.org

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

[mvstanton, 2013-10-23 11:37:16]

Hi guys, PTAL, here is the version without use of dependent code, just a deopt
of managed code. I'd rather push that in a separate CL.
--Michael

[danno, 2013-10-23 12:22:10]

https://codereview.chromium.org/35413006/diff/30001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/35413006/diff/30001/src/heap.cc#newcode470
src/heap.cc:470: void Heap::ClearAllKeyedStoreICs() {
Can you please add a parameter of the IC type and just make this
"ClearAllICsByKind" or something of that effect?

https://codereview.chromium.org/35413006/diff/30001/src/heap.cc#newcode475
src/heap.cc:475: code->ClearInlineCaches(Code::KEYED_STORE_IC);
You don't have to do this if the code is not a normal FUNCTION or OPTIMIZED (I
think), so you might be able to save walking RelocInfos for those by testing the
code type before clearing the caches.

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:714: static void HasElementCallbacksInPrototypeChain(
This (and similar code in other implementations) belongs in the
platform-specific macro assembler.

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:742: static void KeyedStoreGenerateGenericHelper(
I wish *so* badly that we generaed this in Hydrogen. It would make this hackery
unnecessary.

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:779: 
nit: please remove the extraneous whitespace change

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:847: 
nit: please remove the extraneous whitespace change

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:860: 
nit: please remove the extraneous whitespace change

https://codereview.chromium.org/35413006/diff/30001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/35413006/diff/30001/src/ic.cc#newcode1966
src/ic.cc:1966: receiver->MayHaveIndexedCallbacksInPrototypeChain()) {
I think you can and should combine this with the IsMapInArrayPrototypeChain case
above into a separate function that computes and returns use_ic.

https://codereview.chromium.org/35413006/diff/30001/src/ic.cc#newcode1966
src/ic.cc:1966: receiver->MayHaveIndexedCallbacksInPrototypeChain()) {
Why doesn't MayHaveIndexedCallbacksInPrototypeChain also check
receiver->map()->has_element_callbacks()? It seems the first is just a
generalization of the second.

https://codereview.chromium.org/35413006/diff/30001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/35413006/diff/30001/src/objects.cc#newcode6261
src/objects.cc:6261: object->map()->set_has_element_callbacks(true);
This isn't quite right. You might "pollute" normal maps that make this
transition that don't have setters/getters. You need to use the map transition
mechanism (see how freeze/seal map transitions works, this should essentially
work in exactly the same way).

https://codereview.chromium.org/35413006/diff/30001/src/objects.cc#newcode6267
src/objects.cc:6267: // Install the initial map, new_ek_map in the function
prototype for
Is this comment correct?

https://codereview.chromium.org/35413006/diff/30001/src/objects.cc#newcode6271
src/objects.cc:6271: JSFunction::SetPrototype(array_function, object);
You are "rebuilding the cache" by side effect, which is kind of scary. Setting
the prototype triggers (might trigger?) DependentCode deoptimization for
prototype chains, which I don't think is relevant in this case. Please find a
way to do the cache re-building explicitly if it is even necessary at all
(didn't we convince ourselves that it's not?)

https://codereview.chromium.org/35413006/diff/30001/src/objects.cc#newcode6281
src/objects.cc:6281: Deoptimizer::DeoptimizeAll(object->GetIsolate());
Oh, *the humanity* Are you sure it can't be in this CL? It's pretty mechanical
extension of the current mechanism to add a new group and add the dependency in
hydrogen.cc

https://codereview.chromium.org/35413006/diff/60001/test/mjsunit/getters-on-e...
File test/mjsunit/getters-on-elements.js (right):

https://codereview.chromium.org/35413006/diff/60001/test/mjsunit/getters-on-e...
test/mjsunit/getters-on-elements.js:75: // into ignoring the callback. The
object map doesn't have the setter callback.
80 col

https://codereview.chromium.org/35413006/diff/60001/test/mjsunit/setters-on-e...
File test/mjsunit/setters-on-elements.js (right):

https://codereview.chromium.org/35413006/diff/60001/test/mjsunit/setters-on-e...
test/mjsunit/setters-on-elements.js:117: // into ignoring the callback. The
object map doesn't have the setter callback.
80 col

[mvstanton, 2013-10-30 10:42:42]

Hi Danno, I've addressed these issues. PTAL. It might be possible/desirable to
break up the CL a bit.

https://codereview.chromium.org/35413006/diff/30001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/35413006/diff/30001/src/heap.cc#newcode470
src/heap.cc:470: void Heap::ClearAllKeyedStoreICs() {
On 2013/10/23 12:22:11, danno wrote:
> Can you please add a parameter of the IC type and just make this
> "ClearAllICsByKind" or something of that effect?

Done.

https://codereview.chromium.org/35413006/diff/30001/src/heap.cc#newcode475
src/heap.cc:475: code->ClearInlineCaches(Code::KEYED_STORE_IC);
On 2013/10/23 12:22:11, danno wrote:
> You don't have to do this if the code is not a normal FUNCTION or OPTIMIZED (I
> think), so you might be able to save walking RelocInfos for those by testing
the
> code type before clearing the caches.

Done.

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:714: static void HasElementCallbacksInPrototypeChain(
On 2013/10/23 12:22:11, danno wrote:
> This (and similar code in other implementations) belongs in the
> platform-specific macro assembler.

Done.

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:742: static void KeyedStoreGenerateGenericHelper(
On 2013/10/23 12:22:11, danno wrote:
> I wish *so* badly that we generaed this in Hydrogen. It would make this
hackery
> unnecessary.

I've added this to the TODO list.

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:779: 
On 2013/10/23 12:22:11, danno wrote:
> nit: please remove the extraneous whitespace change

Done.

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:847: 
On 2013/10/23 12:22:11, danno wrote:
> nit: please remove the extraneous whitespace change

Done.

https://codereview.chromium.org/35413006/diff/30001/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:860: 
On 2013/10/23 12:22:11, danno wrote:
> nit: please remove the extraneous whitespace change

Done.

https://codereview.chromium.org/35413006/diff/30001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/35413006/diff/30001/src/ic.cc#newcode1966
src/ic.cc:1966: receiver->MayHaveIndexedCallbacksInPrototypeChain()) {
On 2013/10/23 12:22:11, danno wrote:
> Why doesn't MayHaveIndexedCallbacksInPrototypeChain also check
> receiver->map()->has_element_callbacks()? It seems the first is just a
> generalization of the second.

Done.

https://codereview.chromium.org/35413006/diff/30001/src/ic.cc#newcode1966
src/ic.cc:1966: receiver->MayHaveIndexedCallbacksInPrototypeChain()) {
On 2013/10/23 12:22:11, danno wrote:
> I think you can and should combine this with the IsMapInArrayPrototypeChain
case
> above into a separate function that computes and returns use_ic.

Done.

[danno, 2013-10-30 12:17:44]

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#newc...
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...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/35413006/diff/200001/src/ia32/macro-assembler...
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...
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?

[mvstanton, 2013-10-30 18:22:27]

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#newc...
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...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/35413006/diff/200001/src/ia32/macro-assembler...
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...
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.

[mvstanton, 2013-11-04 14:01:06]

Per Toon's idea, I've removed the map flag in favor of making sure keyed stores
in IC and crankshaft go generic on discovery of a dictionary map somewhere in
the prototype chain. This reduces the changeset considerably.
PTAL,
thx,
--Michael

[Michael Starzinger, 2013-11-04 15:18:16]

Looking good. Just a coupe of nits. Some might apply to more than one
architecture, I only looked at x64 for now.

https://codereview.chromium.org/35413006/diff/450001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/35413006/diff/450001/src/heap.h#newcode770
src/heap.h:770: void ClearAllICsByKind(Code::Kind kind);
nit: Let's add a short one-liner comment that states that this iterates the
whole code-space.

https://codereview.chromium.org/35413006/diff/450001/src/x64/macro-assembler-...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/35413006/diff/450001/src/x64/macro-assembler-...
src/x64/macro-assembler-x64.cc:4993: movq(elements, FieldOperand(object,
JSObject::kElementsOffset));
Instead of specializing the semantics of this function to use the elements
register, let's just pass an arbitrary "scratch" register and let the caller
take care of saving and restoring it. AFAICT "rdi" should still be available as
a true scratch register at both call-sites.

https://codereview.chromium.org/35413006/diff/450001/src/x64/macro-assembler-...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/35413006/diff/450001/src/x64/macro-assembler-...
src/x64/macro-assembler-x64.h:1416: void HasDictionaryInPrototypeChain(Register
object, Register elements,
nit: Let's name this "JumpIfDictionaryInPrototypeChain", that name better
implies what the method does.

[mvstanton, 2013-11-04 16:30:04]

thx, addressed Michael's points.

https://codereview.chromium.org/35413006/diff/450001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/35413006/diff/450001/src/heap.h#newcode770
src/heap.h:770: void ClearAllICsByKind(Code::Kind kind);
On 2013/11/04 15:18:17, Michael Starzinger wrote:
> nit: Let's add a short one-liner comment that states that this iterates the
> whole code-space.

Done.

https://codereview.chromium.org/35413006/diff/450001/src/x64/macro-assembler-...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/35413006/diff/450001/src/x64/macro-assembler-...
src/x64/macro-assembler-x64.cc:4993: movq(elements, FieldOperand(object,
JSObject::kElementsOffset));
On 2013/11/04 15:18:17, Michael Starzinger wrote:
> Instead of specializing the semantics of this function to use the elements
> register, let's just pass an arbitrary "scratch" register and let the caller
> take care of saving and restoring it. AFAICT "rdi" should still be available
as
> a true scratch register at both call-sites.

Thx, done.

https://codereview.chromium.org/35413006/diff/450001/src/x64/macro-assembler-...
File src/x64/macro-assembler-x64.h (right):

https://codereview.chromium.org/35413006/diff/450001/src/x64/macro-assembler-...
src/x64/macro-assembler-x64.h:1416: void HasDictionaryInPrototypeChain(Register
object, Register elements,
On 2013/11/04 15:18:17, Michael Starzinger wrote:
> nit: Let's name this "JumpIfDictionaryInPrototypeChain", that name better
> implies what the method does.

Good idea. Done.

[danno, 2013-11-04 17:44:44]

lgtm with nits

https://codereview.chromium.org/35413006/diff/520001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/35413006/diff/520001/src/objects.cc#newcode6256
src/objects.cc:6256: 
nit: two returns between functions

https://codereview.chromium.org/35413006/diff/520001/src/x64/macro-assembler-...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/35413006/diff/520001/src/x64/macro-assembler-...
src/x64/macro-assembler-x64.cc:4969: 
nit: two lines between functions

[mvstanton, 2013-11-05 09:06:50]

Right on, thanks!

https://codereview.chromium.org/35413006/diff/520001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/35413006/diff/520001/src/objects.cc#newcode6256
src/objects.cc:6256: 
On 2013/11/04 17:44:44, danno wrote:
> nit: two returns between functions

Done.

https://codereview.chromium.org/35413006/diff/520001/src/x64/macro-assembler-...
File src/x64/macro-assembler-x64.cc (right):

https://codereview.chromium.org/35413006/diff/520001/src/x64/macro-assembler-...
src/x64/macro-assembler-x64.cc:4969: 
On 2013/11/04 17:44:44, danno wrote:
> nit: two lines between functions

Done.

[mvstanton, 2013-11-06 08:45:44]

Hi Danno,
Could you have another quick look?
thanks,
--Michael

[danno, 2013-11-06 15:37:28]

lgtm

[mvstanton, 2013-11-06 15:45:59]

Committed patchset #13 manually as r17525 (presubmit successful).

[Paul Lind, 2013-11-06 21:32:58]

Drive-by comment: Hi Michael - In your JumpIfDictionaryInPrototypeChain() you
read Map::kBitField2Offset with a word-load on all arch's. This field is not
word-aligned, so it probably takes some tiny, tiny amount longer to load than if
you'd just read the byte. ldrb on arm, movzxbq on x64, and probably just movb on
ia32. This is about as micro as micro-optimiztions get ;-) MIPS is allergic to
unaligned loads, so we have to do this for our port.

[Sven Panne, 2013-11-12 08:58:46]

Just a note: This CL regresses the "--stress-opt --always-opt" variant of
mjsunit/readonly by roughly 60%, not sure if this is a real issue, though.

[mvstanton, 2013-11-12 09:32:03]

On 2013/11/12 08:58:46, Sven Panne wrote:
> Just a note: This CL regresses the "--stress-opt --always-opt" variant of
> mjsunit/readonly by roughly 60%, not sure if this is a real issue, though.

Thx guys. Paul, I'll address the word load issue you mentioned, thx for the
heads up!

Sven, I believe this is because setting a prototype got more expensive in one
case. I clear all KeyedStoreICs in the code on prototype set when the set
introduced dictionary elements into the chain where the receiver elements are
fast. I'll have a look.