[Tracing] Integrate GC object statistics with tracing.

[Tracing] Integrate GC object statistics with tracing.

Memory infra in tracing currently uses Isolate::GetHeapStatistics to fetch
object statistics from V8 at certain frequency, which is not accurate and will
have redundant result. This path adds a trace event as well as a trace category
at where we collect object statistics after marking before sweeping, and dumps
all information to the trace event.

In order to use this functionality, we need to enable two flags:
--track-gc-object-stats and --noincremental-marking.

BUG=v8:5453
Committed: https://crrev.com/a5a9198a9b91619d913161a0d7edf6e2907c70b9
Cr-Commit-Position: refs/heads/master@{#39966}

[lpy, 2016-09-29 20:08:57]

The CQ bit was checked by lpy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 20:09:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lpy, 2016-09-29 20:09:13]

lpy@chromium.org changed reviewers:
+ fmeawad@chromium.org, ssid@chromium.org

[lpy, 2016-09-29 20:09:13]

PTAL

[commit-bot: I haz the power, 2016-09-29 20:38:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 20:38:32]

Dry run: This issue passed the CQ dry run.

[ssid, 2016-09-29 21:45:31]

looks good to me.

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2245: if (FLAG_track_gc_object_stats) {
I thought you wanted to enable this flag when tracing is enabled? Was it not
possible?

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2252:
TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("v8.gc_stats"),
Do we really need a new category? can we just use "v8.gc"?

[lpy, 2016-09-29 22:45:42]

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2245: if (FLAG_track_gc_object_stats) {
On 2016/09/29 21:45:31, ssid wrote:
> I thought you wanted to enable this flag when tracing is enabled? Was it not
> possible?

currently I don't think we can do that, but my plan is to have another iteration
on it to make these flags work with tracing.

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2252:
TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("v8.gc_stats"),
On 2016/09/29 21:45:31, ssid wrote:
> Do we really need a new category? can we just use "v8.gc"?

We can use v8.gc, but, v8.gc will add a bunch of other trace events, which will
bring too many overhead, I want to avoid it.

[lpy, 2016-09-29 22:46:09]

lpy@chromium.org changed reviewers:
+ ulan@chromium.org

[lpy, 2016-09-29 22:46:09]

+ulan@ for review.

[lpy, 2016-09-30 00:35:19]

Description was changed from

==========
[Tracing] Integrate GC object statistics with tracing.

Memory infra in tracing currently use Isolate::GetHeapStatistics to fetch object
statistics from V8 at certain frequency, which is not accurate and will have
redundant result. This path adds a trace event as well as a trace category at
where we collect object statistics after marking before sweeping, and dumps all
information to the trace event.

BUG=
==========

to

==========
[Tracing] Integrate GC object statistics with tracing.

Memory infra in tracing currently uses Isolate::GetHeapStatistics to fetch
object
statistics from V8 at certain frequency, which is not accurate and will have
redundant result. This path adds a trace event as well as a trace category at
where we collect object statistics after marking before sweeping, and dumps all
information to the trace event.

BUG=
==========

[fmeawad, 2016-09-30 00:44:05]

lgtm % merging Print and Dump functionalities.

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2245: if (FLAG_track_gc_object_stats) {
On 2016/09/29 22:45:42, lpy wrote:
> On 2016/09/29 21:45:31, ssid wrote:
> > I thought you wanted to enable this flag when tracing is enabled? Was it not
> > possible?
> 
> currently I don't think we can do that, but my plan is to have another
iteration
> on it to make these flags work with tracing.

Add a comment in the CL description that to test this, please enable the flag

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2252:
TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("v8.gc_stats"),
On 2016/09/29 22:45:42, lpy wrote:
> On 2016/09/29 21:45:31, ssid wrote:
> > Do we really need a new category? can we just use "v8.gc"?
> 
> We can use v8.gc, but, v8.gc will add a bunch of other trace events, which
will
> bring too many overhead, I want to avoid it.

I think gc_stats is fine.
But I wonder if TRACE_EVENT_INSTANT is the right choice here. I am OK with it
for now.

https://codereview.chromium.org/2379823004/diff/1/src/heap/object-stats.cc
File src/heap/object-stats.cc (right):

https://codereview.chromium.org/2379823004/diff/1/src/heap/object-stats.cc#ne...
src/heap/object-stats.cc:45: V8_NOINLINE static void
DumpJSONArray(std::stringstream& stream, size_t* array,
I think you should merge the print and dump functionalities in this file. So
that data would look consistent across. For Print, use the std::out as your
output stream

[ulan, 2016-09-30 08:45:40]

ulan@chromium.org changed reviewers:
+ mlippautz@chromium.org

[ulan, 2016-09-30 08:45:41]

Redirecting to Michael because he did a lot of work with GC object stats and
because I am OOO until Oct 6.

[ulan, 2016-09-30 08:46:06]

ulan@chromium.org changed reviewers:
- ulan@chromium.org

[Michael Lippautz, 2016-09-30 09:09:45]

Thanks for working on this!

If you feel that we need to change the JSON format in some way please reach out
to me. In the end we should settle on something that also works well for the
tracing API and not just the custom tool.

https://codereview.chromium.org/2379823004/diff/1/src/heap/object-stats.cc
File src/heap/object-stats.cc (right):

https://codereview.chromium.org/2379823004/diff/1/src/heap/object-stats.cc#ne...
src/heap/object-stats.cc:45: V8_NOINLINE static void
DumpJSONArray(std::stringstream& stream, size_t* array,
Please merge the printing and dumping where possible, yes.

[Michael Lippautz, 2016-09-30 09:37:39]

Also, if you think that there's more work to be done in v8, maybe create a
tracking bug and reference that in the CL description.

[lpy, 2016-09-30 18:16:01]

Description was changed from

==========
[Tracing] Integrate GC object statistics with tracing.

Memory infra in tracing currently uses Isolate::GetHeapStatistics to fetch
object
statistics from V8 at certain frequency, which is not accurate and will have
redundant result. This path adds a trace event as well as a trace category at
where we collect object statistics after marking before sweeping, and dumps all
information to the trace event.

BUG=
==========

to

==========
[Tracing] Integrate GC object statistics with tracing.
    
Memory infra in tracing currently uses Isolate::GetHeapStatistics to fetch
object statistics from V8 at certain frequency, which is not accurate and will
have redundant result. This path adds a trace event as well as a trace category
at where we collect object statistics after marking before sweeping, and dumps
all information to the trace event.

In order to use this functionality, we need to enable two flags:
--track-gc-object-stats and --noincremental-marking.

BUG=v8:5453
==========

[lpy, 2016-09-30 18:21:58]

I use this JSON format to represent live object and dead object in tracing. See
a skeleton below.

And I create a tracking bug here:
https://bugs.chromium.org/p/v8/issues/detail?id=5453

{
  isolate: $isolate,
  id: $id
  time: $time,
  bucket_size: [...],
  type_data: {
    $name: {
      type: $index,
      overall: $object_sizes_[$index],
      count: $object_counts[$index],
      over_allocated: $over_allocated[$index],
      histogram: [...],
      over_allocated_histogram: [...]
    },
    ...
    END: {}
  }
}

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc
File src/heap/mark-compact.cc (right):

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2245: if (FLAG_track_gc_object_stats) {
On 2016/09/30 00:44:05, fmeawad wrote:
> On 2016/09/29 22:45:42, lpy wrote:
> > On 2016/09/29 21:45:31, ssid wrote:
> > > I thought you wanted to enable this flag when tracing is enabled? Was it
not
> > > possible?
> > 
> > currently I don't think we can do that, but my plan is to have another
> iteration
> > on it to make these flags work with tracing.
> 
> Add a comment in the CL description that to test this, please enable the flag

Done.

https://codereview.chromium.org/2379823004/diff/1/src/heap/mark-compact.cc#ne...
src/heap/mark-compact.cc:2252:
TRACE_EVENT_INSTANT2(TRACE_DISABLED_BY_DEFAULT("v8.gc_stats"),
On 2016/09/30 00:44:05, fmeawad wrote:
> On 2016/09/29 22:45:42, lpy wrote:
> > On 2016/09/29 21:45:31, ssid wrote:
> > > Do we really need a new category? can we just use "v8.gc"?
> > 
> > We can use v8.gc, but, v8.gc will add a bunch of other trace events, which
> will
> > bring too many overhead, I want to avoid it.
> 
> I think gc_stats is fine.
> But I wonder if TRACE_EVENT_INSTANT is the right choice here. I am OK with it
> for now.

Acknowledged.

https://codereview.chromium.org/2379823004/diff/1/src/heap/object-stats.cc
File src/heap/object-stats.cc (right):

https://codereview.chromium.org/2379823004/diff/1/src/heap/object-stats.cc#ne...
src/heap/object-stats.cc:45: V8_NOINLINE static void
DumpJSONArray(std::stringstream& stream, size_t* array,
On 2016/09/30 09:09:44, Michael Lippautz wrote:
> Please merge the printing and dumping where possible, yes.

I am not sure if we should, PrintF will end up calling vprintf.

[Michael Lippautz, 2016-10-04 19:08:41]

LGTM

We should probably just get rid of the JSON printer and also print the tracing
string then.

I will do that once I have some spare cycles.

[lpy, 2016-10-04 23:50:02]

The CQ bit was checked by lpy@chromium.org

[commit-bot: I haz the power, 2016-10-04 23:50:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 00:41:30]

Description was changed from

==========
[Tracing] Integrate GC object statistics with tracing.
    
Memory infra in tracing currently uses Isolate::GetHeapStatistics to fetch
object statistics from V8 at certain frequency, which is not accurate and will
have redundant result. This path adds a trace event as well as a trace category
at where we collect object statistics after marking before sweeping, and dumps
all information to the trace event.

In order to use this functionality, we need to enable two flags:
--track-gc-object-stats and --noincremental-marking.

BUG=v8:5453
==========

to

==========
[Tracing] Integrate GC object statistics with tracing.
    
Memory infra in tracing currently uses Isolate::GetHeapStatistics to fetch
object statistics from V8 at certain frequency, which is not accurate and will
have redundant result. This path adds a trace event as well as a trace category
at where we collect object statistics after marking before sweeping, and dumps
all information to the trace event.

In order to use this functionality, we need to enable two flags:
--track-gc-object-stats and --noincremental-marking.

BUG=v8:5453
==========

[commit-bot: I haz the power, 2016-10-05 00:41:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-05 00:41:53]

Description was changed from

==========
[Tracing] Integrate GC object statistics with tracing.
    
Memory infra in tracing currently uses Isolate::GetHeapStatistics to fetch
object statistics from V8 at certain frequency, which is not accurate and will
have redundant result. This path adds a trace event as well as a trace category
at where we collect object statistics after marking before sweeping, and dumps
all information to the trace event.

In order to use this functionality, we need to enable two flags:
--track-gc-object-stats and --noincremental-marking.

BUG=v8:5453
==========

to

==========
[Tracing] Integrate GC object statistics with tracing.

Memory infra in tracing currently uses Isolate::GetHeapStatistics to fetch
object statistics from V8 at certain frequency, which is not accurate and will
have redundant result. This path adds a trace event as well as a trace category
at where we collect object statistics after marking before sweeping, and dumps
all information to the trace event.

In order to use this functionality, we need to enable two flags:
--track-gc-object-stats and --noincremental-marking.

BUG=v8:5453

Committed: https://crrev.com/a5a9198a9b91619d913161a0d7edf6e2907c70b9
Cr-Commit-Position: refs/heads/master@{#39966}
==========

[commit-bot: I haz the power, 2016-10-05 00:41:54]

Patchset 1 (id:??) landed as
https://crrev.com/a5a9198a9b91619d913161a0d7edf6e2907c70b9
Cr-Commit-Position: refs/heads/master@{#39966}