Precompilation: Have instances calls load the entry point and Code object from the ic data array in…

Precompilation: Change instance call sequence to load the entry point and Code object from the ic data array instead of loading them indirectly from the Function object loaded from the same array.

Load the Code from the ic data array instead of the ObjectPool because we still need the Code object in the frame to find stackmaps / build stacktraces / build profile results.

Keep doing things the old way until all compilation is finished because evaluation of constants still needs functional lazy compilation and the compiler works with ICData that may contain as-yet-uncompiled functions. After compilation is finished, transform all the ICData from (cid, target function, count) to (cid, target code, entry point) and switch to the ICLookup stub that works with this representation.

Golem average +3.133% on ARM, +3.544% on x64

R=srdjan@google.com

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

[rmacnak, 2016-03-14 22:26:50]

Description was changed from

==========
Precompilation: Have instances calls load the entry point and Code object from
the ic data array instead of loading them indirectly from the Function object
loaded from the same array.
==========

to

==========
Precompilation: Change instance call sequence to load the entry point and Code
object from the ic data array instead of loading them indirectly from the
Function object loaded from the same array.

Load the Code from the ic data array instead of the ObjectPool because we still
need the Code object in the frame to find stackmaps / build stacktraces / build
profile results.

Keep doing things the old way until all compilation is finished because
evaluation of constants still needs functional lazy compilation and the compiler
works with ICData that may contain as-yet-uncompiled functions. After
compilation is finished, transform all the ICData from (cid, target function,
count) to (cid, target code, entry point) and switch to the ICLookup stub that
works with this representation.
==========

[rmacnak, 2016-03-15 17:06:43]

Description was changed from

==========
Precompilation: Change instance call sequence to load the entry point and Code
object from the ic data array instead of loading them indirectly from the
Function object loaded from the same array.

Load the Code from the ic data array instead of the ObjectPool because we still
need the Code object in the frame to find stackmaps / build stacktraces / build
profile results.

Keep doing things the old way until all compilation is finished because
evaluation of constants still needs functional lazy compilation and the compiler
works with ICData that may contain as-yet-uncompiled functions. After
compilation is finished, transform all the ICData from (cid, target function,
count) to (cid, target code, entry point) and switch to the ICLookup stub that
works with this representation.
==========

to

==========
Precompilation: Change instance call sequence to load the entry point and Code
object from the ic data array instead of loading them indirectly from the
Function object loaded from the same array.

Load the Code from the ic data array instead of the ObjectPool because we still
need the Code object in the frame to find stackmaps / build stacktraces / build
profile results.

Keep doing things the old way until all compilation is finished because
evaluation of constants still needs functional lazy compilation and the compiler
works with ICData that may contain as-yet-uncompiled functions. After
compilation is finished, transform all the ICData from (cid, target function,
count) to (cid, target code, entry point) and switch to the ICLookup stub that
works with this representation.

Golem average +3.133% on ARM, +3.544% on x64
==========

[rmacnak, 2016-03-15 17:07:22]

rmacnak@google.com changed reviewers:
+ fschneider@google.com, srdjan@google.com

