[tracing] Adding support for light dumps in blink gc dump provider

[tracing] Adding support for light dumps in blink gc dump provider

On light dump request the blink gc dump provider does not call a
snapshot GC and just dumps totals of the process. Also thread id is
removed from the dump name of the totals, since it represents all the
threads together.

BUG=499731
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200317

[ssid, 2015-08-10 17:36:38]

ssid@chromium.org changed reviewers:
+ haraken@chromium.org

[ssid, 2015-08-10 17:36:39]

PTAL. Thanks

https://codereview.chromium.org/1281673003/diff/40001/Source/platform/heap/Bl...
File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1281673003/diff/40001/Source/platform/heap/Bl...
Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:41:
Heap::collectGarbage(ThreadState::NoHeapPointersOnStack,
ThreadState::TakeSnapshot, Heap::ForcedGC);
I guess "Heap::allocatedSpace()" and "Heap::allocatedObjectSize() +
Heap::markedObjectSize()" do not change when doing a snapshot GC. So, dumping
these numbers before or after GC should not make a difference?

[haraken, 2015-08-10 23:41:14]

https://codereview.chromium.org/1281673003/diff/40001/Source/platform/heap/Bl...
File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right):

https://codereview.chromium.org/1281673003/diff/40001/Source/platform/heap/Bl...
Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:41:
Heap::collectGarbage(ThreadState::NoHeapPointersOnStack,
ThreadState::TakeSnapshot, Heap::ForcedGC);
On 2015/08/10 17:36:39, ssid wrote:
> I guess "Heap::allocatedSpace()" and "Heap::allocatedObjectSize() +
> Heap::markedObjectSize()" do not change when doing a snapshot GC. So, dumping
> these numbers before or after GC should not make a difference?

Heap::allocatedSpace() and Heap::allocatedObjectSize() do not change, but
Heap::markedObjectSize() increases in preGC() called at the first phase of the
snapshot GC. So I'd prefer running the snapshot GC before getting the value of
Heap::markedObjectSize().

[ssid, 2015-08-10 23:51:04]

On 2015/08/10 23:41:14, haraken wrote:
>
https://codereview.chromium.org/1281673003/diff/40001/Source/platform/heap/Bl...
> File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right):
> 
>
https://codereview.chromium.org/1281673003/diff/40001/Source/platform/heap/Bl...
> Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:41:
> Heap::collectGarbage(ThreadState::NoHeapPointersOnStack,
> ThreadState::TakeSnapshot, Heap::ForcedGC);
> On 2015/08/10 17:36:39, ssid wrote:
> > I guess "Heap::allocatedSpace()" and "Heap::allocatedObjectSize() +
> > Heap::markedObjectSize()" do not change when doing a snapshot GC. So,
dumping
> > these numbers before or after GC should not make a difference?
> 
> Heap::allocatedSpace() and Heap::allocatedObjectSize() do not change, but
> Heap::markedObjectSize() increases in preGC() called at the first phase of the
> snapshot GC. So I'd prefer running the snapshot GC before getting the value of
> Heap::markedObjectSize().

Hm, yes. If I'm taking a light dump, then blink gc is not triggered, and only
total size is taken. In that case, is it correct to dump object size as
Heap::allocatedObjectSize() +
> > Heap::markedObjectSize() ? Please look at patch 2 if that is correct.

[haraken, 2015-08-11 00:02:13]

On 2015/08/10 23:51:04, ssid wrote:
> On 2015/08/10 23:41:14, haraken wrote:
> >
>
https://codereview.chromium.org/1281673003/diff/40001/Source/platform/heap/Bl...
> > File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1281673003/diff/40001/Source/platform/heap/Bl...
> > Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:41:
> > Heap::collectGarbage(ThreadState::NoHeapPointersOnStack,
> > ThreadState::TakeSnapshot, Heap::ForcedGC);
> > On 2015/08/10 17:36:39, ssid wrote:
> > > I guess "Heap::allocatedSpace()" and "Heap::allocatedObjectSize() +
> > > Heap::markedObjectSize()" do not change when doing a snapshot GC. So,
> dumping
> > > these numbers before or after GC should not make a difference?
> > 
> > Heap::allocatedSpace() and Heap::allocatedObjectSize() do not change, but
> > Heap::markedObjectSize() increases in preGC() called at the first phase of
the
> > snapshot GC. So I'd prefer running the snapshot GC before getting the value
of
> > Heap::markedObjectSize().
> 
> Hm, yes. If I'm taking a light dump, then blink gc is not triggered, and only
> total size is taken. In that case, is it correct to dump object size as
> Heap::allocatedObjectSize() +
> > > Heap::markedObjectSize() ? Please look at patch 2 if that is correct.

Yeah, I'd prefer patch set 2. It will dump an incorrect Heap::markedObjectSize()
for light dumps but dump a correct Heap::markedObjectSize() for full dumps. It
is better than dumping incorrect values for both light and non-light dumps (in
patch set 3).

[haraken, 2015-08-11 00:02:55]

On 2015/08/11 00:02:13, haraken wrote:
> On 2015/08/10 23:51:04, ssid wrote:
> > On 2015/08/10 23:41:14, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/1281673003/diff/40001/Source/platform/heap/Bl...
> > > File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1281673003/diff/40001/Source/platform/heap/Bl...
> > > Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:41:
> > > Heap::collectGarbage(ThreadState::NoHeapPointersOnStack,
> > > ThreadState::TakeSnapshot, Heap::ForcedGC);
> > > On 2015/08/10 17:36:39, ssid wrote:
> > > > I guess "Heap::allocatedSpace()" and "Heap::allocatedObjectSize() +
> > > > Heap::markedObjectSize()" do not change when doing a snapshot GC. So,
> > dumping
> > > > these numbers before or after GC should not make a difference?
> > > 
> > > Heap::allocatedSpace() and Heap::allocatedObjectSize() do not change, but
> > > Heap::markedObjectSize() increases in preGC() called at the first phase of
> the
> > > snapshot GC. So I'd prefer running the snapshot GC before getting the
value
> of
> > > Heap::markedObjectSize().
> > 
> > Hm, yes. If I'm taking a light dump, then blink gc is not triggered, and
only
> > total size is taken. In that case, is it correct to dump object size as
> > Heap::allocatedObjectSize() +
> > > > Heap::markedObjectSize() ? Please look at patch 2 if that is correct.
> 
> Yeah, I'd prefer patch set 2. It will dump an incorrect
Heap::markedObjectSize()
> for light dumps but dump a correct Heap::markedObjectSize() for full dumps. It
> is better than dumping incorrect values for both light and non-light dumps (in
> patch set 3).

Would you add a comment about it?

// Heap::markedObjectSize() can be underestimated if we're still in the process
of lazy sweeping.

[ssid, 2015-08-11 08:40:30]

Patchset #3 (id:40001) has been deleted

[ssid, 2015-08-11 09:07:00]

Added a comment. Thanks!

[haraken, 2015-08-11 09:16:41]

LGTM

[ssid, 2015-08-11 13:44:41]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-08-11 13:44:41]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1281673003/#ps80001
(title: "Fixing test.")

[commit-bot: I haz the power, 2015-08-11 13:44:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1281673003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1281673003/80001

[commit-bot: I haz the power, 2015-08-11 13:50:51]

Committed patchset #4 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200317