Refactor some deoptimization code.

Refactor some deoptimization code.

Primarily this change moves deoptimization state/code out of Isolate
and into DeoptContext (formerly DeoptimizationContext).  The lifetime
of DeoptizationContext changes to survive through the entire
deoptimization process.

Some minor renaming.  DeoptizationContext -> DeoptContext to make it
consistent with DeoptInstr and to save my wrists.

New files deferred_object.{cc,h} contain a bunch of the stuff lifted
from isolate.{cc,h}.

R=fschneider@google.com, srdjan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=28112

[turnidge, 2013-09-26 21:17:28]

Hi Srdjan,

These changes make it easier for me to reuse the deoptimization code in another
CL I'm working on (getting the debugger to inspect locals in optimized/inlined
frames).

Todd

https://codereview.chromium.org/24834002/diff/8001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/24834002/diff/8001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:1594: // TODO(turnidge): Why can't I move
CopySavedRegisters here?
Srdjan, any idea on this one?  When I move CopySavedRegisters down to this line
I start getting bus errors.  Presumably I'm messing up the saved registers
somehow in between here and above.

[srdjan, 2013-09-27 17:04:54]

lgtm

https://codereview.chromium.org/24834002/diff/8001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/24834002/diff/8001/runtime/vm/code_generator....
runtime/vm/code_generator.cc:1594: // TODO(turnidge): Why can't I move
CopySavedRegisters here?
On 2013/09/26 21:17:28, turnidge wrote:
> Srdjan, any idea on this one?  When I move CopySavedRegisters down to this
line
> I start getting bus errors.  Presumably I'm messing up the saved registers
> somehow in between here and above.

I do not understand why. CC-d Kevin and Florian they may help.

https://codereview.chromium.org/24834002/diff/16001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/24834002/diff/16001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1616: - 1;  // For fp.
extra space

https://codereview.chromium.org/24834002/diff/16001/runtime/vm/deopt_instruct...
File runtime/vm/deopt_instructions.h (right):

https://codereview.chromium.org/24834002/diff/16001/runtime/vm/deopt_instruct...
runtime/vm/deopt_instructions.h:30: ~DeoptContext();
virtual, just in case?

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/isolate.h#newc...
runtime/vm/isolate.h:347: void set_deopt_context(DeoptContext* value) {
deopt_context_ = value; }
Maybe assert here that you do not introduce a memory leak:
ASSERT(value == NULL || deopt_context_ == NULL)

[Florian Schneider, 2013-09-30 11:32:28]

dbc:

I find the the diff for the newly added files strange, as it has all the lines
from isolate.h/.cc on the left hand side shown as removed lines.

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1595:
deopt_context->SetSavedRegisters(fpu_registers, cpu_registers);
Wouldn't it work to pass fpu_registers and cpu_registers as arguments to the
DeoptContext constructor instead of initalizing them here?

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1596: CopyFrame(deopt_context, optimized_code,
*caller_frame);
It is quite confusing that the different members of DeoptContext computed here
are initialized in different places: in the constructor, in SetSavedRegisters,
SetFromFrame. Maybe you can do all this from the constructor?

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1735: delete deopt_context;
Maybe change the destructor of DeoptContext so that it deletes everything
attached to it including fpu_registers_, cpu_registers_, and from_frame_

[turnidge, 2013-09-30 17:31:36]

On 2013/09/30 11:32:28, Florian Schneider wrote:
> dbc:
> 
> I find the the diff for the newly added files strange, as it has all the lines
> from isolate.h/.cc on the left hand side shown as removed lines.

By creating the new files using "svn cp" instead of "svn add" I preserve the
edit history of the copied code.  Perhaps more importantly, this allows the
reviewer to track the edits (or errors) that I may have made copying the code. 
For example, you are able to see the addition of the TODO at line 74 of the new
file.  Without the "svn cp" the reviewer cannot track this easily.  For these
reasons, the "svn cp" is a good practice with this kind of refactor.

[turnidge, 2013-09-30 17:40:22]

Florian:

Thank you for your comments.  Please see my discussion below.

Srdjan:

I have done your edits.

