Fix CompilerStats info

Fix CompilerStats info

Move the counters and timers into the isolate. Statically allocated data
does not work anymore now that we always have multiple isolates.

Remove unused counters, add some counters to measure size of compiled code
and meta info.

R=asiva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=45751

[hausner, 2015-05-11 23:18:54]

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

[Cutch, 2015-05-11 23:24:00]

On 2015/05/11 23:19:01, hausner wrote:

DBC:

If you added a Metric (metrics.h) to ISOLATE_METRICS_LIST (metrics.h), these
values would be available within Observatory.

get value: isolate->Get<NAME>Metric().value();
set value: isolate->Get<Name>Metric().set_value(new value);

[hausner, 2015-05-12 17:06:15]

Nice idea. Exposing some of these counters to the Observatory might be a good
idea. I'll reserve this for a future CL. Adding a TODO.

[siva, 2015-05-12 17:39:53]

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/compiler_sta...
File runtime/vm/compiler_stats.cc (right):

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.cc:46: vardesc_size = 0;
Why are these initialized in the body and not in the initializer list? We almost
always initialize fields in the initializer list.

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

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:19: class CompilerStats {
Should this extend ValueObject? See comment in isolate.[h|cc]

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:66: #define CSTAT_TIMER(t)
Isolate::Current()->compiler_stats()->t
Why doesn't this take an isolate param like INC_STAT? We might have
Isolate::Current already available and be able to use it without having to do
Isolate::Current()

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:708: pause_loop_monitor_ = NULL;
free compiler_stats_ here?

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:781: result->compiler_stats_ = new CompilerStats(result);
Should this be done conditionally under FLAG_compiler_stats?

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

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:746: CompilerStats* compiler_stats_;
Can this be a regular field here instead of a pointer? Is it because you do not
want to pay the cost of this under a flag or in some specific isolates?

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

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7968: INC_STAT(Isolate::Current(), num_tokens_total, len);
Might make sense to cache Isolate::Current() in a local variable as it is being
accessed in a for loop above.

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:10617: INC_STAT(Isolate::Current(), pc_desc_size, size);
Ditto comment about caching Isolate::Current() instead of multiple calls.

[hausner, 2015-05-12 21:52:16]

PTAL

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/compiler_sta...
File runtime/vm/compiler_stats.cc (right):

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.cc:46: vardesc_size = 0;
On 2015/05/12 17:39:52, siva wrote:
> Why are these initialized in the body and not in the initializer list? We
almost
> always initialize fields in the initializer list.

Done.

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

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:19: class CompilerStats {
On 2015/05/12 17:39:52, siva wrote:
> Should this extend ValueObject? See comment in isolate.[h|cc]

I want to avoid allocating the CompilerStats structure if it's not needed.

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:66: #define CSTAT_TIMER(t)
Isolate::Current()->compiler_stats()->t
On 2015/05/12 17:39:52, siva wrote:
> Why doesn't this take an isolate param like INC_STAT? We might have
> Isolate::Current already available and be able to use it without having to do
> Isolate::Current()

I tried to keep the code that uses the macro short, but it's a good point.

Changed the macro to work similar to TIMERSCOPE.

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:708: pause_loop_monitor_ = NULL;
On 2015/05/12 17:39:52, siva wrote:
> free compiler_stats_ here?

Done.

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:781: result->compiler_stats_ = new CompilerStats(result);
On 2015/05/12 17:39:52, siva wrote:
> Should this be done conditionally under FLAG_compiler_stats?

Yes.

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

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:746: CompilerStats* compiler_stats_;
On 2015/05/12 17:39:52, siva wrote:
> Can this be a regular field here instead of a pointer? Is it because you do
not
> want to pay the cost of this under a flag or in some specific isolates?

Changed the TimerScope class so it can handle NULL as timer value. Thus, I only
need to allocate compiler_stats_ if we actually collect the stats.

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

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:7968: INC_STAT(Isolate::Current(), num_tokens_total, len);
On 2015/05/12 17:39:52, siva wrote:
> Might make sense to cache Isolate::Current() in a local variable as it is
being
> accessed in a for loop above.

Done.

https://codereview.chromium.org/1127383008/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:10617: INC_STAT(Isolate::Current(), pc_desc_size, size);
On 2015/05/12 17:39:52, siva wrote:
> Ditto comment about caching Isolate::Current() instead of multiple calls.

Done.

[siva, 2015-05-12 22:12:32]

lgtm

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

https://codereview.chromium.org/1127383008/diff/60001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:29: // DODO(hausner): add these timers to the timer
list maintained
TODO

https://codereview.chromium.org/1127383008/diff/60001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:74: iso);
indent of backslash seems off.

https://codereview.chromium.org/1127383008/diff/60001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1127383008/diff/60001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1443: compiler_stats()->Print();
Shouldn't this also be conditional under FLAG_compiler_stats as compiler_stats()
may return NULL

[hausner, 2015-05-12 22:49:53]

Thanks.

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

https://codereview.chromium.org/1127383008/diff/60001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:29: // DODO(hausner): add these timers to the timer
list maintained
On 2015/05/12 22:12:32, siva wrote:
> TODO

:)

https://codereview.chromium.org/1127383008/diff/60001/runtime/vm/compiler_sta...
runtime/vm/compiler_stats.h:74: iso);
On 2015/05/12 22:12:31, siva wrote:
> indent of backslash seems off.

Done.

https://codereview.chromium.org/1127383008/diff/60001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1127383008/diff/60001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1443: compiler_stats()->Print();
On 2015/05/12 22:12:32, siva wrote:
> Shouldn't this also be conditional under FLAG_compiler_stats as
compiler_stats()
> may return NULL
Ah, good catch.

[hausner, 2015-05-12 23:04:02]

Committed patchset #5 (id:80001) manually as r45751 (presubmit successful).