Use off-heap data for type feedback in PolymorphicInstanceCallInstr

Use off-heap data for type feedback in PolymorphicInstanceCallInstr

We have been using IC data for receiver type info in the PolymorphicInstanceCallInstr. This is a data structure optimized for access from hand-coded assembly stubs, and placed on the GC-ed heap, which means it has to be accessed through handles. With this change we move it to the zone memory, which means it can be freed without an old-gen GC.  As a side-effect the zone arrays use exponential growth for amortized constant space allocation instead of growing by 1 for quadratic allocation when we add classes, further reducing memory pressure.

R=vegorov@google.com
BUG=29302
Committed: https://github.com/dart-lang/sdk/commit/d315a53e21ee4f3c80e9854354b69eba2161da43

[Vyacheslav Egorov (Google), 2017-04-10 10:59:28]

Some initial comments.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1654: ZoneGrowableArray<CidRangeTarget>*
sorted_arg,
why did this suddenly become ZoneGrowableArray?

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1795: const PolymorphicTargets*
FlowGraphCompiler::TrySpecializeByReceiverCid(
I think this name now is confusing given that we don't pass ICData here anymore.


Maybe call it:

ResolveCallTargetsForReceiverCid(...)

I also suggest a more natural ordering of the arguments:

(cid, selector, args descriptor)

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1804: if (!LookupMethodFor(cid, args_desc,
selector, &fn)) return NULL;
Similar here: natural arguments order should probably be (cid, selector,
args_desc)

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1807: targets->Add(CidRangeTarget(cid, cid,
&fn, 1));
this 1 needs a comment, e.g. /*count=*/1

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1862: Function& target_function =
for named arguments I suggest:

const bool allow_add = false;
..., allow_add)

or 

/*allow_add=*/false.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1876: const InstanceCallInstr&
original_call_insn,
maybe just original_call instead of original_call_insn

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1501: const int kNoCase = -1;
This code is duplicated in each architecture. Could you move it to some sort of
helper?

EmitTestAndCall on all architectures seems to follow the same pattern:

Prologue()
HandleSmiCase()
SwitchPrologue()
for (...) {
  HandleCase()
}
MegamorphicCall()

We should clean this up and unify these functions.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_inlin...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:344: target ^=
instance_call->FirstTarget().raw();
Why are we recording only a single target here? Makes no sense.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:397: const Function* target =
&instance_call->FirstTarget();
Ditto.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:492: PolymorphicTargets* inlined_variants_;
Why not embed this by value? Even if object is zone allocated you can still use
it as a value object.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:1304: const Function& target =
call->FirstTarget();
Why are we looking only on the FirstTarget()? Is this the case when
PolymorphicInstanceCall has single target?