Todd

https://codereview.chromium.org/24834002/diff/16001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/24834002/diff/16001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1616: - 1;  // For fp.
On 2013/09/27 17:04:54, srdjan wrote:
> extra space

Done.

https://codereview.chromium.org/24834002/diff/16001/runtime/vm/deopt_instruct...
File runtime/vm/deopt_instructions.h (right):

https://codereview.chromium.org/24834002/diff/16001/runtime/vm/deopt_instruct...
runtime/vm/deopt_instructions.h:30: ~DeoptContext();
On 2013/09/27 17:04:54, srdjan wrote:
> virtual, just in case?

Done.

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1595:
deopt_context->SetSavedRegisters(fpu_registers, cpu_registers);
On 2013/09/30 11:32:29, Florian Schneider wrote:
> Wouldn't it work to pass fpu_registers and cpu_registers as arguments to the
> DeoptContext constructor instead of initalizing them here?

See next reply for full discussion.

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1596: CopyFrame(deopt_context, optimized_code,
*caller_frame);
On 2013/09/30 11:32:29, Florian Schneider wrote:
> It is quite confusing that the different members of DeoptContext computed here
> are initialized in different places: in the constructor, in SetSavedRegisters,
> SetFromFrame. Maybe you can do all this from the constructor?

SetToFrame is done in the following runtime call, so I can't combine all
initialization in one place.

I could move regs + from stack initialization to the constructor, but that is
asymmetric with SetToFrame, which is an accessor.

Of note: I am also modifying this further in another change I am working on, so
the constructor arguments could change further to include DeoptInfo, etc.

How about:

1. Constructor is initialized with general info about the deopt: function,
reason, pc, or whatever.
2. Source arguments (regs + frame) are set by a single call: SetSources()?
3. Destination args set by a call to something like SetDest or SetTarget.

While I'm at it: I dislike the "To" and "From" names.  Functions like
GetFromPc() sound like they are getting something from a pc offset rather than
getting the "from" pc.  Since "from" is a preposition it leads readers down the
wrong path.  Any objections to changing these?

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1735: delete deopt_context;
On 2013/09/30 11:32:29, Florian Schneider wrote:
> Maybe change the destructor of DeoptContext so that it deletes everything
> attached to it including fpu_registers_, cpu_registers_, and from_frame_

I agree that it is unsightly to have this memory management here, but here's
some more information...

I have another change coming where from_frame_ isn't a copy -- it's the actual
stack.  This will be used by the debugging api to access variables from
optimized/inlined frames.  In that case, it would be wrong to deallocate
from_frame_.  Since there will be no consistent ownership story across both
uses, I chose to leave memory ownership outside of the class.

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/isolate.h#newc...
runtime/vm/isolate.h:347: void set_deopt_context(DeoptContext* value) {
deopt_context_ = value; }
On 2013/09/27 17:04:54, srdjan wrote:
> Maybe assert here that you do not introduce a memory leak:
> ASSERT(value == NULL || deopt_context_ == NULL)

Done.

[Ivan Posva, 2013-09-30 18:00:01]

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1735: delete deopt_context;
On 2013/09/30 17:40:22, turnidge wrote:
> On 2013/09/30 11:32:29, Florian Schneider wrote:
> > Maybe change the destructor of DeoptContext so that it deletes everything
> > attached to it including fpu_registers_, cpu_registers_, and from_frame_
> 
> I agree that it is unsightly to have this memory management here, but here's
> some more information...
> 
> I have another change coming where from_frame_ isn't a copy -- it's the actual
> stack.  This will be used by the debugging api to access variables from
> optimized/inlined frames.  In that case, it would be wrong to deallocate
> from_frame_.  Since there will be no consistent ownership story across both
> uses, I chose to leave memory ownership outside of the class.

Would it be possible to keep track whether these should be deleted in the
DeoptContext object and then have the destructor conditionally delete them?

[turnidge, 2013-09-30 19:35:10]

Okay.  Thanks for all of the comments, everyone.  PTAL.

