Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5992005:
Mac: Shared and Private memory for Task Manager... (Closed)
|
Created:
10 years ago by sail Modified:
8 years, 3 months ago CC:
chromium-reviews, pam+watch_chromium.org, brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionMac: Shared and Private memory for Task Manager
This change adds support for calculating shared and private memory for the Task Manager window. The goal here is to match the values shown in Activity Monitor.
The old "Memory" column already matched Activity Monitor's "Real Memory Size" field.
With this change the "Private" column now matches Activity Monitor's "Private Memory Size" field too.
Unfortunately the "Shared" column doesn't exactly match Activity Monitor's "Shared Memory Size" field. From what I could understand of the libtop source this field is supposed to represent the resident shared memory that isn't being referenced by any other process. Since our code only looks at the memory for a single process we can't detect references by other processes. This means our shared memory value will always be slightly higher than Activity Monitor's.
BUG=25454
TEST=Opened a few tabs and verified that the Private values in Task Manager matched the values in Activity Monitor. Verified that the Shared values were just slightly higher than those in Activity Monitor.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69863
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 32
Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 8
Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 4
Patch Set 8 : '' #Messages
Total messages: 13 (0 generated)
Mostly LG, mostly just lots of nits. Vaguely non-trivial stuff: - I would just try to dodge any copyright issues by making a sufficiently non-derivative version of that function. - You should probably use a base::hash_set or a std::set, not an std::map. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm File base/process_util_mac.mm (right): http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:14: #include <mach/mach_vm.h> Nit: alphabetical order (see style guide). http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:27: #include <map> Ditto. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:241: static bool GetCPUTypeForProcess(pid_t pid, cpu_type_t* cpuType) { Nit: This is a C++-ish function, so the parameter should be called |cpu_type|. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:257: // From libtop in_shared_region(). If this is basically copied, then we need to do something about it copyright-wise. (At the very least, we'd need to indicate the original copyright. Possibly it should go in third_party/apple or something like that.) It may be sufficiently simple (and there's lots which you could trim out, since we don't support PPC or ARM) that perhaps you could make a sufficiently non-derivative version.... http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:259: mach_vm_address_t base = 0, size = 0; Nit: We don't declare multiple variables in the same statement. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:263: base = SHARED_REGION_BASE_ARM; Nit: Indentation. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:292: return(addr >= base && addr < (base + size)); Nit: No parentheses (and even if you felt they were needed for some reason, there should be a space after "return"). http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:310: vm_size_t pageSize; Nit: |page_size|. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:318: cpu_type_t cpuType; Ditto, etc. for other variables. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:322: // The same region can be reference multiple times. To avoid double counting reference*d* http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:324: std::map<int, bool> objectMap; I wonder if you shouldn't be using base::hash_map instead. Or maybe base::hash_set. (Or you could just be using std::set, I guess, though that's not much lighter weight than std::map.) Also "object map" isn't the most descriptive name. I would use some sort of set, and name it "seen_objects" or something like that. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:327: for (mach_vm_address_t address = MACH_VM_MIN_ADDRESS; ; address += size) { Nit: No space after first ';', I think. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:332: task, I believe you can align these with the '(' on the previous line (and put |task,| on the previous line). http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:357: if (private_bytes) { I think you may as well just calculate these (always) using a local variable, and do the conditional assignments at the end (just to make this a bit easier to read). http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:367: if (shared_bytes) Ditto. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:370: if (info.share_mode == SM_COW) { This might be nicer using a fall-through (explicitly labelling the fall-through); I think the style guide would be okay with that, though you should double-check.
P.S. Adding Nico since I'm out until next week. He should be able to LGTM it once you've updated this CL. I trust him (enough). :P
Addressed review comments. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm File base/process_util_mac.mm (right): http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:14: #include <mach/mach_vm.h> On 2010/12/21 04:21:06, viettrungluu wrote: > Nit: alphabetical order (see style guide). Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:27: #include <map> On 2010/12/21 04:21:06, viettrungluu wrote: > Ditto. Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:241: static bool GetCPUTypeForProcess(pid_t pid, cpu_type_t* cpuType) { On 2010/12/21 04:21:06, viettrungluu wrote: > Nit: This is a C++-ish function, so the parameter should be called |cpu_type|. Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:257: // From libtop in_shared_region(). On 2010/12/21 04:21:06, viettrungluu wrote: > If this is basically copied, then we need to do something about it > copyright-wise. (At the very least, we'd need to indicate the original > copyright. Possibly it should go in third_party/apple or something like that.) > > It may be sufficiently simple (and there's lots which you could trim out, since > we don't support PPC or ARM) that perhaps you could make a sufficiently > non-derivative version.... Implemented a very simple version that just handles the CPU types that we support. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:263: base = SHARED_REGION_BASE_ARM; On 2010/12/21 04:21:06, viettrungluu wrote: > Nit: Indentation. Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:292: return(addr >= base && addr < (base + size)); On 2010/12/21 04:21:06, viettrungluu wrote: > Nit: No parentheses (and even if you felt they were needed for some reason, > there should be a space after "return"). Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:310: vm_size_t pageSize; On 2010/12/21 04:21:06, viettrungluu wrote: > Nit: |page_size|. Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:318: cpu_type_t cpuType; On 2010/12/21 04:21:06, viettrungluu wrote: > Ditto, etc. for other variables. Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:322: // The same region can be reference multiple times. To avoid double counting On 2010/12/21 04:21:06, viettrungluu wrote: > reference*d* Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:324: std::map<int, bool> objectMap; On 2010/12/21 04:21:06, viettrungluu wrote: > I wonder if you shouldn't be using base::hash_map instead. Or maybe > base::hash_set. (Or you could just be using std::set, I guess, though that's not > much lighter weight than std::map.) > > Also "object map" isn't the most descriptive name. I would use some sort of set, > and name it "seen_objects" or something like that. Fixed, used base::hash_set. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:327: for (mach_vm_address_t address = MACH_VM_MIN_ADDRESS; ; address += size) { On 2010/12/21 04:21:06, viettrungluu wrote: > Nit: No space after first ';', I think. Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:332: task, On 2010/12/21 04:21:06, viettrungluu wrote: > I believe you can align these with the '(' on the previous line (and put |task,| > on the previous line). Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:357: if (private_bytes) { On 2010/12/21 04:21:06, viettrungluu wrote: > I think you may as well just calculate these (always) using a local variable, > and do the conditional assignments at the end (just to make this a bit easier to > read). Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:367: if (shared_bytes) On 2010/12/21 04:21:06, viettrungluu wrote: > Ditto. Done. http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:370: if (info.share_mode == SM_COW) { On 2010/12/21 04:21:06, viettrungluu wrote: > This might be nicer using a fall-through (explicitly labelling the > fall-through); I think the style guide would be okay with that, though you > should double-check. Fixed. The C++ style guide doesn't mention fall throughs so I guess it's ok.
Cool! One real comment (the if condition), the rest are nits. Out of interest: Did you check if gathering this data requires a measurable amount of %cpu? http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm File base/process_util_mac.mm (right): http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... base/process_util_mac.mm:249: LOG(ERROR) << "Call to sysctlbyname failed with error " use PLOG instead (it appends strerror(errno) automatically if errno is set) http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... base/process_util_mac.mm:261: else if (type == CPU_TYPE_I386) This probably should be CPU_TYPE_X86_64? (Does gcc not warn about if (c) else if(c)?) http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... base/process_util_mac.mm:274: size_t* shared_bytes) { Can you add a comment with a link to the libtop file this is based on? Can you also add a very short overview of how this function works? http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... base/process_util_mac.mm:330: if (seen_objects.find(info.obj_id) == seen_objects.end()) { Nit: I believe `if (seen_objects.count(info.obj_id) > 0)` is slightly more idiomatic.
Addressed review comments. I'm not sure what the best way to measure the CPU usage of this code. Here's some numbers though: - time it takes to run GetMemoryBytes() on my desktop: --- browser task: 1.3 milliseconds --- renderer task: 0.7 milliseconds --- flash plugin task: 0.3 milliseconds - CPU usage on my desktop with 7 tabs open and the task manager window shown: --- without my change: 0.5% --- with my change: 0.7% http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm File base/process_util_mac.mm (right): http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... base/process_util_mac.mm:249: LOG(ERROR) << "Call to sysctlbyname failed with error " On 2010/12/21 18:30:06, Nico wrote: > use PLOG instead (it appends strerror(errno) automatically if errno is set) Done. http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... base/process_util_mac.mm:261: else if (type == CPU_TYPE_I386) On 2010/12/21 18:30:06, Nico wrote: > This probably should be CPU_TYPE_X86_64? > > (Does gcc not warn about if (c) else if(c)?) Ouch, good catch. Fixed. http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... base/process_util_mac.mm:274: size_t* shared_bytes) { On 2010/12/21 18:30:06, Nico wrote: > Can you add a comment with a link to the libtop file this is based on? Can you > also add a very short overview of how this function works? Done. http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... base/process_util_mac.mm:330: if (seen_objects.find(info.obj_id) == seen_objects.end()) { On 2010/12/21 18:30:06, Nico wrote: > Nit: I believe `if (seen_objects.count(info.obj_id) > 0)` is slightly more > idiomatic. Done.
LG On Tue, Dec 21, 2010 at 11:31 AM, <sail@chromium.org> wrote: > Addressed review comments. > > I'm not sure what the best way to measure the CPU usage of this code. Here's > some numbers though: > - time it takes to run GetMemoryBytes() on my desktop: > --- browser task: 1.3 milliseconds > --- renderer task: 0.7 milliseconds > --- flash plugin task: 0.3 milliseconds > - CPU usage on my desktop with 7 tabs open and the task manager window > shown: > --- without my change: 0.5% > --- with my change: 0.7% Might it be worth to return early if both pointers passed in are NULL? Or are they never NULL even if these two columns are hidden? > > > > > > http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm > File base/process_util_mac.mm (right): > > http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... > base/process_util_mac.mm:249: LOG(ERROR) << "Call to sysctlbyname failed > with error " > On 2010/12/21 18:30:06, Nico wrote: >> >> use PLOG instead (it appends strerror(errno) automatically if errno is > > set) > > Done. > > http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... > base/process_util_mac.mm:261: else if (type == CPU_TYPE_I386) > On 2010/12/21 18:30:06, Nico wrote: >> >> This probably should be CPU_TYPE_X86_64? > >> (Does gcc not warn about if (c) else if(c)?) > > Ouch, good catch. Fixed. > > http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... > base/process_util_mac.mm:274: size_t* shared_bytes) { > On 2010/12/21 18:30:06, Nico wrote: >> >> Can you add a comment with a link to the libtop file this is based on? > > Can you >> >> also add a very short overview of how this function works? > > Done. > > http://codereview.chromium.org/5992005/diff/13001/base/process_util_mac.mm#ne... > base/process_util_mac.mm:330: if (seen_objects.find(info.obj_id) == > seen_objects.end()) { > On 2010/12/21 18:30:06, Nico wrote: >> >> Nit: I believe `if (seen_objects.count(info.obj_id) > 0)` is slightly > > more >> >> idiomatic. > > Done. > > http://codereview.chromium.org/5992005/ >
> Might it be worth to return early if both pointers passed in are NULL? > Or are they never NULL even if these two columns are hidden? Currently this is never called with NULL parameters but it seems like a good idea. Fixed.
Ship it! http://codereview.chromium.org/5992005/diff/21001/base/process_util_mac.mm File base/process_util_mac.mm (right): http://codereview.chromium.org/5992005/diff/21001/base/process_util_mac.mm#ne... base/process_util_mac.mm:271: // We iterate through each VM region in the task's address map. For shared ubernit: i'd move the implementation description (which starts at this paragraph) into the function http://codereview.chromium.org/5992005/diff/21001/base/process_util_mac.mm#ne... base/process_util_mac.mm:272: // memory we add up all the pages that are marked as shared.Like libtop we try space after period
Thanks for the review, submitting. http://codereview.chromium.org/5992005/diff/21001/base/process_util_mac.mm File base/process_util_mac.mm (right): http://codereview.chromium.org/5992005/diff/21001/base/process_util_mac.mm#ne... base/process_util_mac.mm:271: // We iterate through each VM region in the task's address map. For shared On 2010/12/21 19:55:07, Nico wrote: > ubernit: i'd move the implementation description (which starts at this > paragraph) into the function Done. http://codereview.chromium.org/5992005/diff/21001/base/process_util_mac.mm#ne... base/process_util_mac.mm:272: // memory we add up all the pages that are marked as shared.Like libtop we try On 2010/12/21 19:55:07, Nico wrote: > space after period Done.
Wait till mento sees all the "we"s in comments that you're LGing. He'll have a heart attack. ;) On 2010/12/21 19:55:07, Nico wrote: > Ship it! > > http://codereview.chromium.org/5992005/diff/21001/base/process_util_mac.mm > File base/process_util_mac.mm (right): > > http://codereview.chromium.org/5992005/diff/21001/base/process_util_mac.mm#ne... > base/process_util_mac.mm:271: // We iterate through each VM region in the task's > address map. For shared > ubernit: i'd move the implementation description (which starts at this > paragraph) into the function > > http://codereview.chromium.org/5992005/diff/21001/base/process_util_mac.mm#ne... > base/process_util_mac.mm:272: // memory we add up all the pages that are marked > as shared.Like libtop we try > space after period
http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm File base/process_util_mac.mm (right): http://codereview.chromium.org/5992005/diff/4001/base/process_util_mac.mm#new... base/process_util_mac.mm:263: base = SHARED_REGION_BASE_ARM; On 2010/12/21 18:15:33, sail wrote: > On 2010/12/21 04:21:06, viettrungluu wrote: > > Nit: Indentation. > > Done. Done.
5992005 の問題は？良くわかりません。 何をしてもおかしいからSafari を使いましたが、もしあれば教えて下さい。 |
