Encode any deoptimizer entry in serialized data.

Encode any deoptimizer entry in serialized data.

This removes kDeoptTableSerializeEntryCount heuristic constant.

Review-Url: https://codereview.chromium.org/2790573002
Cr-Commit-Position: refs/heads/master@{#44379}
Committed: https://chromium.googlesource.com/v8/v8/+/ddb67ec9da9945b49046127cd65cd28035ff11e4

[Slava Chigrin, 2017-03-30 12:13:47]

Description was changed from

==========
Encode any deoptimizer entry in serialized data.

This removes kDeoptTableSerializeEntryCount heuristic constant.
==========

to

==========
Encode any deoptimizer entry in serialized data.

This removes kDeoptTableSerializeEntryCount heuristic constant.
==========

[Slava Chigrin, 2017-03-30 12:13:47]

vchigrin@yandex-team.ru changed reviewers:
+ yangguo@chromium.org

[Slava Chigrin, 2017-03-30 12:18:19]

Here is our approach to serializing Deoptimizer entries. I did not add new
constants to serializer/deserializer codes since considered it code space
"precious" and want to avoid spending free codes for this.

But if you think it is better to add new constants there rather then introducing
bit flags for external address ids, I will re-design patch.

[Yang, 2017-03-31 06:56:10]

On 2017/03/30 12:18:19, Slava Chigrin wrote:
> Here is our approach to serializing Deoptimizer entries. I did not add new
> constants to serializer/deserializer codes since considered it code space
> "precious" and want to avoid spending free codes for this.
> 
> But if you think it is better to add new constants there rather then
introducing
> bit flags for external address ids, I will re-design patch.

Let's use a new serializer byte code for deoptimization entries. That way, we
don't have to store how many deoptimization entries we need upfront, and can
simply get them on-demand from the Deserializer::GetDeoptimizationEntry. We can
use a byte after the bytecode to store bailout type, and an int after that to
store the ID.

[Yang, 2017-03-31 06:58:16]

https://codereview.chromium.org/2790573002/diff/1/src/external-reference-table.h
File src/external-reference-table.h (right):

https://codereview.chromium.org/2790573002/diff/1/src/external-reference-tabl...
src/external-reference-table.h:41: static const uint32_t kSoftDeoptFlag = 3 <<
27;
We can have at most 16k entries. See Deoptimizer::kMaxNumberOfEntries

With this CL, I think we can also remove Deoptimizer::kMinNumberOfEntries

https://codereview.chromium.org/2790573002/diff/1/src/isolate.cc
File src/isolate.cc (right):

https://codereview.chromium.org/2790573002/diff/1/src/isolate.cc#newcode2813
src/isolate.cc:2813: this, bailout_type, des->max_deopt_entry_id(bailout_type));
Can't we create deoptimization entries on demand inside the deserializer?

[Slava Chigrin, 2017-04-03 16:56:18]

Patchset #2 (id:20001) has been deleted

[Slava Chigrin, 2017-04-03 17:09:37]

Re-designed this CL, now special bytecodes are used for referring Deoptimizer
entries. Also entries are created during deserialization on-demand. It was a bit
tricky since entries can be referred both as kFromCode and in kPlain way. But
seems enum Where does not have space to place new item in it withput making
conflict with anything. Since elements of this enum can be used both in plain
way and with kFromCode mask. So I've added two separate codes for that. If you
think some other approach is better - please, tell me.

Unfortunatelly, this CL does not eliminates need of
Deoptimizer::kMinNumberOfEntries - that constant used only here
https://cs.chromium.org/chromium/src/v8/src/deoptimizer.cc?l=2295
not for serialization, but rather for deciding how many entries to create at
client request. Without it that code hangs since initial entry_count is 0.

[Yang, 2017-04-04 07:59:21]

