I chancged the code to allow the new call-stubs for in the direct prototype
of a
slow properties object only.
http://codereview.chromium.org/2801018/diff/39001/9002
File src/arm/stub-cache-arm.cc (right):
http://codereview.chromium.org/2801018/diff/39001/9002#newcode745
src/arm/stub-cache-arm.cc:745: Register) {
On 2010/06/28 11:40:08, Mads Ager wrote:
Let's move this register argument to before/after scratch and give it
a name?
I added this argument as a last one since is is not used in the most
cases (at least now, while this chenche only exists for ia32). It allows
to keep unchanged ~45 of ~58 usings of CheckPrototypes. Do you really
think it make sense to move this parameter?
http://codereview.chromium.org/2801018/diff/39001/9003
File src/globals.h (right):
http://codereview.chromium.org/2801018/diff/39001/9003#newcode468
src/globals.h:468: PROTOTYPE_MAP // For hashes (except GlobalObjects).
On 2010/06/28 11:40:08, Mads Ager wrote:
hashes -> slow properties objects?
Done.
http://codereview.chromium.org/2801018/diff/39001/9006
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/2801018/diff/39001/9006#newcode154
src/ia32/stub-cache-ia32.cc:154: static const int kProbes = 4;
On 2010/06/28 11:40:08, Mads Ager wrote:
This loop has to be different. We need to check for deleted elements
in the
dictionary. We need to skip deleted elements until we reach an element
that is
not deleted. At that point, if the element is absent we can proceed.
This loop intentionally doesn't check for deleted properties. The check
may decrease false-negative rate, but would cost two additional
instructions per iteration. At least I'd prefer to add it in a separate
CL if an experiment has shown that it makes sense.
http://codereview.chromium.org/2801018/diff/39001/9006#newcode858
src/ia32/stub-cache-ia32.cc:858: // fast and global obgects or do
negative lookup for normal objects.
On 2010/06/28 11:40:08, Mads Ager wrote:
obgects -> objects
Done.
http://codereview.chromium.org/2801018/diff/39001/9007
File src/ic-inl.h (right):
http://codereview.chromium.org/2801018/diff/39001/9007#newcode97
src/ic-inl.h:97: !object->HasFastProperties() &&
On 2010/06/28 11:40:08, Mads Ager wrote:
I don't think this is what we want to do. I don't think we want to use
the
prototype map for all stubs generated for objects that do not have
fast
properties. I think we should only use the prototype map for the new
call stubs.
This code DOES return PROTOTYPE_MAP for the new stubs only (slow
properties objects except globals what don't contain the property).
Since we do not generate load stubs such objects they are call stubs.
http://codereview.chromium.org/2801018/diff/39001/9008
File src/ic.cc (right):
http://codereview.chromium.org/2801018/diff/39001/9008#newcode138
src/ic.cc:138: // Returns false when generating a stub is possible, but
effect is usually
On 2010/06/28 11:40:08, Mads Ager wrote:
I don't understand why these conditions are right? Please explain why
these
conditions make sense?
The idea was to not generate new load stubs. This code don't check for
slow objects in the middle considering it as a rare case. So I checked
the first object (except it's a holder or property is not found: both
cases have special stubs).
Anyway making conditions stricter for now I remover this function using
HasNormalObjectsInPrototypeChain instead.
http://codereview.chromium.org/2801018/diff/39001/9008#newcode140
src/ic.cc:140: static bool DoesWorthToCache(LookupResult* lookup,
Object* receiver) {
On 2010/06/28 11:40:08, Mads Ager wrote:
DoesWorthToCache -> IsWorthCaching
Done.
http://codereview.chromium.org/2801018/diff/39001/9008#newcode150
src/ic.cc:150: static bool
HashNormalObjectsInPrototypeChain(LookupResult* lookup,
On 2010/06/28 11:40:08, Mads Ager wrote:
Hash -> Has?
Done.
http://codereview.chromium.org/2801018/diff/39001/9008#newcode177
src/ic.cc:177: if (cache_holder == OWN_MAP &&
On 2010/06/28 11:40:08, Mads Ager wrote:
I don't understand why you have to change anything for the case where
the cache
holder is the map of the object itself. We should only use the
prototype map for
the newly added call stubs?
This code seems to complicated. We should be able to keep this as is
except for
the new type of IC where we have to do something different.
The condition was complicated in order to recognize cases where
IndexInCodeCache could be avoided. But you are right, this optimization
would be hard to maintain. I left only essential check.
http://codereview.chromium.org/2801018/diff/39001/9008#newcode183
src/ic.cc:183: // or a global object. IC::GetCodeCacheMap is not
appliable.
On 2010/06/28 11:40:08, Mads Ager wrote:
appliable -> applicable
Done.
http://codereview.chromium.org/2801018/diff/39001/9008#newcode187
src/ic.cc:187: // IC::GetCodeCacheMap is not appliable.
On 2010/06/28 11:40:08, Mads Ager wrote:
appliable -> applicable
Done.
http://codereview.chromium.org/2801018/diff/39001/9008#newcode645
src/ic.cc:645: // Cache code holding map should be consistent with
On 2010/06/28 11:40:08, Mads Ager wrote:
I don't understand the need for this change. You only need to change
how this
works for the new type of stub. That new type of stub has special code
flags so
you can figure out which map to use.
It is not the same maps. This map is used to calculate a hash code for
the stub cache table. Here show properties objects use their own map (in
sprite such a stub is stored in prototype's map) to avoid handling
special case in GenerateMonomorphicCacheProbe.
http://codereview.chromium.org/2801018/diff/39001/9009
File src/ic.h (right):
http://codereview.chromium.org/2801018/diff/39001/9009#newcode120
src/ic.h:120: // Determines which map must be used for keeping the code
stub.
On 2010/06/28 11:40:08, Mads Ager wrote:
I don't think this is the right place to determine. I think we should
leave
GetCodeCacheMapForObject intact and split this in the IC LoadFunction
code
instead.
I didn't get it. I see no sense putting this code into
CallICBase::LoadFunction. It could be placed into
CallICBase::UpdateCaches and the result could be passed as an additional
parameter into ComputeCall*. Did you mean that?
http://codereview.chromium.org/2801018/diff/39001/9010
File src/objects-inl.h (right):
http://codereview.chromium.org/2801018/diff/39001/9010#newcode2274
src/objects-inl.h:2274: if (holder == PROTOTYPE_MAP) bits |=
kFlagsICHolderMask;
On 2010/06/28 11:40:08, Mads Ager wrote:
kFlagsCacheInPrototypeMapMask?
Done.
http://codereview.chromium.org/2801018/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
