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#newcode714
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#newcode742
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#newcode779
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#newcode847
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#newcode860
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-elements.js
File test/mjsunit/getters-on-elements.js (right):
https://codereview.chromium.org/35413006/diff/60001/test/mjsunit/getters-on-elements.js#newcode75
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-elements.js
File test/mjsunit/setters-on-elements.js (right):
https://codereview.chromium.org/35413006/diff/60001/test/mjsunit/setters-on-elements.js#newcode117
test/mjsunit/setters-on-elements.js:117: // into ignoring the callback.
The object map doesn't have the setter callback.
80 col
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.
