Provide more accurate results about used heap size via GetHeapStatistics.

Provide more accurate results about used heap size via GetHeapStatistics.

I observed that used heap size provided by Heap::SizeOfObjects() is
usually about ~10% bigger than the number calculated by summing up
heap objects sizes.

This aligns DevTools Timeline stats with Heap profiler stats.

Committed: http://code.google.com/p/v8/source/detail?r=5825

[Mads Ager (chromium), 2010-11-12 11:11:27]

As discussed offline I think we need to investigate this a bit more.

http://codereview.chromium.org/4888001/diff/1/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/4888001/diff/1/src/heap.h#newcode280
src/heap.h:280: // Does the same as the previous function, but more precisely,
Hmm, this looks strange. Computing the same but getting a different result.

That indicates to me that the SizeOfObjects method is either incorrectly named
or should be fixed.

I wonder if the difference here is only due to the free-list being excluded in
your implementation. If that is the case we should fix the original method to
exclude the size of the memory in the free list which we should have available
directly somewhere.

[mnaganov (inactive), 2010-11-12 21:58:13]

On 2010/11/12 11:11:27, Mads Ager wrote:
> As discussed offline I think we need to investigate this a bit more.
> 
> http://codereview.chromium.org/4888001/diff/1/src/heap.h
> File src/heap.h (right):
> 
> http://codereview.chromium.org/4888001/diff/1/src/heap.h#newcode280
> src/heap.h:280: // Does the same as the previous function, but more precisely,
> Hmm, this looks strange. Computing the same but getting a different result.
> 
> That indicates to me that the SizeOfObjects method is either incorrectly named
> or should be fixed.
> 
> I wonder if the difference here is only due to the free-list being excluded in
> your implementation. If that is the case we should fix the original method to
> exclude the size of the memory in the free list which we should have available
> directly somewhere.


I've investigated this issue. Anton suggested to examine reporting differences
on per-space basis. Below are the results:

for google.com:

  Space 0: size 1336, sum: 1336
           iterator: 1336, freelists: 0, sum: 1336
  Space 1: size 803048, waste: 44, freelists: 19928, sum: 823020
           iterator: 803048, freelists: 19972, sum: 823020
  Space 2: size 359660, waste: 24, freelists: 5572, sum: 365256
           iterator: 194344, freelists: 170912, sum: 365256
  Space 3: size 1209408, waste: 0, sum: 1209408
           iterator: 1209408, freelists: 69696, sum: 1279104
  Space 4: size 62976, waste: 0, freelists: 0, sum: 62976
           iterator: 62976, freelists: 0, sum: 62976
  Space 5: size 13400, waste: 0, freelists: 0, sum: 13400
           iterator: 13400, freelists: 0, sum: 13400
  Space 6: size 536576, sum: 536576
           iterator: 366048, freelists: 0, sum: 366048

for corp gmail:

  Space 0: size 2232, sum: 2232
           iterator: 2232, freelists: 0, sum: 2232
  Space 1: size 11644768, waste: 788, freelists: 55924, sum: 11701480
           iterator: 11644768, freelists: 56712, sum: 11701480
  Space 2: size 3074476, waste: 168, freelists: 46092, sum: 3120736
           iterator: 2116692, freelists: 1004044, sum: 3120736
  Space 3: size 5886880, waste: 0, sum: 5886880
           iterator: 5886880, freelists: 220256, sum: 6107136
  Space 4: size 1155776, waste: 0, freelists: 0, sum: 1155776
           iterator: 1155776, freelists: 0, sum: 1155776
  Space 5: size 226976, waste: 0, freelists: 0, sum: 226976
           iterator: 226976, freelists: 0, sum: 226976
  Space 6: size 7344128, sum: 7344128
           iterator: 3964572, freelists: 86604, sum: 4051176


So, to make sizes reported by Spaces match heap iteration results, we need to:
  - fix freelists detection when iterating heap objects; OLD_DATA_SPACE (Space
2) has rather significant skew;
  - find out what's wrong with size reporting for LO_SPACE (Space 6): the size
