The mmap() profiler doesn't hook a large part of memory used for the mmap profiler itself

[GoogleCodeExporter]

The mmap() profiler doesn't hook a large part of memory used for the mmap() 
profiler itself.  A large bug in the mmap() profiler was fixed in the issue 383 
(https://code.google.com/p/gperftools/issues/detail?id=383), but it was not 
perfect.

< How it happens? >

mmap() records from MemoryRegionMap get into AddressMap in 
HeapProfileTable::RefreshMMapData().  The AddressMap is finally used to dump 
heap profiles.

RefreshMMapData() reads mmap() records by RegionIterator in MemoryRegionMap and 
inserts the records to AddressMap (*1) during the iteration.  The iterator is 
implemented by std::set.  The point is that the insertion to AddressMap can 
allocate new memory (with LowLovelAlloc).  If the new memory is allocated 
before the address where the iterator points, the iteration misses the newly 
allocated memory.

(*1)
https://code.google.com/p/gperftools/source/browse/trunk/src/heap-profile-table.
cc?r=170#386

< Did it happen also before the issue 383? >

It looks like yes.  The old version did a similar operation in 
AddRemoveMMapDataLocked() in heap-profiler.cc (*2).  It also iterated 
MemoryRegionMap and called HeapProfileTable::RecordAlloc() during the 
iteration.  RecordAlloc() also inserted to AddressMap (*3).

(*2)
https://code.google.com/p/gperftools/source/browse/trunk/src/heap-profiler.cc?r=
129#191

(*3)
https://code.google.com/p/gperftools/source/browse/trunk/src/heap-profile-table.
cc?r=129#210

< Hot to fix it? >

I'm trying to fix it in Chromium's local branch: 
https://codereview.chromium.org/12388070/ at first.  I'll upstream the patch to 
gperftools soon.

The root problem is merging mmap() regions to AddressMap when heap profile dump 
is requested, not when mmap() is called.  My approach is to count mmap and 
munmap with each stack trace within MemoryRegionMap.

Original issue reported on code.google.com by dmikurube@chromium.org on 8 Mar 2013 at 12:01

[GoogleCodeExporter]

Sounds good. Keep us up to date on your progress.

Original comment by chapp...@gmail.com on 10 Mar 2013 at 8:39

• Changed state: Started

[GoogleCodeExporter]

It is done in Chromium.  I'll be uploading my patch to fix it.

A little more details just in case:

The original mmap profiler (HEAP_PROFILE_MMAP) actually does similar counting.  
The difference is that the original accumulates memory regions for each 
stacktrace in HeapProfileTable for every dump request.

HeapProfileTable reads mmap's stacktraces from |call_stack| in 
MemoryRegionMap::Region, adds them for each stacktrace, dumps merged heap 
profile and finally removes the accumulated mmap profiles. This per-stacktrace 
accumulation is done in HeapProfileTable::RefreshMMapData in the current 
version, and was done in AddRemoveMMapDataLocked (heap-profiler.cc) previously.

The root problem is repeating addition and removal for every dump request.  My 
patch will change it to stop the repeating and to accumulate once in mmap hook.

Original comment by dmikurube@chromium.org on 18 Mar 2013 at 5:23

[GoogleCodeExporter]

Please find the patch attached.

(If recent gperftools has some code review process, I'll upload the patch to 
the code review site.)

Original comment by dmikurube@chromium.org on 18 Mar 2013 at 5:54

Attachments:

• mmap-profiler.001.patch

[GoogleCodeExporter]

I'm not sure of the recent policy of license headers in new files.

This patch adds a new file "heap-profile-stats.h".  I added Google's license 
header in it at this time since I'm also working for Google.  If you have a 
more appropriate header, please tell me that.

Original comment by dmikurube@chromium.org on 18 Mar 2013 at 5:56

[GoogleCodeExporter]

applied. Thanks.

Honestly speaking I did not have time to deeply review this stuff. But I assume 
that Chromium folks did a proper review.

Original comment by alkondratenko on 4 Aug 2013 at 4:20

• Changed state: Fixed