VM: Fix checked mode crash (issue 13831).

VM: Fix checked mode crash (issue 13831).

The AST for static getters differs between parsing the first time, and
subsequent parsings. This leads to a mismatch in deoptimization-ids
between the optimized and the unoptimized code.

This CL avoids creating different ASTs for the same static getters. To allow
better inlining of these getters, the initialization expression is wrapped in a
hidden static initializer-function. As a result the size of such getters is
constant and does not depend on the initializer expression.

R=srdjan@google.com

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

[Florian Schneider, 2013-10-30 17:37:26]

https://codereview.chromium.org/51123003/diff/120001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (left):

https://codereview.chromium.org/51123003/diff/120001/runtime/vm/compiler.cc#o...
runtime/vm/compiler.cc:895: RawFunction::kImplicitStaticFinalGetter,
Unrelated change: I found it confusing to use this kind here. This function has
nothing to do with a static final getter.

https://codereview.chromium.org/51123003/diff/120001/runtime/vm/flow_graph_bu...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/51123003/diff/120001/runtime/vm/flow_graph_bu...
runtime/vm/flow_graph_builder.cc:3803: if (FLAG_print_scopes) {
Unrelated change: Moved scope debug printing out of platform-specific code to
here.

[srdjan, 2013-10-30 17:54:14]

LGTM. When do you plan to add the check that max-deopt-id is always the same in
unoptimized SSA?

https://codereview.chromium.org/51123003/diff/120001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/51123003/diff/120001/runtime/vm/object.cc#new...
runtime/vm/object.cc:5438: String::Concat(Symbols::InitPrefix(), field_name))));
Indent another 4 spaces

https://codereview.chromium.org/51123003/diff/120001/runtime/vm/object.cc#new...
runtime/vm/object.cc:5448: init_function.set_result_type(result_type);
Please add a comment describing the special settings.

[Florian Schneider, 2013-10-31 16:34:53]

I'll add the verification of the deopt-ids used in a separate CL.

https://codereview.chromium.org/51123003/diff/120001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/51123003/diff/120001/runtime/vm/object.cc#new...
runtime/vm/object.cc:5438: String::Concat(Symbols::InitPrefix(), field_name))));
On 2013/10/30 17:54:15, srdjan wrote:
> Indent another 4 spaces

Done.

https://codereview.chromium.org/51123003/diff/120001/runtime/vm/object.cc#new...
runtime/vm/object.cc:5448: init_function.set_result_type(result_type);
On 2013/10/30 17:54:15, srdjan wrote:
> Please add a comment describing the special settings.

Done.

[fschneider, 2013-10-31 16:50:08]

Committed patchset #4 manually as r29680 (presubmit successful).

[hausner, 2013-10-31 21:53:47]

DBC: I don't like this change because it wastes too much space. Function objects
are expensive (big), we don't want more of them in the snapshot and heap. (I
think Lars has already complained that there are too many of them.)

Instead of adding init:x functions to the class, I propose that instead the
getter calls out to the runtime. The VM then generates a function object on the
fly, compiles the expression and sets the field. The function object and
generated code will then be garbage collected. Essentially, it's the same thing
the debugger already does to evaluate expressions in the context of a class or
library.

I will file a bug to implement this.

https://codereview.chromium.org/51123003/diff/330001/runtime/vm/class_finaliz...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/51123003/diff/330001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:1226: const intptr_t initializer_pos =
field.token_pos() + 2;
The + 2 is incorrect. The token position is a byte offset into an array of
variable length objects. You are increasing by two bytes, not two tokens. The
field identifier token you skipping is not one byte long, so you are most likely
pointing into the middle of a token value with this calculated offset.

[hausner, 2013-10-31 22:17:47]

The bug number is 14696

[Florian Schneider, 2013-11-01 10:23:41]

I see the point with the size of the additional Function objects. On the other
hand I did not see a significant degradation in snapshot size (-0.462%) for this
CL.

https://codereview.chromium.org/51123003/diff/330001/runtime/vm/class_finaliz...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/51123003/diff/330001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:1226: const intptr_t initializer_pos =
field.token_pos() + 2;
On 2013/10/31 21:53:47, hausner wrote:
> The + 2 is incorrect. The token position is a byte offset into an array of
> variable length objects. You are increasing by two bytes, not two tokens. The
> field identifier token you skipping is not one byte long, so you are most
likely
> pointing into the middle of a token value with this calculated offset.

Oops, yes that will go wrong. I'll fix it. I think the cleanest way would be to
just not do the eager optimization with static getters in the parser that has to
be undone here. I'll check what the performance impact of this would be.

[hausner, 2013-11-01 16:02:29]

We discussed this here in MTV yesterday. First, most of the classes in the
snapshot are not parsed yet, so the newly introduced function objects are not
included in it. Second, we have a history of adding overhead by small increases,
until one day we find out that the snapshot/heap has become too big. Then we try
to solve the problem by adding more complexity, like delayed class parsing,
compressing the token stream, etc.

I think we found a good solution how to gain this space back by using a runtime
call, so we'll be fine.