Adding freelist statistics to blink gc dump provider.

Adding freelist statistics to blink gc dump provider.

This is the third version of the blink gc dump provider. It now dumpsthe freelist statistics of the normal page heap. The snapshot GC
rebuilds the freelists, so it makes sense to dump the freelist stats
just after the GC.

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

[ssid, 2015-06-23 05:05:42]

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

[ssid, 2015-06-23 05:05:42]

PTAL.

[haraken, 2015-06-23 05:38:58]

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

[haraken, 2015-06-23 05:38:59]

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1143: String dumpName =
dumpBaseName.isolatedCopy();

Shall we pass in a String to takeSnapshot and avoid calling isolatedCopy?

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1144:
dumpName.append(String::format("/bucket_%lu", static_cast<unsigned long>(i)));

It would be more informative to use 2^i instead of i, since the bucket i
contains free slots of size [2^i, 2^(i+1)).

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1313: #if ENABLE(GC_PROFILING)

This function is performance-sensitive and we don't want to add extra overhead.
Shall we drop this change at least from this change? i.e., we want to keep these
variables only in GC_PROFILING.

[ssid, 2015-06-23 06:03:39]

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1143: String dumpName =
dumpBaseName.isolatedCopy();
On 2015/06/23 05:38:58, haraken wrote:
> 
> Shall we pass in a String to takeSnapshot and avoid calling isolatedCopy?

hm, This is inside the for loop. I can't find a better way to do string append.
Do you have any other suggestions?

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1313: #if ENABLE(GC_PROFILING)
On 2015/06/23 05:38:58, haraken wrote:
> 
> This function is performance-sensitive and we don't want to add extra
overhead.
> Shall we drop this change at least from this change? i.e., we want to keep
these
> variables only in GC_PROFILING.
> 

This means that these values will never be added to trace. Should I have
GC_PROFILING macro in the dumper also, so that these will be added when the flag
is enabled? primiano@ WDYT?

[haraken, 2015-06-23 06:24:24]

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1143: String dumpName =
dumpBaseName.isolatedCopy();
On 2015/06/23 06:03:39, ssid wrote:
> On 2015/06/23 05:38:58, haraken wrote:
> > 
> > Shall we pass in a String to takeSnapshot and avoid calling isolatedCopy?
> 
> hm, This is inside the for loop. I can't find a better way to do string
append.
> Do you have any other suggestions?

For string append, we're encouraged to use StringBuilder.

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1313: #if ENABLE(GC_PROFILING)
On 2015/06/23 06:03:39, ssid wrote:
> On 2015/06/23 05:38:58, haraken wrote:
> > 
> > This function is performance-sensitive and we don't want to add extra
> overhead.
> > Shall we drop this change at least from this change? i.e., we want to keep
> these
> > variables only in GC_PROFILING.
> > 
> 
> This means that these values will never be added to trace. Should I have
> GC_PROFILING macro in the dumper also, so that these will be added when the
flag
> is enabled? primiano@ WDYT?

Yeah, it's a bit unfortunate we can't have the values in the trace, but I'd
suggest revisiting the issue later. We need to study performance to add the
values. IMO, these values are not that important to understand Oilpan's
workload.

GC_PROFILING builds are super heavy, so it's not an option to enable it on trunk
:-/

[ssid, 2015-06-23 07:39:57]

Made changes.

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1143: String dumpName =
dumpBaseName.isolatedCopy();
On 2015/06/23 06:24:24, haraken wrote:
> On 2015/06/23 06:03:39, ssid wrote:
> > On 2015/06/23 05:38:58, haraken wrote:
> > > 
> > > Shall we pass in a String to takeSnapshot and avoid calling isolatedCopy?
> > 
> > hm, This is inside the for loop. I can't find a better way to do string
> append.
> > Do you have any other suggestions?
> 
> For string append, we're encouraged to use StringBuilder.

Sorry, i just realized it has a + operator, thought it looks cleaner. If you
still feel I should use StringBuilder, I will change it.

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1144:
dumpName.append(String::format("/bucket_%lu", static_cast<unsigned long>(i)));
On 2015/06/23 05:38:58, haraken wrote:
> 
> It would be more informative to use 2^i instead of i, since the bucket i
> contains free slots of size [2^i, 2^(i+1)).

