Added histogram to track distribution of requested idle notifications.

Added histogram to track distribution of requested idle notifications.

BUG=397026
LOG=n
R=rmcilroy@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=22632

[rmcilroy, 2014-07-25 09:38:29]

https://codereview.chromium.org/420753002/diff/1/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/420753002/diff/1/src/heap.cc#newcode58
src/heap.cc:58: // a multiple of Page::kPageSize.
move up to line above

https://codereview.chromium.org/420753002/diff/1/src/heap.cc#newcode5140
src/heap.cc:5140: "V8.IdleNotification", 0, kMaxIdleNotificationHistogramSample,
ditto (V8.IdleNotificationHint or V8.IdleGCAllottedTime).

https://codereview.chromium.org/420753002/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/420753002/diff/1/src/heap.h#newcode2283
src/heap.h:2283: void* idle_notification_histogram_;
I'd probably call this idle_notification_hint_histogram_ (or maybe something
like idle_gc_allotted_time_histogram_ for the new scheduler based time).

Also, any reason this isn't in counters.h - there is already a
gc_idle_notification in HISTOGRAM_TIMER_LIST.

[Hannes Payer (out of office), 2014-07-25 10:08:20]

https://codereview.chromium.org/420753002/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/420753002/diff/1/src/heap.h#newcode2283
src/heap.h:2283: void* idle_notification_histogram_;
On 2014/07/25 09:38:29, rmcilroy wrote:
> I'd probably call this idle_notification_hint_histogram_ (or maybe something
> like idle_gc_allotted_time_histogram_ for the new scheduler based time).
> 
> Also, any reason this isn't in counters.h - there is already a
> gc_idle_notification in HISTOGRAM_TIMER_LIST.

Yeah, we do not really have a category for this counter. I could create a "fake"
HistogramTimer. The only similar histogram I found was the script_histogram_.
WDYT?

[rmcilroy, 2014-07-25 10:17:18]

On 2014/07/25 10:08:20, Hannes Payer wrote:
> https://codereview.chromium.org/420753002/diff/1/src/heap.h
> File src/heap.h (right):
> 
> https://codereview.chromium.org/420753002/diff/1/src/heap.h#newcode2283
> src/heap.h:2283: void* idle_notification_histogram_;
> On 2014/07/25 09:38:29, rmcilroy wrote:
> > I'd probably call this idle_notification_hint_histogram_ (or maybe something
> > like idle_gc_allotted_time_histogram_ for the new scheduler based time).
> > 
> > Also, any reason this isn't in counters.h - there is already a
> > gc_idle_notification in HISTOGRAM_TIMER_LIST.
> 
> Yeah, we do not really have a category for this counter. I could create a
"fake"
> HistogramTimer. The only similar histogram I found was the script_histogram_.
> WDYT?

Ahh yeah, I forgot HISTOGRAM_TIMER_LIST creates special HistogramTimer and isn't
manual.  I don't think it's worth faking a HistogramTimer, what you have is
probably fine IMO (although I don't know if there is a reason that all
histograms, other than script_histogram, are defined in counters.h other than
convenience.

[jochen (gone - plz use gerrit), 2014-07-25 10:18:18]

On 2014/07/25 at 10:08:20, hpayer wrote:
> https://codereview.chromium.org/420753002/diff/1/src/heap.h
> File src/heap.h (right):
> 
> https://codereview.chromium.org/420753002/diff/1/src/heap.h#newcode2283
> src/heap.h:2283: void* idle_notification_histogram_;
> On 2014/07/25 09:38:29, rmcilroy wrote:
> > I'd probably call this idle_notification_hint_histogram_ (or maybe something
> > like idle_gc_allotted_time_histogram_ for the new scheduler based time).
> > 
> > Also, any reason this isn't in counters.h - there is already a
> > gc_idle_notification in HISTOGRAM_TIMER_LIST.
> 
> Yeah, we do not really have a category for this counter. I could create a
"fake" HistogramTimer. The only similar histogram I found was the
script_histogram_. WDYT?

you should create a new category in counters.h (and file a bug for
script_histogram_) - those histograms won't work, because they're not correctly
initialized via StatsTable::ResetHistograms()

[Hannes Payer (out of office), 2014-07-25 11:26:25]

I added a new category.

[rmcilroy, 2014-07-25 13:49:42]

On 2014/07/25 11:26:25, Hannes Payer wrote:
> I added a new category.

I think you need to add code to counters.cc don't you?  E.g., creating the
Histogram object in Counters::Counters(...).

[Hannes Payer (out of office), 2014-07-25 16:03:52]

Of course, probably didn't save the file :-/ Please have another look.

[rmcilroy, 2014-07-25 16:09:28]

lgtm with a nit and an optional suggestion.

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc#newcode58
src/counters.cc:58: #define HR(name, caption) name##_ = Histogram(#caption, 0,
1000, 1001, isolate);
nit - newline after ")" like those below.

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc#newcode58
src/counters.cc:58: #define HR(name, caption) name##_ = Histogram(#caption, 0,
1000, 1001, isolate);
optional suggestion - if you want this to be generic, maybe you could add
arguments to HR for the range and number of buckets, then they could be
parameterized seperately for each entry in the HISTOGRAM_RANGE_LIST list, wdyt?

[jochen (gone - plz use gerrit), 2014-07-28 08:44:44]

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc#newcode58
src/counters.cc:58: #define HR(name, caption) name##_ = Histogram(#caption, 0,
1000, 1001, isolate);
why cap it at 1000? We might want to allow for larger idle times in the future,
no?

Also, 1001 buckets is a lot, we typically would use 50 or 100 buckets.

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc#newcode150
src/counters.cc:150: HISTOGRAM_MEMORY_LIST(HR)
HISTOGRAM_RANGE_LIST?

[Hannes Payer (out of office), 2014-07-28 09:01:13]

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc#newcode58
src/counters.cc:58: #define HR(name, caption) name##_ = Histogram(#caption, 0,
1000, 1001, isolate);
On 2014/07/25 16:09:28, rmcilroy wrote:
> optional suggestion - if you want this to be generic, maybe you could add
> arguments to HR for the range and number of buckets, then they could be
> parameterized seperately for each entry in the HISTOGRAM_RANGE_LIST list,
wdyt?

Good idea, done.

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc#newcode58
src/counters.cc:58: #define HR(name, caption) name##_ = Histogram(#caption, 0,
1000, 1001, isolate);
On 2014/07/25 16:09:28, rmcilroy wrote:
> nit - newline after ")" like those below.

git cl format moved it up :-/ I will try to force it to keep it below

[Hannes Payer (out of office), 2014-07-28 09:02:40]

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc
File src/counters.cc (right):

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc#newcode58
src/counters.cc:58: #define HR(name, caption) name##_ = Histogram(#caption, 0,
1000, 1001, isolate);
On 2014/07/28 08:44:43, jochen wrote:
> why cap it at 1000? We might want to allow for larger idle times in the
future,
> no?
> 
> Also, 1001 buckets is a lot, we typically would use 50 or 100 buckets.

Yeah, I thought so as well. I increased the max, but I would like to keep at
least a granularity of 10. For the lower numbers it is nice to have a smaller
granularity. For larger numbers I do not care.

https://codereview.chromium.org/420753002/diff/60001/src/counters.cc#newcode150
src/counters.cc:150: HISTOGRAM_MEMORY_LIST(HR)
On 2014/07/28 08:44:43, jochen wrote:
> HISTOGRAM_RANGE_LIST?

Already fixed.

[Hannes Payer (out of office), 2014-07-28 09:13:40]

Committed patchset #5 manually as r22632 (presubmit successful).