[srdjan, 2016-03-15 19:15:49]

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

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12335: } else {
Add a comment, that this is for precompilation only (last phase)

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12338:
Smi::Handle(reinterpret_cast<RawSmi*>(code.EntryPoint()));
Add check that this is a valid Smi value.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12430: return
reinterpret_cast<RawObject*>(raw_data->ptr()->data()[data_pos]);
Do you need the cast? Isn't raw_data->ptr()->data()[data_pos] already RawObject*
?

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12466: data.SetAt(data_pos, value);
Maybe put this two functions after all count functions (GetCountAt,
AggregateCount)?

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:1659: void Precompiler::SwitchICCalls() {
Add brief comment explaining what is being changed/switched.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:1685: if (ic_.NumArgsTested() != 1) continue;
Please explain why only NumArgsTests == 1 is switched.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:1693:
reinterpret_cast<RawSmi*>(target_code_.EntryPoint());
Check that it is smi

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:1718: 
ASSERT that compilation is not allowed?

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:631: ICData& ic = ICData::Handle();
Get zone (thread->zone()) and use it for handle creation

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/stub_code_ar...
File runtime/vm/stub_code_arm.cc (right):

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm.cc:2113: void
StubCode::GenerateICLookupThroughFunctionStub(Assembler* assembler) {
Is anybody using this stub at all?

[Florian Schneider, 2016-03-16 15:45:48]

dbc

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/stub_code_ia...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:2087: void
StubCode::GenerateICLookupThroughFunctionStub(Assembler* assembler) {
Since this stub is not used on ia32, UNREACHABLE()?
Or can you omit the definition completely, so we'd get a linker error instead?

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:2092: void
StubCode::GenerateICLookupThroughCodeStub(Assembler* assembler) {
Ditto

[rmacnak, 2016-03-16 18:21:32]

PTAL

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

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12335: } else {
On 2016/03/15 19:15:48, srdjan wrote:
> Add a comment, that this is for precompilation only (last phase)

Done.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12338:
Smi::Handle(reinterpret_cast<RawSmi*>(code.EntryPoint()));
On 2016/03/15 19:15:48, srdjan wrote:
> Add check that this is a valid Smi value.

Added Smi::FromAlignedAddress.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12430: return
reinterpret_cast<RawObject*>(raw_data->ptr()->data()[data_pos]);
On 2016/03/15 19:15:48, srdjan wrote:
> Do you need the cast? Isn't raw_data->ptr()->data()[data_pos] already
RawObject*
> ?

No, removed.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12466: data.SetAt(data_pos, value);
On 2016/03/15 19:15:48, srdjan wrote:
> Maybe put this two functions after all count functions (GetCountAt,
> AggregateCount)?

Done.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/precompiler.cc
File runtime/vm/precompiler.cc (right):

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:1659: void Precompiler::SwitchICCalls() {
On 2016/03/15 19:15:48, srdjan wrote:
> Add brief comment explaining what is being changed/switched.

Done.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:1685: if (ic_.NumArgsTested() != 1) continue;
On 2016/03/15 19:15:48, srdjan wrote:
> Please explain why only NumArgsTests == 1 is switched.

Done.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:1693:
reinterpret_cast<RawSmi*>(target_code_.EntryPoint());
On 2016/03/15 19:15:48, srdjan wrote:
> Check that it is smi

Smi::FromAlignedAddress

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/precompiler....
runtime/vm/precompiler.cc:1718: 
On 2016/03/15 19:15:48, srdjan wrote:
> ASSERT that compilation is not allowed?

Done.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/snapshot.cc
File runtime/vm/snapshot.cc (right):

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/snapshot.cc#...
runtime/vm/snapshot.cc:631: ICData& ic = ICData::Handle();
On 2016/03/15 19:15:48, srdjan wrote:
> Get zone (thread->zone()) and use it for handle creation

Done.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/stub_code_ar...
File runtime/vm/stub_code_arm.cc (right):

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/stub_code_ar...
runtime/vm/stub_code_arm.cc:2113: void
StubCode::GenerateICLookupThroughFunctionStub(Assembler* assembler) {
On 2016/03/15 19:15:48, srdjan wrote:
> Is anybody using this stub at all?

Instance calls are initially compiled with this stub. The new stub does not
handle lazy compilation, which we need to bring up the service isolate for
loading, etc.

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/stub_code_ia...
File runtime/vm/stub_code_ia32.cc (right):

https://codereview.chromium.org/1799793002/diff/60001/runtime/vm/stub_code_ia...
runtime/vm/stub_code_ia32.cc:2087: void
StubCode::GenerateICLookupThroughFunctionStub(Assembler* assembler) {
On 2016/03/16 15:45:48, Florian Schneider wrote:
> Since this stub is not used on ia32, UNREACHABLE()?
> Or can you omit the definition completely, so we'd get a linker error instead?

It isn't used on IA32, but we generate all stubs on all architectures.

[srdjan, 2016-03-16 19:34:38]

lgtm

[rmacnak, 2016-03-17 22:41:08]

Description was changed from

==========
Precompilation: Change instance call sequence to load the entry point and Code
object from the ic data array instead of loading them indirectly from the
Function object loaded from the same array.

Load the Code from the ic data array instead of the ObjectPool because we still
need the Code object in the frame to find stackmaps / build stacktraces / build
profile results.

Keep doing things the old way until all compilation is finished because
evaluation of constants still needs functional lazy compilation and the compiler
works with ICData that may contain as-yet-uncompiled functions. After
compilation is finished, transform all the ICData from (cid, target function,
count) to (cid, target code, entry point) and switch to the ICLookup stub that
works with this representation.

Golem average +3.133% on ARM, +3.544% on x64
==========

to

==========
Precompilation: Change instance call sequence to load the entry point and Code
object from the ic data array instead of loading them indirectly from the
Function object loaded from the same array.

Load the Code from the ic data array instead of the ObjectPool because we still
need the Code object in the frame to find stackmaps / build stacktraces / build
profile results.

Keep doing things the old way until all compilation is finished because
evaluation of constants still needs functional lazy compilation and the compiler
works with ICData that may contain as-yet-uncompiled functions. After
compilation is finished, transform all the ICData from (cid, target function,
count) to (cid, target code, entry point) and switch to the ICLookup stub that
works with this representation.

Golem average +3.133% on ARM, +3.544% on x64

R=srdjan@google.com

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

[rmacnak, 2016-03-17 22:41:09]

Committed patchset #5 (id:80001) manually as
aefb7a424ce209cde26ecad99cd98ee86a856a71 (presubmit successful).