- Added ability to call histograms from within v8...

- Added ability to call histograms from within v8
- Changed the StatsRates to use the new HistogramTimers


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

[DaveMoore, 2009-03-10 16:25:30]

My last changeset got messed up on my drive (bad svn entries file). So I've made
the changes suggested in http://codereview.chromium.org/39256/show and created a
new one.

[iposva, 2009-03-10 23:34:23]

Many comments about comments, but otherwise LGTM.

-Ivan

http://codereview.chromium.org/42020/diff/1/2
File include/v8.h (right):

http://codereview.chromium.org/42020/diff/1/2#newcode1916
Line 1916: static void
SetAddHistogramSampleFunction(AddHistogramSampleCallback);
Please add comments documenting these two new V8 API calls.

http://codereview.chromium.org/42020/diff/1/6
File src/counters.cc (right):

http://codereview.chromium.org/42020/diff/1/6#newcode68
Line 68: if (histogram_ != NULL) {
I can guess why you would want to not call GetHistogram() at this point, but it
would be clearer if you added a comment.

http://codereview.chromium.org/42020/diff/1/6#newcode76
Line 76: 
These two functions could conceivably also be part of the header file. It is not
clear what make GetHistogram() more special compared to Start() or Stop().

http://codereview.chromium.org/42020/diff/1/7
File src/counters.h (right):

http://codereview.chromium.org/42020/diff/1/7#newcode49
Line 49: static void SetAddHistogramSampleFunction(AddHistogramSampleCallback f)
{
Small comment for the two new functions or extend the comment above to be more
generic.

http://codereview.chromium.org/42020/diff/1/7#newcode68
Line 68: static void *CreateHistogram(const char* name,
Comment missing similar to the one above, describing in what circumstances this
gets called.

http://codereview.chromium.org/42020/diff/1/7#newcode76
Line 76: static void AddHistogramSample(void* histogram, int sample) {
ditto

http://codereview.chromium.org/42020/diff/1/7#newcode179
Line 179: // HistogramTimer t = { L"t:foo", NULL, false };
Histograms are not created with a "t:" prefix as far as I can tell.

http://codereview.chromium.org/42020/diff/1/7#newcode196
Line 196: return histogram_ != NULL && start_time_ != 0 && stop_time_ == 0;
Usually we try to not rely on C precedence rules and format conditions in this
way: (a != b) && (b == c)

[DaveMoore, 2009-03-11 14:19:56]

http://codereview.chromium.org/42020/diff/1/2
File include/v8.h (right):

http://codereview.chromium.org/42020/diff/1/2#newcode1916
Line 1916: static void
SetAddHistogramSampleFunction(AddHistogramSampleCallback);
On 2009/03/10 23:34:23, iposva wrote:
> Please add comments documenting these two new V8 API calls.

Done.

http://codereview.chromium.org/42020/diff/1/6
File src/counters.cc (right):

http://codereview.chromium.org/42020/diff/1/6#newcode76
Line 76: 
I was following the pattern used by the StatsCounters

On 2009/03/10 23:34:23, iposva wrote:
> These two functions could conceivably also be part of the header file. It is
not
> clear what make GetHistogram() more special compared to Start() or Stop().

http://codereview.chromium.org/42020/diff/1/7
File src/counters.h (right):

http://codereview.chromium.org/42020/diff/1/7#newcode49
Line 49: static void SetAddHistogramSampleFunction(AddHistogramSampleCallback f)
{
On 2009/03/10 23:34:23, iposva wrote:
> Small comment for the two new functions or extend the comment above to be more
> generic.

Done.

http://codereview.chromium.org/42020/diff/1/7#newcode68
Line 68: static void *CreateHistogram(const char* name,
On 2009/03/10 23:34:23, iposva wrote:
> Comment missing similar to the one above, describing in what circumstances
this
> gets called.

Done.

http://codereview.chromium.org/42020/diff/1/7#newcode76
Line 76: static void AddHistogramSample(void* histogram, int sample) {
On 2009/03/10 23:34:23, iposva wrote:
> ditto

Done.

[Dean McNamee, 2009-03-11 16:42:24]

http://codereview.chromium.org/42020/diff/1011/1020
File src/v8-counters.cc (right):

http://codereview.chromium.org/42020/diff/1011/1020#newcode35
Line 35: HistogramTimer Counters::name = { "" #caption, NULL, false, 0, 0 }; \
You don't need the "" here do you?

[DaveMoore, 2009-03-12 16:09:29]

http://codereview.chromium.org/42020/diff/1/7
File src/counters.h (right):

http://codereview.chromium.org/42020/diff/1/7#newcode179
Line 179: // HistogramTimer t = { L"t:foo", NULL, false };
On 2009/03/10 23:34:23, iposva wrote:
> Histograms are not created with a "t:" prefix as far as I can tell.

Done.

http://codereview.chromium.org/42020/diff/1/7#newcode196
Line 196: return histogram_ != NULL && start_time_ != 0 && stop_time_ == 0;
On 2009/03/10 23:34:23, iposva wrote:
> Usually we try to not rely on C precedence rules and format conditions in this
> way: (a != b) && (b == c)

Done.

http://codereview.chromium.org/42020/diff/1011/1020
File src/v8-counters.cc (right):

http://codereview.chromium.org/42020/diff/1011/1020#newcode35
Line 35: HistogramTimer Counters::name = { "" #caption, NULL, false, 0, 0 }; \
On 2009/03/11 16:42:24, Dean McNamee wrote:
> You don't need the "" here do you?

Done.