[inspector] deoptimize function in Debug::PrepareStepIn

[inspector] deoptimize function in Debug::PrepareStepIn

We should deoptimize function to get DebugOnFunctionCall callbacks if current function is blackboxed.

BUG=v8:6171
R=yangguo@chromium.org

[kozy, 2017-03-29 16:41:21]

Yang, please take a look.
I found that Deoptimizer::DeoptimizeFunction is not enough and we need to call
PrepareFunctionForBreakPoints. It sounds dangerous to me because we'll regress
performance of PrepareStepIn for blackboxed functions a lot.

[kozy, 2017-03-30 19:01:50]

Description was changed from

==========
[inspector] deoptimized function in Debug::PrepareStepIn

We should deoptimize function to get DebugOnFunctionCall callbacks if current
function is blackboxed.

BUG=v8:6171
R=yangguo@chromium.org
==========

to

==========
[inspector] deoptimize function in Debug::PrepareStepIn

We should deoptimize function to get DebugOnFunctionCall callbacks if current
function is blackboxed.

BUG=v8:6171
R=yangguo@chromium.org
==========

[Yang, 2017-03-31 07:20:43]

https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc
File src/debug/debug.cc (right):

https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc#newc...
src/debug/debug.cc:900: EnsureDebugInfo(shared);
We don't want a DebugInfo, since we are not going to set break points in here.

What you want is
  Deoptimizer::DeoptimizeFunction(frame->function());

See Debug::PrepareStepOnThrow for example.

[kozy, 2017-03-31 07:51:29]

On 2017/03/31 07:20:43, Yang wrote:
> https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc
> File src/debug/debug.cc (right):
> 
>
https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc#newc...
> src/debug/debug.cc:900: EnsureDebugInfo(shared);
> We don't want a DebugInfo, since we are not going to set break points in here.
> 
> What you want is
>   Deoptimizer::DeoptimizeFunction(frame->function());
> 
> See Debug::PrepareStepOnThrow for example.

I tried and it didn't solve an issue. It looks like with always-opt we super
aggressively inline functions and deoptimize function does not process all
inlining correctly. I think that heap iteration in Prepare function call
actually resolve an issue.
But during solving another issue I find that some times when we inline function
to top level function we don't mark function correctly as inlined - it's reason
of crashing console.profile tests. I'll investigate this more.

[Yang, 2017-03-31 08:06:40]

On 2017/03/31 07:51:29, kozy wrote:
> On 2017/03/31 07:20:43, Yang wrote:
> > https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc
> > File src/debug/debug.cc (right):
> > 
> >
>
https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc#newc...
> > src/debug/debug.cc:900: EnsureDebugInfo(shared);
> > We don't want a DebugInfo, since we are not going to set break points in
here.
> > 
> > What you want is
> >   Deoptimizer::DeoptimizeFunction(frame->function());
> > 
> > See Debug::PrepareStepOnThrow for example.
> 
> I tried and it didn't solve an issue. It looks like with always-opt we super
> aggressively inline functions and deoptimize function does not process all
> inlining correctly. I think that heap iteration in Prepare function call
> actually resolve an issue.
> But during solving another issue I find that some times when we inline
function
> to top level function we don't mark function correctly as inlined - it's
reason
> of crashing console.profile tests. I'll investigate this more.

But here we should only be interested in whether this blackboxed function code
inlines anything. By deoptimizing it we should have made sure of that.

Heap interation in PrepareFunctionForBreakPoints makes sure all instances of the
function with the same shared function info is deoptimized.

