Add V8 heap statistics in a time-line manner in tracing.

Add V8 heap statistics in a time-line manner in tracing.

BUG=393091

[Dai Mikurube (NOT FULLTIME), 2014-03-21 16:04:58]

Just a memo. Got an error in building.

Undefined symbols for architecture i386:
  "content::V8HeapStatisticsMonitor::IsMonitoring() const", referenced from:
      content::RenderWidget::DidCommitCompositorFrame() in
libcontent_renderer.a(content_renderer.render_widget.o)
 
"content::V8HeapStatisticsMonitor::V8HeapStatisticsMonitor(scoped_refptr<base::SingleThreadTaskRunner>)",
referenced from:
      content::RenderThreadImpl::Init() in
libcontent_renderer.a(content_renderer.render_thread_impl.o)
  "content::V8HeapStatisticsMonitor::BeginDumpingHeapStats()", referenced from:
      content::RenderWidget::DidCommitCompositorFrame() in
libcontent_renderer.a(content_renderer.render_widget.o)
  "content::V8HeapStatisticsCollector::Instance()", referenced from:
      ChromeRenderProcessObserver::OnControlMessageReceived(IPC::Message const&)
in librenderer.a(renderer.chrome_render_process_observer.o)

[Dai Mikurube (NOT FULLTIME), 2014-03-21 16:13:27]

Ah, I just forgot gyp_chromium.

[Dai Mikurube (NOT FULLTIME), 2014-03-21 16:16:30]

See also: https://codereview.chromium.org/200843002/

[Dai Mikurube (NOT FULLTIME), 2014-03-21 16:38:15]

It worked in about:tracing!

[Dai Mikurube (NOT FULLTIME), 2014-04-11 07:17:52]

I forgot to loop in nduca@. It's a rebased version of your patch.

[nduca, 2014-06-25 02:46:39]

oh i just noticed this patch. is this still something you want to land? :$

[Dai Mikurube (NOT FULLTIME), 2014-06-25 07:50:43]

Ah, I'm not very actively working on it and memory usage testing. (I suspended
it for a while since I was not able to catch up Telemetry's speedy changes.)

But, it's nice if I could land it. (It's almost your patch, though. :))

[Dai Mikurube (NOT FULLTIME), 2014-07-07 05:54:53]

+kouhei

I'll rebase it later soon.

[Dai Mikurube (NOT FULLTIME), 2014-07-07 08:33:28]

Rebased.

[nduca, 2014-07-07 17:13:53]

heheh lgtm on my part. + @fmeawad and @rschoen as fyi since this improves our
ability to support v8 via about:tracing. i think your main challenge here is lg
from jochen. :)

[nduca, 2014-07-07 17:14:23]

+rschon & fmeawad for real

[Dai Mikurube (NOT FULLTIME), 2014-07-08 05:53:50]

Thanks, Nat. :)

