Fix data races caused by compiler stats counters in the parser.

Fix data races caused by compiler stats counters in the parser.

BUG=https://code.google.com/p/dart/issues/detail?id=13025

[Florian Schneider, 2013-09-04 14:48:26]

Instead of hiding them behind the flag, we could also remove those counters, if
there are not used anymore. Matthias, what do you think?

[hausner, 2013-09-04 15:31:38]

I will discuss with Ivan today, but I'd prefer to leave the code unchanged. We
may still want the counter, but we certainly don't want to pay the price of an
additional flag check. Not only does this slow down the compiler in every case,
it also doesn't resolve the race issue.

I think we can live with the race conditions here. On the other hand, if a
little kitten gets killed each time a counter is wrong, we can hide the
instrumentation in preprocessor macros that only add the counters in special
builds.

[srdjan, 2013-09-04 16:42:54]

DBC

The parser tracer change makes sense to me (we increment the indent only with
flag on, we should decrement with flag on as well)

We could move CompilerStats as a field to class Parser and then aggregate them
in the isolate at the end of each parse.

[Ivan Posva, 2013-09-04 16:56:28]

On 2013/09/04 16:42:54, srdjan wrote:
> DBC
> 
> The parser tracer change makes sense to me (we increment the indent only with
> flag on, we should decrement with flag on as well)
> 
> We could move CompilerStats as a field to class Parser and then aggregate them
> in the isolate at the end of each parse.

Please see comment in the relevant bug about this only being a short-term fix.
So I am not sure it is worth the effort, but I'll take a closer look later.

-Ivan

[Florian Schneider, 2013-09-05 09:37:18]

On 2013/09/04 15:31:38, hausner wrote:
> I will discuss with Ivan today, but I'd prefer to leave the code unchanged. We
> may still want the counter, but we certainly don't want to pay the price of an
> additional flag check. Not only does this slow down the compiler in every
case,
> it also doesn't resolve the race issue.
>

I'm much in favor of removing the code using #ifdef.
But how much does a "if (FLAG_xy) { z++ }" slow down the compiler?
It is measureable at all? Inside the if-statemnt we use atomic increments to
avoid the race.
We certainly don't care for the extra cost when running with --compiler-stats.
When disabled, it is load + branch vs. load + store. It's not clear which is
faster.

> I think we can live with the race conditions here. On the other hand, if a
> little kitten gets killed each time a counter is wrong, we can hide the
> instrumentation in preprocessor macros that only add the counters in special
> builds.

[srdjan, 2013-09-26 14:29:17]

On 2013/09/05 09:37:18, Florian Schneider wrote:
> On 2013/09/04 15:31:38, hausner wrote:
> > I will discuss with Ivan today, but I'd prefer to leave the code unchanged.
We
> > may still want the counter, but we certainly don't want to pay the price of
an
> > additional flag check. Not only does this slow down the compiler in every
> case,
> > it also doesn't resolve the race issue.
> >
> 
> I'm much in favor of removing the code using #ifdef.
> But how much does a "if (FLAG_xy) { z++ }" slow down the compiler?
> It is measureable at all? Inside the if-statemnt we use atomic increments to
> avoid the race.
> We certainly don't care for the extra cost when running with --compiler-stats.
> When disabled, it is load + branch vs. load + store. It's not clear which is
> faster.
> 
> > I think we can live with the race conditions here. On the other hand, if a
> > little kitten gets killed each time a counter is wrong, we can hide the
> > instrumentation in preprocessor macros that only add the counters in special
> > builds.

Status on this?

[Florian Schneider, 2013-10-01 08:53:07]

On 2013/09/26 14:29:17, srdjan wrote:
> On 2013/09/05 09:37:18, Florian Schneider wrote:
> > On 2013/09/04 15:31:38, hausner wrote:
> > > I will discuss with Ivan today, but I'd prefer to leave the code
unchanged.
> We
> > > may still want the counter, but we certainly don't want to pay the price
of
> an
> > > additional flag check. Not only does this slow down the compiler in every
> > case,
> > > it also doesn't resolve the race issue.
> > >
> > 
> > I'm much in favor of removing the code using #ifdef.
> > But how much does a "if (FLAG_xy) { z++ }" slow down the compiler?
> > It is measureable at all? Inside the if-statemnt we use atomic increments to
> > avoid the race.
> > We certainly don't care for the extra cost when running with
--compiler-stats.
> > When disabled, it is load + branch vs. load + store. It's not clear which is
> > faster.
> > 
> > > I think we can live with the race conditions here. On the other hand, if a
> > > little kitten gets killed each time a counter is wrong, we can hide the
> > > instrumentation in preprocessor macros that only add the counters in
special
> > > builds.
> 
> Status on this?

Not sure. I still think the data races should be fixed (at least when running
without --compiler-stats).

I did not notice any performance degradation with my "fix".