From b9e0b87a5a15a7f39c9f967f63eee345566e892b Mon Sep 17 00:00:00 2001 From: "ulan@chromium.org" Date: Mon, 3 Mar 2014 11:11:39 +0000 Subject: [PATCH] Clear optimized code cache in shared function info when code gets deoptimized. This adds a pointer to the shared function info into deoptimization data of an optimized code. Whenever the code is deoptimized, it clears the cache in the shared function info. This fixes the problem when the optimized function dies in new space GC before the code is deoptimized due to code dependency and before the optimized code cache is cleared in old space GC (see mjsunit/regress/regress-343609.js). This partially reverts r19603 because we need to be able to evict specific code from the optimized code cache. BUG=343609 LOG=Y TEST=mjsunit/regress/regress-343609.js R=yangguo@chromium.org Review URL: https://codereview.chromium.org/184923002 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19635 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/a64/deoptimizer-a64.cc | 8 +++-- src/a64/lithium-codegen-a64.cc | 5 +++ src/arm/deoptimizer-arm.cc | 7 ++-- src/arm/lithium-codegen-arm.cc | 7 ++++ src/compiler.cc | 5 +-- src/deoptimizer.cc | 2 -- src/factory.cc | 1 + src/ia32/deoptimizer-ia32.cc | 8 +++-- src/ia32/lithium-codegen-ia32.cc | 7 ++++ src/mips/deoptimizer-mips.cc | 7 ++-- src/mips/lithium-codegen-mips.cc | 7 ++++ src/objects-inl.h | 2 -- src/objects.cc | 54 +++++++++++++--------------- src/objects.h | 9 ++--- src/runtime.cc | 8 ++--- src/x64/deoptimizer-x64.cc | 6 ++++ src/x64/lithium-codegen-x64.cc | 7 ++++ test/cctest/test-heap.cc | 5 +++ test/mjsunit/regress/regress-343609.js | 66 ++++++++++++++++++++++++++++++++++ 19 files changed, 166 insertions(+), 55 deletions(-) create mode 100644 test/mjsunit/regress/regress-343609.js diff --git a/src/a64/deoptimizer-a64.cc b/src/a64/deoptimizer-a64.cc index 66e501a..40e3191 100644 --- a/src/a64/deoptimizer-a64.cc +++ b/src/a64/deoptimizer-a64.cc @@ -53,15 +53,17 @@ void Deoptimizer::PatchCodeForDeoptimization(Isolate* isolate, Code* code) { // TODO(jkummerow): if (FLAG_zap_code_space), make the code object's // entry sequence unusable (see other architectures). - // For each LLazyBailout instruction insert a call to the corresponding - // deoptimization entry. DeoptimizationInputData* deopt_data = DeoptimizationInputData::cast(code->deoptimization_data()); + SharedFunctionInfo* shared = + SharedFunctionInfo::cast(deopt_data->SharedFunctionInfo()); + shared->EvictFromOptimizedCodeMap(code, "deoptimized code"); Address code_start_address = code->instruction_start(); #ifdef DEBUG Address prev_call_address = NULL; #endif - + // For each LLazyBailout instruction insert a call to the corresponding + // deoptimization entry. for (int i = 0; i < deopt_data->DeoptCount(); i++) { if (deopt_data->Pc(i)->value() == -1) continue; diff --git a/src/a64/lithium-codegen-a64.cc b/src/a64/lithium-codegen-a64.cc index 2bad671..5e48b66 100644 --- a/src/a64/lithium-codegen-a64.cc +++ b/src/a64/lithium-codegen-a64.cc @@ -919,6 +919,11 @@ void LCodeGen::PopulateDeoptimizationData(Handle code) { data->SetTranslationByteArray(*translations); data->SetInlinedFunctionCount(Smi::FromInt(inlined_function_count_)); data->SetOptimizationId(Smi::FromInt(info_->optimization_id())); + if (info_->IsOptimizing()) { + data->SetSharedFunctionInfo(*info_->shared_info()); + } else { + data->SetSharedFunctionInfo(Smi::FromInt(0)); + } Handle literals = factory()->NewFixedArray(deoptimization_literals_.length(), TENURED); diff --git a/src/arm/deoptimizer-arm.cc b/src/arm/deoptimizer-arm.cc index 7855cbe..59e6e95 100644 --- a/src/arm/deoptimizer-arm.cc +++ b/src/arm/deoptimizer-arm.cc @@ -70,13 +70,16 @@ void Deoptimizer::PatchCodeForDeoptimization(Isolate* isolate, Code* code) { } } - // For each LLazyBailout instruction insert a call to the corresponding - // deoptimization entry. DeoptimizationInputData* deopt_data = DeoptimizationInputData::cast(code->deoptimization_data()); + SharedFunctionInfo* shared = + SharedFunctionInfo::cast(deopt_data->SharedFunctionInfo()); + shared->EvictFromOptimizedCodeMap(code, "deoptimized code"); #ifdef DEBUG Address prev_call_address = NULL; #endif + // For each LLazyBailout instruction insert a call to the corresponding + // deoptimization entry. for (int i = 0; i < deopt_data->DeoptCount(); i++) { if (deopt_data->Pc(i)->value() == -1) continue; Address call_address = code_start_address + deopt_data->Pc(i)->value(); diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc index 8c9221c..7f5900d 100644 --- a/src/arm/lithium-codegen-arm.cc +++ b/src/arm/lithium-codegen-arm.cc @@ -908,6 +908,13 @@ void LCodeGen::PopulateDeoptimizationData(Handle code) { data->SetTranslationByteArray(*translations); data->SetInlinedFunctionCount(Smi::FromInt(inlined_function_count_)); data->SetOptimizationId(Smi::FromInt(info_->optimization_id())); + if (info_->IsOptimizing()) { + // Reference to shared function info does not change between phases. + AllowDeferredHandleDereference allow_handle_dereference; + data->SetSharedFunctionInfo(*info_->shared_info()); + } else { + data->SetSharedFunctionInfo(Smi::FromInt(0)); + } Handle literals = factory()->NewFixedArray(deoptimization_literals_.length(), TENURED); diff --git a/src/compiler.cc b/src/compiler.cc index 284919d..743d333 100644 --- a/src/compiler.cc +++ b/src/compiler.cc @@ -1070,10 +1070,7 @@ static Handle GetCodeFromOptimizedCodeMap(Handle function, } FixedArray* literals = shared->GetLiteralsFromOptimizedCodeMap(index); if (literals != NULL) function->set_literals(literals); - Handle code(shared->GetCodeFromOptimizedCodeMap(index)); - if (!code->marked_for_deoptimization()) return code; - shared->EvictFromOptimizedCodeMap(function->context()->native_context(), - "code was already marked for deopt"); + return Handle(shared->GetCodeFromOptimizedCodeMap(index)); } } return Handle::null(); diff --git a/src/deoptimizer.cc b/src/deoptimizer.cc index 04439f3..1b78bae 100644 --- a/src/deoptimizer.cc +++ b/src/deoptimizer.cc @@ -342,8 +342,6 @@ void Deoptimizer::DeoptimizeMarkedCodeForContext(Context* context) { // Unlink this function and evict from optimized code map. SharedFunctionInfo* shared = function->shared(); function->set_code(shared->code()); - shared->EvictFromOptimizedCodeMap(function->context()->native_context(), - "deoptimized function"); if (FLAG_trace_deopt) { CodeTracer::Scope scope(code->GetHeap()->isolate()->GetCodeTracer()); diff --git a/src/factory.cc b/src/factory.cc index 54ebf85..0b131de 100644 --- a/src/factory.cc +++ b/src/factory.cc @@ -968,6 +968,7 @@ Handle Factory::NewFunctionFromSharedFunctionInfo( function_info->GetLiteralsFromOptimizedCodeMap(index); if (literals != NULL) result->set_literals(literals); Code* code = function_info->GetCodeFromOptimizedCodeMap(index); + ASSERT(!code->marked_for_deoptimization()); result->ReplaceCode(code); return result; } diff --git a/src/ia32/deoptimizer-ia32.cc b/src/ia32/deoptimizer-ia32.cc index 4f1ef8d..546781e 100644 --- a/src/ia32/deoptimizer-ia32.cc +++ b/src/ia32/deoptimizer-ia32.cc @@ -145,9 +145,6 @@ void Deoptimizer::PatchCodeForDeoptimization(Isolate* isolate, Code* code) { Address reloc_end_address = reloc_info->address() + reloc_info->Size(); RelocInfoWriter reloc_info_writer(reloc_end_address, code_start_address); - // For each LLazyBailout instruction insert a call to the corresponding - // deoptimization entry. - // Since the call is a relative encoding, write new // reloc info. We do not need any of the existing reloc info because the // existing code will not be used again (we zap it in debug builds). @@ -155,9 +152,14 @@ void Deoptimizer::PatchCodeForDeoptimization(Isolate* isolate, Code* code) { // Emit call to lazy deoptimization at all lazy deopt points. DeoptimizationInputData* deopt_data = DeoptimizationInputData::cast(code->deoptimization_data()); + SharedFunctionInfo* shared = + SharedFunctionInfo::cast(deopt_data->SharedFunctionInfo()); + shared->EvictFromOptimizedCodeMap(code, "deoptimized code"); #ifdef DEBUG Address prev_call_address = NULL; #endif + // For each LLazyBailout instruction insert a call to the corresponding + // deoptimization entry. for (int i = 0; i < deopt_data->DeoptCount(); i++) { if (deopt_data->Pc(i)->value() == -1) continue; // Patch lazy deoptimization entry. diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc index 1658d71..5f03146 100644 --- a/src/ia32/lithium-codegen-ia32.cc +++ b/src/ia32/lithium-codegen-ia32.cc @@ -1180,6 +1180,13 @@ void LCodeGen::PopulateDeoptimizationData(Handle code) { data->SetTranslationByteArray(*translations); data->SetInlinedFunctionCount(Smi::FromInt(inlined_function_count_)); data->SetOptimizationId(Smi::FromInt(info_->optimization_id())); + if (info_->IsOptimizing()) { + // Reference to shared function info does not change between phases. + AllowDeferredHandleDereference allow_handle_dereference; + data->SetSharedFunctionInfo(*info_->shared_info()); + } else { + data->SetSharedFunctionInfo(Smi::FromInt(0)); + } Handle literals = factory()->NewFixedArray(deoptimization_literals_.length(), TENURED); diff --git a/src/mips/deoptimizer-mips.cc b/src/mips/deoptimizer-mips.cc index 9d7dbc6..d66acb0 100644 --- a/src/mips/deoptimizer-mips.cc +++ b/src/mips/deoptimizer-mips.cc @@ -69,13 +69,16 @@ void Deoptimizer::PatchCodeForDeoptimization(Isolate* isolate, Code* code) { } } - // For each LLazyBailout instruction insert a call to the corresponding - // deoptimization entry. DeoptimizationInputData* deopt_data = DeoptimizationInputData::cast(code->deoptimization_data()); + SharedFunctionInfo* shared = + SharedFunctionInfo::cast(deopt_data->SharedFunctionInfo()); + shared->EvictFromOptimizedCodeMap(code, "deoptimized code"); #ifdef DEBUG Address prev_call_address = NULL; #endif + // For each LLazyBailout instruction insert a call to the corresponding + // deoptimization entry. for (int i = 0; i < deopt_data->DeoptCount(); i++) { if (deopt_data->Pc(i)->value() == -1) continue; Address call_address = code_start_address + deopt_data->Pc(i)->value(); diff --git a/src/mips/lithium-codegen-mips.cc b/src/mips/lithium-codegen-mips.cc index edf8b95..30d7efd 100644 --- a/src/mips/lithium-codegen-mips.cc +++ b/src/mips/lithium-codegen-mips.cc @@ -861,6 +861,13 @@ void LCodeGen::PopulateDeoptimizationData(Handle code) { data->SetTranslationByteArray(*translations); data->SetInlinedFunctionCount(Smi::FromInt(inlined_function_count_)); data->SetOptimizationId(Smi::FromInt(info_->optimization_id())); + if (info_->IsOptimizing()) { + // Reference to shared function info does not change between phases. + AllowDeferredHandleDereference allow_handle_dereference; + data->SetSharedFunctionInfo(*info_->shared_info()); + } else { + data->SetSharedFunctionInfo(Smi::FromInt(0)); + } Handle literals = factory()->NewFixedArray(deoptimization_literals_.length(), TENURED); diff --git a/src/objects-inl.h b/src/objects-inl.h index 15d9b37..991c6c6 100644 --- a/src/objects-inl.h +++ b/src/objects-inl.h @@ -5437,8 +5437,6 @@ void JSFunction::ReplaceCode(Code* code) { if (was_optimized && !is_optimized) { // TODO(titzer): linear in the number of optimized functions; fix! context()->native_context()->RemoveOptimizedFunction(this); - shared()->EvictFromOptimizedCodeMap(context()->native_context(), - "Removing optimized code"); } } diff --git a/src/objects.cc b/src/objects.cc index 11c8ffc..aa6e280 100644 --- a/src/objects.cc +++ b/src/objects.cc @@ -9631,46 +9631,42 @@ void SharedFunctionInfo::ClearOptimizedCodeMap() { } -void SharedFunctionInfo::EvictFromOptimizedCodeMap(Context* context, +void SharedFunctionInfo::EvictFromOptimizedCodeMap(Code* optimized_code, const char* reason) { - ASSERT(context->IsNativeContext()); if (optimized_code_map()->IsSmi()) return; + int i; + bool removed_entry = false; FixedArray* code_map = FixedArray::cast(optimized_code_map()); - int dst = kEntriesStart; - int length = code_map->length(); - for (int src = kEntriesStart; src < length; src += kEntryLength) { - Context* context_key = Context::cast(code_map->get(src)); - ASSERT(context->IsNativeContext()); - if (context_key == context) { + for (i = kEntriesStart; i < code_map->length(); i += kEntryLength) { + ASSERT(code_map->get(i)->IsNativeContext()); + if (Code::cast(code_map->get(i + 1)) == optimized_code) { if (FLAG_trace_opt) { PrintF("[evicting entry from optimizing code map (%s) for ", reason); ShortPrint(); - BailoutId osr(Smi::cast(code_map->get(src + kOsrAstIdOffset))->value()); - if (osr.IsNone()) { - PrintF("]\n"); - } else { - PrintF(" (osr ast id %d)]\n", osr.ToInt()); - } + PrintF("]\n"); } - continue; - } - if (dst != src) { - code_map->set(dst + kContextOffset, - code_map->get(src + kContextOffset)); - code_map->set(dst + kCachedCodeOffset, - code_map->get(src + kCachedCodeOffset)); - code_map->set(dst + kLiteralsOffset, - code_map->get(src + kLiteralsOffset)); - code_map->set(dst + kOsrAstIdOffset, - code_map->get(src + kOsrAstIdOffset)); + removed_entry = true; + break; } - dst += kEntryLength; } - if (dst != length) { + while (i < (code_map->length() - kEntryLength)) { + code_map->set(i + kContextOffset, + code_map->get(i + kContextOffset + kEntryLength)); + code_map->set(i + kCachedCodeOffset, + code_map->get(i + kCachedCodeOffset + kEntryLength)); + code_map->set(i + kLiteralsOffset, + code_map->get(i + kLiteralsOffset + kEntryLength)); + code_map->set(i + kOsrAstIdOffset, + code_map->get(i + kOsrAstIdOffset + kEntryLength)); + i += kEntryLength; + } + if (removed_entry) { // Always trim even when array is cleared because of heap verifier. - RightTrimFixedArray(GetHeap(), code_map, length - dst); - if (code_map->length() == kEntriesStart) ClearOptimizedCodeMap(); + RightTrimFixedArray(GetHeap(), code_map, kEntryLength); + if (code_map->length() == kEntriesStart) { + ClearOptimizedCodeMap(); + } } } diff --git a/src/objects.h b/src/objects.h index 198ab36..f90662f 100644 --- a/src/objects.h +++ b/src/objects.h @@ -4997,7 +4997,8 @@ class DeoptimizationInputData: public FixedArray { static const int kOsrAstIdIndex = 3; static const int kOsrPcOffsetIndex = 4; static const int kOptimizationIdIndex = 5; - static const int kFirstDeoptEntryIndex = 6; + static const int kSharedFunctionInfoIndex = 6; + static const int kFirstDeoptEntryIndex = 7; // Offsets of deopt entry elements relative to the start of the entry. static const int kAstIdRawOffset = 0; @@ -5021,6 +5022,7 @@ class DeoptimizationInputData: public FixedArray { DEFINE_ELEMENT_ACCESSORS(OsrAstId, Smi) DEFINE_ELEMENT_ACCESSORS(OsrPcOffset, Smi) DEFINE_ELEMENT_ACCESSORS(OptimizationId, Smi) + DEFINE_ELEMENT_ACCESSORS(SharedFunctionInfo, Object) #undef DEFINE_ELEMENT_ACCESSORS @@ -6662,9 +6664,8 @@ class SharedFunctionInfo: public HeapObject { // Clear optimized code map. void ClearOptimizedCodeMap(); - // Removed code objects associated to the given native context from - // the optimized code map. - void EvictFromOptimizedCodeMap(Context* context, const char* reason); + // Removed a specific optimized code object from the optimized code map. + void EvictFromOptimizedCodeMap(Code* optimized_code, const char* reason); // Trims the optimized code map after entries have been removed. void TrimOptimizedCodeMap(int shrink_by); diff --git a/src/runtime.cc b/src/runtime.cc index 61eb8ad..fefe1e4 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -8523,6 +8523,10 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_NotifyDeoptimized) { PrintF("]\n"); } function->ReplaceCode(function->shared()->code()); + // Evict optimized code for this function from the cache so that it + // doesn't get used for new closures. + function->shared()->EvictFromOptimizedCodeMap(*optimized_code, + "notify deoptimized"); } } else { // TODO(titzer): we should probably do DeoptimizeCodeList(code) @@ -8530,10 +8534,6 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_NotifyDeoptimized) { // If there is an index by shared function info, all the better. Deoptimizer::DeoptimizeFunction(*function); } - // Evict optimized code for this function from the cache so that it doesn't - // get used for new closures. - function->shared()->EvictFromOptimizedCodeMap( - function->context()->native_context(), "notify deoptimized"); return isolate->heap()->undefined_value(); } diff --git a/src/x64/deoptimizer-x64.cc b/src/x64/deoptimizer-x64.cc index 705da52..0b6791e 100644 --- a/src/x64/deoptimizer-x64.cc +++ b/src/x64/deoptimizer-x64.cc @@ -83,6 +83,12 @@ void Deoptimizer::PatchCodeForDeoptimization(Isolate* isolate, Code* code) { #endif DeoptimizationInputData* deopt_data = DeoptimizationInputData::cast(code->deoptimization_data()); + SharedFunctionInfo* shared = + SharedFunctionInfo::cast(deopt_data->SharedFunctionInfo()); + shared->EvictFromOptimizedCodeMap(code, "deoptimized code"); + deopt_data->SetSharedFunctionInfo(Smi::FromInt(0)); + // For each LLazyBailout instruction insert a call to the corresponding + // deoptimization entry. for (int i = 0; i < deopt_data->DeoptCount(); i++) { if (deopt_data->Pc(i)->value() == -1) continue; // Position where Call will be patched in. diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc index 082a480..64366e2 100644 --- a/src/x64/lithium-codegen-x64.cc +++ b/src/x64/lithium-codegen-x64.cc @@ -793,6 +793,13 @@ void LCodeGen::PopulateDeoptimizationData(Handle code) { data->SetTranslationByteArray(*translations); data->SetInlinedFunctionCount(Smi::FromInt(inlined_function_count_)); data->SetOptimizationId(Smi::FromInt(info_->optimization_id())); + if (info_->IsOptimizing()) { + // Reference to shared function info does not change between phases. + AllowDeferredHandleDereference allow_handle_dereference; + data->SetSharedFunctionInfo(*info_->shared_info()); + } else { + data->SetSharedFunctionInfo(Smi::FromInt(0)); + } Handle literals = factory()->NewFixedArray(deoptimization_literals_.length(), TENURED); diff --git a/test/cctest/test-heap.cc b/test/cctest/test-heap.cc index f8757e1..b3058f7 100644 --- a/test/cctest/test-heap.cc +++ b/test/cctest/test-heap.cc @@ -3036,6 +3036,11 @@ void ReleaseStackTraceDataTest(const char* source, const char* accessor) { TEST(ReleaseStackTraceData) { + if (i::FLAG_always_opt) { + // TODO(ulan): Remove this once the memory leak via code_next_link is fixed. + // See: https://codereview.chromium.org/181833004/ + return; + } FLAG_use_ic = false; // ICs retain objects. FLAG_concurrent_recompilation = false; CcTest::InitializeVM(); diff --git a/test/mjsunit/regress/regress-343609.js b/test/mjsunit/regress/regress-343609.js new file mode 100644 index 0000000..5205ca1 --- /dev/null +++ b/test/mjsunit/regress/regress-343609.js @@ -0,0 +1,66 @@ +// Copyright 2014 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --allow-natives-syntax --block-concurrent-recompilation +// Flags: --no-concurrent-osr --expose-gc + +function Ctor() { + this.a = 1; +} + +function get_closure() { + return function add_field(obj) { + obj.c = 3; + obj.a = obj.a + obj.c; + return obj.a; + } +} +function get_closure2() { + return function cc(obj) { + obj.c = 3; + obj.a = obj.a + obj.c; + } +} + +function dummy() { + (function () { + var o = {c: 10}; + var f1 = get_closure2(); + f1(o); + f1(o); + %OptimizeFunctionOnNextCall(f1); + f1(o); + })(); +} + +var o = new Ctor(); +function opt() { + (function () { + var f1 = get_closure(); + f1(new Ctor()); + f1(new Ctor()); + %OptimizeFunctionOnNextCall(f1); + f1(o); + })(); +} + +// Optimize add_field and install its code in optimized code cache. +opt(); +opt(); +opt(); + +// Optimize dummy function to remove the add_field from head of optimized +// function list in the context. +dummy(); +dummy(); + +// Kill add_field in new space GC. +for(var i = 0; i < 3; i++) gc(true); + +// Trigger deopt. +o.c = 2.2; + +// Fetch optimized code of add_field from cache and crash. +var f2 = get_closure(); +f2(new Ctor()); -- 2.7.4