Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
113543004:
Remove unnecessary #include (Closed)
DescriptionRemove unnecessary #include
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=241541
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Patch Set 3 : add two "#include"s needed #Messages
Total messages: 14 (0 generated)
Please take a look.
On 2013/12/17 11:27:28, hyunki wrote: > Please take a look. I think you can separate this patch into two different patches. - one for removing unnecessary include's - and, one for adding yourself into the AUTHORS file.
It seems there is no reason to hesitate to land this patch.
https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... File tools/android/memdump/memdump.cc (left): https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... tools/android/memdump/memdump.cc:23: #include "base/containers/hash_tables.h" FYI, we are using this include (for base::hash_map) and logging.h below (for DCHECK()).
On 2013/12/17 12:53:42, Philippe wrote: > https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... > File tools/android/memdump/memdump.cc (left): > > https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... > tools/android/memdump/memdump.cc:23: #include "base/containers/hash_tables.h" > FYI, we are using this include (for base::hash_map) and logging.h below (for > DCHECK()). And just out of curiosity, what are you guys considering doing with memdump? I'm interested in anything which has to do with memory :)
On 2013/12/17 13:02:28, Philippe wrote: > On 2013/12/17 12:53:42, Philippe wrote: > > > https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... > > File tools/android/memdump/memdump.cc (left): > > > > > https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... > > tools/android/memdump/memdump.cc:23: #include "base/containers/hash_tables.h" > > FYI, we are using this include (for base::hash_map) and logging.h below (for > > DCHECK()). > (hyunki) It works okay with these removal, however, I'll remove above two "#include"s. > And just out of curiosity, what are you guys considering doing with memdump? I'm > interested in anything which has to do with memory :) (hyunki) Actually, we are finding better method to measure memory usage than PSS for comparing chrome for android to old stock browser. I'm now thinking memdump would be one of candidates because it adds shared_app to total memory (not shared_others). Currently, when I am checking this memdump for old stock browser, results for Native library goes 0 while it seems give correct number on chrome browser. Can I use this tool for measuring the memory usage of stock browser? If not, how about to support it?
As you understood the main advantage of memdump compared to other tools is to allow memory measurement of applications with multiple processes. The stock browser is single process so it should not need memdump although you should still be able to use it with it. memreport.py contains some regexps that we use to map the memdump output to specific areas (e.g. native library) of Chrome. You would very likely have to tune those regexps (e.g. the one for the native library) for the stock browser. You can find out more about the stock browser memory mappings by catting /proc/<PID>/smaps. However keep in mind that the stock browser is part of the system image so its native library should be pre-loaded in the zygote process (meaning that it won't show up as 'private' but rather 'shared'). https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... File tools/android/memdump/memdump.cc (left): https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... tools/android/memdump/memdump.cc:23: #include "base/containers/hash_tables.h" On 2013/12/17 12:53:42, Philippe wrote: > FYI, we are using this include (for base::hash_map) and logging.h below (for > DCHECK()). I don't see any diff between patch set 1 and patch set 2, i.e. you are still removing those two includes that are needed :)
On 2013/12/17 14:09:13, Philippe wrote: > As you understood the main advantage of memdump compared to other tools is to > allow memory measurement of applications with multiple processes. The stock > browser is single process so it should not need memdump although you should > still be able to use it with it. memreport.py contains some regexps that we use > to map the memdump output to specific areas (e.g. native library) of Chrome. You > would very likely have to tune those regexps (e.g. the one for the native > library) for the stock browser. You can find out more about the stock browser > memory mappings by catting /proc/<PID>/smaps. However keep in mind that the > stock browser is part of the system image so its native library should be > pre-loaded in the zygote process (meaning that it won't show up as 'private' but > rather 'shared'). > > https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... > File tools/android/memdump/memdump.cc (left): > > https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... > tools/android/memdump/memdump.cc:23: #include "base/containers/hash_tables.h" > On 2013/12/17 12:53:42, Philippe wrote: > > FYI, we are using this include (for base::hash_map) and logging.h below (for > > DCHECK()). > > I don't see any diff between patch set 1 and patch set 2, i.e. you are still > removing those two includes that are needed :) Sorry, I've just added above reply only without patchset because I've got home. I'll upload it soon.
On 2013/12/17 14:21:33, hyunki wrote: > On 2013/12/17 14:09:13, Philippe wrote: > > As you understood the main advantage of memdump compared to other tools is to > > allow memory measurement of applications with multiple processes. The stock > > browser is single process so it should not need memdump although you should > > still be able to use it with it. memreport.py contains some regexps that we > use > > to map the memdump output to specific areas (e.g. native library) of Chrome. > You > > would very likely have to tune those regexps (e.g. the one for the native > > library) for the stock browser. You can find out more about the stock browser > > memory mappings by catting /proc/<PID>/smaps. However keep in mind that the > > stock browser is part of the system image so its native library should be > > pre-loaded in the zygote process (meaning that it won't show up as 'private' > but > > rather 'shared'). > > > > > https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... > > File tools/android/memdump/memdump.cc (left): > > > > > https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... > > tools/android/memdump/memdump.cc:23: #include "base/containers/hash_tables.h" > > On 2013/12/17 12:53:42, Philippe wrote: > > > FYI, we are using this include (for base::hash_map) and logging.h below (for > > > DCHECK()). > > > > I don't see any diff between patch set 1 and patch set 2, i.e. you are still > > removing those two includes that are needed :) > > Sorry, I've just added above reply only without patchset because I've got home. > I'll upload it soon. No worries :) I will approve as soon as I see your new patch set.
https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... File tools/android/memdump/memdump.cc (left): https://codereview.chromium.org/113543004/diff/10001/tools/android/memdump/me... tools/android/memdump/memdump.cc:23: #include "base/containers/hash_tables.h" On 2013/12/17 14:09:13, Philippe wrote: > On 2013/12/17 12:53:42, Philippe wrote: > > FYI, we are using this include (for base::hash_map) and logging.h below (for > > DCHECK()). > > I don't see any diff between patch set 1 and patch set 2, i.e. you are still > removing those two includes that are needed :) Done.
lgtm, thanks! Please don't hesitate to share your findings with us.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hyunki.baik@samsung.com/113543004/30001
On 2013/12/18 08:24:22, Philippe wrote: > lgtm, thanks! Please don't hesitate to share your findings with us. Sure, Thanks!
Message was sent while issue was closed.
Change committed as 241541 |