is almost twice bigger than the total size of heap objects.

[mnaganov (inactive), 2010-11-13 17:21:38]

On 2010/11/12 21:58:13, Michail Naganov wrote:
> On 2010/11/12 11:11:27, Mads Ager wrote:
> > As discussed offline I think we need to investigate this a bit more.
> > 
> > http://codereview.chromium.org/4888001/diff/1/src/heap.h
> > File src/heap.h (right):
> > 
> > http://codereview.chromium.org/4888001/diff/1/src/heap.h#newcode280
> > src/heap.h:280: // Does the same as the previous function, but more
precisely,
> > Hmm, this looks strange. Computing the same but getting a different result.
> > 
> > That indicates to me that the SizeOfObjects method is either incorrectly
named
> > or should be fixed.
> > 
> > I wonder if the difference here is only due to the free-list being excluded
in
> > your implementation. If that is the case we should fix the original method
to
> > exclude the size of the memory in the free list which we should have
available
> > directly somewhere.
> 
> 
> I've investigated this issue. Anton suggested to examine reporting differences
> on per-space basis. Below are the results:
> 
> for google.com:
> 
>   Space 0: size 1336, sum: 1336
>            iterator: 1336, freelists: 0, sum: 1336
>   Space 1: size 803048, waste: 44, freelists: 19928, sum: 823020
>            iterator: 803048, freelists: 19972, sum: 823020
>   Space 2: size 359660, waste: 24, freelists: 5572, sum: 365256
>            iterator: 194344, freelists: 170912, sum: 365256
>   Space 3: size 1209408, waste: 0, sum: 1209408
>            iterator: 1209408, freelists: 69696, sum: 1279104
>   Space 4: size 62976, waste: 0, freelists: 0, sum: 62976
>            iterator: 62976, freelists: 0, sum: 62976
>   Space 5: size 13400, waste: 0, freelists: 0, sum: 13400
>            iterator: 13400, freelists: 0, sum: 13400
>   Space 6: size 536576, sum: 536576
>            iterator: 366048, freelists: 0, sum: 366048
> 
> for corp gmail:
> 
>   Space 0: size 2232, sum: 2232
>            iterator: 2232, freelists: 0, sum: 2232
>   Space 1: size 11644768, waste: 788, freelists: 55924, sum: 11701480
>            iterator: 11644768, freelists: 56712, sum: 11701480
>   Space 2: size 3074476, waste: 168, freelists: 46092, sum: 3120736
>            iterator: 2116692, freelists: 1004044, sum: 3120736
>   Space 3: size 5886880, waste: 0, sum: 5886880
>            iterator: 5886880, freelists: 220256, sum: 6107136
>   Space 4: size 1155776, waste: 0, freelists: 0, sum: 1155776
>            iterator: 1155776, freelists: 0, sum: 1155776
>   Space 5: size 226976, waste: 0, freelists: 0, sum: 226976
>            iterator: 226976, freelists: 0, sum: 226976
>   Space 6: size 7344128, sum: 7344128
>            iterator: 3964572, freelists: 86604, sum: 4051176
> 
> 
> So, to make sizes reported by Spaces match heap iteration results, we need to:
>   - fix freelists detection when iterating heap objects; OLD_DATA_SPACE (Space
> 2) has rather significant skew;
>   - find out what's wrong with size reporting for LO_SPACE (Space 6): the size
> is almost twice bigger than the total size of heap objects.

OK, I reworked the patch to fix the previously stated issues. Now size of
objects reported by Heap::SizeOfObjects is almost the same as discovered using
HeapIterator with precise free list nodes filtering.

Thanks to Anton for his great suggestions!

[antonm, 2010-11-13 17:37:51]

