Improve internal compiler API so that OSR code is never installed on function.

Improve internal compiler API so that OSR code is never installed on function.

This is forked off and a prerequisite for
https://codereview.chromium.org/2771013002/ which is itself a prerequisite for
a change to make fewer old-space allocations in the optimizing compiler, which
is in turn a fix for the performance regression in
https://codereview.chromium.org/2737303003

Fixes dart-lang/sdk#29160

R=vegorov@google.com
BUG=http://dartbug.com/29160

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

[Vyacheslav Egorov (Google), 2017-03-30 09:11:49]

Description was changed from

==========
Improve internal compiler API so that OSR code is never installed on function.

This is forked off and a prerequisite for
https://codereview.chromium.org/2771013002/ which is itself a prerequisite for
a change to make fewer old-space allocations in the optimizing compiler, which
is in turn a fix for the performance regression in
https://codereview.chromium.org/2737303003

R=vegorov@google.com
BUG=http://dartbug.com/29160
==========

to

==========
Improve internal compiler API so that OSR code is never installed on function.

This is forked off and a prerequisite for
https://codereview.chromium.org/2771013002/ which is itself a prerequisite for
a change to make fewer old-space allocations in the optimizing compiler, which
is in turn a fix for the performance regression in
https://codereview.chromium.org/2737303003

Fixes https://github.com/dart-lang/sdk/issues/29160.

R=vegorov@google.com
BUG=http://dartbug.com/29160
==========

[Vyacheslav Egorov (Google), 2017-03-30 09:12:45]

Description was changed from

==========
Improve internal compiler API so that OSR code is never installed on function.

This is forked off and a prerequisite for
https://codereview.chromium.org/2771013002/ which is itself a prerequisite for
a change to make fewer old-space allocations in the optimizing compiler, which
is in turn a fix for the performance regression in
https://codereview.chromium.org/2737303003

Fixes https://github.com/dart-lang/sdk/issues/29160.

R=vegorov@google.com
BUG=http://dartbug.com/29160
==========

to

==========
Improve internal compiler API so that OSR code is never installed on function.

This is forked off and a prerequisite for
https://codereview.chromium.org/2771013002/ which is itself a prerequisite for
a change to make fewer old-space allocations in the optimizing compiler, which
is in turn a fix for the performance regression in
https://codereview.chromium.org/2737303003

Fixes dart-lang/sdk#29160.

R=vegorov@google.com
BUG=http://dartbug.com/29160
==========

[Vyacheslav Egorov (Google), 2017-03-30 09:13:13]

Description was changed from

==========
Improve internal compiler API so that OSR code is never installed on function.

This is forked off and a prerequisite for
https://codereview.chromium.org/2771013002/ which is itself a prerequisite for
a change to make fewer old-space allocations in the optimizing compiler, which
is in turn a fix for the performance regression in
https://codereview.chromium.org/2737303003

Fixes dart-lang/sdk#29160.

R=vegorov@google.com
BUG=http://dartbug.com/29160
==========

to

==========
Improve internal compiler API so that OSR code is never installed on function.

This is forked off and a prerequisite for
https://codereview.chromium.org/2771013002/ which is itself a prerequisite for
a change to make fewer old-space allocations in the optimizing compiler, which
is in turn a fix for the performance regression in
https://codereview.chromium.org/2737303003

Fixes dart-lang/sdk#29160

R=vegorov@google.com
BUG=http://dartbug.com/29160
==========

[Vyacheslav Egorov (Google), 2017-03-30 09:58:53]

https://codereview.chromium.org/2781483005/diff/1/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/2781483005/diff/1/runtime/lib/mirrors.cc#newc...
runtime/lib/mirrors.cc:99: const Object& result =
There are a lot of places that do this. Maybe you need a helper?

Compile::EnsureCode(thread, func) {
  // ...  
}

notice that returning Null in such places is also an error - so maybe assert
that it does not happen. We expect to have code attached to the function after
this invocation.

Maybe even put something on the function itself:

Function::EnsureCode() {
 if (func.HasCode()) return;
 ...
}

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

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:525: RawObject*
CompileParsedFunctionHelper::FinalizeCompilation(
It looks like this function always returns a Code - so make it RawCode*

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:590: bool code_was_compiled = false;
Maybe you don't need this? 

How about just using code and setting it to `null` if validation failed.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:596: if (!is_osr) {
InstallOptimizedCode() also does set the owner on the code (in the AttachCode).

Who does that for the OSR code? Maybe this needs to be done manually.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:645: }
} else {
  code = Code::null();
}

and use !code.IsNull() everywhere instead of code_was_compiled.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:692: return Object::null();
here you then will be able to just return code.raw().

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:709: RawObject*
CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
This returns Code or null. So maybe this should be RawCode*?

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:732: Code& result = Code::ZoneHandle(zone);
this variable needs to be volatile because of setjmp(...).

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:1148: FinalizeCompilation(&assembler, &graph_compiler,
flow_graph);
This can become result = FinalizeCompilation() if you change FinalizeCompilation
to just return RawCode* - which I think it always does.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:1273: Object& result =
Object::Handle(helper.Compile(pipeline));
This probably needs to be Code because you expect it to be either Code object or
null.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:1316: ASSERT(!optimized);
I don't get this code. We are inside 

if (optimized) {
  ...
}

condition.

[erikcorry, 2017-03-30 12:26:34]

