From 6f3169e57194932ba96df9192417fedd537f8b88 Mon Sep 17 00:00:00 2001 From: "titzer@chromium.org" Date: Thu, 22 Aug 2013 13:03:40 +0000 Subject: [PATCH] Fix deoptimization bug, where recursive call can frighten and confuse the unwitting, simple, poor caveman that is Runtime_NotifyDeoptimized. BUG=274164 R=mstarzinger@chromium.org Review URL: https://codereview.chromium.org/23201016 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@16273 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/deoptimizer.h | 4 +- src/runtime.cc | 79 ++++++++----------- test/mjsunit/compiler/regress-shared-deopt.js | 65 +++++++++++++++ 3 files changed, 102 insertions(+), 46 deletions(-) create mode 100644 test/mjsunit/compiler/regress-shared-deopt.js diff --git a/src/deoptimizer.h b/src/deoptimizer.h index b6e4667a20..e5afd1ae67 100644 --- a/src/deoptimizer.h +++ b/src/deoptimizer.h @@ -166,7 +166,9 @@ class Deoptimizer : public Malloced { int output_count() const { return output_count_; } - Code::Kind compiled_code_kind() const { return compiled_code_->kind(); } + Handle function() const { return Handle(function_); } + Handle compiled_code() const { return Handle(compiled_code_); } + BailoutType bailout_type() const { return bailout_type_; } // Number of created JS frames. Not all created frames are necessarily JS. int jsframe_count() const { return jsframe_count_; } diff --git a/src/runtime.cc b/src/runtime.cc index 6311f4b0cb..da0229429d 100644 --- a/src/runtime.cc +++ b/src/runtime.cc @@ -8292,26 +8292,24 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_InstallRecompiledCode) { class ActivationsFinder : public ThreadVisitor { public: - explicit ActivationsFinder(JSFunction* function) - : function_(function), has_activations_(false) {} + Code* code_; + bool has_code_activations_; + + explicit ActivationsFinder(Code* code) + : code_(code), + has_code_activations_(false) { } void VisitThread(Isolate* isolate, ThreadLocalTop* top) { - if (has_activations_) return; - - for (JavaScriptFrameIterator it(isolate, top); !it.done(); it.Advance()) { - JavaScriptFrame* frame = it.frame(); - if (frame->is_optimized() && frame->function() == function_) { - has_activations_ = true; - return; - } - } + JavaScriptFrameIterator it(isolate, top); + VisitFrames(&it); } - bool has_activations() { return has_activations_; } - - private: - JSFunction* function_; - bool has_activations_; + void VisitFrames(JavaScriptFrameIterator* it) { + for (; !it->done(); it->Advance()) { + JavaScriptFrame* frame = it->frame(); + if (code_->contains(frame->pc())) has_code_activations_ = true; + } + } }; @@ -8334,7 +8332,11 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_NotifyDeoptimized) { Deoptimizer* deoptimizer = Deoptimizer::Grab(isolate); ASSERT(AllowHeapAllocation::IsAllowed()); - ASSERT(deoptimizer->compiled_code_kind() == Code::OPTIMIZED_FUNCTION); + Handle function = deoptimizer->function(); + Handle optimized_code = deoptimizer->compiled_code(); + + ASSERT(optimized_code->kind() == Code::OPTIMIZED_FUNCTION); + ASSERT(type == deoptimizer->bailout_type()); // Make sure to materialize objects before causing any allocation. JavaScriptFrameIterator it(isolate); @@ -8343,10 +8345,6 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_NotifyDeoptimized) { JavaScriptFrame* frame = it.frame(); RUNTIME_ASSERT(frame->function()->IsJSFunction()); - Handle function(frame->function(), isolate); - Handle optimized_code(function->code()); - RUNTIME_ASSERT((type != Deoptimizer::EAGER && - type != Deoptimizer::SOFT) || function->IsOptimized()); // Avoid doing too much work when running with --always-opt and keep // the optimized code around. @@ -8354,33 +8352,24 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_NotifyDeoptimized) { return isolate->heap()->undefined_value(); } - // Find other optimized activations of the function or functions that - // share the same optimized code. - bool has_other_activations = false; - while (!it.done()) { - JavaScriptFrame* frame = it.frame(); - JSFunction* other_function = frame->function(); - if (frame->is_optimized() && other_function->code() == function->code()) { - has_other_activations = true; - break; - } - it.Advance(); - } + // Search for other activations of the same function and code. + ActivationsFinder activations_finder(*optimized_code); + activations_finder.VisitFrames(&it); + isolate->thread_manager()->IterateArchivedThreads(&activations_finder); - if (!has_other_activations) { - ActivationsFinder activations_finder(*function); - isolate->thread_manager()->IterateArchivedThreads(&activations_finder); - has_other_activations = activations_finder.has_activations(); - } - - if (!has_other_activations) { - if (FLAG_trace_deopt) { - PrintF("[removing optimized code for: "); - function->PrintName(); - PrintF("]\n"); + if (!activations_finder.has_code_activations_) { + if (function->code() == *optimized_code) { + if (FLAG_trace_deopt) { + PrintF("[removing optimized code for: "); + function->PrintName(); + PrintF("]\n"); + } + function->ReplaceCode(function->shared()->code()); } - function->ReplaceCode(function->shared()->code()); } else { + // TODO(titzer): we should probably do DeoptimizeCodeList(code) + // unconditionally if the code is not already marked for deoptimization. + // 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 diff --git a/test/mjsunit/compiler/regress-shared-deopt.js b/test/mjsunit/compiler/regress-shared-deopt.js new file mode 100644 index 0000000000..669e0e2f1d --- /dev/null +++ b/test/mjsunit/compiler/regress-shared-deopt.js @@ -0,0 +1,65 @@ +// Copyright 2013 the V8 project authors. All rights reserved. +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following +// disclaimer in the documentation and/or other materials provided +// with the distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived +// from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Flags: --allow-natives-syntax + +var soft = false; + +// disable optimization of this global +soft = true; +soft = false; +soft = true; +soft = false; + +function test() { + var f4 = makeF(4); + var f5 = makeF(5); + + function makeF(i) { + return function f(x) { + if (x == 0) return i; + if (i == 4) if (soft) print("wahoo" + i); + return f4(x - 1); + } + } + + f4(9); + f4(11); + %OptimizeFunctionOnNextCall(f4); + f4(12); + + f5(9); + f5(11); + %OptimizeFunctionOnNextCall(f5); + f5(12); + + soft = true; + f4(1); + f5(9); +} + +test();