DBCs

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc
File src/heap.cc (left):

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#oldcode4408
src/heap.cc:4408: if (FreeListNode::IsFreeListNode(obj)) continue;
thank you very much!

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4852
src/heap.cc:4852: bool marked = object->IsMarked();
MapWord might be the right abstraction for this code, see for example
CountMarkedCallback in mark-compact.cc.  BTW, you could probably just reuse it
here.

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4853
src/heap.cc:4853: if (marked)
nit: either on a single line or in braces, please

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4856
src/heap.cc:4856: if (marked)
ditto

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4872
src/heap.cc:4872: ObjectIterator* iter = new
HeapObjectIterator(Heap::code_space());
why you need to malloc iterator?  can you allocate it on the stack?

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4876
src/heap.cc:4876: if (FreeListNode::IsFreeListNode(obj))
ditto for braces

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4906
src/heap.cc:4906: filter_ = new FreeListNodesFilter();
looks like new trend is to omit () for ctors with no args

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4909
src/heap.cc:4909: space_iterator_ = new SpaceIterator();
ditto

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4926
src/heap.cc:4926: if (filter_ == NULL)
ditto for braces (here and below)

http://codereview.chromium.org/4888001/diff/6001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/4888001/diff/6001/src/heap.h#newcode1576
src/heap.h:1576: // A HeapIterator provides iteration over the whole heap It
aggregates
nit: a dot after 'heap'.

http://codereview.chromium.org/4888001/diff/6001/src/heap.h#newcode1577
src/heap.h:1577: // a the specific iterators for the different spaces as these
can only
nit: 'a' and 'the' at the beginning of the line

http://codereview.chromium.org/4888001/diff/6001/src/heap.h#newcode1595
src/heap.h:1595: explicit HeapIterator(FreeListNodesFiltering filtering);
maybe turn filtering it a param with default value?

[mnaganov (inactive), 2010-11-13 19:05:30]

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4852
src/heap.cc:4852: bool marked = object->IsMarked();
On 2010/11/13 17:37:51, antonm wrote:
> MapWord might be the right abstraction for this code, see for example
> CountMarkedCallback in mark-compact.cc.  BTW, you could probably just reuse it
> here.

Hm. Not sure how using MapWord will help. HeapObject's 'IsMarked', 'SetMark',
and 'ClearMark' delegate to MapWord.

Why I need to have this function is because ObjectIterators need to get the
object size before they return pointer to it. And they will crash if map pointer
will be marked. And I need to restore marking back because I will need to check
it later via IsFreeListNode.

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4853
src/heap.cc:4853: if (marked)
On 2010/11/13 17:37:51, antonm wrote:
> nit: either on a single line or in braces, please

Done.

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4856
src/heap.cc:4856: if (marked)
On 2010/11/13 17:37:51, antonm wrote:
> ditto

Done.

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4872
src/heap.cc:4872: ObjectIterator* iter = new
HeapObjectIterator(Heap::code_space());
On 2010/11/13 17:37:51, antonm wrote:
> why you need to malloc iterator?  can you allocate it on the stack?

Not sure. It is a descendant of the class Malloced.

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4876
src/heap.cc:4876: if (FreeListNode::IsFreeListNode(obj))
On 2010/11/13 17:37:51, antonm wrote:
> ditto for braces

Done.

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4906
src/heap.cc:4906: filter_ = new FreeListNodesFilter();
On 2010/11/13 17:37:51, antonm wrote:
> looks like new trend is to omit () for ctors with no args

Done.

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4909
src/heap.cc:4909: space_iterator_ = new SpaceIterator();
On 2010/11/13 17:37:51, antonm wrote:
> ditto

Done.

http://codereview.chromium.org/4888001/diff/6001/src/heap.cc#newcode4926
src/heap.cc:4926: if (filter_ == NULL)
On 2010/11/13 17:37:51, antonm wrote:
> ditto for braces (here and below)

Done.

