Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8632007:
A deeper heap profile dumper in third_party/tcmalloc/chromium. (Closed)
|
Created:
9 years, 1 month ago by Dai Mikurube (NOT FULLTIME) Modified:
8 years, 8 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionA deeper heap profile dumper in third_party/tcmalloc/chromium.
BUG=114301
TEST=run all existing tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130092
Patch Set 1 #Patch Set 2 : Refactor deep-memory-profiler.* #1 #Patch Set 3 : Refactor deep-memory-profiler.* #2-3 #Patch Set 4 : Fixed the patch, for the HEAP_PROFILE_MMAP fix. #Patch Set 5 : Fixed. #Patch Set 6 : Revived removed code temporarily. #Patch Set 7 : Removed revided code again. #Patch Set 8 : Refactor deep-memory-profiler.* #4-5 #Patch Set 9 : Reduced changes in heap-profiler and heap-profile-table, and removed #define DEEP_PROFILER_ON #Patch Set 10 : fixed and refactored. #Patch Set 11 : restricted only in Linux #Patch Set 12 : fix the restriction to Linux. #Patch Set 13 : Refactored. #
Total comments: 22
Patch Set 14 : reflected the comments. #
Total comments: 2
Patch Set 15 : rebased. #
Total comments: 8
Patch Set 16 : Reflected the comments. #
Total comments: 50
Patch Set 17 : Updated except for the case of overrun. #Patch Set 18 : Fixed the snprintf issues. #
Total comments: 24
Patch Set 19 : Reflected the comment. #
Total comments: 36
Patch Set 20 : Reflected the comments. #Patch Set 21 : Refactored. #
Total comments: 10
Patch Set 22 : separeted mmap part and malloc part. #Patch Set 23 : added a version in deep profile dump. #Patch Set 24 : changed the header again. #Patch Set 25 : fixed header. #Patch Set 26 : ready for review. #
Total comments: 58
Patch Set 27 : Reflected the comments. #
Total comments: 16
Patch Set 28 : rebased. #
Total comments: 22
Patch Set 29 : reflected the comments. #
Messages
Total messages: 30 (0 generated)
I haven't looked at deep-heap-profile.cc closely yet. But my concern is that you should probably inherit DeepHeapProfile from HeapProfileTable. WDYT? http://codereview.chromium.org/8632007/diff/37001/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/8632007/diff/37001/base/allocator/allocator.gy... base/allocator/allocator.gyp:97: '<(tcmalloc_dir)/src/deep-heap-profile.cc', Shouldn't deep-heap-profile.h appear here as well? Also, there are some platform-specific sections where you should exclude your files (probably on Windows, at least if you haven't tested that) http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:12: #include <sys/stat.h> Please mind the #include order, see http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:13: #ifdef HAVE_UNISTD_H What if you don't have unistd.h? http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:339: bool dummy; // "wrote_all" -- did /proc/self/maps fit in its entirety? s/dummy/wrote_all. You even won't need the comment. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:341: tcmalloc::FillProcSelfMaps(profiler_buffer_, kProfilerBufferSize, &dummy); Please check for wrote_all. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:363: // We will use the global buffer here Please put periods at the end of single-line comments. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:11: #define DEEP_HEAP_PROFILE 1 What does your code do if DEEP_HEAP_PROFILE is 0? Maybe you shouldn't compile it at all on non-Linux platforms? http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.cc:62: #include "deep-heap-profile.h" Do you need to include deep-heap-profile.h here? http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profiler.cc:204: bytes_written = heap_profile->FillOrderedProfile(buf, buflen - 1); Does deep_profile replace heap_profile everywhere if FLAGS_deep_heap_profile==1? If so, you may want to initialize heap_profile with NULL and check that it's still NULL in HeapProfilerStop(). Another option is to inherit DeepHeapProfile from HeapProfileTable. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profiler.cc:472: deep_profile = new(ProfilerMalloc(sizeof(DeepHeapProfile))) I wouldn't have aligned the placement new arguments this way, but looks like nobody cares in this file, so it's better to be consistent. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/malloc_hook.cc (right): http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/malloc_hook.cc:680: // i += 1; // skip hook caller frame Please add an "if FLAG_deep_profile..." here instead.
Thank you for reviewing this patch. In regard to inheriting DeepHeapProfile from HeapProfileTable, I thought it can be one good idea, but doesn't have not so large benefit as written in reply to http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... . In addition, just guessing, we may able to avoid using AddressMap for deep_bucket_map_ if we have a new DeepBucket structure which inherit from Bucket. I'll think about it soon. http://codereview.chromium.org/8632007/diff/37001/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/8632007/diff/37001/base/allocator/allocator.gy... base/allocator/allocator.gyp:97: '<(tcmalloc_dir)/src/deep-heap-profile.cc', On 2011/12/20 13:57:23, Alexander Potapenko wrote: > Shouldn't deep-heap-profile.h appear here as well? > Also, there are some platform-specific sections where you should exclude your > files (probably on Windows, at least if you haven't tested that) Ah, good catch. Thanks. Platform-specific sections are only in deep-heap-profile.* and they are excluded by #ifdef's in non-Linux platforms. win_rel and mac_rel were happy with this change. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:12: #include <sys/stat.h> On 2011/12/20 13:57:23, Alexander Potapenko wrote: > Please mind the #include order, see > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... Done. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:13: #ifdef HAVE_UNISTD_H On 2011/12/20 13:57:23, Alexander Potapenko wrote: > What if you don't have unistd.h? It's required for getpid(). I though about the case of no unistd.h, but other code calls getpid() with no consideration. I guess they assume that getpid() is defined in another way. For example, getpid() is defined in windows/port.h which is included in config.h. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:339: bool dummy; // "wrote_all" -- did /proc/self/maps fit in its entirety? On 2011/12/20 13:57:23, Alexander Potapenko wrote: > s/dummy/wrote_all. You even won't need the comment. Done. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:341: tcmalloc::FillProcSelfMaps(profiler_buffer_, kProfilerBufferSize, &dummy); On 2011/12/20 13:57:23, Alexander Potapenko wrote: > Please check for wrote_all. Done. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:363: // We will use the global buffer here On 2011/12/20 13:57:23, Alexander Potapenko wrote: > Please put periods at the end of single-line comments. Done. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:11: #define DEEP_HEAP_PROFILE 1 On 2011/12/20 13:57:23, Alexander Potapenko wrote: > What does your code do if DEEP_HEAP_PROFILE is 0? > Maybe you shouldn't compile it at all on non-Linux platforms? If 0, only the constructor, the destructor and FillOrderedProfile is defined. FillOrderedProfile just forwards the call to HeapProfileTable. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.cc:62: #include "deep-heap-profile.h" On 2011/12/20 13:57:23, Alexander Potapenko wrote: > Do you need to include deep-heap-profile.h here? Thanks, and removed. I forgot to remove it after some changes. http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profiler.cc:204: bytes_written = heap_profile->FillOrderedProfile(buf, buflen - 1); On 2011/12/20 13:57:23, Alexander Potapenko wrote: > Does deep_profile replace heap_profile everywhere if FLAGS_deep_heap_profile==1? > If so, you may want to initialize heap_profile with NULL and check that it's > still NULL in HeapProfilerStop(). > > Another option is to inherit DeepHeapProfile from HeapProfileTable. deep_profile does get information from heap_profile when dumping is requested, merge some information from OS, and then dump the merged information. heap_profile works the same way. We chose this design in order 1) to reduce changes in the original code, and 2) to avoid overhead when FLAGS_deep_heap_profile==0. Additional overhead is unacceptable, for example, in RecordAlloc and RecordFree since they're called every malloc/free. Additional if-statements or virtual functions add overheads. Inheriting DeepHeapProfile from HeapProfileTable might be a good idea if only FillOrderedProfile is a virtual function. But I guess it doesn't have so large benefit at this time... http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profiler.cc:472: deep_profile = new(ProfilerMalloc(sizeof(DeepHeapProfile))) On 2011/12/20 13:57:23, Alexander Potapenko wrote: > I wouldn't have aligned the placement new arguments this way, but looks like > nobody cares in this file, so it's better to be consistent. It's the first time also for me to see this way. But I used the same way to be consistent, as you say. :) http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/malloc_hook.cc (right): http://codereview.chromium.org/8632007/diff/37001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/malloc_hook.cc:680: // i += 1; // skip hook caller frame On 2011/12/20 13:57:23, Alexander Potapenko wrote: > Please add an "if FLAG_deep_profile..." here instead. Done.
http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:9: I'm a big fan of overview comments in header files. It would be nice to have some header file description indicating what this "deep" activity is all about. http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:42: bool is_committed; // Currently, we use only this I'm curious: How many of these are there? If there are a lot, I would have expected a bit field to be much more compact. Comments? http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:97: void RecordRegionStats(uint64 start, uint64 end, RegionStats* stats); I see a lot of uint64 types here. Should some of them be size_t types?
http://codereview.chromium.org/8632007/diff/44001/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/8632007/diff/44001/base/allocator/allocator.gy... base/allocator/allocator.gyp:304: # heap-profiler/checker/cpuprofiler E.g. I think you need to put deep-profiler here. http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:10: #if defined(__linux__) I suggest you just not compile this code on non-linux platforms by using the #error directive. If it does not work on other platforms as intended, you'd better not allow to use it.
Thank you for the comments. http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:9: On 2011/12/22 19:21:12, jar wrote: > I'm a big fan of overview comments in header files. > > It would be nice to have some header file description indicating what this > "deep" activity is all about. Done. http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:10: #if defined(__linux__) On 2011/12/23 09:58:06, Alexander Potapenko wrote: > I suggest you just not compile this code on non-linux platforms by using the > #error directive. If it does not work on other platforms as intended, you'd > better not allow to use it. We chose this design in order : 1. To make changes in the original code (heap-profiler.cc) simple. The current DeepHeapProfile absorbs platform-dependency. It just delegates to HeapProfileTable in non-Linux environments. 2. I plan to support other platforms within DeepHeapProfile. It will require changes only in deep-heap-profile. NOTE: it has overhead ONLY by giving an environment variable DEEP_HEAP_PROFILE. http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:42: bool is_committed; // Currently, we use only this On 2011/12/22 19:21:12, jar wrote: > I'm curious: How many of these are there? If there are a lot, I would have > expected a bit field to be much more compact. Comments? It's currently used only in one local variable of GetCommittedSize(). We actually have a plan of having a map of PageState, but it's not yet in this change. In fact, it's a subset of the original change. I forgot to remove PageStateMap* page_map_. http://codereview.chromium.org/8632007/diff/44003/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:97: void RecordRegionStats(uint64 start, uint64 end, RegionStats* stats); On 2011/12/22 19:21:12, jar wrote: > I see a lot of uint64 types here. Should some of them be size_t types? Changed some of them. Others are not sizes, but pointers which might be 64-bits. I guess it might be better to handle them as pointers, but uint64 is easier to calculate.
I think the use of snprintf() needs to be much more carefully handled. See comments below. There is also a question about care when handling wrapping of large addresses. I also added a lot of nit comments. I still need to go through this more carefully, but it may be simpler after you respond to some of this. Thanks, Jim http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:49: RAW_DCHECK(filename_prefix_ == NULL, ""); I don't understand the need for this DCHECK. The member is initialized on line 42. What is the concern or clarification this is meant to make evident? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:108: snprintf(buf + bucket_length, size - bucket_length, kGlobalStatsHeader); If we would have overrun the buffer, then the bucket_length tally here can be much larger than the allowable size. snprintf() returs what it *wanted* to use, even if it did not over-run the buffer (in reality). http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:114: bucket_length += snprintf(buf + bucket_length, size - bucket_length, Here again you'd get into trouble if bucket_length exceeds size, which can happen if snprintf() on line 113 was forced to truncate. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:123: RAW_DCHECK(bucket_length < size, ""); Between truncations and possible errors along the way, I'm not sure what this assertion will definitively protect against. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:130: int64 dt = CycleClock::Now() - start_time; nit: avoid abreviations like "dt" http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:132: RAW_LOG(0, "Time spent on DeepProfiler: %.3f sec\n", dtf); There has been a move away from logging data than no one will ever look at. Toward that end, most LOG statements have been changed to DLOG. IS that plausible here? (If so, you might want to avoid calling for the time if defined(NDEBUG) http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:222: // Check every pages on which the allocation reside. nit: pages--> page reside-->resides http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:223: while (page_address < address + size) { Does this infinite loop when allocation is made into the largest page number? Do you have assurances that we don't ever allocate there? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:228: RAW_LOG(0, "pagemap read failed @ %#llx %"PRId64" bytes", address, size); DLOG?? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:237: if (address + size < page_address + PAGE_SIZE) Can page_address + PAGE_SIZE == 0? (this is again a wrapping situation) http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:238: bytes -= PAGE_SIZE - (address + size - page_address); I had to read this a bit to be sure it made sense... and then I realized it might be clearer if reordered: If you move lines 237 and 238 up to just after 234, then you can simplify this to just being an assignment (since you know that bytes was set in line 234): bytes = address + size - page_address; WDYT? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:285: filename[0] == EOF) { I'm used to seeing EOF as an int (wider than a char). Is there never a user of an 0xFF character? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:296: uint64 alloc_address = (uint64) pointer; nit: use a reinterpret_cast<>() There may also be a more portable type to use here for casting a pointer to an int. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:353: buflen += snprintf(buf + buflen, bufsize - buflen, " 0x%08" PRIxPTR, Here again, I think there is a problem when buflen exceeds bufsize (due to truncation in line 350). http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:434: return buflen + printed; This again may cause problems when line 426 truncates. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:438: buflen += snprintf(buf + buflen, bufsize - buflen, When buflen is larger than bufsize, we'll get a negative number, which is silently cast to size_t (unsigned). I don't think this is what you want. See callsite comment for an example. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:15: // block is actually on memory, or not. DeepHeapProfile::FillOrderedProfile() nit: "on memory" --> "in memory" (??) http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:16: // uses logged data in HeapProfileTable as one of its data source. nit: "...one of its data source" --> "...one of its data sources" http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:43: DeepHeapProfile(HeapProfileTable* heap_profile, const char* prefix); Please add comments indicating what the function does (and what its args mean). http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:46: // This replaces the same function in heap-profile-table.h That is a fine secondary comment... but it is better to say what the function does. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:54: int id; // Unique ID of the bucket nit: Comments should end with a period. Please correct through this file. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:88: DeepBucket* GetDeepBucket(Bucket* bucket); nit: All these functions should have comments. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:127: HeapProfileTable* heap_profile_; Why isn't this line inside the ifdef? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:140: }; Why doesn't this class have the unsafe copy and constructor macro?
Hi Jim, Thank you so much for the comments. Updated the patch except for the overrun issues. I'll tackle that soon. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:49: RAW_DCHECK(filename_prefix_ == NULL, ""); Good catch. In the previous version, filename_prefix_ was not initialized in the initializer list. I forgot to remove the RAW_DCHECK. Removed it. On 2011/12/28 08:30:17, jar wrote: > I don't understand the need for this DCHECK. The member is initialized on line > 42. What is the concern or clarification this is meant to make evident? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:130: int64 dt = CycleClock::Now() - start_time; On 2011/12/28 08:30:17, jar wrote: > nit: avoid abreviations like "dt" Done. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:132: RAW_LOG(0, "Time spent on DeepProfiler: %.3f sec\n", dtf); We don't have DLOG in third_party/tcmalloc. It's defined in Chromium's base/logging.h. Instead, I put all RAW_LOGs in deep-heap-profile.cc into #ifndef NDEBUG - #endif. What do you think? (I chose #ifndef NDEBUG, not #if !defined(NDEBUG) to be consistent with other files in tcmalloc/.) On 2011/12/28 08:30:17, jar wrote: > There has been a move away from logging data than no one will ever look at. > Toward that end, most LOG statements have been changed to DLOG. > > IS that plausible here? (If so, you might want to avoid calling for the time if > defined(NDEBUG) http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:222: // Check every pages on which the allocation reside. On 2011/12/28 08:30:17, jar wrote: > nit: pages--> page > reside-->resides Done. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:223: while (page_address < address + size) { I guess the page 0xffffffffffff0000- is not mapped in user space on x86_64, but as you say, it might be dangerous. Added a branch to break. On 2011/12/28 08:30:17, jar wrote: > Does this infinite loop when allocation is made into the largest page number? > > Do you have assurances that we don't ever allocate there? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:228: RAW_LOG(0, "pagemap read failed @ %#llx %"PRId64" bytes", address, size); The same as above. On 2011/12/28 08:30:17, jar wrote: > DLOG?? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:237: if (address + size < page_address + PAGE_SIZE) Changed the condition: address + size <= page_address - 1 + PAGE_SIZE. On 2011/12/28 08:30:17, jar wrote: > Can page_address + PAGE_SIZE == 0? (this is again a wrapping situation) http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:238: bytes -= PAGE_SIZE - (address + size - page_address); Cool, agreed. Moved and modified it. On 2011/12/28 08:30:17, jar wrote: > I had to read this a bit to be sure it made sense... and then I realized it > might be clearer if reordered: > > If you move lines 237 and 238 up to just after 234, then you can simplify this > to just being an assignment (since you know that bytes was set in line 234): > bytes = address + size - page_address; > > WDYT? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:285: filename[0] == EOF) { Thanks for the good catch. It's wrong (though /proc/.../maps doesn't contain 0xFF.) Removed the conditional statement. On 2011/12/28 08:30:17, jar wrote: > I'm used to seeing EOF as an int (wider than a char). Is there never a user of > an 0xFF character? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:296: uint64 alloc_address = (uint64) pointer; Exactly. Replaced. uint64 comes from ProcMapsIterator in base/sysinfo.h. Its iterator (Next()) uses uint64 for addresses. I considered using uintptr_t, but chose uint64 to be consistent with ProcMapsIterator. (Finally I used "uint64 var = reinterpret_cast<uintptr_t>(...);") On 2011/12/28 08:30:17, jar wrote: > nit: use a reinterpret_cast<>() > > There may also be a more portable type to use here for casting a pointer to an > int. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:15: // block is actually on memory, or not. DeepHeapProfile::FillOrderedProfile() On 2011/12/28 08:30:17, jar wrote: > nit: "on memory" --> "in memory" (??) Done. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:16: // uses logged data in HeapProfileTable as one of its data source. On 2011/12/28 08:30:17, jar wrote: > nit: "...one of its data source" --> "...one of its data sources" Done. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:43: DeepHeapProfile(HeapProfileTable* heap_profile, const char* prefix); On 2011/12/28 08:30:17, jar wrote: > Please add comments indicating what the function does (and what its args mean). Done. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:46: // This replaces the same function in heap-profile-table.h On 2011/12/28 08:30:17, jar wrote: > That is a fine secondary comment... but it is better to say what the function > does. Done. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:54: int id; // Unique ID of the bucket On 2011/12/28 08:30:17, jar wrote: > nit: Comments should end with a period. > > Please correct through this file. Done. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:88: DeepBucket* GetDeepBucket(Bucket* bucket); On 2011/12/28 08:30:17, jar wrote: > nit: All these functions should have comments. Done. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:127: HeapProfileTable* heap_profile_; It is used to delegate calling of FillOrderedProfile to HeapProfileTable. Chose this design to encapsulate OS dependency within deep-heap-profile. On 2011/12/28 08:30:17, jar wrote: > Why isn't this line inside the ifdef? http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:140: }; Ah, I forgot or deleted it wrongly before. Added. On 2011/12/28 08:30:17, jar wrote: > Why doesn't this class have the unsafe copy and constructor macro?
Fixed the snprintf issues. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:108: snprintf(buf + bucket_length, size - bucket_length, kGlobalStatsHeader); Thanks for the point. I was completely out of touch with that. Fixed it by ignoring the fact we printed anything if it looks like the snprintf failed. On 2011/12/28 08:30:17, jar wrote: > If we would have overrun the buffer, then the bucket_length tally here can be > much larger than the allowable size. snprintf() returs what it *wanted* to use, > even if it did not over-run the buffer (in reality). http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:114: bucket_length += snprintf(buf + bucket_length, size - bucket_length, Fixed in the same way above. On 2011/12/28 08:30:17, jar wrote: > Here again you'd get into trouble if bucket_length exceeds size, which can > happen if snprintf() on line 113 was forced to truncate. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:123: RAW_DCHECK(bucket_length < size, ""); It detects buffer overrun now by the fixes above. On 2011/12/28 08:30:17, jar wrote: > Between truncations and possible errors along the way, I'm not sure what this > assertion will definitively protect against. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:353: buflen += snprintf(buf + buflen, bufsize - buflen, " 0x%08" PRIxPTR, On 2011/12/28 08:30:17, jar wrote: > Here again, I think there is a problem when buflen exceeds bufsize (due to > truncation in line 350). Done. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:434: return buflen + printed; On 2011/12/28 08:30:17, jar wrote: > This again may cause problems when line 426 truncates. Done. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:438: buflen += snprintf(buf + buflen, bufsize - buflen, On 2011/12/28 08:30:17, jar wrote: > When buflen is larger than bufsize, we'll get a negative number, which is > silently cast to size_t (unsigned). I don't think this is what you want. > > See callsite comment for an example. Done.
I think this is much safer/better. Thanks! Mostly just a bunch of nits. The if statement on a single line was bothering me... so I looked in the style guide and found some support for askin' for you to use two lines. I think it might be nicer to use full-blown names for the variables as well. I commented where it was especially noticeable, but you may want to change them consistently in this new file (e.g., buf-->buffer; bufsize-->buffer_size; buflen-->buffer_length). Thanks in advance. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:127: HeapProfileTable* heap_profile_; nit: OK... so if you don't want it in the ifdef, how about putting it on line 186, after you close the ifdef (for good)? On 2012/01/06 01:25:23, Dai Mikurube wrote: > It is used to delegate calling of FillOrderedProfile to HeapProfileTable. Chose > this design to encapsulate OS dependency within deep-heap-profile. > > On 2011/12/28 08:30:17, jar wrote: > > Why isn't this line inside the ifdef? > http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:108: if (printed < 0 || printed >= size) return 0; nit: prefer putting condition on separate line unless condition is very brief. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:109: int bucket_length = printed; nit: I really liked you variable name |printed|, but this |bucket_length| is then much harder to grok. How about: used_in_buf This also calls into question the use of the appreviation |buf|, and perhaps |buffer| would be better. ... leading to: used_in_buffer or some similar (better?) name. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:113: kGlobalStatsHeader); nit: wrap args at paren. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:114: if (printed < 0 || printed >= size - bucket_length) return bucket_length; nit: prefer putting condition on separate line unless condition is very brief. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:122: if (printed < 0 || printed >= size - bucket_length) return bucket_length; nit: prefer putting condition on separate line unless condition is very brief. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:127: if (printed < 0 || printed >= size - bucket_length) return bucket_length; nit: condition on separate line please. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:368: if (printed < 0 || printed >= bufsize) return 0; nit: put return on next line. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:374: if (printed < 0 || printed >= bufsize - buflen) return buflen; nit: return on next line. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:375: buflen += printed; Maybe |buflen| is a better name than bucket_size mentioned earlier.... but again, the style guide suggests not using abbreviation or shortened words. Probably buffer_length would be stylistically better in both places. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:378: if (printed < 0 || printed >= bufsize - buflen) return buflen; nit: return on next line. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:457: if (printed < 0 || printed >= bufsize - buflen) return buflen; nit: two lines. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:466: if (printed < 0 || printed >= bufsize - buflen) return buflen; nit: two lines.
Thank you for the comments. Updated. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:108: if (printed < 0 || printed >= size) return 0; On 2012/01/06 03:05:10, jar wrote: > nit: prefer putting condition on separate line unless condition is very brief. Done. I like the "separated lines" policy actually, but I was following the existing format in heap-profile-table.cc for consistency. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:109: int bucket_length = printed; Agreed. Replaced to 'used_in_buffer'. On 2012/01/06 03:05:10, jar wrote: > nit: I really liked you variable name |printed|, but this |bucket_length| is > then much harder to grok. > > How about: > used_in_buf > > This also calls into question the use of the appreviation |buf|, and perhaps > |buffer| would be better. ... leading to: > > used_in_buffer > > or some similar (better?) name. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:113: kGlobalStatsHeader); On 2012/01/06 03:05:10, jar wrote: > nit: wrap args at paren. Done. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:114: if (printed < 0 || printed >= size - bucket_length) return bucket_length; On 2012/01/06 03:05:10, jar wrote: > nit: prefer putting condition on separate line unless condition is very brief. Done. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:122: if (printed < 0 || printed >= size - bucket_length) return bucket_length; On 2012/01/06 03:05:10, jar wrote: > nit: prefer putting condition on separate line unless condition is very brief. Done. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:127: if (printed < 0 || printed >= size - bucket_length) return bucket_length; On 2012/01/06 03:05:10, jar wrote: > nit: condition on separate line please. Done. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:368: if (printed < 0 || printed >= bufsize) return 0; On 2012/01/06 03:05:10, jar wrote: > nit: put return on next line. Done. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:374: if (printed < 0 || printed >= bufsize - buflen) return buflen; On 2012/01/06 03:05:10, jar wrote: > nit: return on next line. Done. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:375: buflen += printed; On 2012/01/06 03:05:10, jar wrote: > Maybe |buflen| is a better name than bucket_size mentioned earlier.... but > again, the style guide suggests not using abbreviation or shortened words. > Probably buffer_length would be stylistically better in both places. Done. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:378: if (printed < 0 || printed >= bufsize - buflen) return buflen; On 2012/01/06 03:05:10, jar wrote: > nit: return on next line. Done. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:457: if (printed < 0 || printed >= bufsize - buflen) return buflen; On 2012/01/06 03:05:10, jar wrote: > nit: two lines. Done. http://codereview.chromium.org/8632007/diff/61002/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:466: if (printed < 0 || printed >= bufsize - buflen) return buflen; On 2012/01/06 03:05:10, jar wrote: > nit: two lines. Done.
Hi, Just pinging. I'd be happy if you look at it when you have time... (Though http://code.google.com/p/google-perftools/issues/detail?id=384 and updating third_party/tcmalloc is required before landing this change.) Thanks in advance.
I only started to comment, but I'd rather you take a shot at improving comments and such (more globally) along the lines suggested, before I read further. Thanks in advance. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:110: buffer, buffer_size, nit: indent shown here is 5. Better would be to wrap at the paren for the snprintf( http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:119: buffer + used_in_buffer, buffer_size - used_in_buffer, nit: indent. If you wrap long lines, it should be 4. Better is to wrap at the paren for a function call. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:10: // DeepHeapProfile::FillOrderedProfile() which works as an alternative of I found the name "Deep" HeapProfile pretty obscure. This intro gives me the answer (I think), in that we include memory-residence in the report. What other OS-level information is there? If that is the only piece of data, perhaps you could provide a name that is better than "Deep." Perhaps it should be a HeapResidencyProfile. I don't know... but I'm sure there can be a better name. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:72: struct PageState { I'm hopeful that this struct is only used briefly in interfaces, and it is not a memory use issues (and should be made into a bit field). http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:88: RegionStats anonymous; I have no idea what anonymous means here. Please add some comments to this, and a few other obscure slots, like "other," record_tcmalloc," and "record_mmap." http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:98: // Reset committed_size member variables in DeepBucket objects to 0. I'm unclear on the relationship between DeepBucket and the bucket_table argument. I don't understand how many such objects are going to be processed, and am confused by the pointer to pointer to Bucket... which perhaps suggests an array of pointers to buckets(?) or perhaps you're passing in the & of a given Bucket*, but that seems strange, becauuse I don't expect you to change the Bucket* (based on the description). In general, we only pass in a pointer if we intend to be able to modify the the value. I suspect you really meant to pass something like a const Bucket*&... but I can't tell yet.. and it would be nice if the comment and prototype clued me in better. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:101: // Fill bucket data in 'table' into buffer 'buffer' of size 'size', and return nit: If you wish to refer to an argument during your comment, please surround it with vertical bars. If you are refering to an argument, please spell it identically to the real arg. for example, possibly: 'table' ---> |bucket_table| http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:105: int FillBucketTable(Bucket** bucket_table, char buffer[], int buffer_size, Here again, the comment is made confusing by the prototype. I don't understand why you're passing in a pointer to pointer to Bucket. The comment suggests this is only a data source... so I would have expected some const ref to clarify this fact. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:111: // Seek the offset of the open pagemap file pagemap_fd_. nit: "Seek the ..." --> "Seek to the ..." What does the return value signify? http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:114: // Read a pagemap state from the current pagemap_fd_ offset. What does the return value mean? http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:118: // from 'addr' to 'addr + size'. nit: clearer English, plus please spell argument names. Suggest for example: Returns the number of committed or resident bytes of the memory region starting at |address| and ending at |address + size - 1| inclusive. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:122: void InitRegionStats(RegionStats* stats); This looks like it should be a method on RegionStats. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:124: // Add the virtual size (end - start) and the committed size in the region nit: "committed size" --> "committed byte count" nit: The use of the word "Add" confused me, because it seem to say "Add the two values together." Better would be: Update |stats| to include the tallies of virtual_size and committed bytes in the region from |start_adress| to |end_address - 1| inclusive. (note that your comment is especially unclear about whether the end_address is part of the region, or just beyond teh end of the region, but I'm taking a guess here. Why are you sometimes passing in start and length, and other times passing in start and end?? http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:126: void RecordRegionStats(uint64 start_address, uint64 end_address, This also looks like a method on RegionStats. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:134: static void RecordAlloc(const void* pointer, AllocValue* alloc_value, I can't tell what this function does from the comment... and I'm especially at a loss about what the arguments are for.
Thank you for reviewing, Jim. Updating the patch might require some more time due to a cold...
Thanks for the comments, Jim. Updated the patch. I wonder if you could take a look. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:110: buffer, buffer_size, On 2012/01/13 20:37:23, jar wrote: > nit: indent shown here is 5. Better would be to wrap at the paren for the > snprintf( Done. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:119: buffer + used_in_buffer, buffer_size - used_in_buffer, On 2012/01/13 20:37:23, jar wrote: > nit: indent. If you wrap long lines, it should be 4. Better is to wrap at the > paren for a function call. Done. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:10: // DeepHeapProfile::FillOrderedProfile() which works as an alternative of On 2012/01/13 20:37:23, jar wrote: > I found the name "Deep" HeapProfile pretty obscure. This intro gives me the > answer (I think), in that we include memory-residence in the report. What other > OS-level information is there? > > If that is the only piece of data, perhaps you could provide a name that is > better than "Deep." Perhaps it should be a HeapResidencyProfile. I don't > know... but I'm sure there can be a better name. I thought about that, but actually, it is planned to make a tool set called Deep Memory(Heap) Profiler for Chromium, and some more information will be dumped additionally from this extra dumper for the profiler. (This change is the first step.) I guess they are not good 1) to add many files in this third_party/tcmalloc/chromium directory, and 2) to execute dump-related operations in distributed classes and files for performance. In order to encapsulate the extra dumper code in a single file, and to avoid a misleading file name and frequent renaming in future, I chose to use the name deep-heap-profile and DeepHeapProfile for the file and class. If naming for the current dumper code, HeapResidencyProfile sounds good. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:72: struct PageState { On 2012/01/13 20:37:23, jar wrote: > I'm hopeful that this struct is only used briefly in interfaces, and it is not a > memory use issues (and should be made into a bit field). Sorry, I don't understand what you mean... You mean this structure is not required? http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:88: RegionStats anonymous; On 2012/01/13 20:37:23, jar wrote: > I have no idea what anonymous means here. Please add some comments to this, and > a few other obscure slots, like "other," record_tcmalloc," and "record_mmap." Done. These regions should be classified more precisely later for more detailed profiling, for example [heap] and [stack] regions, but it's enough for now. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:98: // Reset committed_size member variables in DeepBucket objects to 0. On 2012/01/13 20:37:23, jar wrote: > I'm unclear on the relationship between DeepBucket and the bucket_table > argument. I don't understand how many such objects are going to be processed, > and am confused by the pointer to pointer to Bucket... which perhaps suggests an > array of pointers to buckets(?) or perhaps you're passing in the & of a given > Bucket*, but that seems strange, becauuse I don't expect you to change the > Bucket* (based on the description). > > In general, we only pass in a pointer if we intend to be able to modify the the > value. I suspect you really meant to pass something like a const Bucket*&... > but I can't tell yet.. and it would be nice if the comment and prototype clued > me in better. 'Bucket**' is a data structure used in the original heap-profile-table.cc for hash tables. (For example, see "Bucket* GetBucket(..., Bucket** table)", "Bucket** MakeSortedBucketList()" and "Bucket** table_" in heap-profile-table.h.) We can use typedef for Bucket**, but I think keeping it Bucket** is better for consistency. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:101: // Fill bucket data in 'table' into buffer 'buffer' of size 'size', and return On 2012/01/13 20:37:23, jar wrote: > nit: If you wish to refer to an argument during your comment, please surround it > with vertical bars. If you are refering to an argument, please spell it > identically to the real arg. > > for example, possibly: 'table' ---> |bucket_table| Like some other styling issues, I followed the existing code style such as heap-profile-table.{h|cc}. What do you think which we should follow: google-perftools (third_party/tcmalloc) or Chromium here? I kept "'" in the updated patch for now. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:105: int FillBucketTable(Bucket** bucket_table, char buffer[], int buffer_size, On 2012/01/13 20:37:23, jar wrote: > Here again, the comment is made confusing by the prototype. I don't understand > why you're passing in a pointer to pointer to Bucket. The comment suggests this > is only a data source... so I would have expected some const ref to clarify this > fact. The same as above comment in Line 98. Another option might be adding comments for Bucket** for all functions using Bucket**. What do you think? http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:111: // Seek the offset of the open pagemap file pagemap_fd_. On 2012/01/13 20:37:23, jar wrote: > nit: "Seek the ..." --> "Seek to the ..." > > What does the return value signify? Modified the function and added a comment. It returns true if succeeded. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:114: // Read a pagemap state from the current pagemap_fd_ offset. On 2012/01/13 20:37:23, jar wrote: > What does the return value mean? Added a comment. It returns true if succeeded. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:118: // from 'addr' to 'addr + size'. On 2012/01/13 20:37:23, jar wrote: > nit: clearer English, plus please spell argument names. > > Suggest for example: > Returns the number of committed or resident bytes of the memory region starting > at |address| and ending at |address + size - 1| inclusive. Thanks for a nice example. I used your example with a little changes. (e.g. used "'" as replied above.) http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:122: void InitRegionStats(RegionStats* stats); On 2012/01/13 20:37:23, jar wrote: > This looks like it should be a method on RegionStats. Made it RegionStats::Initialize(), and made *ProcPagemap and GetCommittedSize static. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:124: // Add the virtual size (end - start) and the committed size in the region On 2012/01/13 20:37:23, jar wrote: > nit: "committed size" --> "committed byte count" > > nit: The use of the word "Add" confused me, because it seem to say "Add the two > values together." Better would be: > > Update |stats| to include the tallies of virtual_size and committed bytes in the > region from |start_adress| to |end_address - 1| inclusive. > > (note that your comment is especially unclear about whether the end_address is > part of the region, or just beyond teh end of the region, but I'm taking a guess > here. > > Why are you sometimes passing in start and length, and other times passing in > start and end?? Decided to use your recommendation, and changed to describe all regions by pairs of 'first_address'es and 'last_address'es inclusive. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:126: void RecordRegionStats(uint64 start_address, uint64 end_address, On 2012/01/13 20:37:23, jar wrote: > This also looks like a method on RegionStats. Made it RegionStats::Record(). http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:134: static void RecordAlloc(const void* pointer, AllocValue* alloc_value, On 2012/01/13 20:37:23, jar wrote: > I can't tell what this function does from the comment... and I'm especially at a > loss about what the arguments are for. Modified the comment as below : Record both virtual and committed byte counts of malloc and mmap regions as callback functions for AllocationMap::Iterate().
Separated a mmap part and a malloc part in dumped result. Changes in malloc_hook.cc can be removed by this separation. (Will be removed later.)
Hi, Removed changes in malloc_hook.cc finally. It's ready for review. Could you PTAL at this?
Here are some comments that were sitting in my draft list... I'll look at the final version now. Jim http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:72: struct PageState { I was surprised to see a struct with 5 bools, rather than one bitfield with 5 one-bit fields used. I then wondered whether this was used often (with a memory footprint that we care about), or trivially (one or two instances? only for argument passing of short lived instances??). On 2012/01/19 11:54:14, Dai Mikurube wrote: > On 2012/01/13 20:37:23, jar wrote: > > I'm hopeful that this struct is only used briefly in interfaces, and it is not > a > > memory use issues (and should be made into a bit field). > > Sorry, I don't understand what you mean... You mean this structure is not > required? http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:98: // Reset committed_size member variables in DeepBucket objects to 0. I think this must be part of newer merges, as I don't recall this stuff, and need to read more. (It could also be that I'm senile, and/or forgot about it). On 2012/01/19 11:54:14, Dai Mikurube wrote: > On 2012/01/13 20:37:23, jar wrote: > > I'm unclear on the relationship between DeepBucket and the bucket_table > > argument. I don't understand how many such objects are going to be processed, > > and am confused by the pointer to pointer to Bucket... which perhaps suggests > an > > array of pointers to buckets(?) or perhaps you're passing in the & of a given > > Bucket*, but that seems strange, becauuse I don't expect you to change the > > Bucket* (based on the description). > > > > In general, we only pass in a pointer if we intend to be able to modify the > the > > value. I suspect you really meant to pass something like a const Bucket*&... > > but I can't tell yet.. and it would be nice if the comment and prototype clued > > me in better. > > 'Bucket**' is a data structure used in the original heap-profile-table.cc for > hash tables. (For example, see "Bucket* GetBucket(..., Bucket** table)", > "Bucket** MakeSortedBucketList()" and "Bucket** table_" in > heap-profile-table.h.) > > We can use typedef for Bucket**, but I think keeping it Bucket** is better for > consistency. http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:101: // Fill bucket data in 'table' into buffer 'buffer' of size 'size', and return In new files, always try to use newer style. When in old files, making minor internal changes, stick with the pre-existing style. Since this is a brand new file, I'd suggest using current style. There is a second question of where we should land new files that are not part of the upstream TCMalloc code. I think it is generally easier to do merges if we avoid adding our files to the list... but I haven't been staring at this code in a while, and don't recall exactly where we hung our add-on files. I think you should find some shim code, and probably try to land your files around that... unless you are planning on up-streaming the changes RSN. On 2012/01/19 11:54:14, Dai Mikurube wrote: > On 2012/01/13 20:37:23, jar wrote: > > nit: If you wish to refer to an argument during your comment, please surround > it > > with vertical bars. If you are refering to an argument, please spell it > > identically to the real arg. > > > > for example, possibly: 'table' ---> |bucket_table| > > Like some other styling issues, I followed the existing code style such as > heap-profile-table.{h|cc}. What do you think which we should follow: > google-perftools (third_party/tcmalloc) or Chromium here? > > I kept "'" in the updated patch for now. http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/heap-profile-table.h (right): http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.h:185: // Refresh the internal mmap information from MemoryRegionMap. Results of nit: "Refresh ..." --> "Refreshes ..." http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.h:186: // FillOrderedProfile and IterateOrderedAllocContexts will contain mmap'ed nit: "...will contain ..." --> "Updates ... to contain ..." http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.h:187: // memory regions as at calling RefreshMMapData. nit: typo? "...as at calling ..." http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.h:190: // Clear the internal mmap information. Results of FillOrderedProfile and nit: "Clear ..." --> "Clears ..." http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.h:192: // calling ClearMMapData. nit suggestion: Change "Results of ... won't contain ..." to "Empties ..."
Here are a large set of questions and nits. I haven't really done a full review, as some of the code should be changed to make it easier to read (as per suggestions below), and then I'll have another go at it. I still really don't get the name "deep_heap_profile.". I'm sure you can provide a better name... and as I grok more and more what you're doing, perhaps I can suggest something better. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:18: #endif For endif, please add comment showing what is terminated: #endif // HAVE_UNISTD_H http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:26: static const int PAGE_SIZE = 4096; It surprises me that we don't have the moral equivalent to this elsewhere in TCMalloc. For instance, why aren't you using getpagesize()? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:28: static const uint64 TOP_ADDRESS = kuint64max; Why do you use uint64 for addresses, when size_t is defined to handle this nicely? (and not waste space and performance on a 32bit OS). http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:55: memcpy(filename_prefix_, prefix, prefix_length); Why not just strcpy(), given that you base this all on strlen in line 52? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:93: // Note: Don't allocate any memory from here. Could you motivate the comment? I'm guessing you're start to snapshot data, and don't want anything to happen during that snapshot. When I've implemented such stuff, I work very hard to make the snapshotting region as small as possible, which also makes it clear there is compliance. In this case, you call a pile of code, and it is really hard to see what is happening where. It might even be nice to include the assertion into the function names so that I'd remember abide by the restriction when I review the called functions. For instance, perhaps: RecordAllAllocs() SnapshotAllAllocs() GetGlobalStats() SnapshotGlobalStats() or maybe you should put on a postfix: RecordAllAllocs --> RecordAllAllocsSansAllocating GetGlobalStats --> GetGlobalStatsSansAllocating All I can tell is that it is very hard to review the code and assertions here. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:108: HeapProfileTable::Stats stats; Please move closer to first (real) use.... around line 145. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:112: int printed = snprintf(buffer, buffer_size, How are you sure than snprintf() doesn't allocate any memory during performance of its function? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:146: buffer_size, used_in_buffer, &stats); This sure looks like a method on stats, but perhaps that is not code you control (otherwise it should be made a method). Maybe it should be a method on a derived class, that you define?? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:206: sprintf(filename, "/proc/%d/pagemap", getpid()); use snprintf http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:217: if (offset < 0) { nit: return offset >= 0; http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:321: char *flags, *filename; nit: Chrome style puts the "*" next to the type. As a result, it is probably better to put the above on two lines as well. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:338: committed_bytes = stats->total.committed_bytes - committed_bytes; Why isn't this merely committed_bytes = 0 In addition, since committed_bytes is never again used in this iteration of the loop, why are you even updating it? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:351: DeepHeapProfile::GetDeepBucket(Bucket* bucket) { nit: Try to put function definition on one line. Wrap at the open paren if you must. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:361: } else { style nit: both sides of the if have a return... so you don't need an else. If you put the tiny branch |return found| first, then you don't need curlies, and don't need to indent the code in lines 354-360. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:368: for (Bucket* b = bucket_table[i]; b != 0; b = b->next) { nit: use NULL for pointers: b != NULL http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:381: for (Bucket* b = bucket_table[i]; b != 0; b = b->next) { nit: NULL instead of 0 http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:385: const DeepBucket& db = *GetDeepBucket(b); nit: suggest: const DeepBucket db = GetDeepBucket(b); then pass "*db" as an argument to the function. nit: don't use abbreviations like "db" (which has a lot of loading meaning besides). http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:59: int FillOrderedProfile(char buffer[], int buffer_size); It appears that the |buffer| is nothing more than a temporary work area. Certainly no data can be returned, as there is no API for indicating what was/is used. Given that it is a temp area, why don't you stack allocate in FillOrderedProfile()? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:80: struct RegionStats { Please make this into a class, and then rename the slots to end in an underscore. The methods are now more than complex enough to warrant a class, and it will help with readability. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:124: static void ClearIsLogged(const void* pointer, DeepBucket* db, nit: one arg per line. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:152: static void GetGlobalStats(int pagemap_fd, GlobalStats* stats); This was made a static method... but I think it all cases we call it with a slot from a specific instance. Why isn't this a non-static member function, that simply accesses pagemap_fd_? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:165: int FillBucketTable(Bucket** bucket_table, char buffer[], int buffer_size, nit: if declaration can't put the whole thing on one line, then put one arg per line (unless the args group together). http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:170: static void RecordAlloc(const void* pointer, AllocValue* alloc_value, nit: one arg per line http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:178: int FillBucketForBucketFile(const DeepBucket* deep_bucket, nit: modified arg |deep_bucket| should be last in list. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:200: int buffer_size); nit: buffer should be the last arg (since it is modified). http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:203: int UnparseGlobalStats(char* buffer, int used_in_buffer, int buffer_size); nit: When a method modifies values, the values that are modified come at the end of the list of args. In this case, presumably |buffer| should be last. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:210: pid_t most_recent_pid_; // Process ID of the last dump. This could change. How does this change? fork? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profiler.cc:205: if (FLAGS_deep_heap_profile) { nit: IMO, more clearly correct is to test: if (deep_profile) here and on line 523. I don't like when we have two ways to decide stuff... and it seems nicer to use the authoritative item that is needed for the code at line 206 (or line 525).
Thank you for the fine comments. Updated the patch and replied inline. http://codereview.chromium.org/8632007/diff/44001/base/allocator/allocator.gyp File base/allocator/allocator.gyp (right): http://codereview.chromium.org/8632007/diff/44001/base/allocator/allocator.gy... base/allocator/allocator.gyp:304: # heap-profiler/checker/cpuprofiler On 2011/12/23 09:58:06, Alexander Potapenko wrote: > E.g. I think you need to put deep-profiler here. Sorry, I overlooked your comment. Thanks for your good catch. Done it. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:127: HeapProfileTable* heap_profile_; Sorry, I overlooked this comment. Done it. On 2012/01/06 03:05:10, jar wrote: > nit: OK... so if you don't want it in the ifdef, how about putting it on line > 186, after you close the ifdef (for good)? > > On 2012/01/06 01:25:23, Dai Mikurube wrote: > > It is used to delegate calling of FillOrderedProfile to HeapProfileTable. > Chose > > this design to encapsulate OS dependency within deep-heap-profile. > > > > On 2011/12/28 08:30:17, jar wrote: > > > Why isn't this line inside the ifdef? > > > http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:72: struct PageState { Got it, thanks. This is used only by ReadProcPagemap, and we have only one PageState instance simultaneously, which is used repeatedly. It's not memory-consuming. I'll change it if we use many PageState instances simutaneously. On 2012/01/26 20:19:02, jar wrote: > I was surprised to see a struct with 5 bools, rather than one bitfield with 5 > one-bit fields used. I then wondered whether this was used often (with a memory > footprint that we care about), or trivially (one or two instances? only for > argument passing of short lived instances??). > > On 2012/01/19 11:54:14, Dai Mikurube wrote: > > On 2012/01/13 20:37:23, jar wrote: > > > I'm hopeful that this struct is only used briefly in interfaces, and it is > not > > a > > > memory use issues (and should be made into a bit field). > > > > Sorry, I don't understand what you mean... You mean this structure is not > > required? > http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:98: // Reset committed_size member variables in DeepBucket objects to 0. Bucket** is a very internal structure of heap-profile-table, not exposed out of heap-profile-table. I think it might be a reason why you don't know it. deep-heap-profile reads Bucket** as it's a friend of heap-profile-table. On 2012/01/26 20:19:02, jar wrote: > I think this must be part of newer merges, as I don't recall this stuff, and > need to read more. (It could also be that I'm senile, and/or forgot about it). > > On 2012/01/19 11:54:14, Dai Mikurube wrote: > > On 2012/01/13 20:37:23, jar wrote: > > > I'm unclear on the relationship between DeepBucket and the bucket_table > > > argument. I don't understand how many such objects are going to be > processed, > > > and am confused by the pointer to pointer to Bucket... which perhaps > suggests > > an > > > array of pointers to buckets(?) or perhaps you're passing in the & of a > given > > > Bucket*, but that seems strange, becauuse I don't expect you to change the > > > Bucket* (based on the description). > > > > > > In general, we only pass in a pointer if we intend to be able to modify the > > the > > > value. I suspect you really meant to pass something like a const > Bucket*&... > > > but I can't tell yet.. and it would be nice if the comment and prototype > clued > > > me in better. > > > > 'Bucket**' is a data structure used in the original heap-profile-table.cc for > > hash tables. (For example, see "Bucket* GetBucket(..., Bucket** table)", > > "Bucket** MakeSortedBucketList()" and "Bucket** table_" in > > heap-profile-table.h.) > > > > We can use typedef for Bucket**, but I think keeping it Bucket** is better for > > consistency. > http://codereview.chromium.org/8632007/diff/66001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:101: // Fill bucket data in 'table' into buffer 'buffer' of size 'size', and return Got it, agreed. Changed the style to |argument|-style. Your second question is a good point to be discussed. I thought about moving it out of third_party, actually. The reason why I finally chose to locate this code in third_party/tcmalloc is because : 1) It has to read some internal data structures in heap-profile-table. 2) For (1), deep-heap-profile must be a friend of heap-profile-table, or heap-profile-table must disclose Bucket** and AllocationMap by some getter functions. 3) I chose the former since : 3-a) 'friend' affects less than disclosing getters in terms of security-like issues. friend allows access only to deep-heap-profile. 3-b) 'friend' changes less lines of code in the original heap-profile*.{cc|h}. Less changes in the original code makes us happy when merging upstream versions of tcmalloc in future. On 2012/01/26 20:19:02, jar wrote: > In new files, always try to use newer style. When in old files, making minor > internal changes, stick with the pre-existing style. > > Since this is a brand new file, I'd suggest using current style. > > There is a second question of where we should land new files that are not part > of the upstream TCMalloc code. I think it is generally easier to do merges if we > avoid adding our files to the list... but I haven't been staring at this code in > a while, and don't recall exactly where we hung our add-on files. I think you > should find some shim code, and probably try to land your files around that... > unless you are planning on up-streaming the changes RSN. > > On 2012/01/19 11:54:14, Dai Mikurube wrote: > > On 2012/01/13 20:37:23, jar wrote: > > > nit: If you wish to refer to an argument during your comment, please > surround > > it > > > with vertical bars. If you are refering to an argument, please spell it > > > identically to the real arg. > > > > > > for example, possibly: 'table' ---> |bucket_table| > > > > Like some other styling issues, I followed the existing code style such as > > heap-profile-table.{h|cc}. What do you think which we should follow: > > google-perftools (third_party/tcmalloc) or Chromium here? > > > > I kept "'" in the updated patch for now. > http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/heap-profile-table.h (right): http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.h:185: // Refresh the internal mmap information from MemoryRegionMap. Results of Thanks for your comments. Hmm, it's sure that these functions are added by me, but they're in the upstream tcmalloc/google-perftools. The change is accepted and hopefully landed in the next release: http://code.google.com/p/gperftools/issues/detail?id=383. I guess we should fix them in the upstream if required. And, they follow other existing comments in heap-profile-table.h. They use verbs without "-s". On 2012/01/26 20:19:02, jar wrote: > nit: "Refresh ..." --> "Refreshes ..." http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.h:186: // FillOrderedProfile and IterateOrderedAllocContexts will contain mmap'ed On 2012/01/26 20:19:02, jar wrote: > nit: "...will contain ..." --> "Updates ... to contain ..." Ditto, but thanks for a good alternative. I'll consider it in the upstream. http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.h:187: // memory regions as at calling RefreshMMapData. On 2012/01/26 20:19:02, jar wrote: > nit: typo? "...as at calling ..." Ditto. http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.h:190: // Clear the internal mmap information. Results of FillOrderedProfile and On 2012/01/26 20:19:02, jar wrote: > nit: "Clear ..." --> "Clears ..." Ditto. http://codereview.chromium.org/8632007/diff/78001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profile-table.h:192: // calling ClearMMapData. On 2012/01/26 20:19:02, jar wrote: > nit suggestion: Change "Results of ... won't contain ..." to "Empties ..." Ditto. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:18: #endif On 2012/01/27 00:46:58, jar wrote: > For endif, please add comment showing what is terminated: > #endif // HAVE_UNISTD_H Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:26: static const int PAGE_SIZE = 4096; On 2012/01/27 00:46:58, jar wrote: > It surprises me that we don't have the moral equivalent to this elsewhere in > TCMalloc. For instance, why aren't you using getpagesize()? getpagesize() is better. Fixed. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:28: static const uint64 TOP_ADDRESS = kuint64max; On 2012/01/27 00:46:58, jar wrote: > Why do you use uint64 for addresses, when size_t is defined to handle this > nicely? (and not waste space and performance on a 32bit OS). Simply, its' sbecause ProcMapsIterator::Next(Ext) (base/sysinfo.h) uses uint64 for addresses even if 32-bits. One reason is written in a comment for ProcMapsIterator::Next, and another reason is mixing uint64 and size_t is confusing. No uint64 values are stored in DeepHeapProfile. They're used only in stacks or as constants. I think it's not a problem. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:55: memcpy(filename_prefix_, prefix, prefix_length); On 2012/01/27 00:46:58, jar wrote: > Why not just strcpy(), given that you base this all on strlen in line 52? I guess we thought that memcpy is faster in that case. strlen is required to allocate memory. strcpy checks null-termination again. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:93: // Note: Don't allocate any memory from here. On 2012/01/27 00:46:58, jar wrote: > Could you motivate the comment? I'm guessing you're start to snapshot data, and > don't want anything to happen during that snapshot. When I've implemented such > stuff, I work very hard to make the snapshotting region as small as possible, > which also makes it clear there is compliance. In this case, you call a pile of > code, and it is really hard to see what is happening where. It might even be > nice to include the assertion into the function names so that I'd remember abide > by the restriction when I review the called functions. For instance, perhaps: > > RecordAllAllocs() SnapshotAllAllocs() > GetGlobalStats() SnapshotGlobalStats() > > or maybe you should put on a postfix: > > RecordAllAllocs --> RecordAllAllocsSansAllocating > GetGlobalStats --> GetGlobalStatsSansAllocating > > All I can tell is that it is very hard to review the code and assertions here. Renamed snapshotting functions here into Snapshot...WithoutMalloc(). Calling malloc here, actually, doesn't cause violation, but it causes some gap from actual memory allocation. I found snprintf may call malloc if large memory is required internally, but, I decided to ignore it since it's a rare case. Exact numbers are not required in our analysis. (The original heap-profiler looks line ingoring it, too. HeapProfileTable::FillOrderedBucket calls HeapProfileTable::UnparseBucket which calls snprintf while sweeping a bucket table. Please look at the comment for HeapProfileTable::UnparseBucket.) Finally, I modified this comment, renamed snapshotting functions and added TODO. What do you think? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:108: HeapProfileTable::Stats stats; On 2012/01/27 00:46:58, jar wrote: > Please move closer to first (real) use.... around line 145. Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:112: int printed = snprintf(buffer, buffer_size, On 2012/01/27 00:46:58, jar wrote: > How are you sure than snprintf() doesn't allocate any memory during performance > of its function? Please look at the above reply. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:146: buffer_size, used_in_buffer, &stats); On 2012/01/27 00:46:58, jar wrote: > This sure looks like a method on stats, but perhaps that is not code you control > (otherwise it should be made a method). > > Maybe it should be a method on a derived class, that you define?? HeapProfileTable::Stats is just a public struct (not a class) which contains four integers. No additional members are required here. Why is deriving required? But, before all, I found that this stats is not required in the current code. Removed it for now. Maybe added in an extended version in future. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:206: sprintf(filename, "/proc/%d/pagemap", getpid()); On 2012/01/27 00:46:58, jar wrote: > use snprintf Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:217: if (offset < 0) { On 2012/01/27 00:46:58, jar wrote: > nit: return offset >= 0; Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:321: char *flags, *filename; On 2012/01/27 00:46:58, jar wrote: > nit: Chrome style puts the "*" next to the type. As a result, it is probably > better to put the above on two lines as well. Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:338: committed_bytes = stats->total.committed_bytes - committed_bytes; Removed. It was a code to be used in a planned extension... On 2012/01/27 00:46:58, jar wrote: > Why isn't this merely committed_bytes = 0 > > In addition, since committed_bytes is never again used in this iteration of the > loop, why are you even updating it? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:351: DeepHeapProfile::GetDeepBucket(Bucket* bucket) { On 2012/01/27 00:46:58, jar wrote: > nit: Try to put function definition on one line. Wrap at the open paren if you > must. Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:361: } else { On 2012/01/27 00:46:58, jar wrote: > style nit: both sides of the if have a return... so you don't need an else. If > you put the tiny branch |return found| first, then you don't need curlies, and > don't need to indent the code in lines 354-360. Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:368: for (Bucket* b = bucket_table[i]; b != 0; b = b->next) { On 2012/01/27 00:46:58, jar wrote: > nit: use NULL for pointers: > b != NULL Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:381: for (Bucket* b = bucket_table[i]; b != 0; b = b->next) { On 2012/01/27 00:46:58, jar wrote: > nit: NULL instead of 0 Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:385: const DeepBucket& db = *GetDeepBucket(b); On 2012/01/27 00:46:58, jar wrote: > nit: suggest: > const DeepBucket db = GetDeepBucket(b); > > then pass "*db" as an argument to the function. > > nit: don't use abbreviations like "db" (which has a lot of loading meaning > besides). Did you mean: const DeepBucket *db = GetDeepBucket(b); ? Renamed db -> deep_bucket, and b -> bucket. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:59: int FillOrderedProfile(char buffer[], int buffer_size); One reason is that the original heap-profiler is already using such a buffer. HeapProfiler already has such a buffer for HeapProfileTable::FillOrderedProfile. DeepHeapProfile uses it. The second reason is that a filled content, actually, can be returned through the buffer. Please look at an external API GetHeapProfile() in heap-profiler.cc, which is in the original heap-profiler. It returns the buffer. (I don't think it's so good to return malloc'ed pointer, though...) Third, allocation cost though it's just a small reason. FillOrderedProfile is called many times. At last, a malloc'ed and fixed buffer can be easily caught by the profiler itself. We can estimate how large memory is used by the profiler. On 2012/01/27 00:46:58, jar wrote: > It appears that the |buffer| is nothing more than a temporary work area. > Certainly no data can be returned, as there is no API for indicating what was/is > used. Given that it is a temp area, why don't you stack allocate in > FillOrderedProfile()? http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:80: struct RegionStats { Agreed. Done it. On 2012/01/27 00:46:58, jar wrote: > Please make this into a class, and then rename the slots to end in an > underscore. > > The methods are now more than complex enough to warrant a class, and it will > help with readability. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:124: static void ClearIsLogged(const void* pointer, DeepBucket* db, On 2012/01/27 00:46:58, jar wrote: > nit: one arg per line. Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:152: static void GetGlobalStats(int pagemap_fd, GlobalStats* stats); On 2012/01/27 00:46:58, jar wrote: > This was made a static method... but I think it all cases we call it with a slot > from a specific instance. Why isn't this a non-static member function, that > simply accesses pagemap_fd_? Because I (myself) sometimes confused where GetGlobalStats get information from... Making all pagemap-related functions static with |pagemap_fd| helps us to understand they're reading from pagemap easily. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:165: int FillBucketTable(Bucket** bucket_table, char buffer[], int buffer_size, On 2012/01/27 00:46:58, jar wrote: > nit: if declaration can't put the whole thing on one line, then put one arg per > line (unless the args group together). Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:170: static void RecordAlloc(const void* pointer, AllocValue* alloc_value, On 2012/01/27 00:46:58, jar wrote: > nit: one arg per line Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:178: int FillBucketForBucketFile(const DeepBucket* deep_bucket, On 2012/01/27 00:46:58, jar wrote: > nit: modified arg |deep_bucket| should be last in list. What do you mean by 'modified'? |deep_bucket| is a constant used to give information to write into buffer. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:200: int buffer_size); On 2012/01/27 00:46:58, jar wrote: > nit: buffer should be the last arg (since it is modified). Done for it and other functions, except for FillOrderedProfile. Kept FillOrderedProfile to be consistent with HeapProfileTable::FillOrderedProfile. At least it should have the same interface with HeapProfileTable. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:203: int UnparseGlobalStats(char* buffer, int used_in_buffer, int buffer_size); On 2012/01/27 00:46:58, jar wrote: > nit: When a method modifies values, the values that are modified come at the end > of the list of args. In this case, presumably |buffer| should be last. Done. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:210: pid_t most_recent_pid_; // Process ID of the last dump. This could change. On 2012/01/27 00:46:58, jar wrote: > How does this change? fork? Yes. Added a comments. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/heap-profiler.cc:205: if (FLAGS_deep_heap_profile) { Agreed. Done it. On 2012/01/27 00:46:58, jar wrote: > nit: IMO, more clearly correct is to test: > if (deep_profile) > here and on line 523. > > I don't like when we have two ways to decide stuff... and it seems nicer to use > the authoritative item that is needed for the code at line 206 (or line 525).
Hi Jim, btw, if it's better to split this change into multiple changes, I'll try it, though it looks hard to separate into many pieces due to its dependency... I guess it'll be just 3 changes at most.
Rebased it for the latest version. This patch applies on the latest tree directly.
Mostly nits now. Please try to do a test-landing to measure perf impact. Thanks, Jim http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:385: const DeepBucket& db = *GetDeepBucket(b); Yes... thanks... the point was to avoid using references inside the method. On 2012/01/30 12:54:53, Dai Mikurube wrote: > On 2012/01/27 00:46:58, jar wrote: > > nit: suggest: > > const DeepBucket db = GetDeepBucket(b); > > > > then pass "*db" as an argument to the function. > > > > nit: don't use abbreviations like "db" (which has a lot of loading meaning > > besides). > > Did you mean: > const DeepBucket *db = GetDeepBucket(b); > ? > > Renamed db -> deep_bucket, and b -> bucket. http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:178: int FillBucketForBucketFile(const DeepBucket* deep_bucket, I was mistaken about |deep_bucket|. Sorry. |buffer| is what is being modified and should probably be last... and it looks like you've moved it. Thanks! On 2012/01/30 12:54:53, Dai Mikurube wrote: > On 2012/01/27 00:46:58, jar wrote: > > nit: modified arg |deep_bucket| should be last in list. > > What do you mean by 'modified'? |deep_bucket| is a constant used to give > information to write into buffer. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:92: // Note: Least malloc from here. malloc here may cause a gap in the observed nit: English: Change: "Least malloc from here" --> "Try to minimize the number of calls to malloc in the following region, up until we call WriteBucketsToBucketFile(), near the end of this function. " http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:94: // allocated memory at maximum. It doesn't cause violation. nit: English: I would have proposed a better sentence, but wasn't sure what you meant by "the size of the gap." For the second sentence, consider changing: "It doesn't cause violation." --> "Calls to malloc won't break anything, but can add some noise to the recorded information." http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:174: // Note: Least malloc until here. nit: English: Suggest changing: "Note: Least malloc until here" --> "Note: Memory snapshots are complete, and malloc may again be used freely." http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:211: snprintf(filename, sizeof(filename), "/proc/%d/pagemap", getpid()); nit: Please add static cast of getpid() to type int, so that the %d is safe. Return type of getpid() is pid_t. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:219: static int page_size = 0; nit: Why bother to cache the page_size in a static? I think the underlying function actually caches it.... and this is not called that often anyway, is it? http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:257: static int page_size = 0; nit: don't bother with static cache. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:308: "%s.%05d.maps", filename_prefix, getpid()); nit: cast getpid() to int http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:107: size_t committed_bytes_; nit: If you don't need to copy these, you can add the DISALLOW_COPY_AND_ASSIGN macro.
A bit more comments. The CL looks mostly good, but let me take another look. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:27: static const uint64 TOP_ADDRESS = kuint64max; This is not true on 32-bit platforms (e.g. Chrome OS) It is better to use uintptr_t for pointers. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:54: memcpy(filename_prefix_, prefix, prefix_length); strcpy? http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:108: RAW_LOG(0, "Difference in committed size: %ld", committed_difference); How about removing NDEBUG and just printing this at another log level? http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:139: "%10s %10s\n", "virtual", "committed"); Suggestion #1: make "virtual" and "committed" constant strings. Suggestion #2: use tabs if this is just about pretty printing: "\tvirtual\tcommitted\n" http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:164: used_in_buffer += printed; This pattern is repeated for 5 times. Please put it into a private method. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:179: #ifndef NDEBUG I suggest you do not use NDEBUG at all. The code in these sections should not take much time, so it should be ok to turn it on by default. If you want to avoid too much logging, you can increase the log level in RAW_LOG. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:223: RAW_DCHECK(offset == index, ""); What shall be printed if this DCHECK fails? http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:305: char buffer[]) { Let the order of (buffer, buffer_size) be consistent in all the functions. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:330: int64 inode; Please call it "unused_inode" to denote it is unused (can't remember whether iterator.Next accepts NULLs) http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.h:186: int buffer_size, I think the buffer size usually goes after the buffer pointer in the parameter list. Leaving this up to you. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.h:231: int pagemap_fd_; // File descriptor of /proc/self/pagemap. Consider using RawFD
Thank you, Jim, Alexander. I tried an experimental landing in r127171, and reverted in r127194. Please look at http://build.chromium.org/f/chromium/perf/dashboard/overview.html. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:92: // Note: Least malloc from here. malloc here may cause a gap in the observed On 2012/03/16 01:20:02, jar wrote: > nit: English: > Change: > "Least malloc from here" --> "Try to minimize the number of calls to malloc in > the following region, up until we call WriteBucketsToBucketFile(), near the end > of this function. " Done. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:94: // allocated memory at maximum. It doesn't cause violation. On 2012/03/16 01:20:02, jar wrote: > nit: English: > I would have proposed a better sentence, but wasn't sure what you meant by "the > size of the gap." > > For the second sentence, consider changing: > "It doesn't cause violation." --> "Calls to malloc won't break anything, but > can add some noise to the recorded information." Done. Thank you for good suggestion in English. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:174: // Note: Least malloc until here. On 2012/03/16 01:20:02, jar wrote: > nit: English: Suggest changing: > "Note: Least malloc until here" --> "Note: Memory snapshots are complete, and > malloc may again be used freely." Done. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:211: snprintf(filename, sizeof(filename), "/proc/%d/pagemap", getpid()); On 2012/03/16 01:20:02, jar wrote: > nit: Please add static cast of getpid() to type int, so that the %d is safe. > Return type of getpid() is pid_t. Done. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:219: static int page_size = 0; On 2012/03/16 01:20:02, jar wrote: > nit: Why bother to cache the page_size in a static? I think the underlying > function actually caches it.... and this is not called that often anyway, is it? Exactly. Done. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:257: static int page_size = 0; On 2012/03/16 01:20:02, jar wrote: > nit: don't bother with static cache. Done. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:308: "%s.%05d.maps", filename_prefix, getpid()); On 2012/03/16 01:20:02, jar wrote: > nit: cast getpid() to int Done. http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/95001/third_party/tcmalloc/chromi... third_party/tcmalloc/chromium/src/deep-heap-profile.h:107: size_t committed_bytes_; On 2012/03/16 01:20:02, jar wrote: > nit: If you don't need to copy these, you can add the DISALLOW_COPY_AND_ASSIGN > macro. Done. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:27: static const uint64 TOP_ADDRESS = kuint64max; On 2012/03/16 11:11:11, Alexander Potapenko wrote: > This is not true on 32-bit platforms (e.g. Chrome OS) > It is better to use uintptr_t for pointers. I used 64-bit int maximum since * 64-bit values are consistently used in the tcmalloc. * It's the upper limit check to avoid overflow in GetCommittedSize. The name TOP_ADDRESS might be confusing... Renamed it to MAX_ADDRESS. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:54: memcpy(filename_prefix_, prefix, prefix_length); On 2012/03/16 11:11:11, Alexander Potapenko wrote: > strcpy? As written in http://codereview.chromium.org/8632007/diff/94001/third_party/tcmalloc/chromi..., we chose memcpy since we already know length. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:108: RAW_LOG(0, "Difference in committed size: %ld", committed_difference); On 2012/03/16 11:11:11, Alexander Potapenko wrote: > How about removing NDEBUG and just printing this at another log level? Ah, we discussed it before. Please read http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... . http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:139: "%10s %10s\n", "virtual", "committed"); On 2012/03/16 11:11:11, Alexander Potapenko wrote: > Suggestion #1: make "virtual" and "committed" constant strings. > Suggestion #2: use tabs if this is just about pretty printing: > "\tvirtual\tcommitted\n" Made them constant strings. Thanks. Since they'll be in log files, and tabs are too flexible, I'd like to keep using spaces. I'd like to keep the column aligned with numbers followed. For example, numbers 0 (1-digit) and 10000000 (8-digit) might come after this line. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:164: used_in_buffer += printed; On 2012/03/16 11:11:11, Alexander Potapenko wrote: > This pattern is repeated for 5 times. Please put it into a private method. Since it contains return, putting them into a method doesn't improve. Instead, I put condition expressions into a method. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:179: #ifndef NDEBUG On 2012/03/16 11:11:11, Alexander Potapenko wrote: > I suggest you do not use NDEBUG at all. The code in these sections should not > take much time, so it should be ok to turn it on by default. If you want to > avoid too much logging, you can increase the log level in RAW_LOG. The same with above. http://codereview.chromium.org/8632007/diff/54001/third_party/tcmalloc/chromi... http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:223: RAW_DCHECK(offset == index, ""); On 2012/03/16 11:11:11, Alexander Potapenko wrote: > What shall be printed if this DCHECK fails? Added a message: "Failed in seeking." http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:305: char buffer[]) { On 2012/03/16 11:11:11, Alexander Potapenko wrote: > Let the order of (buffer, buffer_size) be consistent in all the functions. Basically I chose (buffer_size, buffer) in this file as jar@'s suggestion, except for FillOrderedProfile for consistency with the existing HeapProfileTable. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:330: int64 inode; On 2012/03/16 11:11:11, Alexander Potapenko wrote: > Please call it "unused_inode" to denote it is unused (can't remember whether > iterator.Next accepts NULLs) Done. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... File third_party/tcmalloc/chromium/src/deep-heap-profile.h (right): http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.h:186: int buffer_size, On 2012/03/16 11:11:11, Alexander Potapenko wrote: > I think the buffer size usually goes after the buffer pointer in the parameter > list. Leaving this up to you. jar@ suggested buffer[] is the last... I'm a little confused, but I choose to keep the current style. http://codereview.chromium.org/8632007/diff/105001/third_party/tcmalloc/chrom... third_party/tcmalloc/chromium/src/deep-heap-profile.h:231: int pagemap_fd_; // File descriptor of /proc/self/pagemap. On 2012/03/16 11:11:11, Alexander Potapenko wrote: > Consider using RawFD Kept int since we use open() for pagemap. tcmalloc has only RawOpenForWriting as an opening function for RawFD, but we don't write for pagemap.
From my observation, I didn't find any abnormal values in cpu and vm on Win and Linux perf bots.
lgtm
On 2012/03/27 21:26:39, jar wrote: > lgtm Thanks, Jim! I'll commit it some days later... (waiting for stabilization)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmikurube@chromium.org/8632007/106008
Checked the "Commit" box.
Change committed as 130092 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