looking great. some comments.

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
File src/snapshot/deserializer.cc (right):

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
src/snapshot/deserializer.cc:740: reinterpret_cast<Address>(current) + skip);
Can we cast to intptr_t like everywhere else with skips?

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
src/snapshot/deserializer.cc:744: HandleScope scope(isolate);
why do we need a handlescope here?

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer.cc
File src/snapshot/serializer.cc (right):

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
src/snapshot/serializer.cc:347: sink_->Put(kDeoptimizerEntryPlain,
"DeoptimizationEntry");
please use brackets with if-else. You could also use a ternary operator for the
first parameter.

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
src/snapshot/serializer.cc:636: if (TryEncodeDeoptimizationEntry(kPlain, target,
skip)) {
I'd prefer something like

if (!TryEncode...) {
...
}
bytes_processed_so_far += ...

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
src/snapshot/serializer.cc:652: bytes_processed_so_far_ +=
rinfo->target_address_size();
same here

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
src/snapshot/serializer.cc:692: bytes_processed_so_far_ +=
rinfo->target_address_size();
same here.

[Slava Chigrin, 2017-04-04 10:12:24]

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
File src/snapshot/deserializer.cc (right):

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
src/snapshot/deserializer.cc:740: reinterpret_cast<Address>(current) + skip);
On 2017/04/04 07:59:21, Yang wrote:
> Can we cast to intptr_t like everywhere else with skips?

Done.

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
src/snapshot/deserializer.cc:744: HandleScope scope(isolate);
On 2017/04/04 07:59:21, Yang wrote:
> why do we need a handlescope here?

Deoptimizer::EnsureCodeForDeoptimizationEntry creates MacroAssembler object here

https://cs.chromium.org/chromium/src/v8/src/deoptimizer.cc?q=Deoptimizer::Ens...

and MacroAssembler constructor creates some handles here
https://cs.chromium.org/chromium/src/v8/src/x64/macro-assembler-x64.cc?l=37

so without HandleScope it does not work

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer.cc
File src/snapshot/serializer.cc (right):

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
src/snapshot/serializer.cc:347: sink_->Put(kDeoptimizerEntryPlain,
"DeoptimizationEntry");
On 2017/04/04 07:59:21, Yang wrote:
> please use brackets with if-else. You could also use a ternary operator for
the
> first parameter.

