Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19466005:
Adding extended report to memdump with resident pages bit reporting. (Closed)
|
Created:
7 years, 5 months ago by Primiano Tucci (use gerrit) Modified:
3 years, 11 months ago CC:
chromium-reviews, craigdh+watch_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org, klundberg+watch_chromium.org, frankf+watch_chromium.org, bulach, Torne Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding extended report to memdump with resident pages bit reporting.
This change introduces a new switch (-x) which causes memdump to
add, to each mapping printed out, a bitmap containing resident bit
information for the mapping.
This extended report can be processed by the memsymbols.py script,
part of this CL. The aim of memsymbols.py is to extract the list of
symbols, for a given library mapped in the analyzed process, which was
resident in memory at a given point in time (when the memdump snapshot
was taken).
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213363
Patch Set 1 #
Total comments: 46
Patch Set 2 : Addressing nits. #
Total comments: 12
Patch Set 3 : Addressing bulach@ nits. #
Total comments: 4
Patch Set 4 : Addressing pliard@ nits #Patch Set 5 : Copyright nit #Messages
Total messages: 13 (0 generated)
Thanks Primiano, this is really cool! I look forward to seeing your findings with this new functionality. LGTM modulo some tiny nits (sorry for the nitpicking :)). https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... File tools/android/memdump/memdump.cc (right): https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memdump.cc:35: public: Nit: there should be a leading space before 'public'. https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memdump.cc:36: BitSet() : nbits_(0), data_(0) { } Nit: no space between the braces. https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memdump.cc:36: BitSet() : nbits_(0), data_(0) { } Nit: I believe the |data_| explicit initialization should not be needed. See my comment line 39 but we might be able to completely remove the constructor if you get rid of |nbits|. https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memdump.cc:38: void setSize(int nbits) { Nit: maybe call this 'resize()' and also s/getData/data, s/getLength/size to make this interface closer to std::vector. https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memdump.cc:39: nbits_ = nbits; Nit: do we need to store |nbits|? Can we entirely rely on std::vector? https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memdump.cc:40: data_.resize((nbits_+7)/8); Nit: spaces around binary operators (here and on a few other lines below). https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memdump.cc:41: memset(&data_[0], 0, getLength()); Nit: I believe you can delete this line and use the resize(size_type, value_type) overload of std::vector::resize() unless I'm missing something :) https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memdump.cc:48: const char* getData() const { return &data_[0]; } Nit: data_.data()? https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memdump.cc:85: // committed_pages_bits is a bitmap reflecting the present bit for all the Nit: s/bitmap/bitset maybe. https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memdump.cc:481: BitSet* pages_bits = &it->committed_pages_bits; Nit: 'BitSet* const pages_bits' maybe for consistency with the line above. https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... File tools/android/memdump/memsymbols.py (right): https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memsymbols.py:62: memdump_fh = open(memdump_file, 'r') Nit: will this work with stdin? https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memsymbols.py:81: #if prot == 'r-xp': Nit: should we keep this? :) https://chromiumcodereview.appspot.com/19466005/diff/1/tools/android/memdump/... tools/android/memdump/memsymbols.py:110: resident_pages.add(offset/PAGE_SIZE + i) Nit: spaces around binary operators (also on line 127).
good stuff! some suggestion on top of phillippe's: https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump.cc File tools/android/memdump/memdump.cc (right): https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:34: class BitSet { I suppose the ndk has #include <bitset> ? https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... File tools/android/memdump/memsymbols.py (right): https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:14: """ Extracts the list of resident symbols of a library loaded in a process. nit: remove space before Extracts https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:26: PAGE_SIZE = 4096 nit: _ prefix. also, below should be _TestBit and _HexAddr https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:54: it'd be nicer to split 54-134 into its own function. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:136: main(sys.argv) nit: return main(sys.argv)
https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump.cc File tools/android/memdump/memdump.cc (right): https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:34: class BitSet { On 2013/07/22 12:59:39, bulach wrote: > I suppose the ndk has #include <bitset> ? That's true, but note that std::bitset<> is not a dynamically-sized container, which BitSet here seems to be. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:38: void setSize(int nbits) { Also, you should use the Chromium style guide, which is that method names are CamelCased, except for simple accessors like member() and set_member(). https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:45: data_.at(bit/8) |= (1 << (bit % 8)); nit: since 'bit' is signed, using 'bit & 7' is more efficient than 'bit % 8'. The latter will generate a little more instructions to deal with negative 'bit' values, which isn't needed here. Also, this code is built without exceptions, so it's probably better to use [] and an explicit bound check rather than relying on at() for this. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:50: int getLength() const { return data_.size(); } nit: This is very misleading because this doesn't return the length in bits, but the length of bytes needed for the std::string representation. I'd suggest removing getData() / getLength() completely and instead provide a single function ToString() or AsString() that returns a std::string directly.
Thanks all for the great comments. Apologies for all the nits, I'm still a bit rusty on the code style, I need to get back in the loop :) https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump.cc File tools/android/memdump/memdump.cc (right): https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:34: class BitSet { Yeah, unforunately STL's bitset require the size of the bitmap to be known at compile time (i.e. length it's an argument of the template) https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:35: public: On 2013/07/22 09:20:55, Philippe wrote: > Nit: there should be a leading space before 'public'. Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:36: BitSet() : nbits_(0), data_(0) { } On 2013/07/22 09:20:55, Philippe wrote: > Nit: no space between the braces. Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:36: BitSet() : nbits_(0), data_(0) { } On 2013/07/22 09:20:55, Philippe wrote: > Nit: I believe the |data_| explicit initialization should not be needed. See my > comment line 39 but we might be able to completely remove the constructor if you > get rid of |nbits|. Hmm, agree for nbits (see below), but _data was just for paranoy. I'm not sure that calling the default vector ctor won't pre-reserve an "optimistic" (impl. dependent) chunk of memory. Most of the mappings have very few pages. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:38: void setSize(int nbits) { On 2013/07/22 09:20:55, Philippe wrote: > Nit: maybe call this 'resize()' and also s/getData/data, s/getLength/size to > make this interface closer to std::vector. Right, makes sense! https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:39: nbits_ = nbits; On 2013/07/22 09:20:55, Philippe wrote: > Nit: do we need to store |nbits|? Can we entirely rely on std::vector? Yeah, that was just me being uber-paranoid. The at() accessor should be enough. Ah, I just realized that the entire sense of nbits_ was a CHECK on set(..), which I later removed. So that does not make any sense at all! https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:40: data_.resize((nbits_+7)/8); On 2013/07/22 09:20:55, Philippe wrote: > Nit: spaces around binary operators (here and on a few other lines below). Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:41: memset(&data_[0], 0, getLength()); On 2013/07/22 09:20:55, Philippe wrote: > Nit: I believe you can delete this line and use the resize(size_type, > value_type) overload of std::vector::resize() unless I'm missing something :) Oh, apparently I am the one missing the autofill on resize() :) https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:45: data_.at(bit/8) |= (1 << (bit % 8)); On 2013/07/22 21:03:14, digit1 wrote: > nit: since 'bit' is signed, using 'bit & 7' is more efficient than 'bit % 8'. > The latter will generate a little more instructions to deal with negative 'bit' > values, which isn't needed here. Great catch! Right. For the records: return argc % 8; 0x00008428 <+0>: ldr r3, [pc, #16] ; (0x843c <main(int, char**)+20>) 0x0000842a <+2>: ands r3, r0 0x0000842c <+4>: cmp r3, #0 0x0000842e <+6>: bge.n 0x8438 <main(int, char**)+16> 0x00008430 <+8>: subs r3, #1 0x00008432 <+10>: orn r3, r3, #7 0x00008436 <+14>: adds r3, #1 0x00008438 <+16>: mov r0, r3 0x0000843a <+18>: bx lr 0x0000843c <+20>: andhi r0, r0, r7 return argc & 7; 0x00008428 <+0>: and.w r0, r0, #7 0x0000842c <+4>: bx lr > Also, this code is built without exceptions, so it's probably better to use [] > and an explicit bound check rather than relying on at() for this. Ah right I forgot that exception are not enabled! Good point, I'll revert to the original CHECK. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:48: const char* getData() const { return &data_[0]; } On 2013/07/22 09:20:55, Philippe wrote: > Nit: data_.data()? I'm afraid that is only possible in C++11. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:50: int getLength() const { return data_.size(); } On 2013/07/22 21:03:14, digit1 wrote: > nit: This is very misleading because this doesn't return the length in bits, but > the length of bytes needed for the std::string representation. I'd suggest > removing getData() / getLength() completely and instead provide a single > function ToString() or AsString() that returns a std::string directly. > At this point, let's do directly AsB64String(). https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:85: // committed_pages_bits is a bitmap reflecting the present bit for all the On 2013/07/22 09:20:55, Philippe wrote: > Nit: s/bitmap/bitset maybe. Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:481: BitSet* pages_bits = &it->committed_pages_bits; On 2013/07/22 09:20:55, Philippe wrote: > Nit: 'BitSet* const pages_bits' maybe for consistency with the line above. Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... File tools/android/memdump/memsymbols.py (right): https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:14: """ Extracts the list of resident symbols of a library loaded in a process. On 2013/07/22 12:59:39, bulach wrote: > nit: remove space before Extracts Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:26: PAGE_SIZE = 4096 On 2013/07/22 12:59:39, bulach wrote: > nit: _ prefix. > also, below should be _TestBit and _HexAddr Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:26: PAGE_SIZE = 4096 On 2013/07/22 12:59:39, bulach wrote: > nit: _ prefix. > also, below should be _TestBit and _HexAddr Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:54: On 2013/07/22 12:59:39, bulach wrote: > it'd be nicer to split 54-134 into its own function. Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:62: memdump_fh = open(memdump_file, 'r') On 2013/07/22 09:20:55, Philippe wrote: > Nit: will this work with stdin? Added special '-' handling to read memdump from stdin. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:81: #if prot == 'r-xp': On 2013/07/22 09:20:55, Philippe wrote: > Nit: should we keep this? :) Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:110: resident_pages.add(offset/PAGE_SIZE + i) On 2013/07/22 09:20:55, Philippe wrote: > Nit: spaces around binary operators (also on line 127). Done. https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:136: main(sys.argv) On 2013/07/22 12:59:39, bulach wrote: > nit: return main(sys.argv) You mean sys.exit, right?
lgtm, good stuff! looking forward to see the results! just some nits and suggestions, feel free to go ahead once other are happy https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump.cc File tools/android/memdump/memdump.cc (right): https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memdump... tools/android/memdump/memdump.cc:34: class BitSet { On 2013/07/22 23:22:35, Primiano Tucci wrote: > Yeah, unforunately STL's bitset require the size of the bitmap to be known at > compile time (i.e. length it's an argument of the template) ops, forgot that tiny detail :) thanks for the refresher! https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... File tools/android/memdump/memsymbols.py (right): https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:136: main(sys.argv) On 2013/07/22 23:22:35, Primiano Tucci wrote: > On 2013/07/22 12:59:39, bulach wrote: > > nit: return main(sys.argv) > > You mean sys.exit, right? yep, thanks! https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/memd... File tools/android/memdump/memdump.cc (right): https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/memd... tools/android/memdump/memdump.cc:38: void resize(int nbits) { nit: size_t https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/memd... tools/android/memdump/memdump.cc:42: void set(int bit) { nit: uint32 https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/memd... tools/android/memdump/memdump.cc:56: std::vector<char> data_; nit: uint8 https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/mems... File tools/android/memdump/memsymbols.py (right): https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/mems... tools/android/memdump/memsymbols.py:27: nit: two \n between top levels, i.e., add another one here, 31 and 34 https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/mems... tools/android/memdump/memsymbols.py:123: memdump_fh = open(memdump_file, 'r') nit: could do .readlines() here too so that the file could potentially be closed.. and if you go that way, rather than "fh", perhaps "memdump_contents"? https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/mems... tools/android/memdump/memsymbols.py:146: if __name__=='__main__': nit: space around ==
https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/memd... File tools/android/memdump/memdump.cc (right): https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/memd... tools/android/memdump/memdump.cc:38: void resize(int nbits) { On 2013/07/23 08:10:58, bulach wrote: > nit: size_t Done. https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/memd... tools/android/memdump/memdump.cc:42: void set(int bit) { On 2013/07/23 08:10:58, bulach wrote: > nit: uint32 Done. https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/memd... tools/android/memdump/memdump.cc:56: std::vector<char> data_; On 2013/07/23 08:10:58, bulach wrote: > nit: uint8 Hmm if I do that, than I have to add more boilerplate to line 49 (string ctor). https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/mems... File tools/android/memdump/memsymbols.py (right): https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/mems... tools/android/memdump/memsymbols.py:27: On 2013/07/23 08:10:58, bulach wrote: > nit: two \n between top levels, i.e., add another one here, 31 and 34 Done. https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/mems... tools/android/memdump/memsymbols.py:123: memdump_fh = open(memdump_file, 'r') On 2013/07/23 08:10:58, bulach wrote: > nit: could do .readlines() here too so that the file could potentially be > closed.. and if you go that way, rather than "fh", perhaps "memdump_contents"? Right! Done. https://codereview.chromium.org/19466005/diff/7001/tools/android/memdump/mems... tools/android/memdump/memsymbols.py:146: if __name__=='__main__': On 2013/07/23 08:10:58, bulach wrote: > nit: space around == Done.
Last nits on my side :) https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... File tools/android/memdump/memsymbols.py (right): https://codereview.chromium.org/19466005/diff/1/tools/android/memdump/memsymb... tools/android/memdump/memsymbols.py:62: memdump_fh = open(memdump_file, 'r') On 2013/07/22 23:22:35, Primiano Tucci wrote: > On 2013/07/22 09:20:55, Philippe wrote: > > Nit: will this work with stdin? > > Added special '-' handling to read memdump from stdin. Nice, thanks. https://codereview.chromium.org/19466005/diff/13001/tools/android/memdump/mem... File tools/android/memdump/memdump.cc (right): https://codereview.chromium.org/19466005/diff/13001/tools/android/memdump/mem... tools/android/memdump/memdump.cc:36: BitSet() : data_(0) {} This will use the std::vector<char>::vector<char>(size_t size) constructor (note the |size| as opposed to |capacity|). So this has the same effect as the default constructor. C++'s philosophy is not to pay for what you don't use so I doubt the default constructor (with size 0) is pre-allocating a dynamic buffer. Not sure this is a good argument :) https://codereview.chromium.org/19466005/diff/13001/tools/android/memdump/mem... tools/android/memdump/memdump.cc:48: const std::string AsB64String() const { Nit: this 'const' is superfluous since you are returning a copy. Note that with your signature the caller could still write 'std::string s = bit_set.AsB64String()'. This would cause yet another copy :)
https://codereview.chromium.org/19466005/diff/13001/tools/android/memdump/mem... File tools/android/memdump/memdump.cc (right): https://codereview.chromium.org/19466005/diff/13001/tools/android/memdump/mem... tools/android/memdump/memdump.cc:36: BitSet() : data_(0) {} On 2013/07/23 09:08:22, pliard wrote: > This will use the std::vector<char>::vector<char>(size_t size) constructor (note > the |size| as opposed to |capacity|). So this has the same effect as the default > constructor. C++'s philosophy is not to pay for what you don't use so I doubt > the default constructor (with size 0) is pre-allocating a dynamic buffer. Not > sure this is a good argument :) Well, the default constructor without the argument is slightly different ( vector(const allocator_type& alloc = allocator_type()). Btw, it seems that the inizial capacity, in both cases, is 0. So, agree with you, let's keep it clean. https://codereview.chromium.org/19466005/diff/13001/tools/android/memdump/mem... tools/android/memdump/memdump.cc:48: const std::string AsB64String() const { On 2013/07/23 09:08:22, pliard wrote: > Nit: this 'const' is superfluous since you are returning a copy. Note that with > your signature the caller could still write 'std::string s = > bit_set.AsB64String()'. This would cause yet another copy :) Yeah, that was a leftover from a previous attempt (to use directly a string as back storage and return that one w/ encoding). Done!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/primiano@chromium.org/19466005/19001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/primiano@chromium.org/19466005/28001
Message was sent while issue was closed.
Change committed as 213363 |
