Tracks the amount of committed vs uncommitted memory in tcmalloc's page heap's freelists

Tracks the amount of committed vs uncommitted memory in tcmalloc's page heap's freelists

Keeps track of the number of reserved but not committed pages in the freelist and uses that to calculate a waste metric, which is the ratio of committed pages vs pages used by the application.  This is exposed in the GetStats() call (which is used for about:tcmalloc) and through GetNumericalProperty() in MallocExtension.

BUG=none
TEST=open about:tcmalloc and monitor 'WASTE' columns while using the browser

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28133

[jamesr, 2009-10-02 00:59:47]

In order to properly evaluate different memory allocation algorithms we have to
be able to track the memory effects of the changes.  This patch is a step in
that direction.  Eventually we should be able to keep track of these values when
running perf tests and page cyclers to see what's working and what isn't.

[antonm, 2009-10-02 12:29:42]

lgtm, but minor comments

http://codereview.chromium.org/251065/diff/5001/2002
File third_party/tcmalloc/page_heap.cc (right):

http://codereview.chromium.org/251065/diff/5001/2002#newcode241
Line 241: freelist_returned_bytes_ -= (len << kPageShift);
could we move this into CommitSpan?

http://codereview.chromium.org/251065/diff/5001/2002#newcode499
Line 499: bytes_released += (s->length << kPageShift);
I am not sure we should care about this, but we could count number of pages in
the loop and shift it on return.

http://codereview.chromium.org/251065/diff/5001/2003
File third_party/tcmalloc/page_heap.h (right):

http://codereview.chromium.org/251065/diff/5001/2003#newcode195
Line 195: // Bytes in the returned freelist (allocated but not committed or
used)
maybe, not insisting: I'd use reserved instead of allocated.  Yes, it's too
Windows specific, but communicates more clearly imho.  By no means insisting.

[jar (doing other things), 2009-10-02 16:01:12]

http://codereview.chromium.org/251065/diff/5001/2002
File third_party/tcmalloc/page_heap.cc (right):

http://codereview.chromium.org/251065/diff/5001/2002#newcode241
Line 241: freelist_returned_bytes_ -= (len << kPageShift);
+1  It would be good to move all these delta's to the maximally central place. 
Optimally, it would be maximally close to the VirtualAlloc.  That will give us
the smallest chance of making a counting mistake in the future.

On 2009/10/02 12:29:42, antonm wrote:
> could we move this into CommitSpan?

http://codereview.chromium.org/251065/diff/5001/2002#newcode499
Line 499: bytes_released += (s->length << kPageShift);
On 2009/10/02 12:29:42, antonm wrote:
> I am not sure we should care about this, but we could count number of pages in
> the loop and shift it on return.

I agree that we shouldn't care, as shifts are cheap... and it is nice that the
delta is close to the action (easy to read).  The compiler might even do the
loop hoisting for you automatically ;-).

I'd even prefer the increment to be pushed closer to the system call in
TCMalloc_SystemRelease if possible.  Perhaps ReleaseFreeList should be a private
method of PageHeap, and do the increments to freelist_returned_bytes_
automatically (rather than returning a count to accumulate).

[Mike Belshe, 2009-10-02 17:02:01]

lgtm

http://codereview.chromium.org/251065/diff/5001/2004
File third_party/tcmalloc/tcmalloc.cc (right):

http://codereview.chromium.org/251065/diff/5001/2004#newcode370
Line 370: uint64_t freelist_returned_bytes; // Bytes on freelist that are
reserved but not committed
nit: 80 cols

[jamesr, 2009-10-02 19:04:56]

This patch tracks commits directly rather than freelist entries, it should be a
lot simpler.

[jar (doing other things), 2009-10-02 19:25:38]

I agree, this simplified things.

It LGTM, but see nit/suggestion below.

http://codereview.chromium.org/251065/diff/6004/6005
File third_party/tcmalloc/page_heap.cc (right):

http://codereview.chromium.org/251065/diff/6004/6005#newcode498
Line 498: bytes_released += (s->length << kPageShift);
As per previous comment, I'd still vote for making ReleaseFreeList a private
member, so that we can directly (in this method) decrement committed_bytes_.

[jamesr, 2009-10-02 19:57:02]

On 2009/10/02 19:25:38, jar wrote:
> I agree, this simplified things.
> 
> It LGTM, but see nit/suggestion below.
> 
> http://codereview.chromium.org/251065/diff/6004/6005
> File third_party/tcmalloc/page_heap.cc (right):
> 
> http://codereview.chromium.org/251065/diff/6004/6005#newcode498
> Line 498: bytes_released += (s->length << kPageShift);
> As per previous comment, I'd still vote for making ReleaseFreeList a private
> member, so that we can directly (in this method) decrement committed_bytes_.

Yup, that's much better.  Thanks for the reviews!

[jamesr, 2009-10-06 00:28:35]

Merged up past Jim's recent patch.

[jamesr, 2009-10-06 02:09:29]

I've reduced the delta in PageHeap::ReleaseFreeList() so it should be easier to
check for correctness.

[jar (doing other things), 2009-10-06 04:14:02]

LGTM.

Thanks!