Done.

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
src/snapshot/serializer.cc:636: if (TryEncodeDeoptimizationEntry(kPlain, target,
skip)) {
On 2017/04/04 07:59:21, Yang wrote:
> I'd prefer something like
> 
> if (!TryEncode...) {
> ...
> }
> bytes_processed_so_far += ...

Done.

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
src/snapshot/serializer.cc:652: bytes_processed_so_far_ +=
rinfo->target_address_size();
On 2017/04/04 07:59:21, Yang wrote:
> same here

Done.

https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
src/snapshot/serializer.cc:692: bytes_processed_so_far_ +=
rinfo->target_address_size();
On 2017/04/04 07:59:21, Yang wrote:
> same here.

Done.

[Yang, 2017-04-04 13:44:31]

On 2017/04/04 10:12:24, Slava Chigrin wrote:
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
> File src/snapshot/deserializer.cc (right):
> 
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
> src/snapshot/deserializer.cc:740: reinterpret_cast<Address>(current) + skip);
> On 2017/04/04 07:59:21, Yang wrote:
> > Can we cast to intptr_t like everywhere else with skips?
> 
> Done.
> 
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
> src/snapshot/deserializer.cc:744: HandleScope scope(isolate);
> On 2017/04/04 07:59:21, Yang wrote:
> > why do we need a handlescope here?
> 
> Deoptimizer::EnsureCodeForDeoptimizationEntry creates MacroAssembler object
here
> 
>
https://cs.chromium.org/chromium/src/v8/src/deoptimizer.cc?q=Deoptimizer::Ens...
> 
> and MacroAssembler constructor creates some handles here
> https://cs.chromium.org/chromium/src/v8/src/x64/macro-assembler-x64.cc?l=37
> 
> so without HandleScope it does not work
> 
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer.cc
> File src/snapshot/serializer.cc (right):
> 
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
> src/snapshot/serializer.cc:347: sink_->Put(kDeoptimizerEntryPlain,
> "DeoptimizationEntry");
> On 2017/04/04 07:59:21, Yang wrote:
> > please use brackets with if-else. You could also use a ternary operator for
> the
> > first parameter.
> 
> Done.
> 
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
> src/snapshot/serializer.cc:636: if (TryEncodeDeoptimizationEntry(kPlain,
target,
> skip)) {
> On 2017/04/04 07:59:21, Yang wrote:
> > I'd prefer something like
> > 
> > if (!TryEncode...) {
> > ...
> > }
> > bytes_processed_so_far += ...
> 
> Done.
> 
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
> src/snapshot/serializer.cc:652: bytes_processed_so_far_ +=
> rinfo->target_address_size();
> On 2017/04/04 07:59:21, Yang wrote:
> > same here
> 
> Done.
> 
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
> src/snapshot/serializer.cc:692: bytes_processed_so_far_ +=
> rinfo->target_address_size();
> On 2017/04/04 07:59:21, Yang wrote:
> > same here.
> 
> Done.

LGTM. Thanks!

[Slava Chigrin, 2017-04-04 13:45:37]

The CQ bit was checked by vchigrin@yandex-team.ru

[commit-bot: I haz the power, 2017-04-04 13:45:43]

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

[Slava Chigrin, 2017-04-04 13:46:55]

On 2017/04/04 13:44:31, Yang wrote:
> On 2017/04/04 10:12:24, Slava Chigrin wrote:
> >
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
> > File src/snapshot/deserializer.cc (right):
> > 
> >
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
> > src/snapshot/deserializer.cc:740: reinterpret_cast<Address>(current) +
skip);
> > On 2017/04/04 07:59:21, Yang wrote:
> > > Can we cast to intptr_t like everywhere else with skips?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/deserializ...
> > src/snapshot/deserializer.cc:744: HandleScope scope(isolate);
> > On 2017/04/04 07:59:21, Yang wrote:
> > > why do we need a handlescope here?
> > 
> > Deoptimizer::EnsureCodeForDeoptimizationEntry creates MacroAssembler object
> here
> > 
> >
>
https://cs.chromium.org/chromium/src/v8/src/deoptimizer.cc?q=Deoptimizer::Ens...
> > 
> > and MacroAssembler constructor creates some handles here
> > https://cs.chromium.org/chromium/src/v8/src/x64/macro-assembler-x64.cc?l=37
> > 
> > so without HandleScope it does not work
> > 
> >
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer.cc
> > File src/snapshot/serializer.cc (right):
> > 
> >
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
> > src/snapshot/serializer.cc:347: sink_->Put(kDeoptimizerEntryPlain,
> > "DeoptimizationEntry");
> > On 2017/04/04 07:59:21, Yang wrote:
> > > please use brackets with if-else. You could also use a ternary operator
for
> > the
> > > first parameter.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
> > src/snapshot/serializer.cc:636: if (TryEncodeDeoptimizationEntry(kPlain,
> target,
> > skip)) {
> > On 2017/04/04 07:59:21, Yang wrote:
> > > I'd prefer something like
> > > 
> > > if (!TryEncode...) {
> > > ...
> > > }
> > > bytes_processed_so_far += ...
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
> > src/snapshot/serializer.cc:652: bytes_processed_so_far_ +=
> > rinfo->target_address_size();
> > On 2017/04/04 07:59:21, Yang wrote:
> > > same here
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2790573002/diff/40001/src/snapshot/serializer...
> > src/snapshot/serializer.cc:692: bytes_processed_so_far_ +=
> > rinfo->target_address_size();
> > On 2017/04/04 07:59:21, Yang wrote:
> > > same here.
> > 
> > Done.
> 
> LGTM. Thanks!

Thank you too for reviewing and accepting this patch!

[commit-bot: I haz the power, 2017-04-04 14:25:58]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491313537225330,
"parent_rev": "21f064fcdceb162524ab1bc5ed2b8ff6a803c7b4", "commit_rev":
"ddb67ec9da9945b49046127cd65cd28035ff11e4"}

[commit-bot: I haz the power, 2017-04-04 14:26:04]

Description was changed from

==========
Encode any deoptimizer entry in serialized data.

This removes kDeoptTableSerializeEntryCount heuristic constant.
==========

to

==========
Encode any deoptimizer entry in serialized data.

This removes kDeoptTableSerializeEntryCount heuristic constant.

Review-Url: https://codereview.chromium.org/2790573002
Cr-Commit-Position: refs/heads/master@{#44379}
Committed:
https://chromium.googlesource.com/v8/v8/+/ddb67ec9da9945b49046127cd65cd28035f...
==========

[commit-bot: I haz the power, 2017-04-04 14:26:05]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/ddb67ec9da9945b49046127cd65cd28035f...