I'm not convinced this is a good idea.
It seems like it would be a fairly big overhead for cache misses, and at the
same time there is a lot of error checking missing from the runtime
functions
(possibly only from GenerateGetFromCache, if the one in runtime.cc is only
called through the generated code and the arguments have been checked
there).
The FixedArray should be boxed, probably as the elements array of a
JSArray, and
arguments should be checked.
Also, I think some of the measured speed improvements are spurious, but I
need
to check that.
http://codereview.chromium.org/1563005/diff/12001/13004
File src/heap.cc (right):
http://codereview.chromium.org/1563005/diff/12001/13004#newcode3074
src/heap.cc:3074:
reinterpret_cast<Array*>(result)->set_map(Heap::fixed_array_map());
Perhaps cast to HeapObject, so one doesn't start trying to figure out
why you are using Array :)
http://codereview.chromium.org/1563005/diff/12001/13004#newcode3077
src/heap.cc:3077: ASSERT(!Heap::InNewSpace(filler));
Assert this before doing the raw allocation (just for clarity, so I
won't have to worry what happens if the ASSERT fails and the array
contains garbage.
http://codereview.chromium.org/1563005/diff/12001/13006
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/1563005/diff/12001/13006#newcode6555
src/ia32/codegen-ia32.cc:6555: // tmp.reg() now holds finger offset as a
smi.
Cache.reg is supposed to hold a FixedArray. You should test it before
using the value. You shouldn't trust the parameters of runtime
functions.
Also, document the expected input somewhere, e.g.:
"A JSArray with fast elements FixedArray with the following layout ..."
(or refer to where it's already written).
http://codereview.chromium.org/1563005/diff/12001/13006#newcode6557
src/ia32/codegen-ia32.cc:6557: __ cmp(key.reg(),
FieldOperand(cache.reg(),
Assert that smi-encoding multiplies by two (i.e., tag == 0 and tagSize
== 1), and that tmp holds a Smi.
http://codereview.chromium.org/1563005/diff/12001/13008
File src/runtime.cc (right):
http://codereview.chromium.org/1563005/diff/12001/13008#newcode9995
src/runtime.cc:9995: return *cache;
You are leaking raw FixedArray's into JavaScript code.
We haven't done that anywhere else. What we usually do instead is to
have a JSArray with the FixedArray that we want as fast elements.
Ofcourse these cache objects are supposed to be completely opaque to the
JavaScript code and never be visible to the user (although it's not
obvious that a malicious user can't leak it somehow), but I'm still not
comfortable with it.
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10008
src/runtime.cc:10008: Handle<JSFunction>
factory(JSFunction::cast(cache->get(kFactoryIndex)));
Needs error checking as well, since we haven't checked that this
FixedArray element is a function during this runtime call.
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10015
src/runtime.cc:10015: receiver,
So the factory function is the function being cached?
Am I correct that this moves all calls to that function into C++ code,
which then calls back to JavaSscript? It seems like a big detour for a
cache-miss.
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10018
src/runtime.cc:10018: &pending_exception);
Are we sure that this call can't change the value at key.location()?
(I.e., I'm not, so if you are, please write a comment about it).
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10032
src/runtime.cc:10032: CONVERT_CHECKED(FixedArray, cache, args[0]);
Check the size of the Array as well (must be at least kEntriesIndex or
possibly more).
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10063
src/runtime.cc:10063: }
I'm not sure it's worth the complexity to have two loops instead of just
checking o==key one extra time. Have you checked the impact of just
having one loop?
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10071
src/runtime.cc:10071: int target_index = (finger_index > kEntriesIndex)
? finger_index - 2
We fill up from left to right, and then evict right-to-left.
Seems more consistent to evict the element after the finger instead.
http://codereview.chromium.org/1563005/diff/12001/13011
File test/mjsunit/fuzz-natives.js (right):
http://codereview.chromium.org/1563005/diff/12001/13011#newcode165
test/mjsunit/fuzz-natives.js:165: "_GetFromCache": true,
That's not a sufficient reason not to check the arguments.
That only goes if you are certain that what it does is always safe
(i.e., you can control its arguments completely).
http://codereview.chromium.org/1563005/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
To unsubscribe, reply using "remove me" as the subject.
