From 40a9b4363ef547745ba2c57d6a4c622b023e72d3 Mon Sep 17 00:00:00 2001 From: "ishell@chromium.org" Date: Mon, 29 Sep 2014 08:22:24 +0000 Subject: [PATCH] ArrayConcat regression recover after r20312 (appeared on dromaeo benchmarks). BUG=chromium:358561 LOG=N R=yangguo@chromium.org Review URL: https://codereview.chromium.org/597103003 git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@24269 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/builtins.cc | 6 +-- src/elements.cc | 126 +++++++++++++++++++++++++++--------------------- src/elements.h | 7 +-- 3 files changed, 77 insertions(+), 62 deletions(-) diff --git a/src/builtins.cc b/src/builtins.cc index f9216403c8..c52d22852d 100644 --- a/src/builtins.cc +++ b/src/builtins.cc @@ -987,12 +987,12 @@ BUILTIN(ArrayConcat) { Handle storage(result_array->elements(), isolate); ElementsAccessor* accessor = ElementsAccessor::ForKind(elements_kind); for (int i = 0; i < n_arguments; i++) { - // TODO(ishell): It is crucial to keep |array| as a raw pointer to avoid - // performance degradation. Revisit this later. + // It is crucial to keep |array| in a raw pointer form to avoid performance + // degradation. JSArray* array = JSArray::cast(args[i]); int len = Smi::cast(array->length())->value(); - ElementsKind from_kind = array->GetElementsKind(); if (len > 0) { + ElementsKind from_kind = array->GetElementsKind(); accessor->CopyElements(array, 0, from_kind, storage, j, len); j += len; } diff --git a/src/elements.cc b/src/elements.cc index e2127c4638..abb046725c 100644 --- a/src/elements.cc +++ b/src/elements.cc @@ -247,15 +247,18 @@ static void CopyDictionaryToObjectElements( } -static void CopyDoubleToObjectElements(Handle from_base, +// NOTE: this method violates the handlified function signature convention: +// raw pointer parameters in the function that allocates. +// See ElementsAccessorBase::CopyElements() for details. +static void CopyDoubleToObjectElements(FixedArrayBase* from_base, uint32_t from_start, - Handle to_base, - ElementsKind to_kind, - uint32_t to_start, + FixedArrayBase* to_base, + ElementsKind to_kind, uint32_t to_start, int raw_copy_size) { DCHECK(IsFastSmiOrObjectElementsKind(to_kind)); int copy_size = raw_copy_size; if (raw_copy_size < 0) { + DisallowHeapAllocation no_allocation; DCHECK(raw_copy_size == ElementsAccessor::kCopyToEnd || raw_copy_size == ElementsAccessor::kCopyToEndAndInitializeToHole); copy_size = Min(from_base->length() - from_start, @@ -268,7 +271,7 @@ static void CopyDoubleToObjectElements(Handle from_base, int length = to_base->length() - start; if (length > 0) { Heap* heap = from_base->GetHeap(); - MemsetPointer(FixedArray::cast(*to_base)->data_start() + start, + MemsetPointer(FixedArray::cast(to_base)->data_start() + start, heap->the_hole_value(), length); } } @@ -276,9 +279,12 @@ static void CopyDoubleToObjectElements(Handle from_base, DCHECK((copy_size + static_cast(to_start)) <= to_base->length() && (copy_size + static_cast(from_start)) <= from_base->length()); if (copy_size == 0) return; + + // From here on, the code below could actually allocate. Therefore the raw + // values are wrapped into handles. Isolate* isolate = from_base->GetIsolate(); - Handle from = Handle::cast(from_base); - Handle to = Handle::cast(to_base); + Handle from(FixedDoubleArray::cast(from_base), isolate); + Handle to(FixedArray::cast(to_base), isolate); for (int i = 0; i < copy_size; ++i) { HandleScope scope(isolate); if (IsFastSmiElementsKind(to_kind)) { @@ -702,12 +708,9 @@ class ElementsAccessorBase : public ElementsAccessor { uint32_t key, JSReceiver::DeleteMode mode) OVERRIDE = 0; - static void CopyElementsImpl(Handle from, - uint32_t from_start, - Handle to, - ElementsKind from_kind, - uint32_t to_start, - int packed_size, + static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start, + FixedArrayBase* to, ElementsKind from_kind, + uint32_t to_start, int packed_size, int copy_size) { UNREACHABLE(); } @@ -720,9 +723,15 @@ class ElementsAccessorBase : public ElementsAccessor { uint32_t to_start, int copy_size) FINAL OVERRIDE { DCHECK(!from.is_null()); - ElementsAccessorSubclass::CopyElementsImpl( - from, from_start, to, from_kind, to_start, kPackedSizeNotKnown, - copy_size); + // NOTE: the ElementsAccessorSubclass::CopyElementsImpl() methods + // violate the handlified function signature convention: + // raw pointer parameters in the function that allocates. This is done + // intentionally to avoid ArrayConcat() builtin performance degradation. + // See the comment in another ElementsAccessorBase::CopyElements() for + // details. + ElementsAccessorSubclass::CopyElementsImpl(*from, from_start, *to, + from_kind, to_start, + kPackedSizeNotKnown, copy_size); } virtual void CopyElements( @@ -742,9 +751,18 @@ class ElementsAccessorBase : public ElementsAccessor { packed_size = copy_size; } } - Handle from(from_holder->elements()); + FixedArrayBase* from = from_holder->elements(); + // NOTE: the ElementsAccessorSubclass::CopyElementsImpl() methods + // violate the handlified function signature convention: + // raw pointer parameters in the function that allocates. This is done + // intentionally to avoid ArrayConcat() builtin performance degradation. + // + // Details: The idea is that allocations actually happen only in case of + // copying from object with fast double elements to object with object + // elements. In all the other cases there are no allocations performed and + // handle creation causes noticeable performance degradation of the builtin. ElementsAccessorSubclass::CopyElementsImpl( - from, from_start, to, from_kind, to_start, packed_size, copy_size); + from, from_start, *to, from_kind, to_start, packed_size, copy_size); } virtual MaybeHandle AddElementsToFixedArray( @@ -1018,7 +1036,7 @@ class FastElementsAccessor }; -static inline ElementsKind ElementsKindForArray(Handle array) { +static inline ElementsKind ElementsKindForArray(FixedArrayBase* array) { switch (array->map()->instance_type()) { case FIXED_ARRAY_TYPE: if (array->IsDictionary()) { @@ -1054,38 +1072,42 @@ class FastSmiOrObjectElementsAccessor : FastElementsAccessor(name) {} - static void CopyElementsImpl(Handle from, - uint32_t from_start, - Handle to, - ElementsKind from_kind, - uint32_t to_start, - int packed_size, + // NOTE: this method violates the handlified function signature convention: + // raw pointer parameters in the function that allocates. + // See ElementsAccessor::CopyElements() for details. + // This method could actually allocate if copying from double elements to + // object elements. + static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start, + FixedArrayBase* to, ElementsKind from_kind, + uint32_t to_start, int packed_size, int copy_size) { + DisallowHeapAllocation no_gc; ElementsKind to_kind = KindTraits::Kind; switch (from_kind) { case FAST_SMI_ELEMENTS: case FAST_HOLEY_SMI_ELEMENTS: case FAST_ELEMENTS: case FAST_HOLEY_ELEMENTS: - CopyObjectToObjectElements(*from, from_kind, from_start, *to, to_kind, + CopyObjectToObjectElements(from, from_kind, from_start, to, to_kind, to_start, copy_size); break; case FAST_DOUBLE_ELEMENTS: - case FAST_HOLEY_DOUBLE_ELEMENTS: + case FAST_HOLEY_DOUBLE_ELEMENTS: { + AllowHeapAllocation allow_allocation; CopyDoubleToObjectElements( from, from_start, to, to_kind, to_start, copy_size); break; + } case DICTIONARY_ELEMENTS: - CopyDictionaryToObjectElements(*from, from_start, *to, to_kind, - to_start, copy_size); + CopyDictionaryToObjectElements(from, from_start, to, to_kind, to_start, + copy_size); break; case SLOPPY_ARGUMENTS_ELEMENTS: { // TODO(verwaest): This is a temporary hack to support extending // SLOPPY_ARGUMENTS_ELEMENTS in SetFastElementsCapacityAndLength. // This case should be UNREACHABLE(). - Handle parameter_map = Handle::cast(from); - Handle arguments( - FixedArrayBase::cast(parameter_map->get(1))); + FixedArray* parameter_map = FixedArray::cast(from); + FixedArrayBase* arguments = FixedArrayBase::cast(parameter_map->get(1)); ElementsKind from_kind = ElementsKindForArray(arguments); CopyElementsImpl(arguments, from_start, to, from_kind, to_start, packed_size, copy_size); @@ -1179,31 +1201,29 @@ class FastDoubleElementsAccessor } protected: - static void CopyElementsImpl(Handle from, - uint32_t from_start, - Handle to, - ElementsKind from_kind, - uint32_t to_start, - int packed_size, + static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start, + FixedArrayBase* to, ElementsKind from_kind, + uint32_t to_start, int packed_size, int copy_size) { + DisallowHeapAllocation no_allocation; switch (from_kind) { case FAST_SMI_ELEMENTS: - CopyPackedSmiToDoubleElements(*from, from_start, *to, to_start, + CopyPackedSmiToDoubleElements(from, from_start, to, to_start, packed_size, copy_size); break; case FAST_HOLEY_SMI_ELEMENTS: - CopySmiToDoubleElements(*from, from_start, *to, to_start, copy_size); + CopySmiToDoubleElements(from, from_start, to, to_start, copy_size); break; case FAST_DOUBLE_ELEMENTS: case FAST_HOLEY_DOUBLE_ELEMENTS: - CopyDoubleToDoubleElements(*from, from_start, *to, to_start, copy_size); + CopyDoubleToDoubleElements(from, from_start, to, to_start, copy_size); break; case FAST_ELEMENTS: case FAST_HOLEY_ELEMENTS: - CopyObjectToDoubleElements(*from, from_start, *to, to_start, copy_size); + CopyObjectToDoubleElements(from, from_start, to, to_start, copy_size); break; case DICTIONARY_ELEMENTS: - CopyDictionaryToDoubleElements(*from, from_start, *to, to_start, + CopyDictionaryToDoubleElements(from, from_start, to, to_start, copy_size); break; case SLOPPY_ARGUMENTS_ELEMENTS: @@ -1439,12 +1459,9 @@ class DictionaryElementsAccessor return isolate->factory()->true_value(); } - static void CopyElementsImpl(Handle from, - uint32_t from_start, - Handle to, - ElementsKind from_kind, - uint32_t to_start, - int packed_size, + static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start, + FixedArrayBase* to, ElementsKind from_kind, + uint32_t to_start, int packed_size, int copy_size) { UNREACHABLE(); } @@ -1654,12 +1671,9 @@ class SloppyArgumentsElementsAccessor : public ElementsAccessorBase< return isolate->factory()->true_value(); } - static void CopyElementsImpl(Handle from, - uint32_t from_start, - Handle to, - ElementsKind from_kind, - uint32_t to_start, - int packed_size, + static void CopyElementsImpl(FixedArrayBase* from, uint32_t from_start, + FixedArrayBase* to, ElementsKind from_kind, + uint32_t to_start, int packed_size, int copy_size) { UNREACHABLE(); } @@ -1715,7 +1729,7 @@ class SloppyArgumentsElementsAccessor : public ElementsAccessorBase< ElementsAccessor* ElementsAccessor::ForArray(Handle array) { - return elements_accessors_[ElementsKindForArray(array)]; + return elements_accessors_[ElementsKindForArray(*array)]; } diff --git a/src/elements.h b/src/elements.h index d0bddf982d..f4de4bb010 100644 --- a/src/elements.h +++ b/src/elements.h @@ -146,9 +146,10 @@ class ElementsAccessor { uint32_t destination_start, int copy_size) = 0; - // TODO(ishell): Keeping |source_holder| parameter in a non-handlified form - // helps avoiding ArrayConcat() builtin performance degradation. - // Revisit this later. + // NOTE: this method violates the handlified function signature convention: + // raw pointer parameter |source_holder| in the function that allocates. + // This is done intentionally to avoid ArrayConcat() builtin performance + // degradation. virtual void CopyElements( JSObject* source_holder, uint32_t source_start,