Lasse, thanks a lot for comments.
Let me start with the principal question. My current plan is to move all
the
logic into the codegen eventually. I've finished calling of JS functions
from
codegen in another CL, so the rest of the code could be easily ported into
deferred code. Thus addressing performance concern of calling JS from C++.
Now to fixed array issue. To save us from property lookup I plan to index
all
the caches with smis and turn %_GetFromCache into %_GetFromCache(cache
index,
key). I'd probably require cache index to be compile time number to save us
some more checks. The effect would be that fixed arrays are not exposed
into
JS.
Up to you how much of this stuff you'd prefer to see in this CL vs. series
of
smaller CLs.
And yes, I omit all the checks I could for exactly the reason of
performance.
You should be in better position to see if it has any security implications.
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());
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
Perhaps cast to HeapObject, so one doesn't start trying to figure out
why you
are using Array :)
Done.
http://codereview.chromium.org/1563005/diff/12001/13004#newcode3077
src/heap.cc:3077: ASSERT(!Heap::InNewSpace(filler));
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
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.
Done.
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.
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
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).
I intentionally omit any checks. It's performance critical code and
which is (apparently) only controlled by us, so I'd rather keep the
things as they are now.
http://codereview.chromium.org/1563005/diff/12001/13006#newcode6557
src/ia32/codegen-ia32.cc:6557: __ cmp(key.reg(),
FieldOperand(cache.reg(),
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
Assert that smi-encoding multiplies by two (i.e., tag == 0 and tagSize
== 1),
and that tmp holds a Smi.
Again check for tmp being smi omitted, but assert added.
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;
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
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.
That's the crucial idea of the patch---I'd like to have as few levels of
indirection as possible. Up to you to decide if it's a good or a bad
idea.
And please notice, that my next planned move is to get rid of those
references altogether---instead caches would be indexed by smis (to
bypass property lookup).
The code would look:
%NewCache(0 /* cache id */, 16 /* size */, factory);
%_GetFromCache(0, key);
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10008
src/runtime.cc:10008: Handle<JSFunction>
factory(JSFunction::cast(cache->get(kFactoryIndex)));
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
Needs error checking as well, since we haven't checked that this
FixedArray
element is a function during this runtime call.
But we checked that on creation of the cache, no?
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10015
src/runtime.cc:10015: receiver,
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
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.
That's true. I plan to move all this code into the codegen eventually,
but rather do it in small steps. If you'd prefer to see it in one go,
just let me know.
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10018
src/runtime.cc:10018: &pending_exception);
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
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).
It can if it triggers GC, but this arhv object is not used after the
call. If you like, I can put it into the block.
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10032
src/runtime.cc:10032: CONVERT_CHECKED(FixedArray, cache, args[0]);
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
Check the size of the Array as well (must be at least kEntriesIndex or
possibly
more).
In debug mode? Or for both builds?
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10063
src/runtime.cc:10063: }
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
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?
I'm probably missing something. I need to loop from finger + 1 up to
the end of the cache and then from start of cache up to finger. I could
merge those two loops doing the check inside, but it looks somewhat more
cumbersome to me.
Or did I misinterpret you completely?
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10071
src/runtime.cc:10071: int target_index = (finger_index > kEntriesIndex)
? finger_index - 2
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
We fill up from left to right, and then evict right-to-left.
Seems more consistent to evict the element after the finger instead.
I might be missing something, but I hoped to implement LRU eviction
policy for the case when keys rotate in a loop which is bigger than
cache.
But I am not insisting.
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,
On 2010/04/08 17:04:28, Lasse Reichstein wrote:
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).
Which I believe to be the case as it should only be used in the library.
What am I missing?
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.