Done.

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (left):

https://codereview.chromium.org/1200833008/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1313: #if ENABLE(GC_PROFILING)
On 2015/06/23 06:24:24, haraken wrote:
> On 2015/06/23 06:03:39, ssid wrote:
> > On 2015/06/23 05:38:58, haraken wrote:
> > > 
> > > This function is performance-sensitive and we don't want to add extra
> > overhead.
> > > Shall we drop this change at least from this change? i.e., we want to keep
> > these
> > > variables only in GC_PROFILING.
> > > 
> > 
> > This means that these values will never be added to trace. Should I have
> > GC_PROFILING macro in the dumper also, so that these will be added when the
> flag
> > is enabled? primiano@ WDYT?
> 
> Yeah, it's a bit unfortunate we can't have the values in the trace, but I'd
> suggest revisiting the issue later. We need to study performance to add the
> values. IMO, these values are not that important to understand Oilpan's
> workload.
> 
> GC_PROFILING builds are super heavy, so it's not an option to enable it on
trunk
> :-/

Okay, removing it.

[haraken, 2015-06-23 08:14:50]

https://codereview.chromium.org/1200833008/diff/50001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1200833008/diff/50001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1138: String dumpName = dumpBaseName +
String::format("/buckets/bucket_%lu", static_cast<unsigned long>(1 << i));

I'm just curious, but where is the String::operator+ defined? I think we removed
it since String::operator+ is slow and thus we should encourage developers to
use StringBuilder...

[Yuta Kitamura, 2015-06-23 08:45:11]

On 2015/06/23 08:14:50, haraken wrote:
> I'm just curious, but where is the String::operator+ defined? I think we
removed
> it since String::operator+ is slow and thus we should encourage developers to
> use StringBuilder...

Nope, actually the opposite. We made operator+ fast, so
we started to recommend concatenating with operator+.

FWIF operator+ is defined in StringOperators.h.

[haraken, 2015-06-23 08:54:29]

On 2015/06/23 08:45:11, Yuta Kitamura wrote:
> On 2015/06/23 08:14:50, haraken wrote:
> > I'm just curious, but where is the String::operator+ defined? I think we
> removed
> > it since String::operator+ is slow and thus we should encourage developers
to
> > use StringBuilder...
> 
> Nope, actually the opposite. We made operator+ fast, so
> we started to recommend concatenating with operator+.
> 
> FWIF operator+ is defined in StringOperators.h.

oh, I was misunderstanding! Thanks.

[haraken, 2015-06-23 08:55:34]

LGTM

[Primiano Tucci (use gerrit), 2015-06-23 16:45:15]

On 2015/06/23 08:55:34, haraken wrote:
> LGTM

I applied this patch locally, but all the buckets that I see have 0 size, 0
freesize and 0 freelist_entry_count.
Is that expected?

[haraken, 2015-06-24 03:46:21]

https://codereview.chromium.org/1200833008/diff/50001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1200833008/diff/50001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:966: 

Sorry, you need to call takeSnapshotOfFreeList here (i.e., after
makeConsistentForMutator). makeConsistentForMutator rebuilds the free list.

[ssid, 2015-06-24 05:45:30]

I have moved the freelist snapshot call after makeConsistentForMutator. Also
added edge between buckets/ and pages/.
PTAL.

https://codereview.chromium.org/1200833008/diff/50001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1200833008/diff/50001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:966: 
On 2015/06/24 03:46:21, haraken wrote:
> 
> Sorry, you need to call takeSnapshotOfFreeList here (i.e., after
> makeConsistentForMutator). makeConsistentForMutator rebuilds the free list.
> 

Done.

[haraken, 2015-06-24 05:55:06]

Thanks for being persistent on this... I appreciate it!

https://codereview.chromium.org/1200833008/diff/70001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1200833008/diff/70001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:967:
takeSnapshot(SnapshotType::FreelistSnapshot);

May I ask you one more favor? What happens if you take a snapshot of freelists
before taking a snapshot of the heap?