I think what's going on is that the blackboxed function is marked for
optimization (it's code is set to the CompileOptimized builtin) at the point
were we attempt to deoptimize it. Deoptimization doesn't happen, but then we
optimize as soon as we enter it.

We should, in the debugger, replace all calls to Deoptimizer::DeoptimizeFunction
with a helper function that deoptimizes and also makes sure the function is not
marked for optimization.

[kozy, 2017-03-31 16:27:43]

On 2017/03/31 08:06:40, Yang wrote:
> On 2017/03/31 07:51:29, kozy wrote:
> > On 2017/03/31 07:20:43, Yang wrote:
> > > https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc
> > > File src/debug/debug.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc#newc...
> > > src/debug/debug.cc:900: EnsureDebugInfo(shared);
> > > We don't want a DebugInfo, since we are not going to set break points in
> here.
> > > 
> > > What you want is
> > >   Deoptimizer::DeoptimizeFunction(frame->function());
> > > 
> > > See Debug::PrepareStepOnThrow for example.
> > 
> > I tried and it didn't solve an issue. It looks like with always-opt we super
> > aggressively inline functions and deoptimize function does not process all
> > inlining correctly. I think that heap iteration in Prepare function call
> > actually resolve an issue.
> > But during solving another issue I find that some times when we inline
> function
> > to top level function we don't mark function correctly as inlined - it's
> reason
> > of crashing console.profile tests. I'll investigate this more.
> 
> But here we should only be interested in whether this blackboxed function code
> inlines anything. By deoptimizing it we should have made sure of that.
> 
> Heap interation in PrepareFunctionForBreakPoints makes sure all instances of
the
> function with the same shared function info is deoptimized.
> 
> I think what's going on is that the blackboxed function is marked for
> optimization (it's code is set to the CompileOptimized builtin) at the point
> were we attempt to deoptimize it. Deoptimization doesn't happen, but then we
> optimize as soon as we enter it.
> 
> We should, in the debugger, replace all calls to
Deoptimizer::DeoptimizeFunction
> with a helper function that deoptimizes and also makes sure the function is
not
> marked for optimization.

After debugging I found that this function is not yet compiled in
PrepapreStepIn. Then deoptimizer ignores this function and on first compilation
because of always-opt flag we compile function as optimized.  I think we can
ensure that function is compiled before deoptimization. We already do it in
Debug::PerformSideEffectCheck. And it means that we don't need to improve other
DeoptimizeFunction call sites since they get compiled function from current
stack trace.

Uploaded new patchset.

[kozy, 2017-03-31 16:27:52]

The CQ bit was checked by kozyatinskiy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 16:27:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yang, 2017-03-31 16:39:26]

On 2017/03/31 16:27:43, kozy wrote:
> On 2017/03/31 08:06:40, Yang wrote:
> > On 2017/03/31 07:51:29, kozy wrote:
> > > On 2017/03/31 07:20:43, Yang wrote:
> > > > https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc
> > > > File src/debug/debug.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc#newc...
> > > > src/debug/debug.cc:900: EnsureDebugInfo(shared);
> > > > We don't want a DebugInfo, since we are not going to set break points in
> > here.
> > > > 
> > > > What you want is
> > > >   Deoptimizer::DeoptimizeFunction(frame->function());
> > > > 
> > > > See Debug::PrepareStepOnThrow for example.
> > > 
> > > I tried and it didn't solve an issue. It looks like with always-opt we
super
> > > aggressively inline functions and deoptimize function does not process all
> > > inlining correctly. I think that heap iteration in Prepare function call
> > > actually resolve an issue.
> > > But during solving another issue I find that some times when we inline
> > function
> > > to top level function we don't mark function correctly as inlined - it's
> > reason
> > > of crashing console.profile tests. I'll investigate this more.
> > 
> > But here we should only be interested in whether this blackboxed function
code
> > inlines anything. By deoptimizing it we should have made sure of that.
> > 
> > Heap interation in PrepareFunctionForBreakPoints makes sure all instances of
> the
> > function with the same shared function info is deoptimized.
> > 
> > I think what's going on is that the blackboxed function is marked for
> > optimization (it's code is set to the CompileOptimized builtin) at the point
> > were we attempt to deoptimize it. Deoptimization doesn't happen, but then we
> > optimize as soon as we enter it.
> > 
> > We should, in the debugger, replace all calls to
> Deoptimizer::DeoptimizeFunction
> > with a helper function that deoptimizes and also makes sure the function is
> not
> > marked for optimization.
> 
> After debugging I found that this function is not yet compiled in
> PrepapreStepIn. Then deoptimizer ignores this function and on first
compilation
> because of always-opt flag we compile function as optimized.  I think we can
> ensure that function is compiled before deoptimization. We already do it in
> Debug::PerformSideEffectCheck. And it means that we don't need to improve
other
> DeoptimizeFunction call sites since they get compiled function from current
> stack trace.
> 
> Uploaded new patchset.

Not entirely happy with this. In PerformSideEffectCheck we need the compiled
bytecode to check it for side effects. Can you in this case only compile it if
always-opt is on?

[Yang, 2017-03-31 16:42:25]

On 2017/03/31 07:51:29, kozy wrote:
> On 2017/03/31 07:20:43, Yang wrote:
> > https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc
> > File src/debug/debug.cc (right):
> > 
> >
>
https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc#newc...
> > src/debug/debug.cc:900: EnsureDebugInfo(shared);
> > We don't want a DebugInfo, since we are not going to set break points in
here.
> > 
> > What you want is
> >   Deoptimizer::DeoptimizeFunction(frame->function());
> > 
> > See Debug::PrepareStepOnThrow for example.
> 
> I tried and it didn't solve an issue. It looks like with always-opt we super
> aggressively inline functions and deoptimize function does not process all
> inlining correctly. I think that heap iteration in Prepare function call
> actually resolve an issue.
> But during solving another issue I find that some times when we inline
function
> to top level function we don't mark function correctly as inlined - it's
reason
> of crashing console.profile tests. I'll investigate this more.

[Yang, 2017-03-31 16:51:49]

On 2017/03/31 16:42:25, Yang wrote:
> On 2017/03/31 07:51:29, kozy wrote:
> > On 2017/03/31 07:20:43, Yang wrote:
> > > https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc
> > > File src/debug/debug.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2782783003/diff/20001/src/debug/debug.cc#newc...
> > > src/debug/debug.cc:900: EnsureDebugInfo(shared);
> > > We don't want a DebugInfo, since we are not going to set break points in
> here.
> > > 
> > > What you want is
> > >   Deoptimizer::DeoptimizeFunction(frame->function());
> > > 
> > > See Debug::PrepareStepOnThrow for example.
> > 
> > I tried and it didn't solve an issue. It looks like with always-opt we super
> > aggressively inline functions and deoptimize function does not process all
> > inlining correctly. I think that heap iteration in Prepare function call
> > actually resolve an issue.
> > But during solving another issue I find that some times when we inline
> function
> > to top level function we don't mark function correctly as inlined - it's
> reason
> > of crashing console.profile tests. I'll investigate this more.

let's simply disable always-opt for this test.

[commit-bot: I haz the power, 2017-03-31 17:31:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 17:31:03]

Dry run: This issue passed the CQ dry run.