Remove default monomorphic check code from functions and stubs that do not need it.

Remove default monomorphic check code from functions and stubs that do not need it.

R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/cf52536d0902e7c6b5a3230c2d3fd286c1665735

[regis, 2016-10-31 16:13:23]

regis@google.com changed reviewers:
+ rmacnak@google.com

[regis, 2016-10-31 16:13:24]

Ryan,

The precomp buildbot is red currently, so I am not 100% sure this is working,
but you can have a look. No hurry, I am taking tomorrow off.

Thanks,
Regis

[rmacnak, 2016-11-01 00:50:17]

lgtm

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

https://codereview.chromium.org/2463083002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:14578: ASSERT(!is_optimized() || (pc_offset ==
Instructions::kUncheckedEntryOffset));
If we are missing a stack map, this must either be unoptimized code, or the
entry to an osr function. (In which case all stack slots are considered to have
tagged pointers.) So pc_offset == Instructions::kUncheckedEntryOffset
corresponds to  Instructions::UncheckedEntryPoint(...) in DRT_StackOverflow.

Given the entry isn't an fixed offset now, this should be pc_offset ==
unchecked_entry()) now.

Running with --verify-on-transition should hit this.

[rmacnak, 2016-11-01 00:55:30]

On 2016/11/01 00:50:17, rmacnak wrote:
> lgtm
> 
> https://codereview.chromium.org/2463083002/diff/1/runtime/vm/object.cc
> File runtime/vm/object.cc (right):
> 
>
https://codereview.chromium.org/2463083002/diff/1/runtime/vm/object.cc#newcod...
> runtime/vm/object.cc:14578: ASSERT(!is_optimized() || (pc_offset ==
> Instructions::kUncheckedEntryOffset));
> If we are missing a stack map, this must either be unoptimized code, or the
> entry to an osr function. (In which case all stack slots are considered to
have
> tagged pointers.) So pc_offset == Instructions::kUncheckedEntryOffset
> corresponds to  Instructions::UncheckedEntryPoint(...) in DRT_StackOverflow.
> 
> Given the entry isn't an fixed offset now, this should be pc_offset ==
> unchecked_entry()) now.
> 
> Running with --verify-on-transition should hit this.

Size change for Flutter gallery in product mode:

VMIsolate(CodeSize): 103622
Isolate(CodeSize): 1871991
ReadOnlyData(CodeSize): 1562320
Instructions(CodeSize): 5040656
Total(CodeSize): 8578589
=>
VMIsolate(CodeSize): 103615
Isolate(CodeSize): 1873067
ReadOnlyData(CodeSize): 1565544
Instructions(CodeSize): 4953536 (-1.7%)
Total(CodeSize): 8495762 (-0.97%)

[regis, 2016-11-02 15:45:35]

PTAL, thanks!

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

https://codereview.chromium.org/2463083002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:14578: ASSERT(!is_optimized() || (pc_offset ==
Instructions::kUncheckedEntryOffset));
On 2016/11/01 00:50:17, rmacnak wrote:
> If we are missing a stack map, this must either be unoptimized code, or the
> entry to an osr function. (In which case all stack slots are considered to
have
> tagged pointers.) So pc_offset == Instructions::kUncheckedEntryOffset
> corresponds to  Instructions::UncheckedEntryPoint(...) in DRT_StackOverflow.
> 
> Given the entry isn't an fixed offset now, this should be pc_offset ==
> unchecked_entry()) now.
> 
> Running with --verify-on-transition should hit this.

Thanks!

The flag indeed revealed the issue. I added your explanation as a comment and
fixed the assert.
Generation of a handle in Code::UncheckedEntryPoint() does not seem to cause an
issue, but I could revert the use of a handle, or simply check 3 different
possible offsets (not as strict):
  ASSERT(!is_optimized() ||
         (pc_offset == Instructions::kPayloadOffset) ||
         (pc_offset == Instructions::kUncheckedEntryOffset) ||
         (pc_offset == Instructions::kCheckedEntryOffset));

[regis, 2016-11-02 15:47:01]

On 2016/11/01 00:55:30, rmacnak wrote:
> On 2016/11/01 00:50:17, rmacnak wrote:
> > lgtm
> > 
> > https://codereview.chromium.org/2463083002/diff/1/runtime/vm/object.cc
> > File runtime/vm/object.cc (right):
> > 
> >
>
https://codereview.chromium.org/2463083002/diff/1/runtime/vm/object.cc#newcod...
> > runtime/vm/object.cc:14578: ASSERT(!is_optimized() || (pc_offset ==
> > Instructions::kUncheckedEntryOffset));
> > If we are missing a stack map, this must either be unoptimized code, or the
> > entry to an osr function. (In which case all stack slots are considered to
> have
> > tagged pointers.) So pc_offset == Instructions::kUncheckedEntryOffset
> > corresponds to  Instructions::UncheckedEntryPoint(...) in DRT_StackOverflow.
> > 
> > Given the entry isn't an fixed offset now, this should be pc_offset ==
> > unchecked_entry()) now.
> > 
> > Running with --verify-on-transition should hit this.
> 
> Size change for Flutter gallery in product mode:
> 
> VMIsolate(CodeSize): 103622
> Isolate(CodeSize): 1871991
> ReadOnlyData(CodeSize): 1562320
> Instructions(CodeSize): 5040656
> Total(CodeSize): 8578589
> =>
> VMIsolate(CodeSize): 103615
> Isolate(CodeSize): 1873067
> ReadOnlyData(CodeSize): 1565544
> Instructions(CodeSize): 4953536 (-1.7%)
> Total(CodeSize): 8495762 (-0.97%)

