Fix deoptimization bug, where recursive call can frighten and confuse the unwitting, simple, poor c…

src/runtime.cc

 // Copyright 2012 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
@@ skipping 8274 matching lines @@
   ASSERT(args.length() == 1);
   CONVERT_ARG_HANDLE_CHECKED(JSFunction, function, 0);
   ASSERT(V8::UseCrankshaft() && FLAG_parallel_recompilation);
   isolate->optimizing_compiler_thread()->InstallOptimizedFunctions();
   return function->code();
 }
 
 
 class ActivationsFinder : public ThreadVisitor {
  public:
-  explicit ActivationsFinder(JSFunction* function)
-      : function_(function), has_activations_(false) {}
+  JSFunction* function_;
+  Code* code_;
+  bool has_function_activations_;
+  bool has_code_activations_;
+
+  ActivationsFinder(JSFunction* function, Code* code)
+    : function_(function),
+      code_(code),
+      has_function_activations_(false),
+      has_code_activations_(false) { }
 
   void VisitThread(Isolate* isolate, ThreadLocalTop* top) {
-    if (has_activations_) return;
+    JavaScriptFrameIterator it(isolate, top);
+    VisitFrames(&it);
+  }
 
-    for (JavaScriptFrameIterator it(isolate, top); !it.done(); it.Advance()) {
-      JavaScriptFrame* frame = it.frame();
-      if (frame->is_optimized() && frame->function() == function_) {
-        has_activations_ = true;
-        return;
-      }
+  void VisitFrames(JavaScriptFrameIterator* it) {
+    for (; !it->done(); it->Advance()) {
+      JavaScriptFrame* frame = it->frame();
+      if (frame->function() == function_) has_function_activations_ = true;

[Michael Starzinger, 2013/08/21 16:20:28]

The "has_function_activations" field seems to be unused.

[titzer, 2013/08/21 16:26:29]

Correct, twers for debuggin'

+      if (code_->contains(frame->pc())) has_code_activations_ = true;
     }
   }
-
-  bool has_activations() { return has_activations_; }
-
- private:
-  JSFunction* function_;
-  bool has_activations_;
 };
 
 
 RUNTIME_FUNCTION(MaybeObject*, Runtime_NotifyStubFailure) {
   HandleScope scope(isolate);
   ASSERT(args.length() == 0);
   Deoptimizer* deoptimizer = Deoptimizer::Grab(isolate);
   ASSERT(AllowHeapAllocation::IsAllowed());
   delete deoptimizer;
   return isolate->heap()->undefined_value();
 }
 
 
 RUNTIME_FUNCTION(MaybeObject*, Runtime_NotifyDeoptimized) {
   HandleScope scope(isolate);
   ASSERT(args.length() == 1);
   RUNTIME_ASSERT(args[0]->IsSmi());
   Deoptimizer::BailoutType type =
       static_cast<Deoptimizer::BailoutType>(args.smi_at(0));
   Deoptimizer* deoptimizer = Deoptimizer::Grab(isolate);
   ASSERT(AllowHeapAllocation::IsAllowed());
 
-  ASSERT(deoptimizer->compiled_code_kind() == Code::OPTIMIZED_FUNCTION);
+  Handle<JSFunction> function(deoptimizer->function());
+  Handle<Code> optimized_code(deoptimizer->compiled_code());

[Toon Verwaest, 2013/08/21 15:33:17]

Could we also rename compiled_code() in the deoptimizer to optimized_code() to
make it obvious?

[Michael Starzinger, 2013/08/21 16:20:28]

nit: Use an assignment instead of the copy constructor ...

Handle<JSFunction> function = deoptimizer->function();
Handle<Code> optimized_code = deoptimizer->compiled_code();

[titzer, 2013/08/21 16:20:48]

Will have to do that rename in a later CL; wanted to keep this change small.

[titzer, 2013/08/21 16:26:29]

Done.

+
+  ASSERT(optimized_code->kind() == Code::OPTIMIZED_FUNCTION);
+  ASSERT(type == deoptimizer->bailout_type());
 
   // Make sure to materialize objects before causing any allocation.
   JavaScriptFrameIterator it(isolate);
   deoptimizer->MaterializeHeapObjects(&it);
   delete deoptimizer;
 
   JavaScriptFrame* frame = it.frame();
   RUNTIME_ASSERT(frame->function()->IsJSFunction());
-  Handle<JSFunction> function(frame->function(), isolate);
-  Handle<Code> 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.
   if (FLAG_always_opt || type == Deoptimizer::LAZY) {
     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();
-  }
+  // TODO(titzer): in the case of SOFT or EAGER deopt, we may want to allow
+  // the code to be reused a few times before unlinking it from the function.

[Toon Verwaest, 2013/08/21 15:33:17]

I'm not fully sure how this would work yet. Is it worthwhile putting this
comment in?

[titzer, 2013/08/21 16:20:48]

Removed.

 
-  if (!has_other_activations) {
-    ActivationsFinder activations_finder(*function);
-    isolate->thread_manager()->IterateArchivedThreads(&activations_finder);
-    has_other_activations = activations_finder.has_activations();
-  }
+  // Search for other activations of the same function and code.
+  ActivationsFinder activations_finder(*function, *optimized_code);
+  activations_finder.VisitFrames(&it);
+  isolate->thread_manager()->IterateArchivedThreads(&activations_finder);
 
-  if (!has_other_activations) {
+  if (!activations_finder.has_code_activations_) {
     if (FLAG_trace_deopt) {
       PrintF("[removing optimized code for: ");
       function->PrintName();
       PrintF("]\n");
     }
-    function->ReplaceCode(function->shared()->code());
+    if (function->code() == *optimized_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);
   }

[Toon Verwaest, 2013/08/21 15:33:17]

Can't we just unconditionally do

Deoptimizer::DeoptimizeFunction(*function);
if (function->code() == *optimized_code) {
  function->ReplaceCode(function->shared()->code());
}

It seems inconsistent that we globally DeoptimizeFunction if there are
activations; but if there are no activations, we just unlink the function's
optimized code (if it's there). What's wrong with just always unlinking it; and
notifying existing stackframes using DeoptimizeFunction?

(I'd probably replace the last 3 lines in my example with something like
function->DeoptimizeFrom(optimized_code); that internally checks what code it's
using, and replaces it with shared()->code() if it matches).

   // 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");
 
   return isolate->heap()->undefined_value();
 }
 
 
 RUNTIME_FUNCTION(MaybeObject*, Runtime_NotifyOSR) {
@@ skipping 6245 matching lines @@
     // Handle last resort GC and make sure to allow future allocations
     // to grow the heap without causing GCs (if possible).
     isolate->counters()->gc_last_resort_from_js()->Increment();
     isolate->heap()->CollectAllGarbage(Heap::kNoGCFlags,
                                        "Runtime::PerformGC");
   }
 }
 
 
 } }  // namespace v8::internal