From 5e7da7f04f3881a8a5b881a8446c38345b0fff19 Mon Sep 17 00:00:00 2001 From: "kmillikin@chromium.org" Date: Tue, 28 Jun 2011 15:22:08 +0000 Subject: [PATCH] Remove the fcontext field from all contexts. Before: every context cached the nearest enclosing function context. This assumed that for nested contexts (i.e., with and catch contexts) the enclosing function had a materialized link in the context chain. Now: when necessary, we loop up the context chain to find such a context. This enables catch contexts without forcing the enclosing function to allocate its own context. R=ager@chromium.org BUG= TEST= Review URL: http://codereview.chromium.org/7230047 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@8452 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/code-stubs-arm.cc | 1 - src/arm/full-codegen-arm.cc | 23 +++++----------- src/arm/macro-assembler-arm.cc | 11 -------- src/bootstrapper.cc | 1 - src/contexts.cc | 34 +++++++++++++++-------- src/contexts.h | 25 +++-------------- src/heap.cc | 3 --- src/ia32/code-stubs-ia32.cc | 1 - src/ia32/full-codegen-ia32.cc | 22 +++++---------- src/ia32/macro-assembler-ia32.cc | 17 +++++++----- src/runtime.cc | 46 +++++++++++++++----------------- src/x64/code-stubs-x64.cc | 1 - src/x64/full-codegen-x64.cc | 25 +++++++---------- src/x64/macro-assembler-x64.cc | 17 +++++++----- 14 files changed, 91 insertions(+), 136 deletions(-) diff --git a/src/arm/code-stubs-arm.cc b/src/arm/code-stubs-arm.cc index b92c25a1fe..a19db9ca64 100644 --- a/src/arm/code-stubs-arm.cc +++ b/src/arm/code-stubs-arm.cc @@ -165,7 +165,6 @@ void FastNewContextStub::Generate(MacroAssembler* masm) { // Setup the fixed slots. __ mov(r1, Operand(Smi::FromInt(0))); __ str(r3, MemOperand(r0, Context::SlotOffset(Context::CLOSURE_INDEX))); - __ str(r0, MemOperand(r0, Context::SlotOffset(Context::FCONTEXT_INDEX))); __ str(cp, MemOperand(r0, Context::SlotOffset(Context::PREVIOUS_INDEX))); __ str(r1, MemOperand(r0, Context::SlotOffset(Context::EXTENSION_INDEX))); diff --git a/src/arm/full-codegen-arm.cc b/src/arm/full-codegen-arm.cc index 89ca0bad37..950ebc3de9 100644 --- a/src/arm/full-codegen-arm.cc +++ b/src/arm/full-codegen-arm.cc @@ -713,10 +713,12 @@ void FullCodeGenerator::EmitDeclaration(Variable* variable, // context. ASSERT_EQ(0, scope()->ContextChainLength(variable->scope())); if (FLAG_debug_code) { - // Check that we're not inside a 'with'. - __ ldr(r1, ContextOperand(cp, Context::FCONTEXT_INDEX)); - __ cmp(r1, cp); - __ Check(eq, "Unexpected declaration in current context."); + // Check that we're not inside a with or catch context. + __ ldr(r1, FieldMemOperand(cp, HeapObject::kMapOffset)); + __ CompareRoot(r1, Heap::kWithContextMapRootIndex); + __ Check(ne, "Declaration in with context."); + __ CompareRoot(r1, Heap::kCatchContextMapRootIndex); + __ Check(ne, "Declaration in catch context."); } if (mode == Variable::CONST) { __ LoadRoot(ip, Heap::kTheHoleValueRootIndex); @@ -1867,18 +1869,7 @@ void FullCodeGenerator::EmitVariableAssignment(Variable* var, __ b(ne, &skip); __ str(result_register(), MemOperand(fp, SlotOffset(slot))); break; - case Slot::CONTEXT: { - __ ldr(r1, ContextOperand(cp, Context::FCONTEXT_INDEX)); - __ ldr(r2, ContextOperand(r1, slot->index())); - __ LoadRoot(ip, Heap::kTheHoleValueRootIndex); - __ cmp(r2, ip); - __ b(ne, &skip); - __ str(r0, ContextOperand(r1, slot->index())); - int offset = Context::SlotOffset(slot->index()); - __ mov(r3, r0); // Preserve the stored value in r0. - __ RecordWrite(r1, Operand(offset), r3, r2); - break; - } + case Slot::CONTEXT: case Slot::LOOKUP: __ push(r0); __ mov(r0, Operand(slot->var()->name())); diff --git a/src/arm/macro-assembler-arm.cc b/src/arm/macro-assembler-arm.cc index 59c17e6800..727b4dd985 100644 --- a/src/arm/macro-assembler-arm.cc +++ b/src/arm/macro-assembler-arm.cc @@ -2562,17 +2562,6 @@ void MacroAssembler::LoadContext(Register dst, int context_chain_length) { // cannot be allowed to destroy the context in esi). mov(dst, cp); } - - // We should not have found a 'with' context by walking the context chain - // (i.e., the static scope chain and runtime context chain do not agree). - // A variable occurring in such a scope should have slot type LOOKUP and - // not CONTEXT. - if (emit_debug_code()) { - ldr(ip, MemOperand(dst, Context::SlotOffset(Context::FCONTEXT_INDEX))); - cmp(dst, ip); - Check(eq, "Yo dawg, I heard you liked function contexts " - "so I put function contexts in all your contexts"); - } } diff --git a/src/bootstrapper.cc b/src/bootstrapper.cc index 2ca379f006..8e34b9cf50 100644 --- a/src/bootstrapper.cc +++ b/src/bootstrapper.cc @@ -814,7 +814,6 @@ void Genesis::InitializeGlobal(Handle inner_global, // --- G l o b a l C o n t e x t --- // Use the empty function as closure (no scope info). global_context()->set_closure(*empty_function); - global_context()->set_fcontext(*global_context()); global_context()->set_previous(NULL); // Set extension and global object. global_context()->set_extension(*inner_global); diff --git a/src/contexts.cc b/src/contexts.cc index 4cc6222939..d066d34769 100644 --- a/src/contexts.cc +++ b/src/contexts.cc @@ -34,6 +34,16 @@ namespace v8 { namespace internal { +Context* Context::declaration_context() { + Context* current = this; + while (!current->IsFunctionContext() && !current->IsGlobalContext()) { + current = current->previous(); + ASSERT(current->closure() == closure()); + } + return current; +} + + JSBuiltinsObject* Context::builtins() { GlobalObject* object = global(); if (object->IsJSGlobalObject()) { @@ -246,20 +256,22 @@ void Context::ComputeEvalScopeInfo(bool* outer_scope_calls_eval, bool* outer_scope_calls_non_strict_eval) { // Skip up the context chain checking all the function contexts to see // whether they call eval. - Context* context = fcontext(); + Context* context = this; while (!context->IsGlobalContext()) { - Handle scope_info( - context->closure()->shared()->scope_info()); - if (scope_info->CallsEval()) { - *outer_scope_calls_eval = true; - if (!scope_info->IsStrictMode()) { - // No need to go further since the answers will not change - // from here. - *outer_scope_calls_non_strict_eval = true; - return; + if (context->IsFunctionContext()) { + Handle scope_info( + context->closure()->shared()->scope_info()); + if (scope_info->CallsEval()) { + *outer_scope_calls_eval = true; + if (!scope_info->IsStrictMode()) { + // No need to go further since the answers will not change from + // here. + *outer_scope_calls_non_strict_eval = true; + return; + } } } - context = context->previous()->fcontext(); + context = context->previous(); } } diff --git a/src/contexts.h b/src/contexts.h index d5c493584b..da6e088757 100644 --- a/src/contexts.h +++ b/src/contexts.h @@ -130,13 +130,6 @@ enum ContextLookupFlags { // statically allocated context slots. The names are needed // for dynamic lookups in the presence of 'with' or 'eval'. // -// [ fcontext ] A pointer to the innermost enclosing function context. -// It is the same for all contexts *allocated* inside a -// function, and the function context's fcontext points -// to itself. It is only needed for fast access of the -// function context (used for declarations, and static -// context slot access). -// // [ previous ] A pointer to the previous context. It is NULL for // function contexts, and non-NULL for 'with' contexts. // Used to implement the 'with' statement. @@ -158,16 +151,6 @@ enum ContextLookupFlags { // (via static context addresses) or through 'eval' (dynamic context lookups). // Finally, the global context contains additional slots for fast access to // global properties. -// -// We may be able to simplify the implementation: -// -// - We may be able to get rid of 'fcontext': We can always use the fact that -// previous == NULL for function contexts and so we can search for them. They -// are only needed when doing dynamic declarations, and the context chains -// tend to be very very short (depth of nesting of 'with' statements). At -// the moment we also use it in generated code for context slot accesses - -// and there we don't want a loop because of code bloat - but we may not -// need it there after all (see comment in codegen_*.cc). class Context: public FixedArray { public: @@ -181,7 +164,6 @@ class Context: public FixedArray { enum { // These slots are in all contexts. CLOSURE_INDEX, - FCONTEXT_INDEX, PREVIOUS_INDEX, // The extension slot is used for either the global object (in global // contexts), eval extension object (function contexts), subject of with @@ -264,9 +246,6 @@ class Context: public FixedArray { JSFunction* closure() { return JSFunction::cast(get(CLOSURE_INDEX)); } void set_closure(JSFunction* closure) { set(CLOSURE_INDEX, closure); } - Context* fcontext() { return Context::cast(get(FCONTEXT_INDEX)); } - void set_fcontext(Context* context) { set(FCONTEXT_INDEX, context); } - Context* previous() { Object* result = unchecked_previous(); ASSERT(IsBootstrappingOrContext(result)); @@ -278,6 +257,10 @@ class Context: public FixedArray { Object* extension() { return get(EXTENSION_INDEX); } void set_extension(Object* object) { set(EXTENSION_INDEX, object); } + // Get the context where var declarations will be hoisted to, which + // may be the context itself. + Context* declaration_context(); + GlobalObject* global() { Object* result = get(GLOBAL_INDEX); ASSERT(IsBootstrappingOrGlobalObject(result)); diff --git a/src/heap.cc b/src/heap.cc index 0ce5e34aa0..607eda7ebd 100644 --- a/src/heap.cc +++ b/src/heap.cc @@ -3932,7 +3932,6 @@ MaybeObject* Heap::AllocateFunctionContext(int length, JSFunction* function) { Context* context = reinterpret_cast(result); context->set_map(function_context_map()); context->set_closure(function); - context->set_fcontext(context); context->set_previous(function->context()); context->set_extension(NULL); context->set_global(function->context()->global()); @@ -3952,7 +3951,6 @@ MaybeObject* Heap::AllocateCatchContext(Context* previous, Context* context = reinterpret_cast(result); context->set_map(catch_context_map()); context->set_closure(previous->closure()); - context->set_fcontext(previous->fcontext()); context->set_previous(previous); context->set_extension(name); context->set_global(previous->global()); @@ -3970,7 +3968,6 @@ MaybeObject* Heap::AllocateWithContext(Context* previous, Context* context = reinterpret_cast(result); context->set_map(with_context_map()); context->set_closure(previous->closure()); - context->set_fcontext(previous->fcontext()); context->set_previous(previous); context->set_extension(extension); context->set_global(previous->global()); diff --git a/src/ia32/code-stubs-ia32.cc b/src/ia32/code-stubs-ia32.cc index 8fd272b7eb..d97206d0b7 100644 --- a/src/ia32/code-stubs-ia32.cc +++ b/src/ia32/code-stubs-ia32.cc @@ -136,7 +136,6 @@ void FastNewContextStub::Generate(MacroAssembler* masm) { // Setup the fixed slots. __ Set(ebx, Immediate(0)); // Set to NULL. __ mov(Operand(eax, Context::SlotOffset(Context::CLOSURE_INDEX)), ecx); - __ mov(Operand(eax, Context::SlotOffset(Context::FCONTEXT_INDEX)), eax); __ mov(Operand(eax, Context::SlotOffset(Context::PREVIOUS_INDEX)), esi); __ mov(Operand(eax, Context::SlotOffset(Context::EXTENSION_INDEX)), ebx); diff --git a/src/ia32/full-codegen-ia32.cc b/src/ia32/full-codegen-ia32.cc index f3edad3c05..0496d732ef 100644 --- a/src/ia32/full-codegen-ia32.cc +++ b/src/ia32/full-codegen-ia32.cc @@ -687,10 +687,12 @@ void FullCodeGenerator::EmitDeclaration(Variable* variable, // context. ASSERT_EQ(0, scope()->ContextChainLength(variable->scope())); if (FLAG_debug_code) { - // Check that we're not inside a 'with'. - __ mov(ebx, ContextOperand(esi, Context::FCONTEXT_INDEX)); - __ cmp(ebx, Operand(esi)); - __ Check(equal, "Unexpected declaration in current context."); + // Check that we're not inside a with or catch context. + __ mov(ebx, FieldOperand(esi, HeapObject::kMapOffset)); + __ cmp(ebx, isolate()->factory()->with_context_map()); + __ Check(not_equal, "Declaration in with context."); + __ cmp(ebx, isolate()->factory()->catch_context_map()); + __ Check(not_equal, "Declaration in catch context."); } if (mode == Variable::CONST) { __ mov(ContextOperand(esi, slot->index()), @@ -1818,17 +1820,7 @@ void FullCodeGenerator::EmitVariableAssignment(Variable* var, __ j(not_equal, &skip); __ mov(Operand(ebp, SlotOffset(slot)), eax); break; - case Slot::CONTEXT: { - __ mov(ecx, ContextOperand(esi, Context::FCONTEXT_INDEX)); - __ mov(edx, ContextOperand(ecx, slot->index())); - __ cmp(edx, isolate()->factory()->the_hole_value()); - __ j(not_equal, &skip); - __ mov(ContextOperand(ecx, slot->index()), eax); - int offset = Context::SlotOffset(slot->index()); - __ mov(edx, eax); // Preserve the stored value in eax. - __ RecordWrite(ecx, offset, edx, ebx); - break; - } + case Slot::CONTEXT: case Slot::LOOKUP: __ push(eax); __ push(esi); diff --git a/src/ia32/macro-assembler-ia32.cc b/src/ia32/macro-assembler-ia32.cc index ce07333bb9..a80821f82c 100644 --- a/src/ia32/macro-assembler-ia32.cc +++ b/src/ia32/macro-assembler-ia32.cc @@ -1766,14 +1766,17 @@ void MacroAssembler::LoadContext(Register dst, int context_chain_length) { mov(dst, esi); } - // We should not have found a 'with' context by walking the context chain - // (i.e., the static scope chain and runtime context chain do not agree). - // A variable occurring in such a scope should have slot type LOOKUP and - // not CONTEXT. + // We should not have found a with or catch context by walking the context + // chain (i.e., the static scope chain and runtime context chain do not + // agree). A variable occurring in such a scope should have slot type + // LOOKUP and not CONTEXT. if (emit_debug_code()) { - cmp(dst, Operand(dst, Context::SlotOffset(Context::FCONTEXT_INDEX))); - Check(equal, "Yo dawg, I heard you liked function contexts " - "so I put function contexts in all your contexts"); + cmp(FieldOperand(dst, HeapObject::kMapOffset), + isolate()->factory()->with_context_map()); + Check(not_equal, "Variable resolved to with context."); + cmp(FieldOperand(dst, HeapObject::kMapOffset), + isolate()->factory()->with_context_map()); + Check(not_equal, "Variable resolved to catch context."); } } diff --git a/src/runtime.cc b/src/runtime.cc index 29d4ecd77d..1de2b4a935 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -1236,9 +1236,8 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DeclareContextSlot) { RUNTIME_ASSERT(mode == READ_ONLY || mode == NONE); Handle initial_value(args[3], isolate); - // Declarations are always done in the function context. - context = Handle(context->fcontext()); - ASSERT(context->IsFunctionContext() || context->IsGlobalContext()); + // Declarations are always done in a function or global context. + context = Handle(context->declaration_context()); int index; PropertyAttributes attributes; @@ -1525,8 +1524,8 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_InitializeConstContextSlot) { CONVERT_ARG_CHECKED(Context, context, 1); Handle name(String::cast(args[2])); - // Initializations are always done in the function context. - context = Handle(context->fcontext()); + // Initializations are always done in a function or global context. + context = Handle(context->declaration_context()); int index; PropertyAttributes attributes; @@ -1547,14 +1546,12 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_InitializeConstContextSlot) { // In that case, the initialization behaves like a normal assignment // to property 'x'. if (index >= 0) { - // Property was found in a context. if (holder->IsContext()) { - // The holder cannot be the function context. If it is, there - // should have been a const redeclaration error when declaring - // the const property. - ASSERT(!holder.is_identical_to(context)); - if ((attributes & READ_ONLY) == 0) { - Handle::cast(holder)->set(index, *value); + // Property was found in a context. Perform the assignment if we + // found some non-constant or an uninitialized constant. + Handle context = Handle::cast(holder); + if ((attributes & READ_ONLY) == 0 || context->get(index)->IsTheHole()) { + context->set(index, *value); } } else { // The holder is an arguments object. @@ -9986,9 +9983,6 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetFrameDetails) { Handle scope_info(function->shared()->scope_info()); ScopeInfo<> info(*scope_info); - // Get the nearest enclosing function context. - Handle context(Context::cast(it.frame()->context())->fcontext()); - // Get the locals names and values into a temporary array. // // TODO(1240907): Hide compiler-introduced stack variables @@ -9997,11 +9991,6 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetFrameDetails) { Handle locals = isolate->factory()->NewFixedArray(info.NumberOfLocals() * 2); - // Fill in the names of the locals. - for (int i = 0; i < info.NumberOfLocals(); i++) { - locals->set(i * 2, *info.LocalName(i)); - } - // Fill in the values of the locals. if (is_optimized_frame) { // If we are inspecting an optimized frame use undefined as the @@ -10010,15 +9999,22 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetFrameDetails) { // TODO(1140): We should be able to get the correct values // for locals in optimized frames. for (int i = 0; i < info.NumberOfLocals(); i++) { + locals->set(i * 2, *info.LocalName(i)); locals->set(i * 2 + 1, isolate->heap()->undefined_value()); } } else { - for (int i = 0; i < info.number_of_stack_slots(); i++) { - // Get the value from the stack. + int i = 0; + for (; i < info.number_of_stack_slots(); ++i) { + // Use the value from the stack. + locals->set(i * 2, *info.LocalName(i)); locals->set(i * 2 + 1, it.frame()->GetExpression(i)); } - for (int i = info.number_of_stack_slots(); i < info.NumberOfLocals(); i++) { + // Get the context containing declarations. + Handle context( + Context::cast(it.frame()->context())->declaration_context()); + for (; i < info.NumberOfLocals(); ++i) { Handle name = info.LocalName(i); + locals->set(i * 2, *name); locals->set(i * 2 + 1, context->get(scope_info->ContextSlotIndex(*name, NULL))); } @@ -10239,7 +10235,7 @@ static Handle MaterializeLocalScope(Isolate* isolate, // Third fill all context locals. Handle frame_context(Context::cast(frame->context())); - Handle function_context(frame_context->fcontext()); + Handle function_context(frame_context->declaration_context()); if (!CopyContextLocalsToScopeObject(isolate, serialized_scope_info, scope_info, function_context, local_scope)) { @@ -11121,7 +11117,7 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DebugEvaluate) { context->set_extension(*local_scope); // Copy any with contexts present and chain them in front of this context. Handle frame_context(Context::cast(frame->context())); - Handle function_context(frame_context->fcontext()); + Handle function_context(frame_context->declaration_context()); context = CopyWithContextChain(isolate, frame_context, context); if (additional_context->IsJSObject()) { diff --git a/src/x64/code-stubs-x64.cc b/src/x64/code-stubs-x64.cc index e8642a5b72..81514d1e92 100644 --- a/src/x64/code-stubs-x64.cc +++ b/src/x64/code-stubs-x64.cc @@ -132,7 +132,6 @@ void FastNewContextStub::Generate(MacroAssembler* masm) { // Setup the fixed slots. __ Set(rbx, 0); // Set to NULL. __ movq(Operand(rax, Context::SlotOffset(Context::CLOSURE_INDEX)), rcx); - __ movq(Operand(rax, Context::SlotOffset(Context::FCONTEXT_INDEX)), rax); __ movq(Operand(rax, Context::SlotOffset(Context::PREVIOUS_INDEX)), rsi); __ movq(Operand(rax, Context::SlotOffset(Context::EXTENSION_INDEX)), rbx); diff --git a/src/x64/full-codegen-x64.cc b/src/x64/full-codegen-x64.cc index 77c687045c..21ab4cb10e 100644 --- a/src/x64/full-codegen-x64.cc +++ b/src/x64/full-codegen-x64.cc @@ -680,13 +680,16 @@ void FullCodeGenerator::EmitDeclaration(Variable* variable, // We bypass the general EmitSlotSearch because we know more about // this specific context. - // The variable in the decl always resides in the current context. + // The variable in the decl always resides in the current function + // context. ASSERT_EQ(0, scope()->ContextChainLength(variable->scope())); if (FLAG_debug_code) { - // Check if we have the correct context pointer. - __ movq(rbx, ContextOperand(rsi, Context::FCONTEXT_INDEX)); - __ cmpq(rbx, rsi); - __ Check(equal, "Unexpected declaration in current context."); + // Check that we're not inside a with or catch context. + __ movq(rbx, FieldOperand(rsi, HeapObject::kMapOffset)); + __ CompareRoot(rbx, Heap::kWithContextMapRootIndex); + __ Check(not_equal, "Declaration in with context."); + __ CompareRoot(rbx, Heap::kCatchContextMapRootIndex); + __ Check(not_equal, "Declaration in catch context."); } if (mode == Variable::CONST) { __ LoadRoot(kScratchRegister, Heap::kTheHoleValueRootIndex); @@ -1778,17 +1781,7 @@ void FullCodeGenerator::EmitVariableAssignment(Variable* var, __ j(not_equal, &skip); __ movq(Operand(rbp, SlotOffset(slot)), rax); break; - case Slot::CONTEXT: { - __ movq(rcx, ContextOperand(rsi, Context::FCONTEXT_INDEX)); - __ movq(rdx, ContextOperand(rcx, slot->index())); - __ CompareRoot(rdx, Heap::kTheHoleValueRootIndex); - __ j(not_equal, &skip); - __ movq(ContextOperand(rcx, slot->index()), rax); - int offset = Context::SlotOffset(slot->index()); - __ movq(rdx, rax); // Preserve the stored value in eax. - __ RecordWrite(rcx, offset, rdx, rbx); - break; - } + case Slot::CONTEXT: case Slot::LOOKUP: __ push(rax); __ push(rsi); diff --git a/src/x64/macro-assembler-x64.cc b/src/x64/macro-assembler-x64.cc index 1d375e15f8..7c8a3667e3 100644 --- a/src/x64/macro-assembler-x64.cc +++ b/src/x64/macro-assembler-x64.cc @@ -3628,14 +3628,17 @@ void MacroAssembler::LoadContext(Register dst, int context_chain_length) { movq(dst, rsi); } - // We should not have found a 'with' context by walking the context chain - // (i.e., the static scope chain and runtime context chain do not agree). - // A variable occurring in such a scope should have slot type LOOKUP and - // not CONTEXT. + // We should not have found a with or catch context by walking the context + // chain (i.e., the static scope chain and runtime context chain do not + // agree). A variable occurring in such a scope should have slot type + // LOOKUP and not CONTEXT. if (emit_debug_code()) { - cmpq(dst, Operand(dst, Context::SlotOffset(Context::FCONTEXT_INDEX))); - Check(equal, "Yo dawg, I heard you liked function contexts " - "so I put function contexts in all your contexts"); + CompareRoot(FieldOperand(dst, HeapObject::kMapOffset), + Heap::kWithContextMapRootIndex); + Check(not_equal, "Variable resolved to with context."); + CompareRoot(FieldOperand(dst, HeapObject::kMapOffset), + Heap::kCatchContextMapRootIndex); + Check(not_equal, "Variable resolved to catch context."); } }