Use zone allocated growable array to populate interim static and stub calls table, thus allocating …

Use zone allocated growable array to populate interim static and stub calls table, thus allocating less objects in old space

BUG=
R=koda@google.com

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

[srdjan, 2015-08-19 22:25:22]

srdjan@google.com changed reviewers:
+ koda@google.com

[koda, 2015-08-20 14:14:14]

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

https://codereview.chromium.org/1288863006/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:731: &Function::ZoneHandle(zone()),
As I understand it, exactly one of the 'code' and 'function' of each struct
instance is used, while the unused one remains a handle containing null. So,
it's basically a union, right?

If so, then you could easily avoid some redundant handle allocation by storing a
(C++) NULL instead, and skip those when finalizing the array (and preferably
also ASSERT that exactly one of them is NULL). Although allocating handles is
fairly cheap, the GC might need to traverse them, etc.

(Of course, you could go even further and try to make it an actual union with a
tag bit somewhere, but that seems like overkill.)

[srdjan, 2015-08-20 16:25:40]

PTAL

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

https://codereview.chromium.org/1288863006/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:731: &Function::ZoneHandle(zone()),
On 2015/08/20 14:14:14, koda wrote:
> As I understand it, exactly one of the 'code' and 'function' of each struct
> instance is used, while the unused one remains a handle containing null. So,
> it's basically a union, right?
> 
> If so, then you could easily avoid some redundant handle allocation by storing
a
> (C++) NULL instead, and skip those when finalizing the array (and preferably
> also ASSERT that exactly one of them is NULL). Although allocating handles is
> fairly cheap, the GC might need to traverse them, etc.
> 
> (Of course, you could go even further and try to make it an actual union with
a
> tag bit somewhere, but that seems like overkill.)

Using (C++) NULL instead of creating Null handles.

[koda, 2015-08-20 18:13:18]

LGTM with comment suggestion

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

https://codereview.chromium.org/1288863006/diff/40001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:667: const Function* function;
For readability, please comment (and preferably also assert) that exactly one of
these is NULL.

[srdjan, 2015-08-20 18:43:14]

Committed patchset #4 (id:60001) manually as
92013dd381f109f09d3d80b957d25075e367ff04 (presubmit successful).

[srdjan, 2015-08-20 21:37:46]

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

https://codereview.chromium.org/1288863006/diff/40001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:667: const Function* function;
On 2015/08/20 18:13:18, koda wrote:
> For readability, please comment (and preferably also assert) that exactly one
of
> these is NULL.

Discussed offline, adding comment.

[Ivan Posva, 2015-08-20 23:53:36]

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

[Ivan Posva, 2015-08-20 23:53:36]

DBC -ip

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

https://codereview.chromium.org/1288863006/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:666: struct StaticCallsStruct {
This struct has a constructor, shouldn't it be a class?

https://codereview.chromium.org/1288863006/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:673: : offset(offset_arg),
function(function_arg), code(code_arg) {}
How about asserting that the handles being passed in here are zone handles?

Also, DISALLOW_COPY*?

[srdjan, 2015-08-21 16:56:14]

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

https://codereview.chromium.org/1288863006/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:666: struct StaticCallsStruct {
On 2015/08/20 23:53:36, Ivan Posva wrote:
> This struct has a constructor, shouldn't it be a class?

This is a pattern that we use throughout the VM. Discussed offline and agreed
that it is more efficient to make StaticCallsStruct a subclass of ZoneObject.

https://codereview.chromium.org/1288863006/diff/60001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:673: : offset(offset_arg),
function(function_arg), code(code_arg) {}
On 2015/08/20 23:53:36, Ivan Posva wrote:
> How about asserting that the handles being passed in here are zone handles?
> 
> Also, DISALLOW_COPY*?

Thanks, will add checks for zone handles in next CL. Adding DISALLOW_...