http://codereview.chromium.org/4888001/diff/6001/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/4888001/diff/6001/src/heap.h#newcode1576
src/heap.h:1576: // A HeapIterator provides iteration over the whole heap It
aggregates
On 2010/11/13 17:37:51, antonm wrote:
> nit: a dot after 'heap'.

Done.

http://codereview.chromium.org/4888001/diff/6001/src/heap.h#newcode1577
src/heap.h:1577: // a the specific iterators for the different spaces as these
can only
On 2010/11/13 17:37:51, antonm wrote:
> nit: 'a' and 'the' at the beginning of the line

Done.

http://codereview.chromium.org/4888001/diff/6001/src/heap.h#newcode1595
src/heap.h:1595: explicit HeapIterator(FreeListNodesFiltering filtering);
On 2010/11/13 17:37:51, antonm wrote:
> maybe turn filtering it a param with default value?

The style guide is against it:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

[Mads Ager (chromium), 2010-11-15 08:31:55]

Slava would be the right person to review this patch.

[Vyacheslav Egorov (Chromium), 2010-11-15 09:33:21]

LGTM

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc#newcode4836
src/heap.cc:4836: class FreeListNodesFilter : public Malloced {
Is it really necessary to separate this class from HeapIterator and malloc it?

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc#newcode4851
src/heap.cc:4851: static int ObjectSize(HeapObject* object) {
There is CountMarkedCallback which does exactly what you need but better (saves
two memory writes).

map_word.ClearMark() does not clear mark in object's map word. map_word is a
value type.

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc#newcode4870
src/heap.cc:4870: ObjectIterator* iter = new
HeapObjectIterator(Heap::code_space());
I think there is no need to use new here.

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc#newcode4896
src/heap.cc:4896: Shutdown();
You should assert here that iteration was not interrupted to guarantee that heap
was not left in inconsistent state.

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc#newcode4903
src/heap.cc:4903: filter_ = new FreeListNodesFilter;
It would be cool to forbid allocation/gc while precise HeapIterator is alive.

[mnaganov (inactive), 2010-11-15 10:36:17]

Thank you very much!

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc#newcode4836
src/heap.cc:4836: class FreeListNodesFilter : public Malloced {
On 2010/11/15 09:33:21, Vyacheslav Egorov wrote:
> Is it really necessary to separate this class from HeapIterator and malloc it?

I made it separate from HeapIterator because it's a helper class, and even its
declaration isn't needed outside. But it's a part of HeapIterator's state, so I
needed to put it inside HeapIterator. My solution is to forward-declare it in
heap.h.

OK, it's not necessary to make it a subclass of Malloced.

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc#newcode4851
src/heap.cc:4851: static int ObjectSize(HeapObject* object) {
On 2010/11/15 09:33:21, Vyacheslav Egorov wrote:
> There is CountMarkedCallback which does exactly what you need but better
(saves
> two memory writes).
> 
> map_word.ClearMark() does not clear mark in object's map word. map_word is a
> value type.

Oh, you are right. I didn't noticed that MapWord is a value type. I made
CountMarkedCallback public in MarkCompactCollector and renamed it to
SizeOfMarkedObject.

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc#newcode4870
src/heap.cc:4870: ObjectIterator* iter = new
HeapObjectIterator(Heap::code_space());
On 2010/11/15 09:33:21, Vyacheslav Egorov wrote:
> I think there is no need to use new here.

Done.

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc#newcode4896
src/heap.cc:4896: Shutdown();
On 2010/11/15 09:33:21, Vyacheslav Egorov wrote:
> You should assert here that iteration was not interrupted to guarantee that
heap
> was not left in inconsistent state.

Done.

http://codereview.chromium.org/4888001/diff/15001/src/heap.cc#newcode4903
src/heap.cc:4903: filter_ = new FreeListNodesFilter;
On 2010/11/15 09:33:21, Vyacheslav Egorov wrote:
> It would be cool to forbid allocation/gc while precise HeapIterator is alive.

I've included AssertNoAllocation into FreeListNodesFilter.