LGTM
We should deal with the double keys differently, but feel free to do that
in a
separate changelist.
http://codereview.chromium.org/7473031/diff/3006/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/7473031/diff/3006/src/objects.cc#newcode4810
src/objects.cc:4810: MaybeObject* maybe_obj =
heap->NumberFromDouble(other->get(y));
This is nasty. This can lead to out-of-memory crashes when we are in
fact not out of memory if the double array is large enough. That is
probably very unlikely to happen, but still. I wonder if we can restrict
the API so that we can never get doubles in key arrays (require integers
and strings only)? Alternatively, we should be able to create the key
integer/string directly from the double without going through a heap
number allocation.
Feel free to deal with this in a separate changelist.
http://codereview.chromium.org/7473031/diff/3006/src/objects.cc#newcode4823
src/objects.cc:4823: AssertNoAllocation no_gc;
Maybe we should change the name to AssertNoGC at some point. There will
be lots of allocation below. Maybe limit the scope of the
AssertNoAllocation? This is to make sure that the write barrier mode
stays valid, right?
http://codereview.chromium.org/7473031/diff/3006/src/objects.cc#newcode4835
src/objects.cc:4835: MaybeObject* maybe_obj =
heap->NumberFromDouble(other->get(y));
Here again. So we actually allocate two new heap numbers per entry. We
should create the integer/string key directly from the double.
http://codereview.chromium.org/7473031/diff/3006/test/mjsunit/unbox-double-arrays.js
File test/mjsunit/unbox-double-arrays.js (right):
http://codereview.chromium.org/7473031/diff/3006/test/mjsunit/unbox-double-arrays.js#newcode473
test/mjsunit/unbox-double-arrays.js:473: for (x in names) {
property_name_count++};
++} -> ++; }
http://codereview.chromium.org/7473031/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