You see - the naming is confusing.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_type_...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_type_...
runtime/vm/flow_graph_type_propagator.cc:299: if
(instr->HasSingleRecognizedCid()) {
These changes are highly suspect. They definitely break AOT optimizer because
HasSingleRecognizedCid() is weaker than unique selector requirements:

class A {
  final uniqueField = "world";
  uniqueMethod() => "hello";
}

class B {
  noSuchMethod(mirror) => 42;
}

foobar(obj) {
  try {
    final a = obj.uniqueMethod();
    final b = obj.uniqueField;
    return "${a} ${b}";
  } catch (e) {
    return "thrown";
  }
}

main() {
  print(foobar(new A()));
  print(foobar(new B()));
}

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3174: bool
PolymorphicTargets::HasSingleRecognizedTarget() const {
Please don't intersperse implementations of two different classes - this makes
it very hard to read this code.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3176: return
MethodRecognizer::RecognizeKind(FirstTarget()) !=
I think FirstTarget() should better be named SingleTarget() to match
HasSingleTarget() name.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3205: bool
PolymorphicInstanceCallInstr::HasSingleRecognizedCid() const {
Why is this function called HasSingleRecognizedCid? (e.g. why is it
"recognized"?).

I think a better name is just IsMonomorphic() or HasSingleReceiverCid() or
something along this lines.

Also the question is why is this not a method on targets themselves (ditto
below).

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3211: intptr_t
PolymorphicInstanceCallInstr::SingleCid() const {
Better name would be MonomorphicReceiverCid().

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3217: Function&
PolymorphicTargets::FirstTarget() const {
This should probably be SingleTarget()

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3224: intptr_t
PolymorphicInstanceCallInstr::CallCount() const {
This should probably be just calling the method on the targets called something
like AggregateCallCount().

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3239: const Function& target =
Function::ZoneHandle(targets_[0].target->raw());
Instead of the targets[0].target access use targets.SingleTarget() here.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3291: if (ic_data()->NumArgsTested() != 1)
return this;
Please elaborate why this check is here so that the next person reading this
code knows.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3296: // recognized or inlined when it is
cold.
I don't think coldness has anything to do with it. It's just that we replace it
later in the pipeline - so inlining/specialization does not run again.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:552: struct CidRangeTarget {
This needs some comments: e.g. declaring that this structure describes a method
target for a range of receiver cids and its hotness.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:570: class PolymorphicTargets : public
ZoneAllocated {
I think the better name for this would just be 

CallTargets 

or 

CallTargetSet

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:597: BaseGrowableArray<CidRangeTarget,
ValueObject, Zone> cid_ranges_;
Why can't this be just GrowableArray<CidRangeTarget>?

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:3285: String& selector =
const String&

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:3287: Array& argument_names =
const Array&

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

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/jit_optimizer.cc...
runtime/vm/jit_optimizer.cc:212: const ICData& ic_data =
*call->instance_call()->ic_data();
disregarding ic_data has an interesting effect on optimizations that use
"ic_data" non-traditionallly: 

e.g. when we compile 

a instanceof T 

we sometimes emit 

a._simpleInstanceOf(T)

where Object has 

_simpleInstanceOf
_simpleInstanceOfTrue(t) => true;
_simpleInstanceOfFalse(t) => false;

and on miss we cache _simpleInstanceOfTrue or _simpleInstanceOfFalse in the
ICData instead of _simpleInstanceOf itself. 

We might want to check that none of these optimizations are broken by this.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/jit_optimizer.cc...
runtime/vm/jit_optimizer.cc:1465: PolymorphicTargets*
JitOptimizer::CreatePolymorphicTargets(
Given that this method is used by both Aot and Jit compiler can it also go and
live on the FlowGraphCompiler?

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

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:4503: const intptr_t new_length = list_len + 4 + (list_len
>> 2);
This CL contains seemingly unrelated changes in the growth strategy spread
around. 

Could you split them out into a separate CL?

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

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/weak_code.cc#new...
runtime/vm/weak_code.cc:23: // Try to find and reuse cleared WeakProperty to
avoid allocating new one.
You need to amend the comment because the code below does more than what this
comment states - because you changed how the array grows.

Additionally please reflect this change in the CL description (maybe with
rationale).

[erikcorry, 2017-04-19 15:06:41]

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1654: ZoneGrowableArray<CidRangeTarget>*
sorted_arg,
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> why did this suddenly become ZoneGrowableArray?

Undone.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1795: const PolymorphicTargets*
FlowGraphCompiler::TrySpecializeByReceiverCid(
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> I think this name now is confusing given that we don't pass ICData here
anymore.
> 
> 
> Maybe call it:
> 
> ResolveCallTargetsForReceiverCid(...)
> 
> I also suggest a more natural ordering of the arguments:
> 
> (cid, selector, args descriptor)

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1804: if (!LookupMethodFor(cid, args_desc,
selector, &fn)) return NULL;
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> Similar here: natural arguments order should probably be (cid, selector,
> args_desc)

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1807: targets->Add(CidRangeTarget(cid, cid,
&fn, 1));
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> this 1 needs a comment, e.g. /*count=*/1

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1862: Function& target_function =
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> for named arguments I suggest:
> 
> const bool allow_add = false;
> ..., allow_add)
> 
> or 
> 
> /*allow_add=*/false.
> 

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:1876: const InstanceCallInstr&
original_call_insn,
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> maybe just original_call instead of original_call_insn

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1501: const int kNoCase = -1;
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> This code is duplicated in each architecture. Could you move it to some sort
of
> helper?
> 
> EmitTestAndCall on all architectures seems to follow the same pattern:
> 
> Prologue()
> HandleSmiCase()
> SwitchPrologue()
> for (...) {
>   HandleCase()
> }
> MegamorphicCall()
> 
> We should clean this up and unify these functions.

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_inlin...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:344: target ^=
instance_call->FirstTarget().raw();
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> Why are we recording only a single target here? Makes no sense.

Agreed, but that's how I found it.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:397: const Function* target =
&instance_call->FirstTarget();
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:492: PolymorphicTargets* inlined_variants_;
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> Why not embed this by value? Even if object is zone allocated you can still
use
> it as a value object.

I didn't know you could do that.  Done for inlined_variants_.

In this case of non_inlined_variants_ we want to attach it to an instruction, so
the lifetime doesn't work and it has to be in the zone.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_inlin...
runtime/vm/flow_graph_inliner.cc:1304: const Function& target =
call->FirstTarget();
On 2017/04/10 10:59:27, Vyacheslav Egorov (Google) wrote:
> Why are we looking only on the FirstTarget()? Is this the case when
> PolymorphicInstanceCall has single target?
> 
> You see - the naming is confusing.

Made a synonym called MostPopularTarget.  HTH.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_type_...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_type_...
runtime/vm/flow_graph_type_propagator.cc:299: if
(instr->HasSingleRecognizedCid()) {
The change to HasSingleRecognizedCid was reverted.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/flow_graph_type_...
runtime/vm/flow_graph_type_propagator.cc:1027: if (!HasSingleRecognizedCid())
return CompileType::Dynamic();
The change in this 'if' also reverted

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3174: bool
PolymorphicTargets::HasSingleRecognizedTarget() const {
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> Please don't intersperse implementations of two different classes - this makes
> it very hard to read this code.

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3176: return
MethodRecognizer::RecognizeKind(FirstTarget()) !=
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> I think FirstTarget() should better be named SingleTarget() to match
> HasSingleTarget() name.

I'd like to leave it because it is sometimes used where there is more than one
target where we for some reason want the first. The reasons are not very good,
but I don't want to expand the scope of this change any more.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3205: bool
PolymorphicInstanceCallInstr::HasSingleRecognizedCid() const {
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> Why is this function called HasSingleRecognizedCid? (e.g. why is it
> "recognized"?).
> 
> I think a better name is just IsMonomorphic() or HasSingleReceiverCid() or
> something along this lines.
> 
> Also the question is why is this not a method on targets themselves (ditto
> below).

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3211: intptr_t
PolymorphicInstanceCallInstr::SingleCid() const {
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> Better name would be MonomorphicReceiverCid().

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3217: Function&
PolymorphicTargets::FirstTarget() const {
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> This should probably be SingleTarget()

See above.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3224: intptr_t
PolymorphicInstanceCallInstr::CallCount() const {
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> This should probably be just calling the method on the targets called
something
> like AggregateCallCount().

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3239: const Function& target =
Function::ZoneHandle(targets_[0].target->raw());
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> Instead of the targets[0].target access use targets.SingleTarget() here.

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3291: if (ic_data()->NumArgsTested() != 1)
return this;
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> Please elaborate why this check is here so that the next person reading this
> code knows.

I can't remember :-/.  I have the benchmarking service running a version with
and without this line, so lets see if it helps.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.cc:3296: // recognized or inlined when it is
cold.
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> I don't think coldness has anything to do with it. It's just that we replace
it
> later in the pipeline - so inlining/specialization does not run again.

Things seem to work with this check here, that checks for coldness.  The
regression was on NavierStokesTyped.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:552: struct CidRangeTarget {
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> This needs some comments: e.g. declaring that this structure describes a
method
> target for a range of receiver cids and its hotness.

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:570: class PolymorphicTargets : public
ZoneAllocated {
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> I think the better name for this would just be 
> 
> CallTargets 
> 
> or 
> 
> CallTargetSet
> 

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language.h:597: BaseGrowableArray<CidRangeTarget,
ValueObject, Zone> cid_ranges_;
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> Why can't this be just GrowableArray<CidRangeTarget>?

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:3285: String& selector =
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> const String&

Done.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/intermediate_lan...
runtime/vm/intermediate_language_arm.cc:3287: Array& argument_names =
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> const Array&

Done.

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

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/jit_optimizer.cc...
runtime/vm/jit_optimizer.cc:212: const ICData& ic_data =
*call->instance_call()->ic_data();
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> disregarding ic_data has an interesting effect on optimizations that use
> "ic_data" non-traditionallly: 
> 
> e.g. when we compile 
> 
> a instanceof T 
> 
> we sometimes emit 
> 
> a._simpleInstanceOf(T)
> 
> where Object has 
> 
> _simpleInstanceOf
> _simpleInstanceOfTrue(t) => true;
> _simpleInstanceOfFalse(t) => false;
> 
> and on miss we cache _simpleInstanceOfTrue or _simpleInstanceOfFalse in the
> ICData instead of _simpleInstanceOf itself. 
> 
> We might want to check that none of these optimizations are broken by this.

I'm not sure where this is happening.  If it's in ReplaceWithInstanceOf then
it's not touched.

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/jit_optimizer.cc...
runtime/vm/jit_optimizer.cc:1465: PolymorphicTargets*
JitOptimizer::CreatePolymorphicTargets(
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> Given that this method is used by both Aot and Jit compiler can it also go and
> live on the FlowGraphCompiler?

Moved to static CallTargets::Create(...)

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

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/object.cc#newcod...
runtime/vm/object.cc:4503: const intptr_t new_length = list_len + 4 + (list_len
>> 2);
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> This CL contains seemingly unrelated changes in the growth strategy spread
> around. 
> 
> Could you split them out into a separate CL?

Done.

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

https://codereview.chromium.org/2809583002/diff/1/runtime/vm/weak_code.cc#new...
runtime/vm/weak_code.cc:23: // Try to find and reuse cleared WeakProperty to
avoid allocating new one.
On 2017/04/10 10:59:28, Vyacheslav Egorov (Google) wrote:
> You need to amend the comment because the code below does more than what this
> comment states - because you changed how the array grows.
> 
> Additionally please reflect this change in the CL description (maybe with
> rationale).

Moved to different CL

[Cutch, 2017-04-19 15:47:07]

What is the intention of this CL?

Why is the CL description a single sentence which doesn't accurately reflect
what this CL changes?

What bug is it addressing?

[erikcorry, 2017-04-19 20:48:31]

Description was changed from

==========
Use off-heap data for type feedback in PolymorphicInstanceCallInstr

R=vegorov@google.com
BUG=
==========

to

==========
Use off-heap data for type feedback in PolymorphicInstanceCallInstr

We have been using IC data for receiver type info in the
PolymorphicInstanceCallInstr. This is a data structure optimized for access from
hand-coded assembly stubs, and placed on the GC-ed heap, which means it has to
be accessed through handles. With this change we move it to the zone memory,
which means it can be freed without an old-gen GC.  As a side-effect the zone
arrays use exponential growth for amortized constant space allocation instead of
growing by 1 for quadratic allocation when we add classes, further reducing
memory pressure.

R=vegorov@google.com
BUG=
==========

[erikcorry, 2017-04-19 20:49:34]

Description was changed from

==========
Use off-heap data for type feedback in PolymorphicInstanceCallInstr

We have been using IC data for receiver type info in the
PolymorphicInstanceCallInstr. This is a data structure optimized for access from
hand-coded assembly stubs, and placed on the GC-ed heap, which means it has to
be accessed through handles. With this change we move it to the zone memory,
which means it can be freed without an old-gen GC.  As a side-effect the zone
arrays use exponential growth for amortized constant space allocation instead of
growing by 1 for quadratic allocation when we add classes, further reducing
memory pressure.

R=vegorov@google.com
BUG=
==========

to

==========
Use off-heap data for type feedback in PolymorphicInstanceCallInstr

We have been using IC data for receiver type info in the
PolymorphicInstanceCallInstr. This is a data structure optimized for access from
hand-coded assembly stubs, and placed on the GC-ed heap, which means it has to
be accessed through handles. With this change we move it to the zone memory,
which means it can be freed without an old-gen GC.  As a side-effect the zone
arrays use exponential growth for amortized constant space allocation instead of
growing by 1 for quadratic allocation when we add classes, further reducing
memory pressure.

R=vegorov@google.com
BUG=29302
==========

[erikcorry, 2017-04-20 08:02:28]

Description updated.

[Vyacheslav Egorov (Google), 2017-04-20 16:45:33]

lgtm

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1979: EmitTestAndCallSmiBranch(failed, true);
please add a comment what true/false means in this context.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1990: const int kMaxImmediateInInstruction =
256;
Maybe add a note that this works on all architectures?

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:664: intptr_t
ComputeGoodBiasForCidComparison(const CallTargets& sorted,
it might be good to just package this and all other related helpers into a
single separate class called: 

class TestAndCallEmitter {
 public:

 private:
  LoadReceiver();
  LoadCid();
  ...
};

the compiler class is already gigantic. 

However if you don't feel like it - feel free to ignore.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm.cc:1491: void
FlowGraphCompiler::EmitTestAndCallSmiBranch(Label* label, bool if_smi) {
We already have 

BranchIfNotSmi() in all assemblers just add BranchIfSmi() to all assemblers and
use that instead of introducing this function.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm.cc:1522: void
FlowGraphCompiler::EmitTestAndCallGotoMatchFound(Label* match_found) {
All assemblers have Jump(...) - just use that.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_i...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:493: CallTargets* non_inlined_variants_;
Add a comment that non_inlined_variants_ can be used to create an instruction
that calls them - that's why this is stored by pointer because it needs to
outlive.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1481: if ((target.raw() ==
inlined_variants_.At(i).target->raw()) &&
just [i] should work?

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (left):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2698: 
restore the deleted new line

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:604: Function& MostPopularTarget() const {
return FirstTarget(); }
It might be worth adding an assertion here that CallTargets are sorted.

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

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/jit_optimize...
runtime/vm/jit_optimizer.h:13: struct CidRangeTarget;
Is this forward declaration used in this file?

[erikcorry, 2017-04-21 13:04:44]

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1979: EmitTestAndCallSmiBranch(failed, true);
On 2017/04/20 16:45:33, Vyacheslav Egorov (Google) wrote:
> please add a comment what true/false means in this context.

Done.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:1990: const int kMaxImmediateInInstruction =
256;
On 2017/04/20 16:45:33, Vyacheslav Egorov (Google) wrote:
> Maybe add a note that this works on all architectures?

This wasn't a very noticable win (microscopic code density improvement) so I got
rid of it instead.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.h (right):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:664: intptr_t
ComputeGoodBiasForCidComparison(const CallTargets& sorted,
On 2017/04/20 16:45:33, Vyacheslav Egorov (Google) wrote:
> it might be good to just package this and all other related helpers into a
> single separate class called: 
> 
> class TestAndCallEmitter {
>  public:
> 
>  private:
>   LoadReceiver();
>   LoadCid();
>   ...
> };
> 
> the compiler class is already gigantic. 
> 
> However if you don't feel like it - feel free to ignore.

Two of these methods were just eliminated, so I chose not to do this change.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm.cc:1491: void
FlowGraphCompiler::EmitTestAndCallSmiBranch(Label* label, bool if_smi) {
On 2017/04/20 16:45:33, Vyacheslav Egorov (Google) wrote:
> We already have 
> 
> BranchIfNotSmi() in all assemblers just add BranchIfSmi() to all assemblers
and
> use that instead of introducing this function.

Unfortunately we don't know the register to use here, so we can't use that.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm.cc:1522: void
FlowGraphCompiler::EmitTestAndCallGotoMatchFound(Label* match_found) {
On 2017/04/20 16:45:33, Vyacheslav Egorov (Google) wrote:
> All assemblers have Jump(...) - just use that.

Done.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_i...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:493: CallTargets* non_inlined_variants_;
On 2017/04/20 16:45:33, Vyacheslav Egorov (Google) wrote:
> Add a comment that non_inlined_variants_ can be used to create an instruction
> that calls them - that's why this is stored by pointer because it needs to
> outlive.

Done.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/flow_graph_i...
runtime/vm/flow_graph_inliner.cc:1481: if ((target.raw() ==
inlined_variants_.At(i).target->raw()) &&
On 2017/04/20 16:45:33, Vyacheslav Egorov (Google) wrote:
> just [i] should work?

Done.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (left):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2698: 
On 2017/04/20 16:45:33, Vyacheslav Egorov (Google) wrote:
> restore the deleted new line

Done.

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:604: Function& MostPopularTarget() const {
return FirstTarget(); }
On 2017/04/20 16:45:33, Vyacheslav Egorov (Google) wrote:
> It might be worth adding an assertion here that CallTargets are sorted.

Done.

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

https://codereview.chromium.org/2809583002/diff/20001/runtime/vm/jit_optimize...
runtime/vm/jit_optimizer.h:13: struct CidRangeTarget;
On 2017/04/20 16:45:33, Vyacheslav Egorov (Google) wrote:
> Is this forward declaration used in this file?

Gone

[erikcorry, 2017-04-24 09:15:34]

Description was changed from

==========
Use off-heap data for type feedback in PolymorphicInstanceCallInstr

We have been using IC data for receiver type info in the
PolymorphicInstanceCallInstr. This is a data structure optimized for access from
hand-coded assembly stubs, and placed on the GC-ed heap, which means it has to
be accessed through handles. With this change we move it to the zone memory,
which means it can be freed without an old-gen GC.  As a side-effect the zone
arrays use exponential growth for amortized constant space allocation instead of
growing by 1 for quadratic allocation when we add classes, further reducing
memory pressure.

R=vegorov@google.com
BUG=29302
==========

to

==========
Use off-heap data for type feedback in PolymorphicInstanceCallInstr

We have been using IC data for receiver type info in the
PolymorphicInstanceCallInstr. This is a data structure optimized for access from
hand-coded assembly stubs, and placed on the GC-ed heap, which means it has to
be accessed through handles. With this change we move it to the zone memory,
which means it can be freed without an old-gen GC.  As a side-effect the zone
arrays use exponential growth for amortized constant space allocation instead of
growing by 1 for quadratic allocation when we add classes, further reducing
memory pressure.

R=vegorov@google.com
BUG=29302

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

[erikcorry, 2017-04-24 09:15:36]

Committed patchset #3 (id:40001) manually as
d315a53e21ee4f3c80e9854354b69eba2161da43.