Addressed review comments from previous CLs; adding more Symbols::FromConcat

Addressed review comments from previous CLs; adding more Symbols::FromConcat

BUG=
R=hausner@google.com

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

[srdjan, 2015-08-21 17:05:39]

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

[koda, 2015-08-21 17:21:14]

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

[koda, 2015-08-21 17:21:14]

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

https://codereview.chromium.org/1308013002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:721: new
StaticCallsStruct(assembler()->CodeSize(), &func, NULL));
Now we need to fetch the current zone from TLS and perform at least one new zone
allocation for every insertion (+ growing the array).

This will be a (slight) regression in terms of speed. What do we gain by this
change, except perhaps a couple of words of memory when there are zero entries?

[srdjan, 2015-08-21 17:33:41]

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

https://codereview.chromium.org/1308013002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:721: new
StaticCallsStruct(assembler()->CodeSize(), &func, NULL));
On 2015/08/21 17:21:14, koda wrote:
> Now we need to fetch the current zone from TLS and perform at least one new
zone
> allocation for every insertion (+ growing the array).
> 
> This will be a (slight) regression in terms of speed. What do we gain by this
> change, except perhaps a couple of words of memory when there are zero
entries?

Using new(zone()), thanks for spotting it. Having one instead of three words
allows GrowableArray to grow faster.

[hausner, 2015-08-21 17:41:20]

lgtm

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

https://codereview.chromium.org/1308013002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:721: new
StaticCallsStruct(assembler()->CodeSize(), &func, NULL));
On 2015/08/21 17:33:41, srdjan wrote:
> On 2015/08/21 17:21:14, koda wrote:
> > Now we need to fetch the current zone from TLS and perform at least one new
> zone
> > allocation for every insertion (+ growing the array).
> > 
> > This will be a (slight) regression in terms of speed. What do we gain by
this
> > change, except perhaps a couple of words of memory when there are zero
> entries?
> 
> Using new(zone()), thanks for spotting it. Having one instead of three words
> allows GrowableArray to grow faster.

You mean there is less data to copy when it grows? How about starting with a
larger array so you can avoid the first 2 or 3 growth cycles?

I don't feel strongly about this aspect of the change, but I question whether it
makes sense to micro-optimize pieces like this. So you have less data to copy,
but each access to an array element now requires an additional indirection, and
you have less data locality. Is this really faster?

[srdjan, 2015-08-21 18:12:14]

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

https://codereview.chromium.org/1308013002/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:721: new
StaticCallsStruct(assembler()->CodeSize(), &func, NULL));
On 2015/08/21 17:41:20, hausner wrote:
> On 2015/08/21 17:33:41, srdjan wrote:
> > On 2015/08/21 17:21:14, koda wrote:
> > > Now we need to fetch the current zone from TLS and perform at least one
new
> > zone
> > > allocation for every insertion (+ growing the array).
> > > 
> > > This will be a (slight) regression in terms of speed. What do we gain by
> this
> > > change, except perhaps a couple of words of memory when there are zero
> > entries?
> > 
> > Using new(zone()), thanks for spotting it. Having one instead of three words
> > allows GrowableArray to grow faster.
> 
> You mean there is less data to copy when it grows? How about starting with a
> larger array so you can avoid the first 2 or 3 growth cycles?
> 
> I don't feel strongly about this aspect of the change, but I question whether
it
> makes sense to micro-optimize pieces like this. So you have less data to copy,
> but each access to an array element now requires an additional indirection,
and
> you have less data locality. Is this really faster?

In this case it is probably not measurably faster. We can go either way. Choose
this way since passing objects by value always makes me itchy ;-). Some code
reviewers think this is better, some think it is not. I guess it is my call
unless there is a clear disadvantage of this approach (apart from the fact that
it changes code).

[srdjan, 2015-08-21 18:17:26]

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

[Florian Schneider, 2015-08-24 07:04:06]

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

[Florian Schneider, 2015-08-24 07:04:07]

style dbc:

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

https://codereview.chromium.org/1308013002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:666: class StaticCallsStruct : public
ZoneAllocated {
We usually make classes with public member variables structs. This way you also
don't need to make members public explicitly and you can remove 'public:' below.

[srdjan, 2015-08-24 18:02:07]

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

https://codereview.chromium.org/1308013002/diff/20001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.h:666: class StaticCallsStruct : public
ZoneAllocated {
On 2015/08/24 07:04:06, Florian Schneider wrote:
> We usually make classes with public member variables structs. This way you
also
> don't need to make members public explicitly and you can remove 'public:'
below.

There is quite a bit of different opinion about when to use struct when not. It
seems that most people here agree on:

Use struct if all members are public and no methods/constructors are defined. It
is OK to have a class with all public members.