Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
10 years ago by sail Modified:
9 years, 7 months ago Reviewers:
Mark Mentovai, viettrungluu CC:
chromium-reviews, arv (Not doing code reviews), pam+watch_chromium.org, Paweł Hajdan Jr., Nico Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionUse /usr/bin/top in about:memory
Currently we get memory information for about:memory using /bin/ps. Unfortuantely ps doesn't give us information about shared and private memory. Also, it's total virtual memory value doesn't match Activity Monitor's.
This change adds code to get memory information using /usr/bin/top. The new code falls back to ps if there's a parsing error.
Once about:memory displays all the information we need there are still a couple of things left to do:
- refactor ProcessInfoSnapshot to use ProcessMetrics where possible
- do a better job of detecting shared objects between our child processes and show it some how
BUG=25454
TEST=Opened about:memory and verified that the values matched those in Activity Monitor. Tested on both 10.6 and 10.5.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71114
Patch Set 1 #
Total comments: 64
Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 5
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 7
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Patch Set 10 : '' #
Total comments: 3
Patch Set 11 : '' #
Created: 9 years, 11 months ago
Messages
Total messages: 19 (0 generated)
Overall nice first cut. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... File chrome/browser/process_info_snapshot.h (right): http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot.h:53: // We explicitly use uint64_t instead of size_t incase the process we're in case is two words. Avoid using “we,” “our” in comments. Revise. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot.h:54: // measure is 64bit but we're not. measure -> measuring 64-bit is not a single word. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot.h:62: : pid(0), Odd formatting. Initializer lists ought to be indented as described in http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... File chrome/browser/process_info_snapshot_mac.cc (right): http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:8: #include <sys/sysctl.h> C system headers should precede C++ system headers and should be in a separate section. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:27: static bool GetKInfoForProcessID(kinfo_proc* kinfo, pid_t pid) { I suggest putting “in” parameters before “out” ones. Same on line 43. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:28: int mib[4]; Why not just use int mib[] = {CTL_KERN, KERN_PROC, KERN_PROC_PID, pid};? http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:34: size_t miblen = 4; Why not just use arraysize(mib)? (You don’t even need a separate variable for it.) http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:43: static bool GetExecutableNameForProcessID(char** executable_name, pid_t pid) { The interface would probably be better if the out parameter were an std::string*. That would make ownership much clearer. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:44: static bool s_should_init_arg_max = true; These usually work a little better in terms of code size, data size, or both when the initial state is 0 (false). Use the power of zero to your advantage. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:45: static int s_arg_max = 0; You may not need two separate variables here, either. s_arg_max == 0 can just be the signal that initialization never happened. This will mean that you’ll keep retrying the sysctl each time through the function if it keeps failing for some reason, but that should be harmless. Less code, less data. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:52: if (s_should_init_arg_max) { The declarations should be closer to the point of first use. This is the point of first use. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:54: int mib[2]; Simplify mib and size as discussed above. Same on 67-76. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:75: return false; Leaks arg? See, the ownership model is spaghetti here. This is something else that would be cleaner with std::string *executable_name. Instead of doing |new char|, you would just do |executable_name->resize(s_arg_max + 1);| and then pass |&(*executable_name)[0]| to sysctl. Of course, you’d probably also want to find the first NUL character before returning, and resizing the string so that it ONLY refers to the executable name and not the whole argv, envp, apple, etc. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:83: // http://www.opensource.apple.com/source/libutil/libutil-21/humanize_number.c The function name doesn’t really tell me what to pass for the arguments or what to expect the function to do, and neither does this comment. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:84: static bool ConvertUnitToScale(std::string unit, uint64_t* scale) { unit can be a const std::string&. unit can also just be a char. That might be even better. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:89: uint64_t shift = 0; This is fine as just an int. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:94: case 'K': Care about lowercase 'k'? http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:117: for (size_t i = 0; i < shift; i++) { This is fine as just an int too. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:118: *scale *= 1024; Rather than dereferencing scale each time through the loop, why not do the work on a local temporary and then stash it in scale after the loop? As an alternative, you can have this function return scale as a uint64_t instead of a bool. You can use 0 as a special “I failed” return value. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:129: const char* kPsPathName = "/bin/ps"; Some of the comments below, like using a const char[], might be relevant in this function too. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:192: const char* kPsPathName = "/usr/bin/top"; Isn’t this more like kTopPathName? And wouldn’t you prefer a const char[]? http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:216: std::istringstream topIn(output, std::istringstream::in); top_in, not topIn. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:218: while (std::getline(topIn, line)) { Show an example line or two, so that the reader has some sense of the expected data without having to cobble together a top invocation manually to figure it out. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:268: const char* kPsPathName = "/usr/bin/top"; Refer to similar comments above. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:356: // Note, p_comm is truncated to 15 characters. Really? 15 or 16? p_comm is a char[MAXCOMLEN + 1], and MAXCOMLEN is 16. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:361: // Use KERN_PROCARGS to get the full executable name. This may fail if we we? http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:369: delete exectuable_name; See? Doesn’t this suck? http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:374: bool memoryInfoSuccess = false; memory_info_success, because of http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:380: memoryInfoSuccess = GetProcessMemoryInfoUsingTop(proc_info_entries_); I wouldn’t even try this unless ((major == 10 && minor >= 6) || major > 10). http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:382: // If top didn't work then fallback to ps. “Fall back” as a verb is two words. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:384: memoryInfoSuccess = GetProcessMemoryInfoUsingPS(pid_list, This needs {braces} because it’s a multi-line. http://codereview.chromium.org/6052005/diff/1/chrome/browser/resources/about_... File chrome/browser/resources/about_memory_mac.html (right): http://codereview.chromium.org/6052005/diff/1/chrome/browser/resources/about_... chrome/browser/resources/about_memory_mac.html:186: Shared What will the values in these columns look like if rshrd or rprvt weren’t available? I think it’d be better for them to show up as blank instead of 0.
I tested this on thakis's 10.5 laptop and it seems to work well. The only remaining problem is that the EffectiveVsRealUserIDTest unit test is still failing. I think it might be because top is temporarily dropping permissions but I can't tell. At this point I'm thinking of removing that test. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... File chrome/browser/process_info_snapshot.h (right): http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot.h:53: // We explicitly use uint64_t instead of size_t incase the process we're On 2011/01/04 20:09:31, Mark Mentovai wrote: > in case is two words. > > Avoid using “we,” “our” in comments. Revise. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot.h:54: // measure is 64bit but we're not. On 2011/01/04 20:09:31, Mark Mentovai wrote: > measure -> measuring > > 64-bit is not a single word. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot.h:62: : pid(0), On 2011/01/04 20:09:31, Mark Mentovai wrote: > Odd formatting. Initializer lists ought to be indented as described in > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... File chrome/browser/process_info_snapshot_mac.cc (right): http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:8: #include <sys/sysctl.h> On 2011/01/04 20:09:31, Mark Mentovai wrote: > C system headers should precede C++ system headers and should be in a separate > section. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:27: static bool GetKInfoForProcessID(kinfo_proc* kinfo, pid_t pid) { On 2011/01/04 20:09:31, Mark Mentovai wrote: > I suggest putting “in” parameters before “out” ones. Same on line 43. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:28: int mib[4]; On 2011/01/04 20:09:31, Mark Mentovai wrote: > Why not just use int mib[] = {CTL_KERN, KERN_PROC, KERN_PROC_PID, pid};? Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:34: size_t miblen = 4; On 2011/01/04 20:09:31, Mark Mentovai wrote: > Why not just use arraysize(mib)? (You don’t even need a separate variable for > it.) Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:43: static bool GetExecutableNameForProcessID(char** executable_name, pid_t pid) { On 2011/01/04 20:09:31, Mark Mentovai wrote: > The interface would probably be better if the out parameter were an > std::string*. That would make ownership much clearer. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:44: static bool s_should_init_arg_max = true; On 2011/01/04 20:09:31, Mark Mentovai wrote: > These usually work a little better in terms of code size, data size, or both > when the initial state is 0 (false). Use the power of zero to your advantage. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:45: static int s_arg_max = 0; On 2011/01/04 20:09:31, Mark Mentovai wrote: > You may not need two separate variables here, either. s_arg_max == 0 can just be > the signal that initialization never happened. This will mean that you’ll keep > retrying the sysctl each time through the function if it keeps failing for some > reason, but that should be harmless. Less code, less data. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:52: if (s_should_init_arg_max) { On 2011/01/04 20:09:31, Mark Mentovai wrote: > The declarations should be closer to the point of first use. This is the point > of first use. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:54: int mib[2]; On 2011/01/04 20:09:31, Mark Mentovai wrote: > Simplify mib and size as discussed above. Same on 67-76. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:75: return false; On 2011/01/04 20:09:31, Mark Mentovai wrote: > Leaks arg? See, the ownership model is spaghetti here. > > This is something else that would be cleaner with std::string *executable_name. > Instead of doing |new char|, you would just do > |executable_name->resize(s_arg_max + 1);| and then pass |&(*executable_name)[0]| > to sysctl. > > Of course, you’d probably also want to find the first NUL character before > returning, and resizing the string so that it ONLY refers to the executable name > and not the whole argv, envp, apple, etc. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:83: // http://www.opensource.apple.com/source/libutil/libutil-21/humanize_number.c On 2011/01/04 20:09:31, Mark Mentovai wrote: > The function name doesn’t really tell me what to pass for the arguments or what > to expect the function to do, and neither does this comment. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:84: static bool ConvertUnitToScale(std::string unit, uint64_t* scale) { On 2011/01/04 20:09:31, Mark Mentovai wrote: > unit can be a const std::string&. > > unit can also just be a char. That might be even better. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:89: uint64_t shift = 0; On 2011/01/04 20:09:31, Mark Mentovai wrote: > This is fine as just an int. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:94: case 'K': On 2011/01/04 20:09:31, Mark Mentovai wrote: > Care about lowercase 'k'? Done. Libtop doesn't use lowercase 'k' but it doesn't hurt to be able to parse it. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:117: for (size_t i = 0; i < shift; i++) { On 2011/01/04 20:09:31, Mark Mentovai wrote: > This is fine as just an int too. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:118: *scale *= 1024; On 2011/01/04 20:09:31, Mark Mentovai wrote: > Rather than dereferencing scale each time through the loop, why not do the work > on a local temporary and then stash it in scale after the loop? > > As an alternative, you can have this function return scale as a uint64_t instead > of a bool. You can use 0 as a special “I failed” return value. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:129: const char* kPsPathName = "/bin/ps"; On 2011/01/04 20:09:31, Mark Mentovai wrote: > Some of the comments below, like using a const char[], might be relevant in this > function too. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:192: const char* kPsPathName = "/usr/bin/top"; On 2011/01/04 20:09:31, Mark Mentovai wrote: > Isn’t this more like kTopPathName? > > And wouldn’t you prefer a const char[]? Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:216: std::istringstream topIn(output, std::istringstream::in); On 2011/01/04 20:09:31, Mark Mentovai wrote: > top_in, not topIn. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:218: while (std::getline(topIn, line)) { On 2011/01/04 20:09:31, Mark Mentovai wrote: > Show an example line or two, so that the reader has some sense of the expected > data without having to cobble together a top invocation manually to figure it > out. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:268: const char* kPsPathName = "/usr/bin/top"; On 2011/01/04 20:09:31, Mark Mentovai wrote: > Refer to similar comments above. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:356: // Note, p_comm is truncated to 15 characters. On 2011/01/04 20:09:31, Mark Mentovai wrote: > Really? 15 or 16? p_comm is a char[MAXCOMLEN + 1], and MAXCOMLEN is 16. Ahh, you're right. Fixed. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:361: // Use KERN_PROCARGS to get the full executable name. This may fail if we On 2011/01/04 20:09:31, Mark Mentovai wrote: > we? Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:369: delete exectuable_name; On 2011/01/04 20:09:31, Mark Mentovai wrote: > See? Doesn’t this suck? Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:374: bool memoryInfoSuccess = false; On 2011/01/04 20:09:31, Mark Mentovai wrote: > memory_info_success, because of > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab... Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:380: memoryInfoSuccess = GetProcessMemoryInfoUsingTop(proc_info_entries_); On 2011/01/04 20:09:31, Mark Mentovai wrote: > I wouldn’t even try this unless ((major == 10 && minor >= 6) || major > 10). Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:382: // If top didn't work then fallback to ps. On 2011/01/04 20:09:31, Mark Mentovai wrote: > “Fall back” as a verb is two words. Done. http://codereview.chromium.org/6052005/diff/1/chrome/browser/process_info_sna... chrome/browser/process_info_snapshot_mac.cc:384: memoryInfoSuccess = GetProcessMemoryInfoUsingPS(pid_list, On 2011/01/04 20:09:31, Mark Mentovai wrote: > This needs {braces} because it’s a multi-line. Done.
http://codereview.chromium.org/6052005/diff/2002/chrome/browser/process_info_... File chrome/browser/process_info_snapshot_mac.cc (right): http://codereview.chromium.org/6052005/diff/2002/chrome/browser/process_info_... chrome/browser/process_info_snapshot_mac.cc:70: executable_name->resize(end_pos + 1); Don’t you want end_pos - 1? You should also check that end_pos != 0 in that case. Also, if you don’t wind up here (executable_name->resize), you should probably return false. That would be a totally unexpected case. http://codereview.chromium.org/6052005/diff/2002/chrome/browser/resources/abo... File chrome/browser/resources/about_memory_mac.html (right): http://codereview.chromium.org/6052005/diff/2002/chrome/browser/resources/abo... chrome/browser/resources/about_memory_mac.html:186: Shared I had a question here before that you haven’t answered: http://codereview.chromium.org/6052005/diff/1/chrome/browser/resources/about_...
http://codereview.chromium.org/6052005/diff/2002/chrome/browser/process_info_... File chrome/browser/process_info_snapshot_mac.cc (right): http://codereview.chromium.org/6052005/diff/2002/chrome/browser/process_info_... chrome/browser/process_info_snapshot_mac.cc:70: executable_name->resize(end_pos + 1); > Don’t you want end_pos - 1? Hm... I guess I don't understand how std::string works. Don't I want the NULL terminator in the std::string object? > Also, if you don’t wind up here (executable_name->resize), you should probably > return false. That would be a totally unexpected case. Yea, that's a good idea. http://codereview.chromium.org/6052005/diff/2002/chrome/browser/resources/abo... File chrome/browser/resources/about_memory_mac.html (right): http://codereview.chromium.org/6052005/diff/2002/chrome/browser/resources/abo... chrome/browser/resources/about_memory_mac.html:186: Shared On 2011/01/06 18:56:06, Mark Mentovai wrote: > I had a question here before that you haven’t answered: > > http://codereview.chromium.org/6052005/diff/1/chrome/browser/resources/about_... Oops, missed that. Looking into it.
sail@chromium.org wrote: > http://codereview.chromium.org/6052005/diff/2002/chrome/browser/process_info_... > chrome/browser/process_info_snapshot_mac.cc:70: > executable_name->resize(end_pos + 1); >> >> Don’t you want end_pos - 1? > > Hm... I guess I don't understand how std::string works. Don't I want the > NULL terminator in the std::string object? No. std::string doesn’t know or care about NUL termination (unless you ask it to read from a C-string). NUL is just another legal character in the string from its perspective. I guess you want end_pos, not +1 or -1. When you resize() a string, it will report its new size via size() or length(). You want that to be the same number that would show up if you ran strlen() on its c_str(), assuming (obviously) that you don’t intend for there to be any embedded NUL characters.
http://codereview.chromium.org/6052005/diff/1/chrome/browser/resources/about_... File chrome/browser/resources/about_memory_mac.html (right): http://codereview.chromium.org/6052005/diff/1/chrome/browser/resources/about_... chrome/browser/resources/about_memory_mac.html:186: Shared On 2011/01/04 20:09:31, Mark Mentovai wrote: > What will the values in these columns look like if rshrd or rprvt weren’t > available? > > I think it’d be better for them to show up as blank instead of 0. If rshrd or rprvt aren't available then they'll be displayed as N/A. This is handled by the formatNumber() function in about_memory_mac.html.
http://codereview.chromium.org/6052005/diff/2002/chrome/browser/process_info_... File chrome/browser/process_info_snapshot_mac.cc (right): http://codereview.chromium.org/6052005/diff/2002/chrome/browser/process_info_... chrome/browser/process_info_snapshot_mac.cc:70: executable_name->resize(end_pos + 1); On 2011/01/06 18:56:06, Mark Mentovai wrote: > Don’t you want end_pos - 1? > > You should also check that end_pos != 0 in that case. > > Also, if you don’t wind up here (executable_name->resize), you should probably > return false. That would be a totally unexpected case. Done.
LGTM
Thanks for the review. What do you think of removing the uid vs euid unit test?
sail@chromium.org wrote: > Thanks for the review. What do you think of removing the uid vs euid unit > test? Does it fail on 10.5 only, or on 10.5 and 10.6? Which portion fails? You only gave a reason that you thought it was failing. I don’t see which line (or lines) is giving trouble, or a transcript of the test run, or anything like that.
http://codereview.chromium.org/6052005/diff/33001/chrome/browser/process_info... File chrome/browser/process_info_snapshot_mac.cc (right): http://codereview.chromium.org/6052005/diff/33001/chrome/browser/process_info... chrome/browser/process_info_snapshot_mac.cc:130: argv.push_back("pid=,rss=,vsz="); Why did you take the ruid and uid out for ps? http://codereview.chromium.org/6052005/diff/33001/chrome/browser/process_info... chrome/browser/process_info_snapshot_mac.cc:197: "vsize"); // Total virtual memory You can do uid (which gives you effective user ID), but top doesn’t seem to have anything to give you the real user ID. That’s clearly why your test is failing—it doesn’t have anything to do with “dropping privileges.” Maybe it’s best to collect [effective] uid (since it seems like we can do it in all cases), never collect ruid (even when we can get it?), and just call that the limit of the interface on the Mac? http://codereview.chromium.org/6052005/diff/33001/chrome/browser/process_info... chrome/browser/process_info_snapshot_mac.cc:201: Add |-s 0| too, to prevent top from sleeping for one second after dumping its one and only sample. http://codereview.chromium.org/6052005/diff/33001/chrome/browser/process_info... chrome/browser/process_info_snapshot_mac.cc:282: 10.5 top seems to exit immediately even without -s 0, but you may wish to include it anyway for consistency.
http://codereview.chromium.org/6052005/diff/33001/chrome/browser/process_info... File chrome/browser/process_info_snapshot_mac.cc (right): http://codereview.chromium.org/6052005/diff/33001/chrome/browser/process_info... chrome/browser/process_info_snapshot_mac.cc:130: argv.push_back("pid=,rss=,vsz="); On 2011/01/06 21:50:04, Mark Mentovai wrote: > Why did you take the ruid and uid out for ps? I'm trying to use kinfo.kp_eproc.e_pcred.p_ruid and kinfo.kp_eproc.e_ucred.cr_uid instead. Maybe these items don't return the correct value if the target process is running as root. Dropping support for ruid and gathering that information only from ps/top seems fine to me. Want me to do that? http://codereview.chromium.org/6052005/diff/33001/chrome/browser/process_info... chrome/browser/process_info_snapshot_mac.cc:201: On 2011/01/06 21:50:04, Mark Mentovai wrote: > Add |-s 0| too, to prevent top from sleeping for one second after dumping its > one and only sample. Wow, I don't know how I missed that. Thanks so much. This makes things a lot faster. http://codereview.chromium.org/6052005/diff/33001/chrome/browser/process_info... chrome/browser/process_info_snapshot_mac.cc:282: On 2011/01/06 21:50:04, Mark Mentovai wrote: > 10.5 top seems to exit immediately even without -s 0, but you may wish to > include it anyway for consistency. Done.
>> Why did you take the ruid and uid out for ps? > > I'm trying to use kinfo.kp_eproc.e_pcred.p_ruid and > kinfo.kp_eproc.e_ucred.cr_uid instead. Oh, I see, of course. > Maybe these items don't return the correct value if the target process > is running as root. It should work. mark@rj bash$ top -stats pid -l 10 >& /dev/null & [1] 12538 mark@rj bash$ ./tsy 12538 i am pid=12540 ppid=4671 uid=502 (ok) euid=502 (ok) comm=tsy other is pid=12538 ppid=4671 uid=502 euid=0 comm=top tsy is basically a wrapper around GetKInfoForProcessID, calling it with getpid() and then with the command-line argument. I found that if the pid doesn’t exist, sysctl will still return 0 (success), but you can use len == 0 as a signal that the pid wasn’t found. You should do that. It may help turn up bugs. > Dropping support for ruid and gathering that information only from > ps/top seems fine to me. Want me to do that? No, it looks like we ought to be able to get this with the sysctl. Can you run the problem test and let me know what’s going on inside GetKInfoForProcessID? What values are you getting for pid, ppid, uid, uid, and comm? Do they look right? Does the sysctl set a good-looking len?
> I found that if the pid doesn’t exist, sysctl will still return 0 > (success), but you can use len == 0 as a signal that the pid wasn’t > found. You should do that. It may help turn up bugs. Good idea. Fixed. > Can you run the problem test and let me know what’s going on inside > GetKInfoForProcessID? What values are you getting for pid, ppid, uid, > uid, and comm? Do they look right? Does the sysctl set a good-looking > len? Here's what I get: proc_info.ppid: 0, proc_info.uid: 0, proc_info.euid: 0, kinfo.kp_proc.p_comm: unit_tests So it looks like top is being launched by calling fork() then exec(). At the point where GetKInfoForProcessID() is called exec() hasn't had a chance to run yet. I've added a 1 millisecond delay to the unit test and it seems to fix things.
sail@chromium.org wrote: > So it looks like top is being launched by calling fork() then exec(). At the > point where GetKInfoForProcessID() is called exec() hasn't had a chance to > run yet. Aha! > I've added a 1 millisecond delay to the unit test and it seems to fix > things. This will be a source of flake. 1ms works on your machine, but we don’t know if it works on all machines in all cases. It might not work if a machine is heavily-loaded, for example. Synchronizing with the child process, that would be better. For example, you could use the base::LaunchApp variant that accepts a file_handle_mapping_vector, assign one end of a pipe to the new process’ stdout, and then try to read from the other end of the pipe. It looks like you might need to use -l 0 or to give top a live stderr, otherwise it might exit immediately without doing anything (but only on 10.6?) int fds[2]; PCHECK(pipe(fds) == 0); base::file_handle_mapping_vector fds_to_remap; fds_to_remap.push_back(std::make_pair(fds[1], 1)); // If you want to keep top's stderr hooked up to the test process' stderr: // fds_to_remap.push_back(std::make_pair(fileno(stderr), 2)); std::vector<std::string> argv; argv.push_back("/usr/bin/top"); argv.push_back("-l"); argv.push_back("0"); base::ProcessHandle process_handle; ASSERT_TRUE(base::LaunchApp(argv, fds_to_remap, false, &process_handle)); PCHECK(HANDLE_EINTR(close(fds[1])) == 0); char buf[1]; PCHECK(HANDLE_EINTR(read(fds[0], buf, 1)) == 1); std::vector<base::ProcessId> pid_list; pid_list.push_back(process_handle); // The rest of the test goes here. ASSERT_TRUE(base::KillProcess(process_handle, 0, true)); PCHECK(HANDLE_EINTR(close(fds[0])) == 0);
> For example, you could use the base::LaunchApp variant that accepts a file_handle_mapping_vector, assign one end of a pipe to the new process’ stdout, and then try to read from the other end of the pipe. Good idea. Added. The unit test passes on my machine. Just sent a try request as well.
LGTM! http://codereview.chromium.org/6052005/diff/52001/chrome/browser/process_info... File chrome/browser/process_info_snapshot_mac_unittest.cc (right): http://codereview.chromium.org/6052005/diff/52001/chrome/browser/process_info... chrome/browser/process_info_snapshot_mac_unittest.cc:96: // uid this test runs top. top should have a uid of the caller and effective uid Comma after uid. http://codereview.chromium.org/6052005/diff/52001/chrome/browser/process_info... chrome/browser/process_info_snapshot_mac_unittest.cc:97: // of 0. // of 0 (root). http://codereview.chromium.org/6052005/diff/52001/chrome/browser/process_info... chrome/browser/process_info_snapshot_mac_unittest.cc:118: // the fork() and exec() calls are done and top is actually running. Technically, you don’t need to do anything to synchronize with fork(). By the time the parent process gets control back from fork(), it’s done by definition. You can eliminate the reference to fork() here.
Thanks for the review. Will submit once the try request succeeds. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
