Reserve a dedicated arena for every construction of mmap_address_map.

Reserve a dedicated arena for every construction of mmap_address_map in order to reduce memory usage for HEAP_PROFILE_MMAP.

Large memory usage is observed for heap profiling while profiling mmap().  This change fixes the bloat by reserving a dedicated arena for every construction of mmap_address_map.

This change is planned to be upstreamed.  If another approach is chosen in the upstream, this change will be replaced.


BUG=123759
TEST=run all existing tests.

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

[Dai Mikurube (NOT FULLTIME), 2012-04-03 06:29:14]

Hi Jim,

It's not in a hurry, but could you take a look at it when you have time?

I found that the current HEAP_PROFILE_MMAP takes large amount of memory for
profiling.  It's because of allocation/deallocation for mmap'ed regions in the
arena for the profiler.  (LowLevelAlloc may not reuse free'd memory well?)

In this patch, an arena is created for each mmap dump request, and discarded
every time.
- It may affect a little in dumping performance only when HEAP_PROFILE_MMAP is
specified.
- It doesn't affect anything when HEAP_PROFILE_MMAP is not specified.
- It doesn't affect ordinary execution while "not dumping" (== while not
executing DoGetHeapProfileLocked()).

I'm planning to upstream it.  But, I'm applying it in Chromium at first because
"upstreaming tcmalloc => importing the latest tcmalloc" takes long time.

[jar (doing other things), 2012-04-04 17:38:40]

I noticed lots of nits... but not sure which ones you should address, if you're
upstreaming.

LGTM with at least nits in the new code changed.

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right):

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:127: : alloc_(alloc),
dealloc_(dealloc) {
nit: One initializer per line (once you've wrapped to a new line).

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:135: num_alloc_buckets_
= 0;
nit: these slots initialized with 0 and NULL should be in the initializer list.

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:154:
DeallocateAllocationMap(mmap_address_map_);
This call will still used the default deallocator, rather than mmap_dealloc.

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:393: if
(mmap_address_map_ != NULL) {
nit: early return on "== NULL" will mean you won't need curlies, and will have
less indent (and won't have to look to see what else happens).

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:521: return true;
Since both clauses have a return, there is no need for else.
Best to list short case first (LOG(ERROR) with a return.

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
File third_party/tcmalloc/chromium/src/heap-profiler.cc (right):

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profiler.cc:165: if
(mmap_heap_profiler_memory != NULL) {
nit: Early return tends to improve readability (you don't have to look for the
else or continuation), and reduce indentation of other code.

[Dai Mikurube (NOT FULLTIME), 2012-04-05 05:19:26]

Thanks, Jim.

Updated the patchset.

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right):

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:127: : alloc_(alloc),
dealloc_(dealloc) {
On 2012/04/04 17:38:40, jar wrote:
> nit: One initializer per line (once you've wrapped to a new line).

Done.

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:135: num_alloc_buckets_
= 0;
On 2012/04/04 17:38:40, jar wrote:
> nit: these slots initialized with 0 and NULL should be in the initializer
list.

Done.

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:154:
DeallocateAllocationMap(mmap_address_map_);
On 2012/04/04 17:38:40, jar wrote:
> This call will still used the default deallocator, rather than mmap_dealloc.

Changed that mmap_address_map_ itself is allocated with alloc_.

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:393: if
(mmap_address_map_ != NULL) {
On 2012/04/04 17:38:40, jar wrote:
> nit: early return on "== NULL" will mean you won't need curlies, and will have
> less indent (and won't have to look to see what else happens).

Done.

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profile-table.cc:521: return true;
On 2012/04/04 17:38:40, jar wrote:
> Since both clauses have a return, there is no need for else.
> Best to list short case first (LOG(ERROR) with a return.

Done.

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
File third_party/tcmalloc/chromium/src/heap-profiler.cc (right):

http://codereview.chromium.org/9963095/diff/1/third_party/tcmalloc/chromium/s...
third_party/tcmalloc/chromium/src/heap-profiler.cc:165: if
(mmap_heap_profiler_memory != NULL) {
On 2012/04/04 17:38:40, jar wrote:
> nit: Early return tends to improve readability (you don't have to look for the
> else or continuation), and reduce indentation of other code.

Done.

[jar (doing other things), 2012-04-05 23:20:31]

lgtm

[Dai Mikurube (NOT FULLTIME), 2012-04-06 04:25:50]

On 2012/04/05 23:20:31, jar wrote:
> lgtm

Thank you, Jim.  I'll commit it after lgtm from upstream.

[Dai Mikurube (NOT FULLTIME), 2012-04-18 07:32:47]

We're in discussion at the upstream.

I'm committing this change to solve the issue at hand since the discussion may
take long time.

[commit-bot: I haz the power, 2012-04-18 07:47:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmikurube@chromium.org/9963095/17001

[commit-bot: I haz the power, 2012-04-18 09:45:46]

Change committed as 132771