I have introduced some api changes which I hope make Florian feel the code is
less confusing.  I have also moved some deallocation inside of the DeoptContext
destructor, which had the nice effect of allowing me to drop some ugly
accessors.

I have also changed all of the to/from usage in deopt_instructions.h/cc to
source/dest.  This clears up  functions like this...

intptr_t DeoptimizationContext::GetFromFp() const {
   48   return from_frame_[from_frame_size_ - 1 - num_args_ -
kParamEndSlotFromFp];
}

When FromFp means two different things at the same time.  This edit adds a bunch
of lines to the CL.

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1596: CopyFrame(deopt_context, optimized_code,
*caller_frame);
Okay, I have cleanup up the interface a bit here in the latest revision.  
Hopefully this is less confusing.

On 2013/09/30 17:40:22, turnidge wrote:
> On 2013/09/30 11:32:29, Florian Schneider wrote:
> > It is quite confusing that the different members of DeoptContext computed
here
> > are initialized in different places: in the constructor, in
SetSavedRegisters,
> > SetFromFrame. Maybe you can do all this from the constructor?
> 
> SetToFrame is done in the following runtime call, so I can't combine all
> initialization in one place.
> 
> I could move regs + from stack initialization to the constructor, but that is
> asymmetric with SetToFrame, which is an accessor.
> 
> Of note: I am also modifying this further in another change I am working on,
so
> the constructor arguments could change further to include DeoptInfo, etc.
> 
> How about:
> 
> 1. Constructor is initialized with general info about the deopt: function,
> reason, pc, or whatever.
> 2. Source arguments (regs + frame) are set by a single call: SetSources()?
> 3. Destination args set by a call to something like SetDest or SetTarget.
> 
> While I'm at it: I dislike the "To" and "From" names.  Functions like
> GetFromPc() sound like they are getting something from a pc offset rather than
> getting the "from" pc.  Since "from" is a preposition it leads readers down
the
> wrong path.  Any objections to changing these?

https://codereview.chromium.org/24834002/diff/24001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1735: delete deopt_context;
You and Florian are right here.  Changed.

On 2013/09/30 18:00:01, Ivan Posva wrote:
> On 2013/09/30 17:40:22, turnidge wrote:
> > On 2013/09/30 11:32:29, Florian Schneider wrote:
> > > Maybe change the destructor of DeoptContext so that it deletes everything
> > > attached to it including fpu_registers_, cpu_registers_, and from_frame_
> > 
> > I agree that it is unsightly to have this memory management here, but here's
> > some more information...
> > 
> > I have another change coming where from_frame_ isn't a copy -- it's the
actual
> > stack.  This will be used by the debugging api to access variables from
> > optimized/inlined frames.  In that case, it would be wrong to deallocate
> > from_frame_.  Since there will be no consistent ownership story across both
> > uses, I chose to leave memory ownership outside of the class.
> 
> Would it be possible to keep track whether these should be deleted in the
> DeoptContext object and then have the destructor conditionally delete them?

[Florian Schneider, 2013-10-01 16:44:26]

Code LGTM.

https://codereview.chromium.org/24834002/diff/36001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/24834002/diff/36001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1596: // TODO(turnidge): Why can't I move
CopySavedRegisters here?
It would be good to resolve this before landing. How did you reproduce the
segfault?

[turnidge, 2013-10-01 16:53:06]

https://codereview.chromium.org/24834002/diff/36001/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/24834002/diff/36001/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1596: // TODO(turnidge): Why can't I move
CopySavedRegisters here?
On 2013/10/01 16:44:27, Florian Schneider wrote:
> It would be good to resolve this before landing. How did you reproduce the
> segfault?

Before I was able to get a SIGBUS by reordering the code that copies the saved
registers.  I am no longer able to reproduce this failure in a number of
different reorderings fortunately/unfortunately.  Perhaps I had some other error
that caused the SIGBUS.  Or perhaps the refactor of CopySavedRegisters perturbed
something.

At this point I cannot reproduce the original problem at all, so I have put the
code into a more natural order and I will re-upload shortly.

[turnidge, 2013-10-01 19:34:20]

Committed patchset #7 manually as r28112 (presubmit successful).