Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12388070:
Count m(un)map for each stacktrace in MemoryRegionMap instead of HeapProfileTable. (Closed)
|
Created:
7 years, 9 months ago by Dai Mikurube (NOT FULLTIME) Modified:
7 years, 9 months ago CC:
chromium-reviews, dmikurube+memory_chromium.org, jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCount m(un)map for each stacktrace in MemoryRegionMap instead of HeapProfileTable.
This patch fixes a bug that gperftools(TCMalloc)'s mmap profiler
(HEAP_PROFILE_MMAP) doesn't hook some memory pages used by the
profiler itself.
This problem has been lived in gperftools for a long time.
It is discussed in gperftools' issue 502.
https://code.google.com/p/gperftools/issues/detail?id=502
Some bugs in the mmap profiler were fixed by
https://code.google.com/p/gperftools/issues/detail?id=383,
but the patch in the issue 383 didn't fix the bug mentioned in
the issue 502.
This change reverts the previous patch and http://crrev.com/132771
at first. Then, it modifies MemoryRegionMap to count m(un)map
calls for each stacktrace in itself instead of merging the counts
for each stacktrace in HeapProfileTable.
This change also cleans up heap-profiler, heap-profile-table and
deep-heap-profile.
This change is to be upstreamed to the original gperftools against
the issue 502.
BUG=181517
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188176
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : ready #
Total comments: 2
Patch Set 8 : rebased #Patch Set 9 : nit fix #Patch Set 10 : nit fix #
Total comments: 44
Patch Set 11 : addressed the comments #
Total comments: 6
Patch Set 12 : addressed the comments #8 #Patch Set 13 : added comments for functions #
Total comments: 44
Patch Set 14 : addressed jar's comments #
Total comments: 20
Patch Set 15 : addressed willchan's comments #Messages
Total messages: 21 (0 generated)
Hi Alexander, Could you take a look at this patch? It fixes a problem described above. I'll ask jar@ to review it when you're happy with it.
gently ping..., or tell me if you're busy. :)
Hi Dai, sorry, I haven't finished the review yesterday and forgot to send the comments. I'll send them right away. On Thu, Mar 7, 2013 at 10:05 AM, <dmikurube@chromium.org> wrote: > gently ping..., or tell me if you're busy. :) > > https://codereview.chromium.org/12388070/ -- Alexander Potapenko Software Engineer Google Moscow
https://codereview.chromium.org/12388070/diff/11001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): https://codereview.chromium.org/12388070/diff/11001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:115: // TODO(dmikurube): Consider using mincore(2). Just wondering - how can mincore help you? https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:149: // free allocation map Uppercase for the first letter, period at the end of a comment. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:601: //I'd like to do but...: recursive_insert = true; I didn't get this. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:672: if (bucket_table_ != NULL) { Can this be put into a function? https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.h (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:376: for (Bucket* x = bucket_table_[b]; x != 0; x = x->next) { x != NULL this is.
https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:445: UnparseBucket(*bucket, args->buf, args->buflen, args->bufsize, "mmap", NULL); This line seems to be longer than 80 chars. Is it? https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:246: for (int i = 0; i < kHashTableSize; i++) { How about using IterateBuckets here?
Some style nits. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:680: NULL, Please provide an inline comment describing what this NULL stands for. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:740: if (deep_bucket != NULL) Please put curly braces around the if and else branches. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-stats.h (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:8: class HeapProfileStats { A class with no members and no methods, huh? Maybe use a namespace instead? https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:9: public: 1-space indentation. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:11: static const int kMaxStackDepth = 32; Is that enough? Webkit stacks may have up to 250 frames. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:19: // semantic equality Please fix the comment. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu... https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:21: return allocs - frees == x.allocs - x.frees && Please remove the extra space before &&. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:136: // Make the table Please mind the periods at the end of one-line comments. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:142: new(alloc_(sizeof(AllocationMap))) AllocationMap(alloc_, dealloc_); 4-space indentation here. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:155: for (Bucket* x = bucket_table_[b]; x != 0; /**/) { Why not put x = x->next into the loop statement? You'll save one line :) https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:331: reinterpret_cast<Bucket**>(alloc_(sizeof(Bucket) * num_buckets_)); 4-space indentation. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-table.h (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.h:284: char* buf; Please put the methods (including ctors) before members in class declarations. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.h:290: }; Please add DISALLOW_COPY_AND_ASSIGN here and to other structs/classes you aren't going to copy or assign. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C... https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profiler.cc:208: (void)stats; // avoid an unused-variable warning in non-debug mode. I think this pattern is common enough to omit the comment, but YMMV https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:504: (found->depth == b.depth) && I think it's fine to fit this condition on the previous line. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:515: continue; Should be fine to put continue on the previous line.
Thanks, Alexander! Updated the patch. I forgot to tell that some code (most of style and comment nits) is from old TCMalloc's code. I thought to keep them as old to keep a kind of consistency (since I plan to upstream them), but finally I fixed them and I'll upstream the fixed code. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:680: NULL, On 2013/03/07 06:48:38, Alexander Potapenko wrote: > Please provide an inline comment describing what this NULL stands for. Done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:740: if (deep_bucket != NULL) On 2013/03/07 06:48:38, Alexander Potapenko wrote: > Please put curly braces around the if and else branches. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit... Done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-stats.h (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:8: class HeapProfileStats { On 2013/03/07 06:48:38, Alexander Potapenko wrote: > A class with no members and no methods, huh? Maybe use a namespace instead? Hmm, agreed. I had other methods in my local changes, but I removed them. Finally, I made them HeapProfileStats and HeapProfileBucket like other classes such as HeapProfileTable in heap-profile-table.h. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:9: public: On 2013/03/07 06:48:38, Alexander Potapenko wrote: > 1-space indentation. It's unnecessary now because of the change above. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:11: static const int kMaxStackDepth = 32; On 2013/03/07 06:48:38, Alexander Potapenko wrote: > Is that enough? Webkit stacks may have up to 250 frames. It's the default of TCMalloc's original value. We may want more if required, but it's enough for now. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:19: // semantic equality On 2013/03/07 06:48:38, Alexander Potapenko wrote: > Please fix the comment. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu... Done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:21: return allocs - frees == x.allocs - x.frees && On 2013/03/07 06:48:38, Alexander Potapenko wrote: > Please remove the extra space before &&. Done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-table.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:136: // Make the table On 2013/03/07 06:48:38, Alexander Potapenko wrote: > Please mind the periods at the end of one-line comments. These comments are actually from the original TCMalloc. They got back since I "reverted" some changes in the original https://code.google.com/p/gperftools/issues/detail?id=383. I thought to keep the original, but finally I fix them and upstream the fixed version. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:142: new(alloc_(sizeof(AllocationMap))) AllocationMap(alloc_, dealloc_); On 2013/03/07 06:48:38, Alexander Potapenko wrote: > 4-space indentation here. Ditto, but done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:149: // free allocation map On 2013/03/07 06:08:25, Alexander Potapenko wrote: > Uppercase for the first letter, period at the end of a comment. Ditto, but done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:155: for (Bucket* x = bucket_table_[b]; x != 0; /**/) { On 2013/03/07 06:48:38, Alexander Potapenko wrote: > Why not put x = x->next into the loop statement? You'll save one line :) It's also from the original TCMalloc. Simply moving x = x->next doesn't work. x has been already deallocated when x = x->next runs since delloc_(b) actually deallocates x. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:331: reinterpret_cast<Bucket**>(alloc_(sizeof(Bucket) * num_buckets_)); On 2013/03/07 06:48:38, Alexander Potapenko wrote: > 4-space indentation. It's also from the original TCMalloc, but done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.cc:445: UnparseBucket(*bucket, args->buf, args->buflen, args->bufsize, "mmap", NULL); On 2013/03/07 06:15:13, Alexander Potapenko wrote: > This line seems to be longer than 80 chars. Is it? Done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-table.h (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.h:284: char* buf; On 2013/03/07 06:48:38, Alexander Potapenko wrote: > Please put the methods (including ctors) before members in class declarations. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order Done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.h:290: }; On 2013/03/07 06:48:38, Alexander Potapenko wrote: > Please add DISALLOW_COPY_AND_ASSIGN here and to other structs/classes you aren't > going to copy or assign. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Copy_C... Done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profiler.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profiler.cc:208: (void)stats; // avoid an unused-variable warning in non-debug mode. On 2013/03/07 06:48:38, Alexander Potapenko wrote: > I think this pattern is common enough to omit the comment, but YMMV It's also from the original TCMalloc. I keep it. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.cc (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:246: for (int i = 0; i < kHashTableSize; i++) { On 2013/03/07 06:15:13, Alexander Potapenko wrote: > How about using IterateBuckets here? It doesn't work because of the same reason as https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro.... https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:504: (found->depth == b.depth) && On 2013/03/07 06:48:38, Alexander Potapenko wrote: > I think it's fine to fit this condition on the previous line. Done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:515: continue; On 2013/03/07 06:48:38, Alexander Potapenko wrote: > Should be fine to put continue on the previous line. Done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:601: //I'd like to do but...: recursive_insert = true; On 2013/03/07 06:08:25, Alexander Potapenko wrote: > I didn't get this. Ah, I had forgotten to work on it. Fixed it. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:672: if (bucket_table_ != NULL) { On 2013/03/07 06:08:25, Alexander Potapenko wrote: > Can this be put into a function? Done. https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.h (right): https://codereview.chromium.org/12388070/diff/25001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:376: for (Bucket* x = bucket_table_[b]; x != 0; x = x->next) { On 2013/03/07 06:08:25, Alexander Potapenko wrote: > x != NULL this is. Done.
Looks mostly good, I think it's ok to add Jim. https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:681: Nit: spare newline https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-stats.h (right): https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:13: int32 allocs; // Number of allocation calls. Again, methods before members and DISALLOW_COPY_AND_ASSIGN (if needed), please. https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.cc (right): https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:730: I suggest to remove this newline since the function is small enough to be understandable without it. Leaving this for you to decide.
Thank you, Alexander. Updated the patch, and answered to your first question at https://codereview.chromium.org/12388070/diff/11001/third_party/tcmalloc/chro.... (Sorry, I overlooked the question.) https://codereview.chromium.org/12388070/diff/11001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): https://codereview.chromium.org/12388070/diff/11001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:115: // TODO(dmikurube): Consider using mincore(2). On 2013/03/07 06:08:25, Alexander Potapenko wrote: > Just wondering - how can mincore help you? This function is reading /proc/$PID/pagemap, which has similar information with mincore(2). I'm wondering that mincore(2) could be a little faster than reading via procfs. https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:681: On 2013/03/07 14:50:52, Alexander Potapenko wrote: > Nit: spare newline Done. https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-stats.h (right): https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:13: int32 allocs; // Number of allocation calls. On 2013/03/07 14:50:52, Alexander Potapenko wrote: > Again, methods before members and DISALLOW_COPY_AND_ASSIGN (if needed), please. Reordered. And, I don't add DISALLOW_COPY_AND_ASSIGN for this since this struct comes from the original TCMalloc, and it doesn't have. https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.cc (right): https://codereview.chromium.org/12388070/diff/36001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:730: On 2013/03/07 14:50:52, Alexander Potapenko wrote: > I suggest to remove this newline since the function is small enough to be > understandable without it. Leaving this for you to decide. Done.
Hi Jim, Could you take a look at this change? It changes some non-"profile" files, but they're also a part of profiler. memory_region_map is for "mmap()" profiling. heap-checker is not a profiler, but a kind of memory validity checker. They don't affect usual runs without specific envs. Also, I plan to upstream the change (except for deep-heap-profile) to the original gperftools. The patch may have some style issues as Chromium, but they're basically to follow the existing convention of gperftools. (I however changed the style for a part of them which I thought it's reasonable based on the discussion with Alexander.) Please take a look at the discussion with Alexander. Thanks.
Unless there is a compelling reason (enhancing readability by being consistent with local file customs), try to comply with google3 and Chromium style guides. I didn't go through all the code, but the use of single letter variable names is the most common violation. At best, that can be justified if the entire file uses a special variable such as |p| for specific pointer. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:623: MemoryRegionMap::Lock(); The pattern of manually locking, and unlocking, is problematic. Much better is to use a helper object, allocated on the stack as an automatic variable, that locks on construction, and unlocks when destructed (at function exit in this case). https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-stats.h (right): https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:14: bool Equivalent(const HeapProfileStats& x) const { nit: avoid use of one letter variables. Suggest "other" rather than "x". https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:16: alloc_size - free_size == x.alloc_size - x.free_size; nit: indent 4 https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:20: int32 frees; // Number of free calls. Why aren't these unsigned? What is the meaning after overflows? https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:28: static const int kMaxStackDepth = 32; Is the intiialization of a static inside a class legal on all our compilers? https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-table.h (right): https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.h:284: BufferArgs(char* a, int b, int c) nit: avoid using one character variable names. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.h:285: : buf(a), buflen(b), bufsize(c) { } nit: indent https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.h (right): https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:76: static const int kHashTableSize = 179999; nit: please justify this magic number in a comment. Assuming the template does not need to be in this file, this constant shouldn't be here either. I don't believe static member initialization in a class definition is (was?) legal (and accepted by all our compilers). Do all our current compilers accept this? https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:93: // It counts mmap and munmap sizes per stack trace if "use_buckets". net: Contrary to the comment, I doubt that "counting" is done during this call, but rather we possibly enable the counting to be done. In addition, it doesn't discuss the multiple call case, which was described (for the other arguments) just above this new comment. A plausible relpacement comment would be: When "use_buckets" is true, then counts of mmap and munmap sizes will be recorded with each stack trace. If Init() is called more than once, then counting will be effective after any call contained "use_buckets" of true. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:106: // Return true if MemoryRegionMap is initialized and working. I understand what initialized means, but I'm unclear on "working." Is there a way this could "break" and stop working? ...or is there a way it can be disabled? https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:130: typedef HeapProfileBucket Bucket; What is the advantage of defining smaller typedef names? I expect a type like "Stats" to only be confusing. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:234: // creating the bucket if needed. I couldn't understand what this comment meant. I have no clue about |depth|, but might guess it is stack depth, but not sure. I have no idea what |key| is. I have no idea why we pass an array of const pointers to const void values. I don't know how to tell how large the array is. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:319: static Bucket** bucket_table_; Can you clarify in the comment what this data structure is? The comment says "hash table," but it appears to be a rather free-form data structure (pointer to pointer to buckets) with no explanation of how the table is used, indexed, traversed, etc. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:322: // Number of unprocessed bucket inserts. It is not clear what "unprocessed buckets" are, presumably as opposed to "processed." https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:329: static Bucket saved_buckets_[20]; Please justify the magic number. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:345: void (*insert_func)(const Region& region)); nit: indent should 4 characters. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:348: inline static void HandleSavedBucketsLocked(); The word "Handle" is a poor choice for a verb. Better might be: ProcessSavedBacketsLocked(). Unlike the call on line 344, where the "handling" is defined to be "call the function," you need to indicate what the processing is. Can you justify why you're using the inline directive? (other than copying questionable patterns on the previous lines?) https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:361: // Record deletion of a memory region in a bucket. You need to explain what the arguments mean. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:385: void MemoryRegionMap::IterateBuckets( nit: this is a line continuation, and should be indented 4, or probably more reasonably, put on the tail of the previous line. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:386: void (*callback)(const Bucket*, Type), Type arg) { nit: provide meaningful variable names for all arguments, including the prototypes, such in the argument list for this callback. |arg| is not a good variable name. In this case, perhaps |instance| would be better. ...also... are you copying this pattern from somewhere else? Although something closs to this pattern is perhaps more common in C (without a template), in C++ it is much more common to see member functions called, and |this| passed implicitly (I guess here you're passing the full blown instance, rather than a pointer or a ref though :-/ ). Is there a reason for this pattern (using a callback function)? ... passing an instance of a type? https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:387: for (int b = 0; b < kHashTableSize; b++) { nit: Why do you want the source code in the header? https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:388: for (Bucket* x = bucket_table_[b]; x != NULL; x = x->next) { nit: avoid use of single letter varible names such as |x| an |b|.
Switched the reviewer to willchan@ with an agreement. Thanks for taking it, willchan! This change has been already said "Looks mostly good" by glider@ in https://codereview.chromium.org/12388070/#msg8. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/deep-heap-profile.cc (right): https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/deep-heap-profile.cc:623: MemoryRegionMap::Lock(); On 2013/03/08 04:43:31, jar wrote: > The pattern of manually locking, and unlocking, is problematic. Much better is > to use a helper object, allocated on the stack as an automatic variable, that > locks on construction, and unlocks when destructed (at function exit in this > case). I found MemoryRegionMap::LockHolder. I chose it. Thanks for the advice. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-stats.h (right): https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:14: bool Equivalent(const HeapProfileStats& x) const { On 2013/03/08 04:43:31, jar wrote: > nit: avoid use of one letter variables. Suggest "other" rather than "x". Ok, done. It was just a move from HeapProfileTable::Stats. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:16: alloc_size - free_size == x.alloc_size - x.free_size; On 2013/03/08 04:43:31, jar wrote: > nit: indent 4 Done. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:20: int32 frees; // Number of free calls. On 2013/03/08 04:43:31, jar wrote: > Why aren't these unsigned? What is the meaning after overflows? As I wrote above, it's just a move from the existing HeapProfileTable::Stats. I'm not sure of the perfect historical reason, but I think we should keep the types. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-stats.h:28: static const int kMaxStackDepth = 32; On 2013/03/08 04:43:31, jar wrote: > Is the intiialization of a static inside a class legal on all our compilers? It works for static and const variables. We have such initialization for a very long time. I heard that it actually fails in a very old compiler "Visual C++ 6.0", but we should blame the compiler in this case. It's allowed in the C++03. http://stackoverflow.com/questions/2605520/c-where-to-initialize-static-const http://www.devx.com/tips/Tip/5602 http://chanduthedev.blogspot.jp/2012/02/how-to-initialize-const-variable-in-c... I'm interested in a document that says they're wrong. Also, these are examples used in the Chromium tree. (There are more in V8.) http://src.chromium.org/viewvc/chrome/trunk/src/base/lazy_instance.h?view=markup http://src.chromium.org/viewvc/chrome/trunk/src/base/time.h?view=markup http://src.chromium.org/viewvc/chrome/trunk/src/base/metrics/stats_table.h?vi... http://src.chromium.org/viewvc/chrome/trunk/src/courgette/ensemble.h?view=markup http://src.chromium.org/viewvc/chrome/trunk/src/net/quic/quic_http_stream_tes... https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/heap-profile-table.h (right): https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.h:284: BufferArgs(char* a, int b, int c) On 2013/03/08 04:43:31, jar wrote: > nit: avoid using one character variable names. Ok, done. Also, I fixed (refactored) other existing structs (e.g. DumpArgs) by jochen@ which you actually l-g-t-m'ed at https://chromiumcodereview.appspot.com/10541026. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/heap-profile-table.h:285: : buf(a), buflen(b), bufsize(c) { } On 2013/03/08 04:43:31, jar wrote: > nit: indent Done, and same for others. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.h (right): https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:76: static const int kHashTableSize = 179999; On 2013/03/08 04:43:31, jar wrote: > nit: please justify this magic number in a comment. > > Assuming the template does not need to be in this file, this constant shouldn't > be here either. > > I don't believe static member initialization in a class definition is (was?) > legal (and accepted by all our compilers). > > Do all our current compilers accept this? Added a comment on this constant, and a detailed description in heap-profile-stats.h. The template should be in this header file like addressmap-inl.h does. If it's defined in memory_region_map.cc, a linker error happens since the template is not instantiated. For static const member initialization, see my reply at https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro.... https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:93: // It counts mmap and munmap sizes per stack trace if "use_buckets". On 2013/03/08 04:43:31, jar wrote: > net: Contrary to the comment, I doubt that "counting" is done during this call, > but rather we possibly enable the counting to be done. In addition, it doesn't > discuss the multiple call case, which was described (for the other arguments) > just above this new comment. A plausible relpacement comment would be: > > > When "use_buckets" is true, then counts of mmap and munmap sizes will be > recorded with each stack trace. If Init() is called more than once, then > counting will be effective after any call contained "use_buckets" of true. > > > Done. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:106: // Return true if MemoryRegionMap is initialized and working. On 2013/03/08 04:43:31, jar wrote: > I understand what initialized means, but I'm unclear on "working." Is there a > way this could "break" and stop working? ...or is there a way it can be > disabled? Changed the comment to: Return true if MemoryRegionMap is initialized and recording, i.e. when then number of Init() calls are more than the number of Shutdown() calls. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:130: typedef HeapProfileBucket Bucket; On 2013/03/08 04:43:31, jar wrote: > What is the advantage of defining smaller typedef names? I expect a type like > "Stats" to only be confusing. Removed them. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:234: // creating the bucket if needed. On 2013/03/08 04:43:31, jar wrote: > I couldn't understand what this comment meant. > I have no clue about |depth|, but might guess it is stack depth, but not sure. > I have no idea what |key| is. > I have no idea why we pass an array of const pointers to const void values. > I don't know how to tell how large the array is. Changed the comment. The details of the bucket table is described in heap-profile-stats.h. btw, the bucket table is the "core" data structure of the heap profiler in tcmalloc (gperftools). We can't understand the heap profiler without understanding the bucket table. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:319: static Bucket** bucket_table_; On 2013/03/08 04:43:31, jar wrote: > Can you clarify in the comment what this data structure is? The comment says > "hash table," but it appears to be a rather free-form data structure (pointer to > pointer to buckets) with no explanation of how the table is used, indexed, > traversed, etc. Done. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:322: // Number of unprocessed bucket inserts. On 2013/03/08 04:43:31, jar wrote: > It is not clear what "unprocessed buckets" are, presumably as opposed to > "processed." Done. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:329: static Bucket saved_buckets_[20]; On 2013/03/08 04:43:31, jar wrote: > Please justify the magic number. Done. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:345: void (*insert_func)(const Region& region)); On 2013/03/08 04:43:31, jar wrote: > nit: indent should 4 characters. We should not change the existing code that makes unnecessary difference from the original. Such difference makes us unhappy to roll this library. We should follow the Chromium Coding Style guide which says "Try to follow any per-file conventions you notice when modifying existing code." http://dev.chromium.org/developers/coding-style Reasonable rationale (our public guideline, not personal liking) is required to break the existing conventions. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:348: inline static void HandleSavedBucketsLocked(); On 2013/03/08 04:43:31, jar wrote: > The word "Handle" is a poor choice for a verb. > Better might be: > ProcessSavedBacketsLocked(). > > Unlike the call on line 344, where the "handling" is defined to be "call the > function," you need to indicate what the processing is. > > Can you justify why you're using the inline directive? (other than copying > questionable patterns on the previous lines?) Sounds reasonable. Finally renamed it to Restore-() and added comments. Removed inline. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:361: // Record deletion of a memory region in a bucket. On 2013/03/08 04:43:31, jar wrote: > You need to explain what the arguments mean. Done. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:385: void MemoryRegionMap::IterateBuckets( On 2013/03/08 04:43:31, jar wrote: > nit: this is a line continuation, and should be indented 4, or probably more > reasonably, put on the tail of the previous line. This newline style (template in a single line, and no indent in the second line) is a very common convention *for templates* in Chromium. Examples or a clear guideline in Chromium are required to change it. I can show many examples of this style like: http://src.chromium.org/viewvc/chrome/trunk/src/base/template_util.h?view=markup http://src.chromium.org/viewvc/chrome/trunk/src/sync/notifier/invalidator_tes... http://src.chromium.org/viewvc/chrome/trunk/src/base/test/sequenced_task_runn... http://src.chromium.org/viewvc/chrome/trunk/src/cc/layer_iterator.h?view=markup http://src.chromium.org/viewvc/chrome/trunk/src/base/bind.h?view=markup https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:386: void (*callback)(const Bucket*, Type), Type arg) { On 2013/03/08 04:43:31, jar wrote: > nit: provide meaningful variable names for all arguments, including the > prototypes, such in the argument list for this callback. |arg| is not a good > variable name. In this case, perhaps |instance| would be better. > This |arg| is just an argument passed to |callback|. It may not be a kind of instances. Renamed it to |callback_arg|. > > ...also... are you copying this pattern from somewhere else? Although something > closs to this pattern is perhaps more common in C (without a template), in C++ > it is much more common to see member functions called, and |this| passed > implicitly (I guess here you're passing the full blown instance, rather than a > pointer or a ref though :-/ ). Is there a reason for this pattern (using a > callback function)? ... passing an instance of a type? Yes, I know it's not so common in C++, and it's the same approach that tcmalloc's addressmap-inl.h does. https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:387: for (int b = 0; b < kHashTableSize; b++) { On 2013/03/08 04:43:31, jar wrote: > nit: Why do you want the source code in the header? Written at: https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... https://codereview.chromium.org/12388070/diff/47002/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:388: for (Bucket* x = bucket_table_[b]; x != NULL; x = x->next) { On 2013/03/08 04:43:31, jar wrote: > nit: avoid use of single letter varible names such as |x| an |b|. Done. fyi in general, http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml has many "for (int i = 0; i < size; i++)".
Hey Dai, I'm looking at it in more detail now and grokking everything. I want to focus on the high level design first. MemoryRegionMap is primarily used for tracking all the memory regions. HeapProfileTable is used for counting heap allocations. Counting m(un)map per each stacktrace sounds more like counting allocations like the HeapProfileTable does. Are you sure this is the right way to do it? Can you explain why? Will the upstream maintainer accept this patch? Have you checked the design with him? If he's OK with it, I'm OK with it. Btw, why is this landing in Chromium first? I'd definitely rubberstamp anything that rolled Chromium to pick up new changes in upstream google-perftools. I'm worried that this design change may cost roundtrips in terms of us moving in a certain direction with the code, and this change needing to be redone to get into the upstream repo. Are we doing this in order to get it into the bots sooner? If so, that's fine. I'm just worried that it will cost you more time personally. But I trust you to prioritize your own time appropriately :) I'm going to keep reading this and sending in more detailed comments. But I wanted to ask that high level question first.
Thank you, Will! On 2013/03/13 15:49:20, willchan wrote: > Hey Dai, I'm looking at it in more detail now and grokking everything. I want to > focus on the high level design first. MemoryRegionMap is primarily used for > tracking all the memory regions. HeapProfileTable is used for counting heap > allocations. Counting m(un)map per each stacktrace sounds more like counting > allocations like the HeapProfileTable does. Are you sure this is the right way > to do it? Can you explain why? Will the upstream maintainer accept this patch? > Have you checked the design with him? If he's OK with it, I'm OK with it. > Yes, that's what I want to do. The original mmap profiler (HEAP_PROFILE_MMAP) actually does similar counting. The difference is that the original accumulates memory regions per stacktrace in HeapProfileTable for every dump request. HeapProfileTable reads mmap's stacktraces from |call_stack| in MemoryRegionMap::Region, adds them per stack trace, dumps merged heap profile and finally removes the accumulated mmap profiles. This per-stacktrace accumulation is done in HeapProfileTable::RefreshMMapData in the current version, and was done in AddRemoveMMapDataLocked (heap-profiler.cc) previously. The root problem is repeating addition and removal for every dump request. My change is to stop the repeating and to accumulate once in mmap hook. Regarding to the OSS gperftools (out of google), the owner says "Sounds good." for my proposal. Please take a look at his comment at https://code.google.com/p/gperftools/issues/detail?id=502. > Btw, why is this landing in Chromium first? I'd definitely rubberstamp anything > that rolled Chromium to pick up new changes in upstream google-perftools. I'm > worried that this design change may cost roundtrips in terms of us moving in a > certain direction with the code, and this change needing to be redone to get > into the upstream repo. Are we doing this in order to get it into the bots > sooner? If so, that's fine. I'm just worried that it will cost you more time > personally. But I trust you to prioritize your own time appropriately :) > Yes, the main reason is to fix the Chrome Endure graphs sooner. The "unhooked-anonymous" problem of mysterious memory usage was caused by this issue (yes it was not a problem of Chromium/WebKit ;)), so I'd like to fix it quickly. Upstreaming may take a little more time, but there's no hurry to upstream. I think I can do it on the side. Another smaller reason is to fit deep-heap-profile (DMP) with MemoryRegionMap easily. DMP (only in Chromium) reads data from MemoryRegionMap. Changing both at the same time makes me happy. :) > I'm going to keep reading this and sending in more detailed comments. But I > wanted to ask that high level question first.
The only major comment I had was about the potential race. Everything else is just style nits that I hope we can resolve within a roundtrip. I am not the most familiar about static_cast<> vs reinterpret_cast<>, but I based my comments on http://stackoverflow.com/questions/573294/when-to-use-reinterpret-cast. I also explained the style guide section on single letter variable names. Other than commonly accepted conventions like counters/iterators/generic math, I'd like to discourage their use. As far as the style in google-perftools, they adhere to the Google style guide, and any parts that do not comply with the style guide are mistakes and theoretically should be fixed (but I don't think you should waste your time fixing them unless you feel like it). Other than these comments, I think this change is good to go! https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.cc (right): https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:153: int MemoryRegionMap::saved_buckets_count_ = 0; Are these variables guarded by any locks? Should we be using the GUARDED_BY annotation? I think that's common in google3. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:226: bucket_table_ = reinterpret_cast<HeapProfileBucket**>( static_cast<>? https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:249: HeapProfileBucket* b = x; Commenting generally about this since you brought it up offline earlier. |i| is commonly used for counters. |x| is also somewhat commonly used for counters, but this isn't a counter. |b| is not commonly used. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... says: """ Poorly-chosen names use ambiguous abbreviations or arbitrary characters that do not convey meaning: int n; // Bad - meaningless. """ I think |i| as a counter is a fine exception to this rule. |x| also for counters, or arbitrary math. |b| is not commonly used except for arbitrary math. Here I'd like to see |b| renamed. |x| is more debatable. I suggest x=>curr (curr is a common abbreviation for linked list iteration) and b=>bucket. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:282: return is_working; Just doublechecking...this looks like it could be racy. Does it matter? Obviously after Unlock() is called, |client_count_| can change. Does that matter? It looks like Shutdown() deletes the buckets, but after the IsRecording() check, deep-heap-profile.cc may try to access buckets directly. Can we get use after frees in races? Does this matter? https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:389: unsigned int buck = ((unsigned int) h) % kHashTableSize; Is |buck| short for |bucket|? As the style guide indicated earlier, don't abbreviate it like that. I know it's debatable, but arguably |buck| is not common enough for people to understand. Let's call it |index|. Then we can stop using |b| for HeapProfileBucket* and call it |bucket| instead. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:411: const void** kcopy = reinterpret_cast<const void**>( Don't we want static_cast<> here? What does kcopy mean? Does it mean key copy? Should we name it |key_copy|? https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:501: unsigned int buck = ((unsigned int) b.hash) % kHashTableSize; static_cast<>. we discourage c-style casts in the style guide. i know it's done in some places in google-perftools, but that's primarily because it's very old code before google got strict about the style guide. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:519: const void** kcopy = reinterpret_cast<const void**>( static_cast https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:522: HeapProfileBucket* new_b = reinterpret_cast<HeapProfileBucket*>( s/new_b/new_bucket/ https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.h (right): https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:322: // Bucket hash table which is described in heap-profile-stats.h. Ugh, this class ordering is wrong. Functions should come first. Silly csilvers/maxim, writing it incorrectly way back when we were looser about the style guide. Not worth fixing though.
Thank you, Will. I renamed variable names in new functions (and functions which got back from the very old version). I also changed some of reinterpret_cast<> to static_cast<>. Some of existing reinterpret_cast<> in heap-profile-table need to be changed, but I think it should be done in upstream if we want. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.cc (right): https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:153: int MemoryRegionMap::saved_buckets_count_ = 0; On 2013/03/13 21:58:48, willchan wrote: > Are these variables guarded by any locks? Should we be using the GUARDED_BY > annotation? I think that's common in google3. Yes, they should be accessed between Lock() and Unlock() (the lock variable is MemoryRegionMap::lock_). I was not sure of usage of GUARDED_BY in the OSS tcmalloc. It's defined in base/thread_annotations.h, but used only in profile-handler.cc. Many other existing variables may want GUARDED_BY, but they don't have. Hmm. I think other existing variables should be cared in upstream on the side. At this time, I added GUARDED_BY(_lock) for new variable declaration in memory_region_map.h and added comments like MemoryRegionMap::recursion_count_ in this .cc. What do you think? https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:226: bucket_table_ = reinterpret_cast<HeapProfileBucket**>( On 2013/03/13 21:58:48, willchan wrote: > static_cast<>? static_cast works here. Replaced. Many of existing reinterpret_cast in heap-profile-table may need replacing, but it should be done on the side if we want. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:249: HeapProfileBucket* b = x; On 2013/03/13 21:58:48, willchan wrote: > Commenting generally about this since you brought it up offline earlier. > > |i| is commonly used for counters. > |x| is also somewhat commonly used for counters, but this isn't a counter. > |b| is not commonly used. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... > says: > """ > Poorly-chosen names use ambiguous abbreviations or arbitrary characters that do > not convey meaning: > > int n; // Bad - meaningless. > """ > > I think |i| as a counter is a fine exception to this rule. |x| also for > counters, or arbitrary math. |b| is not commonly used except for arbitrary math. > > Here I'd like to see |b| renamed. |x| is more debatable. > > I suggest x=>curr (curr is a common abbreviation for linked list iteration) and > b=>bucket. Sounds reasonable renaming. Done it (also in some other new functions). https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:282: return is_working; On 2013/03/13 21:58:48, willchan wrote: > Just doublechecking...this looks like it could be racy. Does it matter? > Obviously after Unlock() is called, |client_count_| can change. Does that > matter? > > It looks like Shutdown() deletes the buckets, but after the IsRecording() check, > deep-heap-profile.cc may try to access buckets directly. Can we get use after > frees in races? Does this matter? Ahh, you're right. It could be a problem. Changed this function to "IsRecordingLocked", and it requires a lock before calling this function. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:389: unsigned int buck = ((unsigned int) h) % kHashTableSize; On 2013/03/13 21:58:48, willchan wrote: > Is |buck| short for |bucket|? As the style guide indicated earlier, don't > abbreviate it like that. I know it's debatable, but arguably |buck| is not > common enough for people to understand. Let's call it |index|. Then we can stop > using |b| for HeapProfileBucket* and call it |bucket| instead. It actually comes from heap-profile-table's GetBucket, and I'm not sure what |buck| means in fact. I think it should not be a "bucket". It's just an index in the hash table. I kept the original variable names at first not to lose the context, but I think h => hash buck => hash_index b => bucket might be reasonable. What do you think? https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:411: const void** kcopy = reinterpret_cast<const void**>( On 2013/03/13 21:58:48, willchan wrote: > Don't we want static_cast<> here? What does kcopy mean? Does it mean key copy? > Should we name it |key_copy|? Replaced it to static_cast and renamed kcopy to key_copy. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:501: unsigned int buck = ((unsigned int) b.hash) % kHashTableSize; On 2013/03/13 21:58:48, willchan wrote: > static_cast<>. we discourage c-style casts in the style guide. i know it's done > in some places in google-perftools, but that's primarily because it's very old > code before google got strict about the style guide. Replaced to static_cast (also in GetBucket). Also renamed many variables in this function. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:519: const void** kcopy = reinterpret_cast<const void**>( On 2013/03/13 21:58:48, willchan wrote: > static_cast Done. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.cc:522: HeapProfileBucket* new_b = reinterpret_cast<HeapProfileBucket*>( On 2013/03/13 21:58:48, willchan wrote: > s/new_b/new_bucket/ Done. https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... File third_party/tcmalloc/chromium/src/memory_region_map.h (right): https://codereview.chromium.org/12388070/diff/53001/third_party/tcmalloc/chro... third_party/tcmalloc/chromium/src/memory_region_map.h:322: // Bucket hash table which is described in heap-profile-stats.h. On 2013/03/13 21:58:48, willchan wrote: > Ugh, this class ordering is wrong. Functions should come first. Silly > csilvers/maxim, writing it incorrectly way back when we were looser about the > style guide. > > Not worth fixing though. Yeah, we usually violate the style guide or we need great refactoring in tcmalloc. ;)
Flying to Orlando in a bit, but hopefully I'll finish this before I have to board. Quick FYI, if I nitpick you on style issues that were pre-existing and you just moved the code, it's perfectly fine to tell me that they were pre-existing. I hate it myself when someone else tells me to fix code that I just moved. It's hard when reviewing code to tell sometimes :) OK, hopefully you'll hear back from me soon, otherwise I'll get to it tonight.
Great, LGTM
Thank you, Will! I'll be committing...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmikurube@chromium.org/12388070/67001
Message was sent while issue was closed.
Change committed as 188176 |
