[Isolates] StatsTable moved to Isolate....

[Isolates] StatsTable moved to Isolate.
Its static fields / methods were transformed to instance one.

Committed: http://code.google.com/p/v8/source/detail?r=4910

[Maxim.Mossienko, 2010-06-19 21:05:04]

Please, review

[Dmitry Titov, 2010-06-21 20:50:36]

http://codereview.chromium.org/2817018/diff/1/6
File src/compilation-cache.cc (right):

http://codereview.chromium.org/2817018/diff/1/6#newcode188
src/compilation-cache.cc:188: if (stats_table_ == NULL) {
Since this round of changes is mostly mechanical, we try to not add caching
variables like stats_table_ just yet, except glaringly obvious cases (like
Heap::isolate_). Every such optimization should be done only if perf test shows
benefit.

It's better to keep Isolate::Current()->stats_table()-> form here.

http://codereview.chromium.org/2817018/diff/1/6#newcode190
src/compilation-cache.cc:190: script_histogram_ = stats_table_->CreateHistogram(
script_histogram seems to be a local variable, why did it move to be a class
member?

[Maxim.Mossienko, 2010-06-21 21:48:01]

http://codereview.chromium.org/2817018/diff/1/6
File src/compilation-cache.cc (right):

http://codereview.chromium.org/2817018/diff/1/6#newcode188
src/compilation-cache.cc:188: if (stats_table_ == NULL) {
No, we can not use Isolate::Current()->stats_table() here:
before change script_histogram is local static variable initialized once, to do
the same with Isolates we initialize stats_table_ once and request
script_histogram_ once. So stats_table_ is also a flag of initialization with
some performance benefit.

http://codereview.chromium.org/2817018/diff/1/6#newcode190
src/compilation-cache.cc:190: script_histogram_ = stats_table_->CreateHistogram(
Because script_histogram has static scope with initialized once value, please,
see explanation above

[Dmitry Titov, 2010-06-21 22:06:58]

http://codereview.chromium.org/2817018/diff/1/6
File src/compilation-cache.cc (right):

http://codereview.chromium.org/2817018/diff/1/6#newcode190
src/compilation-cache.cc:190: script_histogram_ = stats_table_->CreateHistogram(
On 2010/06/21 21:48:01, Maxim.Mossienko wrote:
> Because script_histogram has static scope with initialized once value, please,
> see explanation above
> 

You're right, script_histogram_ should be a class member :-)

I guess you want to use stats_table_ as a flag to avoid multiple callas to
CreateHistogram if there is no external function or external function actually
returns NULL, to preserve current behavior.

It could be way more readable if instead of using stats_table_ as a flag there
was actual script_histogram_initialized_ bool for that.

if (!script_histogram_initialized_)
  script_histogram_ =
    Isolate::Current()->stats_table()->CreateHistogram(...)
  script_histogram_initialized_ = true;
}

[Maxim.Mossienko, 2010-06-21 22:32:00]

Rebased, introduced explicit script_histogram_initialized_ flag instead of
stats_table_ cache field with flag semantic

[Vitaly Repeshko, 2010-06-21 22:38:59]

LGTM

http://codereview.chromium.org/2817018/diff/7002/14004
File src/counters.cc (right):

http://codereview.chromium.org/2817018/diff/7002/14004#newcode31
src/counters.cc:31: #include "platform.h"
Please explicitly include "isolate.h".

http://codereview.chromium.org/2817018/diff/7002/14008
File src/counters.h (right):

http://codereview.chromium.org/2817018/diff/7002/14008#newcode58
src/counters.h:58: bool HasCounterFunction() {
"const"

http://codereview.chromium.org/2817018/diff/7002/14008#newcode68
src/counters.h:68: int* FindLocation(const char* name) {
"const"

[Maxim.Mossienko, 2010-06-21 23:04:09]

const modifiers added + explicit "isolate.h" include

[Dmitry Titov, 2010-06-21 23:35:37]

Thanks! Landed in r4910.