Move CompilerStats from isolate to thread. Aggregate stats.

Move CompilerStats from isolate to thread. Aggregate stats.

The mutator thread holds the aggregate values, the background compiler adds its value to it.

BUG=
R=hausner@google.com

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

[srdjan, 2016-03-30 01:50:30]

Description was changed from

==========
Move CompilerStats from isolate to thread. Aggregate stats.

BUG=
==========

to

==========
Move CompilerStats from isolate to thread. Aggregate stats.

The mutator thread holds the aggregate values, the background compiler adds its
value to it.

BUG=
==========

[srdjan, 2016-03-30 02:31:43]

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

[siva, 2016-03-30 14:24:28]

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

[siva, 2016-03-30 14:24:29]

One high level comment, why did you choose to do it this way not leave the stats
in the Isolate structure and use atomic incrments to ensure consistent values
are recorded from multiple threads?

[srdjan, 2016-03-30 16:49:02]

On 2016/03/30 14:24:29, siva wrote:
> One high level comment, why did you choose to do it this way not leave the
stats
> in the Isolate structure and use atomic incrments to ensure consistent values
> are recorded from multiple threads?

Atomic increments work only for counter, but CompilerStats carry the timers as
well, which cannot be shared. At the end of the day I need to aggregate timers
anyway (every timer has a field 'total', that aggregates all its uses in one
thread).

[hausner, 2016-03-30 16:59:20]

Also, incrementing a stat timer should be light-weight, so you don't want a lock
around each increment.

Looking at the change now.

[srdjan, 2016-03-30 17:37:58]

On 2016/03/30 16:59:20, hausner wrote:
> Also, incrementing a stat timer should be light-weight, so you don't want a
lock
> around each increment.
> 
> Looking at the change now.

Updated the change to use atomic increment when aggregating and remove a lock
when incrementing stat counters.

[hausner, 2016-03-30 19:49:21]

I don't think the increments of timers (when it stops) and counters is thread
safe. See comments below.

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/atomic.h
File runtime/vm/atomic.h (right):

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/atomic.h#new...
runtime/vm/atomic.h:29: static void IncrementInt64By(int64_t* p, int64_t value);
There seem to be missing implementations of this, e.g. for windows and the
simulator.

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/compiler_sta...
File runtime/vm/compiler_stats.h (right):

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:65: // we would be loosing the comments.
loosing -> losing

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:123: (thread)->compiler_stats()->counter += (incr); 
                           \
Does this work correctly if the background thread can increment concurrently (in
and Add() operation)?

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/timer.h
File runtime/vm/timer.h (right):

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/timer.h#newc...
runtime/vm/timer.h:38: total_ += elapsed;
What if another thread does AddTotal at the same time this addition is performed
in the mutator thread?

[srdjan, 2016-03-30 20:14:04]

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/atomic.h
File runtime/vm/atomic.h (right):

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/atomic.h#new...
runtime/vm/atomic.h:29: static void IncrementInt64By(int64_t* p, int64_t value);
On 2016/03/30 19:49:20, hausner wrote:
> There seem to be missing implementations of this, e.g. for windows and the
> simulator.

Implemented on windows, not needed for simulator (uses native implementations).

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/compiler_sta...
File runtime/vm/compiler_stats.h (right):

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:65: // we would be loosing the comments.
On 2016/03/30 19:49:20, hausner wrote:
> loosing -> losing

Done.

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:123: (thread)->compiler_stats()->counter += (incr); 
                           \
On 2016/03/30 19:49:20, hausner wrote:
> Does this work correctly if the background thread can increment concurrently
(in
> and Add() operation)?

Added AtomicOperation on it (and total_ in Timer).

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/timer.h
File runtime/vm/timer.h (right):

https://codereview.chromium.org/1841213003/diff/90001/runtime/vm/timer.h#newc...
runtime/vm/timer.h:38: total_ += elapsed;
On 2016/03/30 19:49:20, hausner wrote:
> What if another thread does AddTotal at the same time this addition is
performed
> in the mutator thread?

Using Atomic increment.

[hausner, 2016-03-30 20:29:11]

LGTM

[srdjan, 2016-03-30 20:38:50]

Description was changed from

==========
Move CompilerStats from isolate to thread. Aggregate stats.

The mutator thread holds the aggregate values, the background compiler adds its
value to it.

BUG=
==========

to

==========
Move CompilerStats from isolate to thread. Aggregate stats.

The mutator thread holds the aggregate values, the background compiler adds its
value to it.

BUG=
R=hausner@google.com

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

[srdjan, 2016-03-30 20:38:52]

Committed patchset #7 (id:110001) manually as
1a9fc8b2fa7ea786f155b523abb4a8c7db1a90d6 (presubmit successful).