Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(483)

Issues Search
    My Issues | Starred     Open | Closed | All

Unified Diff: src/builtins.cc

Issue 223413002: Partial recover from performance degradation after handlification of ElementsAccessor::CopyElements… (Closed) Base URL: https://v8.googlecode.com/svn/branches/bleeding_edge
Patch Set: Created 6 years, 9 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | src/elements.h » ('j') | src/factory.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: src/builtins.cc
diff --git a/src/builtins.cc b/src/builtins.cc
index 200f7fed632619b30f0011dc8bbbac118c34196d..5db6f259b77bbda1672703a143895c93cbfc3610 100644
--- a/src/builtins.cc
+++ b/src/builtins.cc
@@ -302,7 +302,7 @@ static inline Handle<FixedArrayBase> EnsureJSArrayWithWritableFastElements(
Handle<JSArray> array = Handle<JSArray>::cast(receiver);
if (array->map()->is_observed()) return Handle<FixedArrayBase>::null();
if (!array->map()->is_extensible()) return Handle<FixedArrayBase>::null();
- Handle<FixedArrayBase> elms(array->elements());
+ Handle<FixedArrayBase> elms(array->elements(), isolate);
Heap* heap = isolate->heap();
Map* map = elms->map();
if (map == heap->fixed_array_map()) {
@@ -319,7 +319,7 @@ static inline Handle<FixedArrayBase> EnsureJSArrayWithWritableFastElements(
// Need to ensure that the arguments passed in args can be contained in
// the array.
int args_length = args->length();
- if (first_added_arg >= args_length) return handle(array->elements());
+ if (first_added_arg >= args_length) return handle(array->elements(), isolate);
ElementsKind origin_kind = array->map()->elements_kind();
ASSERT(!IsFastObjectElementsKind(origin_kind));
@@ -339,7 +339,7 @@ static inline Handle<FixedArrayBase> EnsureJSArrayWithWritableFastElements(
}
if (target_kind != origin_kind) {
JSObject::TransitionElementsKind(array, target_kind);
- return handle(array->elements());
+ return handle(array->elements(), isolate);
}
return elms;
}
@@ -361,6 +361,7 @@ MUST_USE_RESULT static MaybeObject* CallJsBuiltin(
Isolate* isolate,
const char* name,
BuiltinArguments<NO_EXTRA_ARGUMENTS> args) {
+ AllowHeapAllocation allow_allocation;
Yang 2014/04/03 08:10:43 Add a comment that the callees return immediately
Igor Sheludko 2014/04/03 09:01:00 As discussed offline, it is safer to put explicit
HandleScope handleScope(isolate);
Handle<Object> js_builtin =
@@ -418,8 +419,8 @@ BUILTIN(ArrayPush) {
ElementsAccessor* accessor = array->GetElementsAccessor();
accessor->CopyElements(
- Handle<JSObject>::null(), 0, kind, new_elms, 0,
- ElementsAccessor::kCopyToEndAndInitializeToHole, elms_obj);
+ elms_obj, 0, kind, new_elms, 0,
+ ElementsAccessor::kCopyToEndAndInitializeToHole);
elms = new_elms;
}
@@ -461,8 +462,8 @@ BUILTIN(ArrayPush) {
ElementsAccessor* accessor = array->GetElementsAccessor();
accessor->CopyElements(
- Handle<JSObject>::null(), 0, kind, new_elms, 0,
- ElementsAccessor::kCopyToEndAndInitializeToHole, elms_obj);
+ elms_obj, 0, kind, new_elms, 0,
+ ElementsAccessor::kCopyToEndAndInitializeToHole);
} else {
// to_add is > 0 and new_length <= elms_len, so elms_obj cannot be the
@@ -489,18 +490,6 @@ BUILTIN(ArrayPush) {
}
-// TODO(ishell): Temporary wrapper until handlified.
-static bool ElementsAccessorHasElementWrapper(
- ElementsAccessor* accessor,
- Handle<Object> receiver,
- Handle<JSObject> holder,
- uint32_t key,
- Handle<FixedArrayBase> backing_store = Handle<FixedArrayBase>::null()) {
- return accessor->HasElement(*receiver, *holder, key,
- backing_store.is_null() ? NULL : *backing_store);
-}
-
-
BUILTIN(ArrayPop) {
HandleScope scope(isolate);
Handle<Object> receiver = args.receiver();
@@ -517,8 +506,7 @@ BUILTIN(ArrayPop) {
ElementsAccessor* accessor = array->GetElementsAccessor();
int new_length = len - 1;
Handle<Object> element;
- if (ElementsAccessorHasElementWrapper(
- accessor, array, array, new_length, elms_obj)) {
+ if (accessor->HasElement(*array, *array, new_length, *elms_obj)) {
element = accessor->Get(
array, array, new_length, elms_obj);
} else {
@@ -618,8 +606,8 @@ BUILTIN(ArrayUnshift) {
ElementsKind kind = array->GetElementsKind();
ElementsAccessor* accessor = array->GetElementsAccessor();
accessor->CopyElements(
- Handle<JSObject>::null(), 0, kind, new_elms, to_add,
- ElementsAccessor::kCopyToEndAndInitializeToHole, elms);
+ elms, 0, kind, new_elms, to_add,
+ ElementsAccessor::kCopyToEndAndInitializeToHole);
elms = new_elms;
array->set_elements(*elms);
@@ -645,17 +633,16 @@ BUILTIN(ArraySlice) {
HandleScope scope(isolate);
Heap* heap = isolate->heap();
Handle<Object> receiver = args.receiver();
- Handle<FixedArrayBase> elms;
int len = -1;
+ DisallowHeapAllocation no_allocations_before_creating_result;
Yang 2014/04/03 08:10:43 I would prefer if you still use scopes instead of
Igor Sheludko 2014/04/03 09:01:00 Done.
+
if (receiver->IsJSArray()) {
- Handle<JSArray> array = Handle<JSArray>::cast(receiver);
- if (!IsJSArrayFastElementMovingAllowed(heap, *array)) {
+ JSArray* array = JSArray::cast(*receiver);
+ if (!IsJSArrayFastElementMovingAllowed(heap, array)) {
return CallJsBuiltin(isolate, "ArraySlice", args);
}
- if (array->HasFastElements()) {
- elms = handle(array->elements());
- } else {
+ if (!array->HasFastElements()) {
return CallJsBuiltin(isolate, "ArraySlice", args);
}
@@ -663,35 +650,31 @@ BUILTIN(ArraySlice) {
} else {
// Array.slice(arguments, ...) is quite a common idiom (notably more
// than 50% of invocations in Web apps). Treat it in C++ as well.
- Handle<Map> arguments_map(isolate->context()->native_context()->
- sloppy_arguments_boilerplate()->map());
+ Map* arguments_map = isolate->context()->native_context()->
+ sloppy_arguments_boilerplate()->map();
bool is_arguments_object_with_fast_elements =
receiver->IsJSObject() &&
- Handle<JSObject>::cast(receiver)->map() == *arguments_map;
+ JSObject::cast(*receiver)->map() == arguments_map;
if (!is_arguments_object_with_fast_elements) {
return CallJsBuiltin(isolate, "ArraySlice", args);
}
- Handle<JSObject> object = Handle<JSObject>::cast(receiver);
+ JSObject* object = JSObject::cast(*receiver);
- if (object->HasFastElements()) {
- elms = handle(object->elements());
- } else {
+ if (!object->HasFastElements()) {
return CallJsBuiltin(isolate, "ArraySlice", args);
}
- Handle<Object> len_obj(
- object->InObjectPropertyAt(Heap::kArgumentsLengthIndex), isolate);
+
+ Object* len_obj = object->InObjectPropertyAt(Heap::kArgumentsLengthIndex);
if (!len_obj->IsSmi()) {
return CallJsBuiltin(isolate, "ArraySlice", args);
}
- len = Handle<Smi>::cast(len_obj)->value();
- if (len > elms->length()) {
+ len = Smi::cast(len_obj)->value();
+ if (len > object->elements()->length()) {
return CallJsBuiltin(isolate, "ArraySlice", args);
}
}
- Handle<JSObject> object = Handle<JSObject>::cast(receiver);
-
ASSERT(len >= 0);
int n_arguments = args.length() - 1;
@@ -701,11 +684,11 @@ BUILTIN(ArraySlice) {
int relative_start = 0;
int relative_end = len;
if (n_arguments > 0) {
- Handle<Object> arg1 = args.at<Object>(1);
+ Object* arg1 = args[1];
if (arg1->IsSmi()) {
- relative_start = Handle<Smi>::cast(arg1)->value();
+ relative_start = Smi::cast(arg1)->value();
} else if (arg1->IsHeapNumber()) {
- double start = Handle<HeapNumber>::cast(arg1)->value();
+ double start = HeapNumber::cast(arg1)->value();
if (start < kMinInt || start > kMaxInt) {
return CallJsBuiltin(isolate, "ArraySlice", args);
}
@@ -714,11 +697,11 @@ BUILTIN(ArraySlice) {
return CallJsBuiltin(isolate, "ArraySlice", args);
}
if (n_arguments > 1) {
- Handle<Object> arg2 = args.at<Object>(2);
+ Object* arg2 = args[2];
if (arg2->IsSmi()) {
- relative_end = Handle<Smi>::cast(arg2)->value();
+ relative_end = Smi::cast(arg2)->value();
} else if (arg2->IsHeapNumber()) {
- double end = Handle<HeapNumber>::cast(arg2)->value();
+ double end = HeapNumber::cast(arg2)->value();
if (end < kMinInt || end > kMaxInt) {
return CallJsBuiltin(isolate, "ArraySlice", args);
}
@@ -740,13 +723,15 @@ BUILTIN(ArraySlice) {
// Calculate the length of result array.
int result_len = Max(final - k, 0);
+ Handle<JSObject> object = Handle<JSObject>::cast(receiver);
+ Handle<FixedArrayBase> elms(object->elements(), isolate);
+
ElementsKind kind = object->GetElementsKind();
if (IsHoleyElementsKind(kind)) {
Yang 2014/04/03 08:10:43 Then add a DisallowHeapAllocation scope here.
Igor Sheludko 2014/04/03 09:01:00 Done.
bool packed = true;
ElementsAccessor* accessor = ElementsAccessor::ForKind(kind);
for (int i = k; i < final; i++) {
- if (!ElementsAccessorHasElementWrapper(
- accessor, object, object, i, elms)) {
+ if (!accessor->HasElement(*object, *object, i, *elms)) {
packed = false;
break;
}
@@ -758,15 +743,16 @@ BUILTIN(ArraySlice) {
}
}
+ AllowHeapAllocation allow_allocation_to_create_a_result;
Yang 2014/04/03 08:10:43 This is now unnecessary.
Igor Sheludko 2014/04/03 09:01:00 Done.
Handle<JSArray> result_array =
isolate->factory()->NewJSArray(kind, result_len, result_len);
- DisallowHeapAllocation no_gc;
+ DisallowHeapAllocation no_more_allocations_allowed;
if (result_len == 0) return *result_array;
ElementsAccessor* accessor = object->GetElementsAccessor();
- accessor->CopyElements(Handle<JSObject>::null(), k, kind,
- handle(result_array->elements()), 0, result_len, elms);
+ accessor->CopyElements(
+ elms, k, kind, handle(result_array->elements(), isolate), 0, result_len);
return *result_array;
}
@@ -791,11 +777,11 @@ BUILTIN(ArraySplice) {
int relative_start = 0;
if (n_arguments > 0) {
- Handle<Object> arg1 = args.at<Object>(1);
+ Object* arg1 = args[1];
if (arg1->IsSmi()) {
- relative_start = Handle<Smi>::cast(arg1)->value();
+ relative_start = Smi::cast(arg1)->value();
} else if (arg1->IsHeapNumber()) {
- double start = Handle<HeapNumber>::cast(arg1)->value();
+ double start = HeapNumber::cast(arg1)->value();
if (start < kMinInt || start > kMaxInt) {
return CallJsBuiltin(isolate, "ArraySplice", args);
}
@@ -856,8 +842,8 @@ BUILTIN(ArraySplice) {
DisallowHeapAllocation no_gc;
ElementsAccessor* accessor = array->GetElementsAccessor();
accessor->CopyElements(
- Handle<JSObject>::null(), actual_start, elements_kind,
- handle(result_array->elements()), 0, actual_delete_count, elms_obj);
+ elms_obj, actual_start, elements_kind,
+ handle(result_array->elements(), isolate), 0, actual_delete_count);
}
bool elms_changed = false;
@@ -881,7 +867,7 @@ BUILTIN(ArraySplice) {
if (heap->CanMoveObjectStart(*elms_obj)) {
// On the fast path we move the start of the object in memory.
- elms_obj = handle(LeftTrimFixedArray(heap, *elms_obj, delta));
+ elms_obj = handle(LeftTrimFixedArray(heap, *elms_obj, delta), isolate);
} else {
// This is the slow path. We are going to move the elements to the left
// by copying them. For trimmed values we store the hole.
@@ -935,12 +921,12 @@ BUILTIN(ArraySplice) {
if (actual_start > 0) {
// Copy the part before actual_start as is.
accessor->CopyElements(
- Handle<JSObject>::null(), 0, kind, new_elms, 0, actual_start, elms);
+ elms, 0, kind, new_elms, 0, actual_start);
}
accessor->CopyElements(
- Handle<JSObject>::null(), actual_start + actual_delete_count, kind,
+ elms, actual_start + actual_delete_count, kind,
new_elms, actual_start + item_count,
- ElementsAccessor::kCopyToEndAndInitializeToHole, elms);
+ ElementsAccessor::kCopyToEndAndInitializeToHole);
elms_obj = new_elms;
elms_changed = true;
@@ -984,9 +970,9 @@ BUILTIN(ArraySplice) {
BUILTIN(ArrayConcat) {
HandleScope scope(isolate);
Heap* heap = isolate->heap();
- Handle<Context> native_context(isolate->context()->native_context());
+ Handle<Context> native_context(isolate->context()->native_context(), isolate);
Handle<JSObject> array_proto(
- JSObject::cast(native_context->array_function()->prototype()));
+ JSObject::cast(native_context->array_function()->prototype()), isolate);
if (!ArrayPrototypeHasNoElements(heap, *native_context, *array_proto)) {
return CallJsBuiltin(isolate, "ArrayConcat", args);
}
@@ -999,13 +985,14 @@ BUILTIN(ArrayConcat) {
bool has_double = false;
bool is_holey = false;
for (int i = 0; i < n_arguments; i++) {
- Handle<Object> arg = args.at<Object>(i);
+ DisallowHeapAllocation no_gc;
+ Object* arg = args[i];
if (!arg->IsJSArray() ||
- !Handle<JSArray>::cast(arg)->HasFastElements() ||
- Handle<JSArray>::cast(arg)->GetPrototype() != *array_proto) {
+ !JSArray::cast(arg)->HasFastElements() ||
+ JSArray::cast(arg)->GetPrototype() != *array_proto) {
return CallJsBuiltin(isolate, "ArrayConcat", args);
}
- int len = Smi::cast(Handle<JSArray>::cast(arg)->length())->value();
+ int len = Smi::cast(JSArray::cast(arg)->length())->value();
// We shouldn't overflow when adding another len.
const int kHalfOfMaxInt = 1 << (kBitsPerInt - 2);
@@ -1018,7 +1005,7 @@ BUILTIN(ArrayConcat) {
return CallJsBuiltin(isolate, "ArrayConcat", args);
}
- ElementsKind arg_kind = Handle<JSArray>::cast(arg)->map()->elements_kind();
+ ElementsKind arg_kind = JSArray::cast(arg)->map()->elements_kind();
has_double = has_double || IsFastDoubleElementsKind(arg_kind);
is_holey = is_holey || IsFastHoleyElementsKind(arg_kind);
if (IsMoreGeneralElementsKindTransition(elements_kind, arg_kind)) {
@@ -1042,10 +1029,12 @@ BUILTIN(ArrayConcat) {
if (result_len == 0) return *result_array;
int j = 0;
- Handle<FixedArrayBase> storage(result_array->elements());
+ Handle<FixedArrayBase> storage(result_array->elements(), isolate);
ElementsAccessor* accessor = ElementsAccessor::ForKind(elements_kind);
for (int i = 0; i < n_arguments; i++) {
- Handle<JSArray> array = args.at<JSArray>(i);
+ // It is crucial to keep |array| as a raw pointer 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) {
« no previous file with comments | « no previous file | src/elements.h » ('j') | src/factory.h » ('J')

Powered by Google App Engine
This is Rietveld 408576698