All issues addressed. Ported to x64 and arm. Also fixed a bug in generic
keyed
load (broke the internal "elements in ecx" assumption).
Please take another look.
Thanks,
Vitaly
http://codereview.chromium.org/3144002/diff/1/2
File src/builtins.cc (right):
http://codereview.chromium.org/3144002/diff/1/2#newcode355
src/builtins.cc:355: if (elms->map() == Heap::fixed_array_map()) return
FixedArray::cast(elms);
On 2010/08/11 11:28:22, antonm wrote:
I don't think you need FixedArray::cast anymore.
Done.
http://codereview.chromium.org/3144002/diff/1/2#newcode357
src/builtins.cc:357: return array->EnsureWritableFastElements();
On 2010/08/11 12:39:38, antonm wrote:
just as an idea for speed up: maybe we could add utility function to
turn
elements into writables with api like:
JSArray::TurnToWritables(FixedArray*
elements) which bypasses map check and elements lookup. But I don't
know how
much will it buy us.
Given the conversion cost I think the benefit is negligible.
Overall, do you know how many COW arrays come into builtins in Dromaeo
array
benchmark?
Very very few.
http://codereview.chromium.org/3144002/diff/1/2#newcode363
src/builtins.cc:363: static bool IsJSArrayElementMovingAllowed(JSArray*
receiver) {
On 2010/08/11 11:28:22, antonm wrote:
you might want to keep Fast in the name---this check allows to use
memmove/memcopy to move elements around.
Done.
http://codereview.chromium.org/3144002/diff/1/4
File src/heap.cc (right):
http://codereview.chromium.org/3144002/diff/1/4#newcode1533
src/heap.cc:1533: obj = AllocateMap(FIXED_ARRAY_TYPE,
FixedArray::kHeaderSize);
On 2010/08/11 11:28:22, antonm wrote:
maybe move to allocation of fixed_array_map?
fixed_array_map is allocated early in a special way. COW map wont' fit
there.
http://codereview.chromium.org/3144002/diff/1/6
File src/ia32/codegen-ia32.cc (right):
http://codereview.chromium.org/3144002/diff/1/6#newcode9484
src/ia32/codegen-ia32.cc:9484: // dictionary.
On 2010/08/11 11:28:22, antonm wrote:
this comment probably needs an update. Overall, is there a way to
check other
places?
Done.
http://codereview.chromium.org/3144002/diff/1/8
File src/ia32/ic-ia32.cc (right):
http://codereview.chromium.org/3144002/diff/1/8#newcode454
src/ia32/ic-ia32.cc:454: // Loads an indexed element from a fast case
array.
On 2010/08/11 10:42:31, Mads Ager wrote:
Maybe update the comment (either here or in the header file) with
information
about the use of the not_fast_array label?
Done.
http://codereview.chromium.org/3144002/diff/1/8#newcode568
src/ia32/ic-ia32.cc:568: 1 << Map::kHasFastElements);
On 2010/08/11 10:42:31, Mads Ager wrote:
Should we update the name of the bit to HasFastElementsForReading to
reflect the
new meaning?
Maybe that is not needed with the new explanation to the general
HasFastElements
method, but it might clearify the code generators to be explicit about
it in the
name of the bit.
Yeah, I'd like to avoid renaming the bit. The bit is new, very little
code relied on it, so I think it's okay to give it a new meaning in the
new explanation.
http://codereview.chromium.org/3144002/diff/1/8#newcode980
src/ia32/ic-ia32.cc:980: // Check that the object is in fast mode (not
dictionary).
On 2010/08/11 11:28:22, antonm wrote:
again fast writable mode?
Done.
http://codereview.chromium.org/3144002/diff/1/8#newcode1036
src/ia32/ic-ia32.cc:1036: // array. Check that the array is in fast
mode; if it is the
On 2010/08/11 11:28:22, antonm wrote:
again fast writable mode? or we should just make COW an exception?
Done.
http://codereview.chromium.org/3144002/diff/1/8#newcode1886
src/ia32/ic-ia32.cc:1886: __ CmpObjectType(scratch, FIXED_ARRAY_TYPE,
scratch);
On 2010/08/11 11:28:22, antonm wrote:
just to double check: that should be fine as StoreIC would take care
of COWs,
correct?
Good question. StoreIC takes care of COWs, but this is subtle and needs
a comment.
http://codereview.chromium.org/3144002/diff/1/11
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/3144002/diff/1/11#newcode1398
src/ia32/stub-cache-ia32.cc:1398: // Check that the elements are in fast
mode (not dictionary).
On 2010/08/11 11:28:22, antonm wrote:
you may want to update the comment to mention fast writable mode.
Done.
http://codereview.chromium.org/3144002/diff/1/13
File src/objects-inl.h (right):
http://codereview.chromium.org/3144002/diff/1/13#newcode1401
src/objects-inl.h:1401: ASSERT(map_word().ToMap() !=
Heap::raw_unchecked_fixed_cow_array_map());
On 2010/08/11 10:42:31, Mads Ager wrote:
Just use 'map()' in all of these asserts?
Are these used during GC so that you need to use
raw_unchecked_fixed_cow_array_map or could you just use
fixed_cow_array_map()?
Changed map_word().ToMap() to map(). Some of these are used during GC
(our descriptor dance) so I used "unchecked" in all of them for
consistency.
http://codereview.chromium.org/3144002/diff/1/14
File src/objects.cc (right):
http://codereview.chromium.org/3144002/diff/1/14#newcode5670
src/objects.cc:5670: Object* obj =
SetFastElementsCapacityAndLength(new_capacity, value);
On 2010/08/11 11:28:22, antonm wrote:
that would apparently keep COWness of original array, was it intended?
As discussed offline this is not an issue. We don't attempt mutation of
COW arrays here.
http://codereview.chromium.org/3144002/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