I mean, what happens if you call
m_heaps[HeapType##HeapIndex]->takeFreelistSnapshot() just before calling
m_heaps[HeapType##HeapIndex]->takeSnapshot()?

If it works, it would be better since it will capture the actual shape of the
freelists (instead of the rebuilt freelists after the snapshot GC).

[ssid, 2015-06-24 06:22:17]

On 2015/06/24 05:55:06, haraken wrote:
> Thanks for being persistent on this... I appreciate it!
> 
>
https://codereview.chromium.org/1200833008/diff/70001/Source/platform/heap/Th...
> File Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/1200833008/diff/70001/Source/platform/heap/Th...
> Source/platform/heap/ThreadState.cpp:967:
> takeSnapshot(SnapshotType::FreelistSnapshot);
> 
> May I ask you one more favor? What happens if you take a snapshot of freelists
> before taking a snapshot of the heap?
> 
> I mean, what happens if you call
> m_heaps[HeapType##HeapIndex]->takeFreelistSnapshot() just before calling
> m_heaps[HeapType##HeapIndex]->takeSnapshot()?
> 
> If it works, it would be better since it will capture the actual shape of the
> freelists (instead of the rebuilt freelists after the snapshot GC).

How is it different from taking freelist snapshot just after the heap?
ie. 
m_heaps[HeapType##HeapIndex]->takeSnapshot()
m_heaps[HeapType##HeapIndex]->takeFreelistSnapshot() ?

This was the previous version which returned 0 for all the buckets.

[haraken, 2015-06-24 07:23:44]

On 2015/06/24 06:22:17, ssid wrote:
> On 2015/06/24 05:55:06, haraken wrote:
> > Thanks for being persistent on this... I appreciate it!
> > 
> >
>
https://codereview.chromium.org/1200833008/diff/70001/Source/platform/heap/Th...
> > File Source/platform/heap/ThreadState.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1200833008/diff/70001/Source/platform/heap/Th...
> > Source/platform/heap/ThreadState.cpp:967:
> > takeSnapshot(SnapshotType::FreelistSnapshot);
> > 
> > May I ask you one more favor? What happens if you take a snapshot of
freelists
> > before taking a snapshot of the heap?
> > 
> > I mean, what happens if you call
> > m_heaps[HeapType##HeapIndex]->takeFreelistSnapshot() just before calling
> > m_heaps[HeapType##HeapIndex]->takeSnapshot()?
> > 
> > If it works, it would be better since it will capture the actual shape of
the
> > freelists (instead of the rebuilt freelists after the snapshot GC).
> 
> How is it different from taking freelist snapshot just after the heap?
> ie. 
> m_heaps[HeapType##HeapIndex]->takeSnapshot()
> m_heaps[HeapType##HeapIndex]->takeFreelistSnapshot() ?
> 
> This was the previous version which returned 0 for all the buckets.

Ah, sorry, you're right. There would be no difference since the freelists are
cleared in preGC()...

LGTM.

[Primiano Tucci (use gerrit), 2015-06-24 08:43:34]

LGTM to get things moving.
I still have some doubt as regards as the data that comes out when applying both
CLs.
I grabbed a trace opening theverge.com and the data I see for blink_gc is as
follows:

total: 1.0 M
  allocated objects: 16 K
which suggests me that there are just 16k requested by blinkgc clients and a
total of 1M totally requested from blinkgc to the system.
Is it just 16K / 1M the expected size for a heavy site like theverge.com?
At the same time I see that some heaps, for instance CSSValue have a free_size
of 1.8 M
How can the free_size of CSSValue be >> the total size of blink_gc?

See www/~primiano/memory_infra/misc/blink_gc_doubts.png

[haraken, 2015-06-24 08:51:41]

On 2015/06/24 08:43:34, Primiano Tucci wrote:
> LGTM to get things moving.
> I still have some doubt as regards as the data that comes out when applying
both
> CLs.
> I grabbed a trace opening http://theverge.com and the data I see for blink_gc
is as
> follows:
> 
> total: 1.0 M
>   allocated objects: 16 K
> which suggests me that there are just 16k requested by blinkgc clients and a
> total of 1M totally requested from blinkgc to the system.
> Is it just 16K / 1M the expected size for a heavy site like theverge.com?
> At the same time I see that some heaps, for instance CSSValue have a free_size
> of 1.8 M
> How can the free_size of CSSValue be >> the total size of blink_gc?
> 
> See www/~primiano/memory_infra/misc/blink_gc_doubts.png

Hmm, that sounds strange...

Let's land this. Landing them on trunk makes it easier for us to run the
experiment locally :)

[Primiano Tucci (use gerrit), 2015-06-24 09:33:51]

> Let's land this. Landing them on trunk makes it easier for us to run the
> experiment locally :)

Agree. Let's land the CLs
Note: I think this depends on crrev.com/1149703002 which needs to land first,
right?

[ssid, 2015-06-24 09:34:47]

On 2015/06/24 09:33:51, Primiano Tucci wrote:
> > Let's land this. Landing them on trunk makes it easier for us to run the
> > experiment locally :)
> 
> Agree. Let's land the CLs
> Note: I think this depends on crrev.com/1149703002 which needs to land first,
> right?

Ticked that one, will land this next.

[ssid, 2015-06-24 12:45:37]

The CQ bit was checked by ssid@chromium.org

[commit-bot: I haz the power, 2015-06-24 12:45:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1200833008/90001

[commit-bot: I haz the power, 2015-06-24 13:42:39]

Committed patchset #6 (id:90001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197738

[ssid, 2015-06-24 15:40:12]

On 2015/06/24 08:43:34, Primiano Tucci wrote:
> LGTM to get things moving.
> I still have some doubt as regards as the data that comes out when applying
both
> CLs.
> I grabbed a trace opening http://theverge.com and the data I see for blink_gc
is as
> follows:
> 
> total: 1.0 M
>   allocated objects: 16 K
> which suggests me that there are just 16k requested by blinkgc clients and a
> total of 1M totally requested from blinkgc to the system.
> Is it just 16K / 1M the expected size for a heavy site like theverge.com?

This memory is very low because oilpan is not enabled in your build. I get 3-4
Mb building with oilpan-enable=1. I am guessing oilpan is currently not used for
all modules in blink. This will change soon.

> At the same time I see that some heaps, for instance CSSValue have a free_size
> of 1.8 M
> How can the free_size of CSSValue be >> the total size of blink_gc?
> 
> See www/~primiano/memory_infra/misc/blink_gc_doubts.png

Hmm, The free pages don't seem to be accounted in the total size. The total
allocated size (marked + newly allocated) is equal to dead + live objects. And
the total size is the size allocated from system for these allocated objects,
excluding the freed objects.
I am not sure how to get the total size used. I think we should remove the total
numbers from the trace, and the sum of the objects will give the total size
(live + dead + free). I am also not sure if the freed objects are still in
committed memory from the system?

Also, I just realized For GC with snapshot, the takeSnapshot() gets called twice
from two threads. One is main thread, other is some other thread. This causes an
extra dump on the trace from that thread. Can this be fixed, or I could change
the dumper to dump only when on main thread.

primiano@ did you find any other issue?

[haraken, 2015-06-24 23:43:20]

Thanks for being persistent on this!

On 2015/06/24 15:40:12, ssid wrote:
> On 2015/06/24 08:43:34, Primiano Tucci wrote:
> > LGTM to get things moving.
> > I still have some doubt as regards as the data that comes out when applying
> both
> > CLs.
> > I grabbed a trace opening http://theverge.com and the data I see for
blink_gc
> is as
> > follows:
> > 
> > total: 1.0 M
> >   allocated objects: 16 K
> > which suggests me that there are just 16k requested by blinkgc clients and a
> > total of 1M totally requested from blinkgc to the system.
> > Is it just 16K / 1M the expected size for a heavy site like theverge.com?
> 
> This memory is very low because oilpan is not enabled in your build. I get 3-4
> Mb building with oilpan-enable=1. I am guessing oilpan is currently not used
for
> all modules in blink. This will change soon.
> 
> > At the same time I see that some heaps, for instance CSSValue have a
free_size
> > of 1.8 M
> > How can the free_size of CSSValue be >> the total size of blink_gc?
> > 
> > See www/~primiano/memory_infra/misc/blink_gc_doubts.png
> 
> Hmm, The free pages don't seem to be accounted in the total size. The total
> allocated size (marked + newly allocated) is equal to dead + live objects. And
> the total size is the size allocated from system for these allocated objects,
> excluding the freed objects.
> I am not sure how to get the total size used. I think we should remove the
total
> numbers from the trace, and the sum of the objects will give the total size
> (live + dead + free). I am also not sure if the freed objects are still in
> committed memory from the system?


Total allocated size = marked + newly allocated  <--- expected
Total allocated size = live + dead  <--- expected
Total size = the size allocated from system excluding freed objects  <---
unexpected

The total size should be simply the size allocated from system including freed
objects. The memory consumed by the freed objects are committed.

> Also, I just realized For GC with snapshot, the takeSnapshot() gets called
twice
> from two threads. One is main thread, other is some other thread. This causes
an
> extra dump on the trace from that thread. Can this be fixed, or I could change
> the dumper to dump only when on main thread.

I'll take a look at this.

[haraken, 2015-06-25 00:49:27]

On 2015/06/24 23:43:20, haraken wrote:
> Thanks for being persistent on this!
> 
> On 2015/06/24 15:40:12, ssid wrote:
> > On 2015/06/24 08:43:34, Primiano Tucci wrote:
> > > LGTM to get things moving.
> > > I still have some doubt as regards as the data that comes out when
applying
> > both
> > > CLs.
> > > I grabbed a trace opening http://theverge.com and the data I see for
> blink_gc
> > is as
> > > follows:
> > > 
> > > total: 1.0 M
> > >   allocated objects: 16 K
> > > which suggests me that there are just 16k requested by blinkgc clients and
a
> > > total of 1M totally requested from blinkgc to the system.
> > > Is it just 16K / 1M the expected size for a heavy site like theverge.com?
> > 
> > This memory is very low because oilpan is not enabled in your build. I get
3-4
> > Mb building with oilpan-enable=1. I am guessing oilpan is currently not used
> for
> > all modules in blink. This will change soon.
> > 
> > > At the same time I see that some heaps, for instance CSSValue have a
> free_size
> > > of 1.8 M
> > > How can the free_size of CSSValue be >> the total size of blink_gc?
> > > 
> > > See www/~primiano/memory_infra/misc/blink_gc_doubts.png
> > 
> > Hmm, The free pages don't seem to be accounted in the total size. The total
> > allocated size (marked + newly allocated) is equal to dead + live objects.
And
> > the total size is the size allocated from system for these allocated
objects,
> > excluding the freed objects.
> > I am not sure how to get the total size used. I think we should remove the
> total
> > numbers from the trace, and the sum of the objects will give the total size
> > (live + dead + free). I am also not sure if the freed objects are still in
> > committed memory from the system?
> 
> 
> Total allocated size = marked + newly allocated  <--- expected
> Total allocated size = live + dead  <--- expected
> Total size = the size allocated from system excluding freed objects  <---
> unexpected
> 
> The total size should be simply the size allocated from system including freed
> objects. The memory consumed by the freed objects are committed.
> 
> > Also, I just realized For GC with snapshot, the takeSnapshot() gets called
> twice
> > from two threads. One is main thread, other is some other thread. This
causes
> an
> > extra dump on the trace from that thread. Can this be fixed, or I could
change
> > the dumper to dump only when on main thread.

Hmm, this sounds strange. The expected behavior is:

(1) Heap::collectGarbage(TakeSnapshot) is called on a thread X (in most cases
the main thread).
(2) The thread X stops all other threads.
(3) The thread X calls ThreadState::takeSnapshot for all ThreadStates.
(4) The thread X resumes all the other threads.

Do you mean that a thread Y calls ThreadState::takeSnapshot after (4)?

[haraken, 2015-06-25 00:50:21]

https://codereview.chromium.org/1200833008/diff/90001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1200833008/diff/90001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:568: if (m_freeList.takeSnapshot(dumpName) &&
m_firstUnsweptPage) {

What is the '&& m_firstUnsweptPage' check for?

[ssid, 2015-06-25 02:46:24]

> Total allocated size = marked + newly allocated  <--- expected
> Total allocated size = live + dead  <--- expected
> Total size = the size allocated from system excluding freed objects  <---
> unexpected
> 
> The total size should be simply the size allocated from system including freed
> objects. The memory consumed by the freed objects are committed.

Hmm, looks like decreaseAllocatedSpace is called when at freePage(). So,
allocatedSpace doesn't actually include free pages.

[ssid, 2015-06-25 03:08:07]

https://codereview.chromium.org/1200833008/diff/90001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1200833008/diff/90001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:568: if (m_freeList.takeSnapshot(dumpName) &&
m_firstUnsweptPage) {
On 2015/06/25 00:50:21, haraken wrote:
> 
> What is the '&& m_firstUnsweptPage' check for?

the m_firstUnsweptPage checks for if there is any page that is dumped (the
takeSnapshot dumps pages in the list starting with m_firstUnsweptPage). If there
is no page that is dumped then this ownership part is not required.
I am not sure if this check is redundant.

[ssid, 2015-06-25 03:30:56]

> (1) Heap::collectGarbage(TakeSnapshot) is called on a thread X (in most cases
> the main thread).
> (2) The thread X stops all other threads.
> (3) The thread X calls ThreadState::takeSnapshot for all ThreadStates.
> (4) The thread X resumes all the other threads.
> 
> Do you mean that a thread Y calls ThreadState::takeSnapshot after (4)?

Ah, okay. I understand what is going on now. So, looking at the traces, there
are always 2 threads dumped per renderer, and one of them shows always 0 sizes. 

I think I shouldn't dump when the size of page in the heap is 0. This will
resolve the extra thread with just 0s. Primiano@, you told me to remove this
check and return, because the trace should be consistent and should always dump
the pages even when it is 0. What should be done here?

[haraken, 2015-06-25 03:49:23]

On 2015/06/25 03:30:56, ssid wrote:
> > (1) Heap::collectGarbage(TakeSnapshot) is called on a thread X (in most
cases
> > the main thread).
> > (2) The thread X stops all other threads.
> > (3) The thread X calls ThreadState::takeSnapshot for all ThreadStates.
> > (4) The thread X resumes all the other threads.
> > 
> > Do you mean that a thread Y calls ThreadState::takeSnapshot after (4)?
> 
> Ah, okay. I understand what is going on now. So, looking at the traces, there
> are always 2 threads dumped per renderer, and one of them shows always 0
sizes. 
> 
> I think I shouldn't dump when the size of page in the heap is 0. This will
> resolve the extra thread with just 0s. Primiano@, you told me to remove this
> check and return, because the trace should be consistent and should always
dump
> the pages even when it is 0. What should be done here?

I think that's the HTML parser thread, which won't allocate many things.

[haraken, 2015-06-25 03:50:41]

On 2015/06/25 03:49:23, haraken wrote:
> On 2015/06/25 03:30:56, ssid wrote:
> > > (1) Heap::collectGarbage(TakeSnapshot) is called on a thread X (in most
> cases
> > > the main thread).
> > > (2) The thread X stops all other threads.
> > > (3) The thread X calls ThreadState::takeSnapshot for all ThreadStates.
> > > (4) The thread X resumes all the other threads.
> > > 
> > > Do you mean that a thread Y calls ThreadState::takeSnapshot after (4)?
> > 
> > Ah, okay. I understand what is going on now. So, looking at the traces,
there
> > are always 2 threads dumped per renderer, and one of them shows always 0
> sizes. 
> > 
> > I think I shouldn't dump when the size of page in the heap is 0. This will
> > resolve the extra thread with just 0s. Primiano@, you told me to remove this
> > check and return, because the trace should be consistent and should always
> dump
> > the pages even when it is 0. What should be done here?
> 
> I think that's the HTML parser thread, which won't allocate many things.

Maybe we should expose an API like Platform::current()->currentThread()->name()
which gives us the name of that thread. Chrome should know the name (we display
the name in about:tracing).

[haraken, 2015-06-25 03:52:56]

On 2015/06/25 03:08:07, ssid wrote:
>
https://codereview.chromium.org/1200833008/diff/90001/Source/platform/heap/He...
> File Source/platform/heap/Heap.cpp (right):
> 
>
https://codereview.chromium.org/1200833008/diff/90001/Source/platform/heap/He...
> Source/platform/heap/Heap.cpp:568: if (m_freeList.takeSnapshot(dumpName) &&
> m_firstUnsweptPage) {
> On 2015/06/25 00:50:21, haraken wrote:
> > 
> > What is the '&& m_firstUnsweptPage' check for?
> 
> the m_firstUnsweptPage checks for if there is any page that is dumped (the
> takeSnapshot dumps pages in the list starting with m_firstUnsweptPage). If
there
> is no page that is dumped then this ownership part is not required.
> I am not sure if this check is redundant.

Then I think this should be '&& m_firstPage'. Remember that pages in
m_firstUnsweptPage will be moved to m_firstPage in makeConsistentForMutator()
and that takeFreeListSnapshot() is called after makeConsistentForMutator(). I
guess m_firstUnsweptPage is always null here.

I'd prefer just removing the check unless it messes up the dump.

[haraken, 2015-06-25 03:54:54]

On 2015/06/25 02:46:24, ssid wrote:
> > Total allocated size = marked + newly allocated  <--- expected
> > Total allocated size = live + dead  <--- expected
> > Total size = the size allocated from system excluding freed objects  <---
> > unexpected
> > 
> > The total size should be simply the size allocated from system including
freed
> > objects. The memory consumed by the freed objects are committed.
> 
> Hmm, looks like decreaseAllocatedSpace is called when at freePage(). So,
> allocatedSpace doesn't actually include free pages.

Freed objects (objects in the committed heap; header->isFree() returns true) are
different from freed pages (decommitted pages to the system). allocatedSpace
keeps track of committed pages, which should include all freed objects...

[ssid, 2015-06-25 04:14:11]

> Freed objects (objects in the committed heap; header->isFree() returns true)
are
> different from freed pages (decommitted pages to the system). allocatedSpace
> keeps track of committed pages, which should include all freed objects...

Okay please try taking a trace with tip of tree. The total free size is more
than the allocatedSpace.

[ssid, 2015-06-25 06:36:06]

> Maybe we should expose an API like
Platform::current()->currentThread()->name()
> which gives us the name of that thread. Chrome should know the name (we
display
> the name in about:tracing).

Hm, I think the api should be getNameFor(PlatformThreadId) since we need to get
the names of thread from the main thread.

[haraken, 2015-06-26 02:34:03]

Hmm, I cannot reproduce a scenario where freed size becomes larger than the
total size. Can you tell me the repro step?

Freed size is smaller than the total size, which looks correct to me...
http://haraken.info/a/free_size.png

BTW, I'm observing a couple of crashes when we call takeSnapshot. Looking.

[Primiano Tucci (use gerrit), 2015-06-26 08:38:01]

On 2015/06/26 02:34:03, haraken wrote:
> Hmm, I cannot reproduce a scenario where freed size becomes larger than the
> total size. Can you tell me the repro step?

I just built chrome release from ToT Chrome + Blink (chromium/src.git @
deae010542c, blink.git@ f6a73d33ae) and I still see that.

Repro steps:
 - Build chrome in release
 - start out/Release/chrome --no-sandbox
 - open chrome://tracing, enable just memory-infra and start tracing
 - open a new tab type www.theverge.com
 - stop tracing after just few seconds.
 - Look at the blink_gc dump of the last points

free_size: 2.1M, total size for blink_gc: 1.3M

Attaching the trace: 
www/~primiano/memory_infra/traces/trace_blink_gc_freesize.json.gz

Screenshot:
ww/~primiano/memory_infra/misc/trace_blink_gc_freesize.png

[Primiano Tucci (use gerrit), 2015-06-26 08:42:05]

On 2015/06/26 08:38:01, Primiano Tucci wrote:
> On 2015/06/26 02:34:03, haraken wrote:
> > Hmm, I cannot reproduce a scenario where freed size becomes larger than the
> > total size. Can you tell me the repro step?

Ok I just had a chat with ssid. This seems to be a problem with the dumper.
There is some double-counting happening.
What happens is that the trace looks as follows
blink_gc
  heaps
    EagerSweep
      buckets        free_size= 10
      pages          free_size= 10

So the actual free_size is 10, but gets reported as 20.
It seems all good in Oilpan. Will fix the dumper on our side. 
Sorry for the noise.