Not a huge gain. Thanks for measuring!

[Florian Schneider, 2016-11-02 15:49:01]

On 2016/11/02 15:47:01, regis wrote:
> On 2016/11/01 00:55:30, rmacnak wrote:
> > On 2016/11/01 00:50:17, rmacnak wrote:
> > > lgtm
> > > 
> > > https://codereview.chromium.org/2463083002/diff/1/runtime/vm/object.cc
> > > File runtime/vm/object.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2463083002/diff/1/runtime/vm/object.cc#newcod...
> > > runtime/vm/object.cc:14578: ASSERT(!is_optimized() || (pc_offset ==
> > > Instructions::kUncheckedEntryOffset));
> > > If we are missing a stack map, this must either be unoptimized code, or
the
> > > entry to an osr function. (In which case all stack slots are considered to
> > have
> > > tagged pointers.) So pc_offset == Instructions::kUncheckedEntryOffset
> > > corresponds to  Instructions::UncheckedEntryPoint(...) in
DRT_StackOverflow.
> > > 
> > > Given the entry isn't an fixed offset now, this should be pc_offset ==
> > > unchecked_entry()) now.
> > > 
> > > Running with --verify-on-transition should hit this.
> > 
> > Size change for Flutter gallery in product mode:
> > 
> > VMIsolate(CodeSize): 103622
> > Isolate(CodeSize): 1871991
> > ReadOnlyData(CodeSize): 1562320
> > Instructions(CodeSize): 5040656
> > Total(CodeSize): 8578589
> > =>
> > VMIsolate(CodeSize): 103615
> > Isolate(CodeSize): 1873067
> > ReadOnlyData(CodeSize): 1565544
> > Instructions(CodeSize): 4953536 (-1.7%)
> > Total(CodeSize): 8495762 (-0.97%)
> 
> Not a huge gain. Thanks for measuring!

Still, a percent here and there adds up!

[rmacnak, 2016-11-02 17:11:13]

lgtm

[regis, 2016-11-03 08:41:19]

Description was changed from

==========
Remove default monomorphic check code from functions and stubs that do not need
it.
==========

to

==========
Remove default monomorphic check code from functions and stubs that do not need
it.

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/cf52536d0902e7c6b5a3230c2d3fd286c1665735
==========

[regis, 2016-11-03 08:41:22]

Committed patchset #2 (id:20001) manually as
cf52536d0902e7c6b5a3230c2d3fd286c1665735 (presubmit successful).

[Vyacheslav Egorov (Google), 2016-11-10 13:30:48]

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

[Vyacheslav Egorov (Google), 2016-11-10 13:30:49]

DBC

These comments are motivated by the fact that we spent some time hunting crashes
in our code after rebasing against HEAD.

https://codereview.chromium.org/2463083002/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/2463083002/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:4143: intptr_t size() const { return abs(raw_ptr()->size_);
}
Please add a comment to RawInstructions::size_ that it can be negative in
raw_object.h

It's a very nice neat trick - but it's utterly confusing that size_ can be
negative, so this needs to be well documented.

A much more maintainable approach would be to rename size_ to size_and_flags_
and store flag using bitfields helpers.

https://codereview.chromium.org/2463083002/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:4183: if (instr->ptr()->size_ < 0) {
I suggest having helper:

static bool HasSingleEntryPoint(RawIntructions* instr);

and using it here and below (and above) instead of magical expression. 

This way if we decide to refactor how this sort of flags are stored - we would
not need to search all code base for size_ < 0, size_ > 0 comparisons.

[regis, 2016-11-10 19:11:42]

On 2016/11/10 13:30:49, Vyacheslav Egorov (Google) wrote:
> DBC
> 
> These comments are motivated by the fact that we spent some time hunting
crashes
> in our code after rebasing against HEAD.
> 
> https://codereview.chromium.org/2463083002/diff/20001/runtime/vm/object.h
> File runtime/vm/object.h (right):
> 
>
https://codereview.chromium.org/2463083002/diff/20001/runtime/vm/object.h#new...
> runtime/vm/object.h:4143: intptr_t size() const { return
abs(raw_ptr()->size_);
> }
> Please add a comment to RawInstructions::size_ that it can be negative in
> raw_object.h
> 
> It's a very nice neat trick - but it's utterly confusing that size_ can be
> negative, so this needs to be well documented.
> 
> A much more maintainable approach would be to rename size_ to size_and_flags_
> and store flag using bitfields helpers.
> 
>
https://codereview.chromium.org/2463083002/diff/20001/runtime/vm/object.h#new...
> runtime/vm/object.h:4183: if (instr->ptr()->size_ < 0) {
> I suggest having helper:
> 
> static bool HasSingleEntryPoint(RawIntructions* instr);
> 
> and using it here and below (and above) instead of magical expression. 
> 
> This way if we decide to refactor how this sort of flags are stored - we would
> not need to search all code base for size_ < 0, size_ > 0 comparisons.

Point well taken and sorry for the caused trouble.
I'll add comments and rename the field in a forthcoming cl.