Remove redundant variable descriptors from optimized code.

Remove redundant variable descriptors from optimized code.

The unoptimized code already contains a copy of these, so
we don't need to store them in two places.

R=hausner@google.com

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

[Florian Schneider, 2015-04-14 14:24:46]

fschneider@google.com changed reviewers:
+ hausner@google.com

[hausner, 2015-04-14 15:44:30]

Two questions:

1) are the stack slots and context indices for variables guaranteed to be the
same in optimized and unoptimized code? If so, then the variable descriptors
should probably be attached to the function object, not the code object. (I
attached it to the code object because I was told that the info depends on the
generated code, e.g. for inlining, optimizations etc.)

2) Does this CL work when we collect unoptimized code? How about when we
re-compile unoptimized code after it got collected?

[srdjan, 2015-04-14 16:06:13]

On 2015/04/14 15:44:30, hausner wrote:
> Two questions:
> 
> 1) are the stack slots and context indices for variables guaranteed to be the
> same in optimized and unoptimized code? If so, then the variable descriptors
> should probably be attached to the function object, not the code object. (I
> attached it to the code object because I was told that the info depends on the
> generated code, e.g. for inlining, optimizations etc.)

The stack slots etc are not in the same place in optimized and unoptimized
frame, however I believe we always map optimized frames to unoptimized ones
before accessing locals, i.e., only unoptimized code var descriptors end up
being used. Is that correct?

Storing var descriptors in the function object is possible, but I think not
advisable: deferred loading can change those and we do not need to hang on them
after unoptimized code is collected.

> 
> 2) Does this CL work when we collect unoptimized code? How about when we
> re-compile unoptimized code after it got collected?

[srdjan, 2015-04-14 16:53:01]

srdjan@google.com changed reviewers:
+ srdjan@google.com

[srdjan, 2015-04-14 16:53:01]

https://codereview.chromium.org/1077793003/diff/1/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1077793003/diff/1/runtime/vm/debugger.cc#newc...
runtime/vm/debugger.cc:464: ?
Code::Handle(function().unoptimized_code()).var_descriptors()
unoptimized_code may not exist, you may need to trigger compilation here, isn't
it?

[Florian Schneider, 2015-04-15 09:38:21]

On 2015/04/14 15:44:30, hausner wrote:
> Two questions:
> 
> 1) are the stack slots and context indices for variables guaranteed to be the
> same in optimized and unoptimized code? If so, then the variable descriptors
> should probably be attached to the function object, not the code object. (I
> attached it to the code object because I was told that the info depends on the
> generated code, e.g. for inlining, optimizations etc.)
> 
> 2) Does this CL work when we collect unoptimized code? How about when we
> re-compile unoptimized code after it got collected?

1. Srdjan's comment is correct. The stack indices don't matter for optimized
frames
because they only have a meaning in unoptimized code. Optimized frames use the
deoptimization
 info to read local variable values.

2. I could not trigger a failure - even when collecting unoptimized code on
every GC,
but calling EnsureUnoptimizedCode is probably necessary here.

[Florian Schneider, 2015-04-15 10:07:07]

https://codereview.chromium.org/1077793003/diff/1/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1077793003/diff/1/runtime/vm/debugger.cc#newc...
runtime/vm/debugger.cc:464: ?
Code::Handle(function().unoptimized_code()).var_descriptors()
On 2015/04/14 16:53:01, srdjan wrote:
> unoptimized_code may not exist, you may need to trigger compilation here,
isn't
> it?

Done.

https://codereview.chromium.org/1077793003/diff/1/runtime/vm/debugger.cc#newc...
runtime/vm/debugger.cc:659: // TODO(hausner): Handle captured variables.
This TODO seems obsolete: captured variables are already handled in VariableAt.

[hausner, 2015-04-15 16:11:29]

LGTM.

I still have a bad gut feeling about having to recompile functions as a side
effect of collecting a stack trace. The variable descriptors are needed to find
the saved context and the context level of each activation frame.

Instead of recompiling unoptimized code, could the var descriptors be shared
between unoptimized and optimized code?

https://codereview.chromium.org/1077793003/diff/1/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1077793003/diff/1/runtime/vm/debugger.cc#newc...
runtime/vm/debugger.cc:659: // TODO(hausner): Handle captured variables.
On 2015/04/15 10:07:07, Florian Schneider wrote:
> This TODO seems obsolete: captured variables are already handled in
VariableAt.

Correct, this comment is no longer true. We handle context variables in
ActivationFrame::VariableAt().

[srdjan, 2015-04-15 16:25:45]

