Reenable d8 stats timers, using the histogram mechanism

Reenable d8 stats timers, using the histogram mechanism


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

[iposva, 2009-04-13 23:09:15]

First set of comments. -Ivan

http://codereview.chromium.org/67100/diff/1/2
File src/d8.cc (right):

http://codereview.chromium.org/67100/diff/1/2#newcode329
Line 329: Counter* counter = counter_map_->Lookup(name);
I know it should not happen, but you should assert that the counter you found
matches the histogram state that is being requested.

http://codereview.chromium.org/67100/diff/1/2#newcode362
Line 362: Counter* counter = static_cast<Counter*>(histogram);
Please use reinterpret_cast instead of static_cast here according to the C++
style guide.

http://codereview.chromium.org/67100/diff/1/3
File src/d8.h (right):

http://codereview.chromium.org/67100/diff/1/3#newcode53
Line 53: int32_t total_;
Personally I find it confusing how these fields are used in the counter and
histogram cases. This is probably because they conflict in their use. How about
using something like this, you would need to adjust the names of the accessors
and callers accordingly:

int32_t count_;  // Number of events or samples in the case of histogram use.
int64_t value_;  // Value is only used when the Counter is used as a histogram.
It represents a total sum of all added samples.

You can delay overflows for the value this way as we do not expose the pointer
to this memory location to V8 internals directly.

[DaveMoore, 2009-04-13 23:57:14]

http://codereview.chromium.org/67100/diff/1/2
File src/d8.cc (right):

http://codereview.chromium.org/67100/diff/1/2#newcode329
Line 329: Counter* counter = counter_map_->Lookup(name);
On 2009/04/13 23:09:15, iposva wrote:
> I know it should not happen, but you should assert that the counter you found
> matches the histogram state that is being requested.

Done.

http://codereview.chromium.org/67100/diff/1/2#newcode362
Line 362: Counter* counter = static_cast<Counter*>(histogram);
On 2009/04/13 23:09:15, iposva wrote:
> Please use reinterpret_cast instead of static_cast here according to the C++
> style guide.

Done.

http://codereview.chromium.org/67100/diff/1/3
File src/d8.h (right):

http://codereview.chromium.org/67100/diff/1/3#newcode53
Line 53: int32_t total_;
I changed
counter_ -> count_
total_ -> sample_total_
value() -> count()
total() -> sample_total()

On 2009/04/13 23:09:15, iposva wrote:
> Personally I find it confusing how these fields are used in the counter and
> histogram cases. This is probably because they conflict in their use. How
about
> using something like this, you would need to adjust the names of the accessors
> and callers accordingly:
> 
> int32_t count_;  // Number of events or samples in the case of histogram use.
> int64_t value_;  // Value is only used when the Counter is used as a
histogram.
> It represents a total sum of all added samples.
> 
> You can delay overflows for the value this way as we do not expose the pointer
> to this memory location to V8 internals directly.
> 
>

[iposva, 2009-04-14 00:03:50]

LGTM -ip