fmeawad and rschoen: could you take a look? (I'll update the subject later.)

[fmeawad, 2014-07-10 18:21:17]

On 2014/07/08 05:53:50, Dai Mikurube wrote:
> Thanks, Nat. :)
> 
> fmeawad and rschoen: could you take a look? (I'll update the subject later.)

looks good, we are looking forward to have more V8 related numbers in tracing.
I would recommend jochen for review. (You need owner review as well).

[fmeawad, 2014-07-10 18:22:23]

Were these files moved, or the year need update?

https://codereview.chromium.org/196103006/diff/60001/content/public/renderer/...
File content/public/renderer/v8_heap_statistics_collector.h (right):

https://codereview.chromium.org/196103006/diff/60001/content/public/renderer/...
content/public/renderer/v8_heap_statistics_collector.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
2014?

https://codereview.chromium.org/196103006/diff/60001/content/renderer/v8_heap...
File content/renderer/v8_heap_statistics_collector_impl.h (right):

https://codereview.chromium.org/196103006/diff/60001/content/renderer/v8_heap...
content/renderer/v8_heap_statistics_collector_impl.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2014?

[Dai Mikurube (NOT FULLTIME), 2014-07-11 04:34:38]

Thanks, fmeawad!

Jochen, could you take a look?

https://codereview.chromium.org/196103006/diff/60001/content/public/renderer/...
File content/public/renderer/v8_heap_statistics_collector.h (right):

https://codereview.chromium.org/196103006/diff/60001/content/public/renderer/...
content/public/renderer/v8_heap_statistics_collector.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
On 2014/07/10 18:22:23, fmeawad-cr wrote:
> 2014?

Done.

https://codereview.chromium.org/196103006/diff/60001/content/renderer/v8_heap...
File content/renderer/v8_heap_statistics_collector_impl.h (right):

https://codereview.chromium.org/196103006/diff/60001/content/renderer/v8_heap...
content/renderer/v8_heap_statistics_collector_impl.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2014/07/10 18:22:23, fmeawad-cr wrote:
> 2014?

Done.

[jochen (gone - plz use gerrit), 2014-07-11 09:19:41]

why not collect the stats per worker (instead of showing them all together in
one metric)?

You could also consider using v8::Isolate::RequestInterrupt() to interrupt a
running worker to get its heap stats, wdyt?

https://codereview.chromium.org/196103006/diff/100001/content/public/renderer...
File content/public/renderer/v8_heap_statistics_collector.h (right):

https://codereview.chromium.org/196103006/diff/100001/content/public/renderer...
content/public/renderer/v8_heap_statistics_collector.h:36: protected:
you don't need to make those protected, since you have pure virtual methods.
Nobody can instantiate this class...

it should be enough to have a public virtual dtor

https://codereview.chromium.org/196103006/diff/100001/content/renderer/v8_hea...
File content/renderer/v8_heap_statistics_collector_impl.cc (right):

https://codereview.chromium.org/196103006/diff/100001/content/renderer/v8_hea...
content/renderer/v8_heap_statistics_collector_impl.cc:92: void
V8HeapStatisticsCollectorImpl::ReceiveStats(int round_id,
please clang-format

https://codereview.chromium.org/196103006/diff/100001/content/renderer/v8_hea...
File content/renderer/v8_heap_statistics_collector_impl.h (right):

https://codereview.chromium.org/196103006/diff/100001/content/renderer/v8_hea...
content/renderer/v8_heap_statistics_collector_impl.h:17: static
V8HeapStatisticsCollector* Instance();
nit. static before ctor

[Dai Mikurube (NOT FULLTIME), 2014-07-17 05:21:39]

Thanks, Jochen.

The main reason of all together in one metric is consistency with the existing
ones. Task manager and memory_internals use (V8)HeapStatisticsCollector. Metrics
for each worker would finally be better, but I thought that starting from this
point is good enough because changing V8HeapStatisticsCollector may cause bigger
change on existing components.

I think collecting stats per worker would be a good next step. Wdyt, jochen and
nduca?

https://codereview.chromium.org/196103006/diff/100001/content/public/renderer...
File content/public/renderer/v8_heap_statistics_collector.h (right):

https://codereview.chromium.org/196103006/diff/100001/content/public/renderer...
content/public/renderer/v8_heap_statistics_collector.h:36: protected:
On 2014/07/11 09:19:41, jochen wrote:
> you don't need to make those protected, since you have pure virtual methods.
> Nobody can instantiate this class...
> 
> it should be enough to have a public virtual dtor

Done.

https://codereview.chromium.org/196103006/diff/100001/content/renderer/v8_hea...
File content/renderer/v8_heap_statistics_collector_impl.cc (right):

https://codereview.chromium.org/196103006/diff/100001/content/renderer/v8_hea...
content/renderer/v8_heap_statistics_collector_impl.cc:92: void
V8HeapStatisticsCollectorImpl::ReceiveStats(int round_id,
On 2014/07/11 09:19:41, jochen wrote:
> please clang-format

Done.

https://codereview.chromium.org/196103006/diff/100001/content/renderer/v8_hea...
File content/renderer/v8_heap_statistics_collector_impl.h (right):

https://codereview.chromium.org/196103006/diff/100001/content/renderer/v8_hea...
content/renderer/v8_heap_statistics_collector_impl.h:17: static
V8HeapStatisticsCollector* Instance();
On 2014/07/11 09:19:41, jochen wrote:
> nit. static before ctor

Done.

[jochen (gone - plz use gerrit), 2014-07-17 07:06:00]

the memory usage will still jump randomly up and down depending on which workers
are busy. You should use RequestInterrupt() and query the usage.

[Dai Mikurube (NOT FULLTIME), 2014-07-17 08:45:01]

I'm not very sure, but I wonder:
* Doesn't it affect performance in some way?
* include/v8.h says that "V8 can remember only a single callback". Doesn't it
affect any other parts?

The Collector is called also from task manager and memory_internals, not only
from about:tracing. Changing it may affect them, existing parts. (In other
words, the current task manager is working with the current
HeapStatisticsCollector.)

[jochen (gone - plz use gerrit), 2014-07-17 09:04:47]

On 2014/07/17 at 08:45:01, dmikurube wrote:
> I'm not very sure, but I wonder:
> * Doesn't it affect performance in some way?
no

> * include/v8.h says that "V8 can remember only a single callback". Doesn't it
affect any other parts?
yeah, that might require synchronizing with oilpan.

> 
> The Collector is called also from task manager and memory_internals, not only
from about:tracing. Changing it may affect them, existing parts. (In other
words, the current task manager is working with the current
HeapStatisticsCollector.)

does the current collector also report a random subsets of workers or just the
main isolate?

taking a step back, for about:tracing, we should just create the right trace
events (or actually, hook them up, because v8 already creates histograms for
heap usage).