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

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

Committed: https://code.google.com/p/v8/source/detail?r=16273

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

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc#newcode8341
src/runtime.cc:8341: Handle<Code> optimized_code(deoptimizer->compiled_code());
Could we also rename compiled_code() in the deoptimizer to optimized_code() to
make it obvious?

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc#newcode8361
src/runtime.cc:8361: // the code to be reused a few times before unlinking it
from the function.
I'm not fully sure how this would work yet. Is it worthwhile putting this
comment in?

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc#newcode8382
src/runtime.cc:8382: }
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).

https://codereview.chromium.org/23201016/diff/1/test/mjsunit/compiler/regress...
File test/mjsunit/compiler/regress-shared-deopt.js (right):

https://codereview.chromium.org/23201016/diff/1/test/mjsunit/compiler/regress...
test/mjsunit/compiler/regress-shared-deopt.js:38: function test() {
2-space indent.

[Michael Starzinger, 2013-08-21 16:20:27]

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc#newcode8314
src/runtime.cc:8314: if (frame->function() == function_)
has_function_activations_ = true;
The "has_function_activations" field seems to be unused.

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc#newcode8341
src/runtime.cc:8341: Handle<Code> optimized_code(deoptimizer->compiled_code());
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]

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc#newcode8341
src/runtime.cc:8341: Handle<Code> optimized_code(deoptimizer->compiled_code());
On 2013/08/21 15:33:17, Toon Verwaest wrote:
> Could we also rename compiled_code() in the deoptimizer to optimized_code() to
> make it obvious?

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

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc#newcode8361
src/runtime.cc:8361: // the code to be reused a few times before unlinking it
from the function.
On 2013/08/21 15:33:17, Toon Verwaest wrote:
> I'm not fully sure how this would work yet. Is it worthwhile putting this
> comment in?

Removed.

https://codereview.chromium.org/23201016/diff/1/test/mjsunit/compiler/regress...
File test/mjsunit/compiler/regress-shared-deopt.js (right):

https://codereview.chromium.org/23201016/diff/1/test/mjsunit/compiler/regress...
test/mjsunit/compiler/regress-shared-deopt.js:38: function test() {
On 2013/08/21 15:33:17, Toon Verwaest wrote:
> 2-space indent.

Done.

[titzer, 2013-08-21 16:26:28]

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc#newcode8314
src/runtime.cc:8314: if (frame->function() == function_)
has_function_activations_ = true;
On 2013/08/21 16:20:28, Michael Starzinger wrote:
> The "has_function_activations" field seems to be unused.

Correct, twers for debuggin'

https://codereview.chromium.org/23201016/diff/1/src/runtime.cc#newcode8341
src/runtime.cc:8341: Handle<Code> optimized_code(deoptimizer->compiled_code());
On 2013/08/21 16:20:28, Michael Starzinger wrote:
> nit: Use an assignment instead of the copy constructor ...
> 
> Handle<JSFunction> function = deoptimizer->function();
> Handle<Code> optimized_code = deoptimizer->compiled_code();

Done.

[Michael Starzinger, 2013-08-22 12:39:42]

LGTM with one final nit.

https://codereview.chromium.org/23201016/diff/2001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/23201016/diff/2001/src/runtime.cc#newcode8362
src/runtime.cc:8362: PrintF("[removing optimized code for: ");
The trace should be inside the code comparison done below. Otherwise the tracing
line is inconclusive.

[titzer, 2013-08-22 12:47:08]

On 2013/08/22 12:39:42, Michael Starzinger wrote:
> LGTM with one final nit.
> 
> https://codereview.chromium.org/23201016/diff/2001/src/runtime.cc
> File src/runtime.cc (right):
> 
> https://codereview.chromium.org/23201016/diff/2001/src/runtime.cc#newcode8362
> src/runtime.cc:8362: PrintF("[removing optimized code for: ");
> The trace should be inside the code comparison done below. Otherwise the
tracing
> line is inconclusive.

Done.

[titzer, 2013-08-22 13:03:49]

Committed patchset #4 manually as r16273.