https://codereview.chromium.org/2781483005/diff/1/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/2781483005/diff/1/runtime/lib/mirrors.cc#newc...
runtime/lib/mirrors.cc:99: const Object& result =
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> There are a lot of places that do this. Maybe you need a helper?
> 
> Compile::EnsureCode(thread, func) {
>   // ...  
> }
> 
> notice that returning Null in such places is also an error - so maybe assert
> that it does not happen. We expect to have code attached to the function after
> this invocation.
> 
> Maybe even put something on the function itself:
> 
> Function::EnsureCode() {
>  if (func.HasCode()) return;
>  ...
> }

Done.

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

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:525: RawObject*
CompileParsedFunctionHelper::FinalizeCompilation(
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> It looks like this function always returns a Code - so make it RawCode*

Done.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:590: bool code_was_compiled = false;
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> Maybe you don't need this? 
> 
> How about just using code and setting it to `null` if validation failed.

Done.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:596: if (!is_osr) {
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> InstallOptimizedCode() also does set the owner on the code (in the
AttachCode).
> 
> Who does that for the OSR code? Maybe this needs to be done manually. 

That's done on line 545.  I added an assert.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:645: }
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> } else {
>   code = Code::null();
> }
> 
> and use !code.IsNull() everywhere instead of code_was_compiled.

Done.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:692: return Object::null();
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> here you then will be able to just return code.raw().

Done.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:709: RawObject*
CompileParsedFunctionHelper::Compile(CompilationPipeline* pipeline) {
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> This returns Code or null. So maybe this should be RawCode*?

Done.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:732: Code& result = Code::ZoneHandle(zone);
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> this variable needs to be volatile because of setjmp(...).

Yuck!

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:1148: FinalizeCompilation(&assembler, &graph_compiler,
flow_graph);
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> This can become result = FinalizeCompilation() if you change
FinalizeCompilation
> to just return RawCode* - which I think it always does.

Done.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:1273: Object& result =
Object::Handle(helper.Compile(pipeline));
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> This probably needs to be Code because you expect it to be either Code object
or
> null.

Done.

https://codereview.chromium.org/2781483005/diff/1/runtime/vm/compiler.cc#newc...
runtime/vm/compiler.cc:1316: ASSERT(!optimized);
On 2017/03/30 09:58:52, Vyacheslav Egorov (Google) wrote:
> I don't get this code. We are inside 
> 
> if (optimized) {
>   ...
> }
> 
> condition. 

No.  I added the assert exactly to clarify which "{" the "}" belonged to.

[Vyacheslav Egorov (Google), 2017-03-30 13:25:01]

lgtm

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

https://codereview.chromium.org/2781483005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:643: code ^= Code::null();
code = Code::null() should also work just fine.

https://codereview.chromium.org/2781483005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:1267: Code& result =
Code::Handle(helper.Compile(pipeline));
const Code&?

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

https://codereview.chromium.org/2781483005/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:2304: RawCode* EnsureHasCode() const;
Write a comment about what this function does.

[erikcorry, 2017-03-30 13:33:33]

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

https://codereview.chromium.org/2781483005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:643: code ^= Code::null();
On 2017/03/30 13:25:01, Vyacheslav Egorov (Google) wrote:
> code = Code::null() should also work just fine.

Done.

https://codereview.chromium.org/2781483005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:1267: Code& result =
Code::Handle(helper.Compile(pipeline));
On 2017/03/30 13:25:01, Vyacheslav Egorov (Google) wrote:
> const Code&?

Done.

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

https://codereview.chromium.org/2781483005/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:2304: RawCode* EnsureHasCode() const;
On 2017/03/30 13:25:01, Vyacheslav Egorov (Google) wrote:
> Write a comment about what this function does.

Done.

[erikcorry, 2017-03-30 13:33:34]

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

https://codereview.chromium.org/2781483005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:643: code ^= Code::null();
On 2017/03/30 13:25:01, Vyacheslav Egorov (Google) wrote:
> code = Code::null() should also work just fine.

Done.

https://codereview.chromium.org/2781483005/diff/20001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:1267: Code& result =
Code::Handle(helper.Compile(pipeline));
On 2017/03/30 13:25:01, Vyacheslav Egorov (Google) wrote:
> const Code&?

Done.

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

https://codereview.chromium.org/2781483005/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:2304: RawCode* EnsureHasCode() const;
On 2017/03/30 13:25:01, Vyacheslav Egorov (Google) wrote:
> Write a comment about what this function does.

Done.

[erikcorry, 2017-03-30 13:40:13]

Description was changed from

==========
Improve internal compiler API so that OSR code is never installed on function.

This is forked off and a prerequisite for
https://codereview.chromium.org/2771013002/ which is itself a prerequisite for
a change to make fewer old-space allocations in the optimizing compiler, which
is in turn a fix for the performance regression in
https://codereview.chromium.org/2737303003

Fixes dart-lang/sdk#29160

R=vegorov@google.com
BUG=http://dartbug.com/29160
==========

to

==========
Improve internal compiler API so that OSR code is never installed on function.

This is forked off and a prerequisite for
https://codereview.chromium.org/2771013002/ which is itself a prerequisite for
a change to make fewer old-space allocations in the optimizing compiler, which
is in turn a fix for the performance regression in
https://codereview.chromium.org/2737303003

Fixes dart-lang/sdk#29160

R=vegorov@google.com
BUG=http://dartbug.com/29160

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

[erikcorry, 2017-03-30 13:40:14]

Committed patchset #3 (id:40001) manually as
48607be33dbfff82c877f04646b4fae64c7ff91c (presubmit successful).