https://codereview.chromium.org/1077793003/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1077793003/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:468: : code().var_descriptors();
Could this be just :
var_descriptors = Code::Handle(function().unoptimized_code()).var_descriptors(),
no need to check if code is optimized. 

We could store var_descriptors into function instead of unoptimized-code, but
that would mean larger footprint (var_descriptors alive as long as function is).

[Florian Schneider, 2015-04-16 11:37:19]

https://codereview.chromium.org/1077793003/diff/20001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1077793003/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:468: : code().var_descriptors();
On 2015/04/15 16:25:45, srdjan wrote:
> Could this be just :
> var_descriptors =
Code::Handle(function().unoptimized_code()).var_descriptors(),
> no need to check if code is optimized. 
> 
> We could store var_descriptors into function instead of unoptimized-code, but
> that would mean larger footprint (var_descriptors alive as long as function
is).

Yes. I'll simplify it.

I think it is fine to have it on unoptimized code. And since they are only
needed for variable inspection, normal stack traces should not be affected
performance-wise.

[Florian Schneider, 2015-04-16 11:44:38]

On 2015/04/15 16:11:29, hausner wrote:
> LGTM.
> 
> I still have a bad gut feeling about having to recompile functions as a side
> effect of collecting a stack trace. The variable descriptors are needed to
find
> the saved context and the context level of each activation frame.
> 

In CollectDartFrame the saved context is retrieved eagerly, but it seems to me
that
it is not needed until we actually need to inspect a variable. I think
GetSavedCurrentContext
can be performed lazily as well. I don't see any other uses of the saved context
and 
the context level except for variable inspection, or did I miss something?

[Florian Schneider, 2015-04-16 13:08:02]

Matthias PTAL at latest patch: GetSavedCurrentContext now lazily computes the
context and caches it in the ActivationFrame, instead of doing it eagerly as
part of CollectDartFrame.

Since it may potentially trigger recompilation of the unoptimized code, only
perform it when actually needed (i.e. for variable inspection) - thus avoiding
it when just collecting a stack trace.

[hausner, 2015-04-16 16:27:20]

Now my concern is that each time a current context is needed, we allocate a
handle.

In practice, I think each time a debugger stack trace is collected, the
variables will be inspected as well.

LGTM, either this version or the one I already LGTM'ed.

https://codereview.chromium.org/1077793003/diff/60001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1077793003/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:521: RawContext*
ActivationFrame::GetSavedCurrentContext() {
It would be nice if this function could return an alias to the ctx_ handle
field, or if there was another way to avoid having the caller of this function
allocate a handle each time.

https://codereview.chromium.org/1077793003/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:522: if (!ctx_.IsNull()) return ctx_.raw();
What happens for functions that don't have a context? In that case, ctx_ remains
null and we go thorugh this loop each time GetSavedCurrentContext is called.

Is there always a current context variable for each function now that it is
stored in a stack slot instead of a dedicated register?

https://codereview.chromium.org/1077793003/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:754: const Context& ctx =
Context::Handle(GetSavedCurrentContext());
VaraibleAt is called in a loop for each variable in each activation frame.  It
would be nice if you didn't have to allocate a new handle each time we find a
captured variable. (There is an implicit, somewhat expensive operation to get to
the current thread and zone to allocate the handle).

[Florian Schneider, 2015-04-17 09:11:47]

Committed patchset #5 (id:80001) manually as r45228 (presubmit successful).

[Florian Schneider, 2015-04-17 09:12:07]

https://codereview.chromium.org/1077793003/diff/60001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1077793003/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:521: RawContext*
ActivationFrame::GetSavedCurrentContext() {
On 2015/04/16 16:27:20, hausner wrote:
> It would be nice if this function could return an alias to the ctx_ handle
> field, or if there was another way to avoid having the caller of this function
> allocate a handle each time.

Done.

https://codereview.chromium.org/1077793003/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:522: if (!ctx_.IsNull()) return ctx_.raw();
On 2015/04/16 16:27:20, hausner wrote:
> What happens for functions that don't have a context? In that case, ctx_
remains
> null and we go thorugh this loop each time GetSavedCurrentContext is called.
> 
> Is there always a current context variable for each function now that it is
> stored in a stack slot instead of a dedicated register?

Yes, the context should never be null for any Dart function. If there is none,
it will be the canonical empty context.

https://codereview.chromium.org/1077793003/diff/60001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:754: const Context& ctx =
Context::Handle(GetSavedCurrentContext());
On 2015/04/16 16:27:20, hausner wrote:
> VaraibleAt is called in a loop for each variable in each activation frame.  It
> would be nice if you didn't have to allocate a new handle each time we find a
> captured variable. (There is an implicit, somewhat expensive operation to get
to
> the current thread and zone to allocate the handle).

Done.