From b1721d4c3efb4325ed057efbd76921bd09e5e76d Mon Sep 17 00:00:00 2001 From: "kasperl@chromium.org" Date: Wed, 16 Dec 2009 15:43:20 +0000 Subject: [PATCH] Improve performance of allocating closures for nested functions by allocating them in new space without entering the runtime system. Review URL: http://codereview.chromium.org/506037 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@3477 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/code-stubs.h | 1 + src/codegen.h | 11 +++++++ src/factory.cc | 15 ++++++---- src/factory.h | 6 ++-- src/heap.cc | 7 +++-- src/heap.h | 3 +- src/ia32/codegen-ia32.cc | 73 ++++++++++++++++++++++++++++++++++++++------- src/ia32/stub-cache-ia32.cc | 21 +++++++++++-- src/objects-inl.h | 2 +- src/objects.cc | 14 ++++++--- src/objects.h | 9 ++++-- src/runtime.cc | 11 ++++--- 12 files changed, 138 insertions(+), 35 deletions(-) diff --git a/src/code-stubs.h b/src/code-stubs.h index 9012fe7..b19f304 100644 --- a/src/code-stubs.h +++ b/src/code-stubs.h @@ -43,6 +43,7 @@ namespace internal { V(ConvertToDouble) \ V(WriteInt32ToHeapNumber) \ V(StackCheck) \ + V(FastNewClosure) \ V(UnarySub) \ V(RevertToNumber) \ V(ToBoolean) \ diff --git a/src/codegen.h b/src/codegen.h index 85a08d5..10bf5db 100644 --- a/src/codegen.h +++ b/src/codegen.h @@ -233,6 +233,17 @@ class StackCheckStub : public CodeStub { }; +class FastNewClosureStub : public CodeStub { + public: + void Generate(MacroAssembler* masm); + + private: + const char* GetName() { return "FastNewClosureStub"; } + Major MajorKey() { return FastNewClosure; } + int MinorKey() { return 0; } +}; + + class InstanceofStub: public CodeStub { public: InstanceofStub() { } diff --git a/src/factory.cc b/src/factory.cc index 83775ef..2a80953 100644 --- a/src/factory.cc +++ b/src/factory.cc @@ -284,7 +284,8 @@ Handle Factory::CopyFixedArray(Handle array) { Handle Factory::BaseNewFunctionFromBoilerplate( Handle boilerplate, - Handle function_map) { + Handle function_map, + PretenureFlag pretenure) { ASSERT(boilerplate->IsBoilerplate()); ASSERT(!boilerplate->has_initial_map()); ASSERT(!boilerplate->has_prototype()); @@ -292,20 +293,22 @@ Handle Factory::BaseNewFunctionFromBoilerplate( ASSERT(boilerplate->elements() == Heap::empty_fixed_array()); CALL_HEAP_FUNCTION(Heap::AllocateFunction(*function_map, boilerplate->shared(), - Heap::the_hole_value()), + Heap::the_hole_value(), + pretenure), JSFunction); } Handle Factory::NewFunctionFromBoilerplate( Handle boilerplate, - Handle context) { - Handle result = - BaseNewFunctionFromBoilerplate(boilerplate, Top::function_map()); + Handle context, + PretenureFlag pretenure) { + Handle result = BaseNewFunctionFromBoilerplate( + boilerplate, Top::function_map(), pretenure); result->set_context(*context); int number_of_literals = boilerplate->NumberOfLiterals(); Handle literals = - Factory::NewFixedArray(number_of_literals, TENURED); + Factory::NewFixedArray(number_of_literals, pretenure); if (number_of_literals > 0) { // Store the object, regexp and array functions in the literals // array prefix. These functions will be used when creating diff --git a/src/factory.h b/src/factory.h index 951c043..fd277f2 100644 --- a/src/factory.h +++ b/src/factory.h @@ -219,7 +219,8 @@ class Factory : public AllStatic { static Handle NewFunctionFromBoilerplate( Handle boilerplate, - Handle context); + Handle context, + PretenureFlag pretenure = TENURED); static Handle NewCode(const CodeDesc& desc, ZoneScopeInfo* sinfo, @@ -374,7 +375,8 @@ class Factory : public AllStatic { static Handle BaseNewFunctionFromBoilerplate( Handle boilerplate, - Handle function_map); + Handle function_map, + PretenureFlag pretenure); // Create a new map cache. static Handle NewMapCache(int at_least_space_for); diff --git a/src/heap.cc b/src/heap.cc index b9aa95c..3de5ba8 100644 --- a/src/heap.cc +++ b/src/heap.cc @@ -2233,8 +2233,11 @@ Object* Heap::AllocateFunctionPrototype(JSFunction* function) { Object* Heap::AllocateFunction(Map* function_map, SharedFunctionInfo* shared, - Object* prototype) { - Object* result = Allocate(function_map, OLD_POINTER_SPACE); + Object* prototype, + PretenureFlag pretenure) { + AllocationSpace space = + (pretenure == TENURED) ? OLD_POINTER_SPACE : NEW_SPACE; + Object* result = Allocate(function_map, space); if (result->IsFailure()) return result; return InitializeFunction(JSFunction::cast(result), shared, prototype); } diff --git a/src/heap.h b/src/heap.h index 05ff16c..12835e6 100644 --- a/src/heap.h +++ b/src/heap.h @@ -487,7 +487,8 @@ class Heap : public AllStatic { // Please note this does not perform a garbage collection. static Object* AllocateFunction(Map* function_map, SharedFunctionInfo* shared, - Object* prototype); + Object* prototype, + PretenureFlag pretenure = TENURED); // Indicies for direct access into argument objects. static const int arguments_callee_index = 0; diff --git a/src/ia32/codegen-ia32.cc b/src/ia32/codegen-ia32.cc index 417eb0f..b265588 100644 --- a/src/ia32/codegen-ia32.cc +++ b/src/ia32/codegen-ia32.cc @@ -3593,18 +3593,28 @@ void CodeGenerator::VisitDebuggerStatement(DebuggerStatement* node) { void CodeGenerator::InstantiateBoilerplate(Handle boilerplate) { - // Call the runtime to instantiate the function boilerplate object. - // The inevitable call will sync frame elements to memory anyway, so - // we do it eagerly to allow us to push the arguments directly into - // place. ASSERT(boilerplate->IsBoilerplate()); - frame_->SyncRange(0, frame_->element_count() - 1); - // Create a new closure. - frame_->EmitPush(esi); - frame_->EmitPush(Immediate(boilerplate)); - Result result = frame_->CallRuntime(Runtime::kNewClosure, 2); - frame_->Push(&result); + // Use the fast case closure allocation code that allocated in new + // space for nested functions that don't need literals cloning. + if (scope()->is_function_scope() && boilerplate->NumberOfLiterals() == 0) { + FastNewClosureStub stub; + frame_->Push(boilerplate); + Result answer = frame_->CallStub(&stub, 1); + frame_->Push(&answer); + } else { + // Call the runtime to instantiate the function boilerplate + // object. The inevitable call will sync frame elements to memory + // anyway, so we do it eagerly to allow us to push the arguments + // directly into place. + frame_->SyncRange(0, frame_->element_count() - 1); + + // Create a new closure. + frame_->EmitPush(esi); + frame_->EmitPush(Immediate(boilerplate)); + Result result = frame_->CallRuntime(Runtime::kNewClosure, 2); + frame_->Push(&result); + } } @@ -6537,6 +6547,49 @@ void Reference::SetValue(InitState init_state) { } +void FastNewClosureStub::Generate(MacroAssembler* masm) { + // Clone the boilerplate in new space. Set the context to the + // current context in esi. + Label gc; + __ AllocateInNewSpace(JSFunction::kSize, eax, ebx, ecx, &gc, TAG_OBJECT); + + // Get the boilerplate function from the stack. + __ mov(edx, Operand(esp, 1 * kPointerSize)); + + // Compute the function map in the current global context and set that + // as the map of the allocated object. + __ mov(ecx, Operand(esi, Context::SlotOffset(Context::GLOBAL_INDEX))); + __ mov(ecx, FieldOperand(ecx, GlobalObject::kGlobalContextOffset)); + __ mov(ecx, Operand(ecx, Context::SlotOffset(Context::FUNCTION_MAP_INDEX))); + __ mov(FieldOperand(eax, JSObject::kMapOffset), ecx); + + // Clone the rest of the boilerplate fields. We don't have to update + // the write barrier because the allocated object is in new space. + for (int offset = kPointerSize; + offset < JSFunction::kSize; + offset += kPointerSize) { + if (offset == JSFunction::kContextOffset) { + __ mov(FieldOperand(eax, offset), esi); + } else { + __ mov(ebx, FieldOperand(edx, offset)); + __ mov(FieldOperand(eax, offset), ebx); + } + } + + // Return and remove the on-stack parameter. + __ ret(1 * kPointerSize); + + // Create a new closure through the slower runtime call. + __ bind(&gc); + __ pop(ecx); // Temporarily remove return address. + __ pop(edx); + __ push(esi); + __ push(edx); + __ push(ecx); // Restore return address. + __ TailCallRuntime(ExternalReference(Runtime::kNewClosure), 2, 1); +} + + // NOTE: The stub does not handle the inlined cases (Smis, Booleans, undefined). void ToBooleanStub::Generate(MacroAssembler* masm) { Label false_result, true_result, not_string; diff --git a/src/ia32/stub-cache-ia32.cc b/src/ia32/stub-cache-ia32.cc index 846b667..8aca1ef 100644 --- a/src/ia32/stub-cache-ia32.cc +++ b/src/ia32/stub-cache-ia32.cc @@ -1154,8 +1154,25 @@ Object* CallStubCompiler::CompileCallGlobal(JSObject* object, __ mov(edi, FieldOperand(edi, JSGlobalPropertyCell::kValueOffset)); // Check that the cell contains the same function. - __ cmp(Operand(edi), Immediate(Handle(function))); - __ j(not_equal, &miss, not_taken); + if (Heap::InNewSpace(function)) { + // We can't embed a pointer to a function in new space so we have + // to verify that the shared function info is unchanged. This has + // the nice side effect that multiple closures based on the same + // function can all use this call IC. Before we load through the + // function, we have to verify that it still is a function. + __ test(edi, Immediate(kSmiTagMask)); + __ j(zero, &miss, not_taken); + __ CmpObjectType(edi, JS_FUNCTION_TYPE, ebx); + __ j(not_equal, &miss, not_taken); + + // Check the shared function info. Make sure it hasn't changed. + __ cmp(FieldOperand(edi, JSFunction::kSharedFunctionInfoOffset), + Immediate(Handle(function->shared()))); + __ j(not_equal, &miss, not_taken); + } else { + __ cmp(Operand(edi), Immediate(Handle(function))); + __ j(not_equal, &miss, not_taken); + } // Patch the receiver on the stack with the global proxy. if (object->IsGlobalObject()) { diff --git a/src/objects-inl.h b/src/objects-inl.h index 8514a41..ce32eed 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -1499,7 +1499,7 @@ void DescriptorArray::Set(int descriptor_number, Descriptor* desc) { // Range check. ASSERT(descriptor_number < number_of_descriptors()); - // Make sure non of the elements in desc are in new space. + // Make sure none of the elements in desc are in new space. ASSERT(!Heap::InNewSpace(desc->GetKey())); ASSERT(!Heap::InNewSpace(desc->GetValue())); diff --git a/src/objects.cc b/src/objects.cc index 0f8dca3..52f1f9a 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -1351,6 +1351,8 @@ Object* JSObject::AddFastProperty(String* name, Object* JSObject::AddConstantFunctionProperty(String* name, JSFunction* function, PropertyAttributes attributes) { + ASSERT(!Heap::InNewSpace(function)); + // Allocate new instance descriptors with (name, function) added ConstantFunctionDescriptor d(name, function, attributes); Object* new_descriptors = @@ -1437,7 +1439,7 @@ Object* JSObject::AddProperty(String* name, // Ensure the descriptor array does not get too big. if (map()->instance_descriptors()->number_of_descriptors() < DescriptorArray::kMaxNumberOfDescriptors) { - if (value->IsJSFunction()) { + if (value->IsJSFunction() && !Heap::InNewSpace(value)) { return AddConstantFunctionProperty(name, JSFunction::cast(value), attributes); @@ -3254,7 +3256,8 @@ Object* DescriptorArray::Allocate(int number_of_descriptors) { return Heap::empty_descriptor_array(); } // Allocate the array of keys. - Object* array = Heap::AllocateFixedArray(ToKeyIndex(number_of_descriptors)); + Object* array = + Heap::AllocateFixedArray(ToKeyIndex(number_of_descriptors)); if (array->IsFailure()) return array; // Do not use DescriptorArray::cast on incomplete object. FixedArray* result = FixedArray::cast(array); @@ -7962,7 +7965,10 @@ Object* StringDictionary::TransformPropertiesToFastFor( PropertyType type = DetailsAt(i).type(); ASSERT(type != FIELD); instance_descriptor_length++; - if (type == NORMAL && !value->IsJSFunction()) number_of_fields += 1; + if (type == NORMAL && + (!value->IsJSFunction() || Heap::InNewSpace(value))) { + number_of_fields += 1; + } } } @@ -7993,7 +7999,7 @@ Object* StringDictionary::TransformPropertiesToFastFor( PropertyDetails details = DetailsAt(i); PropertyType type = details.type(); - if (value->IsJSFunction()) { + if (value->IsJSFunction() && !Heap::InNewSpace(value)) { ConstantFunctionDescriptor d(String::cast(key), JSFunction::cast(value), details.attributes(), diff --git a/src/objects.h b/src/objects.h index 671978a..1545bbf 100644 --- a/src/objects.h +++ b/src/objects.h @@ -1662,6 +1662,7 @@ class DescriptorArray: public FixedArray { public: // Is this the singleton empty_descriptor_array? inline bool IsEmpty(); + // Returns the number of descriptors in the array. int number_of_descriptors() { return IsEmpty() ? 0 : length() - kFirstIndex; @@ -1801,13 +1802,15 @@ class DescriptorArray: public FixedArray { static int ToKeyIndex(int descriptor_number) { return descriptor_number+kFirstIndex; } - static int ToValueIndex(int descriptor_number) { - return descriptor_number << 1; - } + static int ToDetailsIndex(int descriptor_number) { return( descriptor_number << 1) + 1; } + static int ToValueIndex(int descriptor_number) { + return descriptor_number << 1; + } + bool is_null_descriptor(int descriptor_number) { return PropertyDetails(GetDetails(descriptor_number)).type() == NULL_DESCRIPTOR; diff --git a/src/runtime.cc b/src/runtime.cc index ac61de2..8349833 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -720,7 +720,7 @@ static Object* Runtime_DeclareGlobals(Arguments args) { // Copy the function and update its context. Use it as value. Handle boilerplate = Handle::cast(value); Handle function = - Factory::NewFunctionFromBoilerplate(boilerplate, context); + Factory::NewFunctionFromBoilerplate(boilerplate, context, TENURED); value = function; } @@ -4502,8 +4502,11 @@ static Object* Runtime_NewClosure(Arguments args) { CONVERT_ARG_CHECKED(Context, context, 0); CONVERT_ARG_CHECKED(JSFunction, boilerplate, 1); + PretenureFlag pretenure = (context->global_context() == *context) + ? TENURED // Allocate global closures in old space. + : NOT_TENURED; // Allocate local closures in new space. Handle result = - Factory::NewFunctionFromBoilerplate(boilerplate, context); + Factory::NewFunctionFromBoilerplate(boilerplate, context, pretenure); return *result; } @@ -5219,7 +5222,7 @@ static Object* Runtime_CompileString(Arguments args) { validate); if (boilerplate.is_null()) return Failure::Exception(); Handle fun = - Factory::NewFunctionFromBoilerplate(boilerplate, context); + Factory::NewFunctionFromBoilerplate(boilerplate, context, NOT_TENURED); return *fun; } @@ -5247,7 +5250,7 @@ static Object* CompileDirectEval(Handle source) { Compiler::DONT_VALIDATE_JSON); if (boilerplate.is_null()) return Failure::Exception(); Handle fun = - Factory::NewFunctionFromBoilerplate(boilerplate, context); + Factory::NewFunctionFromBoilerplate(boilerplate, context, NOT_TENURED); return *fun; } -- 2.7.4