Compress deopt instructions in memory using variable length encoding.

Compress deopt instructions in memory using variable length encoding.

Remove DeoptInfo type entirely and store compressed instructions in the TypedData.

BUG=
R=fschneider@google.com, iposva@google.com

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

[Vyacheslav Egorov (Google), 2015-04-07 21:29:37]

vegorov@google.com changed reviewers:
+ fschneider@google.com, iposva@google.com, srdjan@google.com

[Vyacheslav Egorov (Google), 2015-04-07 21:29:38]

Please take a look. 

After compiling HelloWorldHtml once with dart2js on x64 we had 1.2mb live /
2.4mb total DeoptInfo objects. With this we have 283kb live / 599kb total. 

After warming up for 4 iterations I observe similar factor (4.2mb vs. 1mb on
x64, 2.0mb vs 745mb on the ia32).

[Ivan Posva, 2015-04-08 07:12:13]

LGTM with some comments.

-Ivan

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (left):

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:719: // suffix.
Is this saving unnecessary given the compression? How much did suffix reuse
contribute to savings in the original, non-compressed layout?

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:1197: if (suffix_length > 1) {
bool use_suffix = suffix_length > 1;

and use this instead of (suffix_length > 1) at multiple places.

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

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11192: UnpackInto(table, packed, unpacked, kMaxInt32);
It is not really obvious why kMaxInt32 is being passed here. Please add a
comment that we don't need a limit here as we want to get the whole stream and
not just a suffix.

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12063: const Array& table =
Array::Handle(deopt_info_array());
Please add a TODO here to consider removing the table of triplets, where one of
the entries is another table (the TypeData containing the deopt info) with a
single consolidated TypedData object.

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (left):

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:1247: // Contains an array of deoptimization commands,
e.g., move a specific register
Thanks!

[Florian Schneider, 2015-04-08 08:50:04]

lgtm

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

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11194: const intptr_t length = unpacked->length();
Add a reverse method to GrowableArray?

[Vyacheslav Egorov (Google), 2015-04-08 13:07:39]

Thanks. Landing.

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (left):

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:719: // suffix.
On 2015/04/08 07:12:12, Ivan Posva wrote:
> Is this saving unnecessary given the compression? How much did suffix reuse
> contribute to savings in the original, non-compressed layout?

Suffix compression gives us 30-40% saving on top - according to my measurements.

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/deopt_instru...
File runtime/vm/deopt_instructions.cc (right):

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/deopt_instru...
runtime/vm/deopt_instructions.cc:1197: if (suffix_length > 1) {
On 2015/04/08 07:12:12, Ivan Posva wrote:
> bool use_suffix = suffix_length > 1;
> 
> and use this instead of (suffix_length > 1) at multiple places.

Done.

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

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11192: UnpackInto(table, packed, unpacked, kMaxInt32);
On 2015/04/08 07:12:13, Ivan Posva wrote:
> It is not really obvious why kMaxInt32 is being passed here. Please add a
> comment that we don't need a limit here as we want to get the whole stream and
> not just a suffix.

Done.

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:11194: const intptr_t length = unpacked->length();
On 2015/04/08 08:50:04, Florian Schneider wrote:
> Add a reverse method to GrowableArray?

Done.

https://codereview.chromium.org/1054393003/diff/40001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12063: const Array& table =
Array::Handle(deopt_info_array());
On 2015/04/08 07:12:13, Ivan Posva wrote:
> Please add a TODO here to consider removing the table of triplets, where one
of
> the entries is another table (the TypeData containing the deopt info) with a
> single consolidated TypedData object.

Done.

Though this structure needs random access, so I doubt it will be considerably
more compact than the current structure (though we would save some small
percent).

I realized that on the architectures with constant pool we can get rid of this
table entirely by doing the following:

1. deopt info for eager deopts shoudld be inlined into the call site for the
deopt stub (it will be just passed as a parameter to it).

2. deopt info for lazy deopts should be placed into the constant pool right
after the constant pool entry that corresponds to the call target.

Whether this will give any savings or not is a question though.

[Vyacheslav Egorov (Google), 2015-04-08 13:09:00]

Committed patchset #4 (id:60001) manually as 44966 (presubmit successful).