Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
888193002:
ProcessMetrics::GetCPUUsage() is Slow (Closed)
DescriptionProcessMetrics::GetCPUUsage() is Slow
Instead of parsing the whole proc stats just to extract two values, we
skim through it just to extract the two values we're interested in.
BUG=453871
TEST=base_unittests --gtest_filter=ProcessMetricsTest.*
Committed: https://crrev.com/597a85175a34512b09b95cb241842ebadebd77ae
Cr-Commit-Position: refs/heads/master@{#314691}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 1
Patch Set 4 : This is probably a more readable version #
Total comments: 21
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #Patch Set 7 : #
Total comments: 2
Patch Set 8 : #Messages
Total messages: 38 (2 generated)
afakhry@chromium.org changed reviewers: + danakj@chromium.org, oshima@chromium.org
I left the old implementation as a reference. I also added a unit test.
https://codereview.chromium.org/888193002/diff/1/base/process/process_metrics.h File base/process/process_metrics.h (right): https://codereview.chromium.org/888193002/diff/1/base/process/process_metrics... base/process/process_metrics.h:256: BASE_EXPORT int GetCpuFromProcStats(const std::string& input); why are you adding a new method with the same return values as the old one, and replacing the one callsite, instead of just changing the old method? https://codereview.chromium.org/888193002/diff/1/base/process/process_metrics... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/1/base/process/process_metrics... base/process/process_metrics_linux.cc:415: // Exposed for testing. this comment is in a weird place, itd expect it in a header where exposed or not is decided https://codereview.chromium.org/888193002/diff/1/base/process/process_metrics... base/process/process_metrics_linux.cc:423: bool prevWasSpace = false; fix style
do you have some performance measurements to justify the change?
On 2015/02/03 18:52:07, danakj wrote: > do you have some performance measurements to justify the change? Yes, I posted screenshots of trace results on the bug report: https://code.google.com/p/chromium/issues/detail?id=453871
https://codereview.chromium.org/888193002/diff/1/base/process/process_metrics.h File base/process/process_metrics.h (right): https://codereview.chromium.org/888193002/diff/1/base/process/process_metrics... base/process/process_metrics.h:256: BASE_EXPORT int GetCpuFromProcStats(const std::string& input); On 2015/02/03 18:51:09, danakj wrote: > why are you adding a new method with the same return values as the old one, and > replacing the one callsite, instead of just changing the old method? I wanted to keep the old one for reference, but you're right, no need to do this, Hence, removed. https://codereview.chromium.org/888193002/diff/1/base/process/process_metrics... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/1/base/process/process_metrics... base/process/process_metrics_linux.cc:415: // Exposed for testing. On 2015/02/03 18:51:09, danakj wrote: > this comment is in a weird place, itd expect it in a header where exposed or not > is decided I was following the same style of the original ParseProcStatCPU(). I removed it anyway in this patch. https://codereview.chromium.org/888193002/diff/1/base/process/process_metrics... base/process/process_metrics_linux.cc:423: bool prevWasSpace = false; On 2015/02/03 18:51:09, danakj wrote: > fix style Done.
https://codereview.chromium.org/888193002/diff/20001/base/process/process_met... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/20001/base/process/process_met... base/process/process_metrics_linux.cc:418: // Restart the counter, we're looking to count from the last ')' we Can you use input.rfind to get the last ')'? This custom string searching code is really hard to read and to review, if we can make use of some string methods to make this more obvious, or something, that would really help. Or, can we do this with a regular expression?
https://codereview.chromium.org/888193002/diff/20001/base/process/process_met... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/20001/base/process/process_met... base/process/process_metrics_linux.cc:418: // Restart the counter, we're looking to count from the last ')' we On 2015/02/03 20:35:51, danakj wrote: > Can you use input.rfind to get the last ')'? > > This custom string searching code is really hard to read and to review, if we > can make use of some string methods to make this more obvious, or something, > that would really help. > > Or, can we do this with a regular expression? I understand the need for readability. But this is a critical call and the intent is to make it as efficient as possible. The last ')' is likely to be a few characters away from the beginning, and more than a couple of hundred characters away from the end. It would be a waste scanning and comparing from the end looking for the last ')' just to rescan and compare again from its position looking for the space just before utime's value. That's why we're doing both on the way while scanning once from the beginning.
On Tue, Feb 3, 2015 at 1:21 PM, <afakhry@chromium.org> wrote: > > https://codereview.chromium.org/888193002/diff/20001/base/ > process/process_metrics_linux.cc > File base/process/process_metrics_linux.cc (right): > > https://codereview.chromium.org/888193002/diff/20001/base/ > process/process_metrics_linux.cc#newcode418 > base/process/process_metrics_linux.cc:418: // Restart the counter, we're > looking to count from the last ')' we > On 2015/02/03 20:35:51, danakj wrote: > >> Can you use input.rfind to get the last ')'? >> > > This custom string searching code is really hard to read and to >> > review, if we > >> can make use of some string methods to make this more obvious, or >> > something, > >> that would really help. >> > > Or, can we do this with a regular expression? >> > > I understand the need for readability. But this is a critical call and > the intent is to make it as efficient as possible. > The last ')' is likely to be a few characters away from the beginning, > and more than a couple of hundred characters away from the end. It would > be a waste scanning and comparing from the end looking for the last ')' > just to rescan and compare again from its position looking for the space > just before utime's value. That's why we're doing both on the way while > scanning once from the beginning. > Does walking a few hundred characters show up as a noticible performance difference? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/03 21:53:56, danakj wrote: > On Tue, Feb 3, 2015 at 1:21 PM, <mailto:afakhry@chromium.org> wrote: > > > > > https://codereview.chromium.org/888193002/diff/20001/base/ > > process/process_metrics_linux.cc > > File base/process/process_metrics_linux.cc (right): > > > > https://codereview.chromium.org/888193002/diff/20001/base/ > > process/process_metrics_linux.cc#newcode418 > > base/process/process_metrics_linux.cc:418: // Restart the counter, we're > > looking to count from the last ')' we > > On 2015/02/03 20:35:51, danakj wrote: > > > >> Can you use input.rfind to get the last ')'? > >> > > > > This custom string searching code is really hard to read and to > >> > > review, if we > > > >> can make use of some string methods to make this more obvious, or > >> > > something, > > > >> that would really help. > >> > > > > Or, can we do this with a regular expression? > >> > > > > I understand the need for readability. But this is a critical call and > > the intent is to make it as efficient as possible. > > The last ')' is likely to be a few characters away from the beginning, > > and more than a couple of hundred characters away from the end. It would > > be a waste scanning and comparing from the end looking for the last ')' > > just to rescan and compare again from its position looking for the space > > just before utime's value. That's why we're doing both on the way while > > scanning once from the beginning. > > > > Does walking a few hundred characters show up as a noticible performance > difference? > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Indeed not noticeable (as a standalone call at least :) ), but the key point in my previous comment was "as efficient as possible". This function will be called several times per second (about 140 times per second from the task manager if you have a couple of tabs open and a couple of extensions -- see the screenshots https://code.google.com/p/chromium/issues/detail?id=453871#c5). What's not noticeable on its own adds up to be noticeable collectively. I don't think doing an rfind() will improve much in terms of readability. If the readability was the concern here, we would've left it as it was originally, very clean, very readable, but very slow :). What do you think?
On Tue, Feb 3, 2015 at 2:22 PM, <afakhry@chromium.org> wrote: > On 2015/02/03 21:53:56, danakj wrote: > >> On Tue, Feb 3, 2015 at 1:21 PM, <mailto:afakhry@chromium.org> wrote: >> > > > >> > https://codereview.chromium.org/888193002/diff/20001/base/ >> > process/process_metrics_linux.cc >> > File base/process/process_metrics_linux.cc (right): >> > >> > https://codereview.chromium.org/888193002/diff/20001/base/ >> > process/process_metrics_linux.cc#newcode418 >> > base/process/process_metrics_linux.cc:418: // Restart the counter, >> we're >> > looking to count from the last ')' we >> > On 2015/02/03 20:35:51, danakj wrote: >> > >> >> Can you use input.rfind to get the last ')'? >> >> >> > >> > This custom string searching code is really hard to read and to >> >> >> > review, if we >> > >> >> can make use of some string methods to make this more obvious, or >> >> >> > something, >> > >> >> that would really help. >> >> >> > >> > Or, can we do this with a regular expression? >> >> >> > >> > I understand the need for readability. But this is a critical call and >> > the intent is to make it as efficient as possible. >> > The last ')' is likely to be a few characters away from the beginning, >> > and more than a couple of hundred characters away from the end. It would >> > be a waste scanning and comparing from the end looking for the last ')' >> > just to rescan and compare again from its position looking for the space >> > just before utime's value. That's why we're doing both on the way while >> > scanning once from the beginning. >> > >> > > Does walking a few hundred characters show up as a noticible performance >> difference? >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > Indeed not noticeable (as a standalone call at least :) ), but the key > point in > my previous comment was "as efficient as possible". This function will be > called > several times per second (about 140 times per second from the task manager > if > you have a couple of tabs open and a couple of extensions -- see the > screenshots > https://code.google.com/p/chromium/issues/detail?id=453871#c5). What's not > noticeable on its own adds up to be noticeable collectively. I don't think > doing > an rfind() will improve much in terms of readability. If the readability > was the > concern here, we would've left it as it was originally, very clean, very > readable, but very slow :). What do you think? > From the profiling results this function can be a lot faster. If profiling doesn't show using something like rfind() as being slower, then we should feel free to use those. Presumably there's more work being slow than just a string search in 100 characters. Let's use profiling data to decide how far to go on writing crazy custom code. If there's performance data saying we need to do it this way we can go for it. If we can use some string methods to make this easier to read and still get good performance, then we should prefer that. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/03 22:25:08, danakj wrote: > On Tue, Feb 3, 2015 at 2:22 PM, <mailto:afakhry@chromium.org> wrote: > > > On 2015/02/03 21:53:56, danakj wrote: > > > >> On Tue, Feb 3, 2015 at 1:21 PM, <mailto:afakhry@chromium.org> wrote: > >> > > > > > > >> > https://codereview.chromium.org/888193002/diff/20001/base/ > >> > process/process_metrics_linux.cc > >> > File base/process/process_metrics_linux.cc (right): > >> > > >> > https://codereview.chromium.org/888193002/diff/20001/base/ > >> > process/process_metrics_linux.cc#newcode418 > >> > base/process/process_metrics_linux.cc:418: // Restart the counter, > >> we're > >> > looking to count from the last ')' we > >> > On 2015/02/03 20:35:51, danakj wrote: > >> > > >> >> Can you use input.rfind to get the last ')'? > >> >> > >> > > >> > This custom string searching code is really hard to read and to > >> >> > >> > review, if we > >> > > >> >> can make use of some string methods to make this more obvious, or > >> >> > >> > something, > >> > > >> >> that would really help. > >> >> > >> > > >> > Or, can we do this with a regular expression? > >> >> > >> > > >> > I understand the need for readability. But this is a critical call and > >> > the intent is to make it as efficient as possible. > >> > The last ')' is likely to be a few characters away from the beginning, > >> > and more than a couple of hundred characters away from the end. It would > >> > be a waste scanning and comparing from the end looking for the last ')' > >> > just to rescan and compare again from its position looking for the space > >> > just before utime's value. That's why we're doing both on the way while > >> > scanning once from the beginning. > >> > > >> > > > > Does walking a few hundred characters show up as a noticible performance > >> difference? > >> > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > Indeed not noticeable (as a standalone call at least :) ), but the key > > point in > > my previous comment was "as efficient as possible". This function will be > > called > > several times per second (about 140 times per second from the task manager > > if > > you have a couple of tabs open and a couple of extensions -- see the > > screenshots > > https://code.google.com/p/chromium/issues/detail?id=453871#c5). What's not > > noticeable on its own adds up to be noticeable collectively. I don't think > > doing > > an rfind() will improve much in terms of readability. If the readability > > was the > > concern here, we would've left it as it was originally, very clean, very > > readable, but very slow :). What do you think? > > > > From the profiling results this function can be a lot faster. If profiling > doesn't show using something like rfind() as being slower, then we should > feel free to use those. > > Presumably there's more work being slow than just a string search in 100 > characters. Let's use profiling data to decide how far to go on writing > crazy custom code. If there's performance data saying we need to do it this > way we can go for it. If we can use some string methods to make this easier > to read and still get good performance, then we should prefer that. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Agreed. I will profile it and trace it both with this implementation and with rfind, and share with you the results.
The results of the tracing showed that the improvement of not using rfind() is negligible, hence this patch.
https://codereview.chromium.org/888193002/diff/40001/base/process/process_met... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/40001/base/process/process_met... base/process/process_metrics_linux.cc:409: auto start = input.rfind(')'); Thanks. Did you consider a regular expression for this? It seems well suited to this problem.
I submitted a third patch. It's probably much simpler than the previous one. No, I haven't considered regular expressions, I think it's simple enough that we don't need to bother.
https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:403: if (input.empty()) Can you leave a comment like ParseProcStats? // |input| may be empty if the process disappeared somehow. // e.g. http://crbug.com/145811 https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:408: int last_space_index = internal::VM_UTIME - 1; why do you call this index? they seem like counts, would num_spaces_remaining be accurate for this variable? https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:411: if (start == input.npos || start == 0) why do you have to check for 0 here? https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:414: auto end = input.size() - 1; can you call this last instead of end? end is usually past the end, ie size(), or end(). https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:415: for (auto j = start + 1; j < end; ++j) { a nit, but why j instead of i? usually we'd use i for an outermost loop. can you leave a comment mentioning why it skips the last char? https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:416: if (input[j] != ' ' && input[j - 1] == ' ') { could you just string::find(" ") from the current position for the next space N times, then read the numbers there? https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:429: if (sscanf(&input.data()[utime_start_index], "%d %d", &utime, &stime) != 2) Why not StringToInt instead of sscanf?
https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:409: int utime_start_index = 0; move these variable after next if block https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:410: auto start = input.find_last_of(')'); just use size_t. it isn't worth it imo. https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:416: if (input[j] != ' ' && input[j - 1] == ' ') { I think he wanted to deal with two space case. I'm fine to use find if you think it's not important. It's easier to understand if you flip j and j-1 if we want to keep this way. https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:420: } optional: you can actually increase j here because you know [j] isn't ' '.
https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:416: if (input[j] != ' ' && input[j - 1] == ' ') { On 2015/02/04 01:49:46, oshima wrote: > I think he wanted to deal with two space case. I'm fine to use find > if you think it's not important. > > It's easier to understand if you flip j and j-1 if we want to keep this way. Can just skip past contiguous spaces if that can happen in the input. Something like this, modulo off-by-ones while (...) { pos = std::find(" ", pos); if (pos == npos) break; while (*pos == ' ') ++pos; --num_spaces_remaining;
https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:416: if (input[j] != ' ' && input[j - 1] == ' ') { On 2015/02/04 01:52:54, danakj wrote: > On 2015/02/04 01:49:46, oshima wrote: > > I think he wanted to deal with two space case. I'm fine to use find > > if you think it's not important. > > > > It's easier to understand if you flip j and j-1 if we want to keep this way. > > Can just skip past contiguous spaces if that can happen in the input. Something > like this, modulo off-by-ones > > while (...) { > pos = std::find(" ", pos); > if (pos == npos) break; > while (*pos == ' ') ++pos; > --num_spaces_remaining; That's fine too. I don't have much preference. https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:429: if (sscanf(&input.data()[utime_start_index], "%d %d", &utime, &stime) != 2) On 2015/02/04 01:29:18, danakj wrote: > Why not StringToInt instead of sscanf? Is there any issue with sscanf?
https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:403: if (input.empty()) On 2015/02/04 01:29:18, danakj wrote: > Can you leave a comment like ParseProcStats? > > // |input| may be empty if the process disappeared somehow. > // e.g. http://crbug.com/145811 Done. https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:408: int last_space_index = internal::VM_UTIME - 1; On 2015/02/04 01:29:18, danakj wrote: > why do you call this index? they seem like counts, would num_spaces_remaining be > accurate for this variable? Renamed. https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:409: int utime_start_index = 0; On 2015/02/04 01:49:46, oshima wrote: > move these variable after next if block Done. https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:410: auto start = input.find_last_of(')'); On 2015/02/04 01:49:46, oshima wrote: > just use size_t. it isn't worth it imo. Done. https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:414: auto end = input.size() - 1; On 2015/02/04 01:29:18, danakj wrote: > can you call this last instead of end? end is usually past the end, ie size(), > or end(). Done. https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:416: if (input[j] != ' ' && input[j - 1] == ' ') { On 2015/02/04 01:29:18, danakj wrote: > could you just string::find(" ") from the current position for the next space N > times, then read the numbers there? Done. https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... base/process/process_metrics_linux.cc:429: if (sscanf(&input.data()[utime_start_index], "%d %d", &utime, &stime) != 2) On 2015/02/04 01:29:18, danakj wrote: > Why not StringToInt instead of sscanf? Because it's much simpler. No additional parsing is needed once you find the index of utime. Used to extract the two values we're interested in from a formatted input.
On Feb 3, 2015 6:28 PM, <oshima@chromium.org> wrote: > > > https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... > File base/process/process_metrics_linux.cc (right): > > https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... > base/process/process_metrics_linux.cc:416: if (input[j] != ' ' && > input[j - 1] == ' ') { > On 2015/02/04 01:52:54, danakj wrote: >> >> On 2015/02/04 01:49:46, oshima wrote: >> > I think he wanted to deal with two space case. I'm fine to use find >> > if you think it's not important. >> > >> > It's easier to understand if you flip j and j-1 if we want to keep > > this way. > >> Can just skip past contiguous spaces if that can happen in the input. > > Something >> >> like this, modulo off-by-ones > > >> while (...) { >> pos = std::find(" ", pos); >> if (pos == npos) break; >> while (*pos == ' ') ++pos; >> --num_spaces_remaining; > > > That's fine too. I don't have much preference. > > > https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... > base/process/process_metrics_linux.cc:429: if > (sscanf(&input.data()[utime_start_index], "%d %d", &utime, &stime) != 2) > On 2015/02/04 01:29:18, danakj wrote: >> >> Why not StringToInt instead of sscanf? > > > Is there any issue with sscanf? I dont know but the old path used this instead of sscanf. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/04 02:35:54, danakj wrote: > On Feb 3, 2015 6:28 PM, <mailto:oshima@chromium.org> wrote: > > > > > > > https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... > > File base/process/process_metrics_linux.cc (right): > > > > > https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... > > base/process/process_metrics_linux.cc:416: if (input[j] != ' ' && > > input[j - 1] == ' ') { > > On 2015/02/04 01:52:54, danakj wrote: > >> > >> On 2015/02/04 01:49:46, oshima wrote: > >> > I think he wanted to deal with two space case. I'm fine to use find > >> > if you think it's not important. > >> > > >> > It's easier to understand if you flip j and j-1 if we want to keep > > > > this way. > > > >> Can just skip past contiguous spaces if that can happen in the input. > > > > Something > >> > >> like this, modulo off-by-ones > > > > > >> while (...) { > >> pos = std::find(" ", pos); > >> if (pos == npos) break; > >> while (*pos == ' ') ++pos; > >> --num_spaces_remaining; > > > > > > That's fine too. I don't have much preference. > > > > > > > https://codereview.chromium.org/888193002/diff/60001/base/process/process_met... > > base/process/process_metrics_linux.cc:429: if > > (sscanf(&input.data()[utime_start_index], "%d %d", &utime, &stime) != 2) > > On 2015/02/04 01:29:18, danakj wrote: > >> > >> Why not StringToInt instead of sscanf? > > > > > > Is there any issue with sscanf? > > I dont know but the old path used this instead of sscanf. It made sense because it used to tokenize them into std::string first. sscanf does all of tokenizing, parsing and counting in one shot, so I think this is fine unless there is a reason not to. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Wed, Feb 4, 2015 at 10:17 AM, <oshima@chromium.org> wrote: > On 2015/02/04 02:35:54, danakj wrote: > > On Feb 3, 2015 6:28 PM, <mailto:oshima@chromium.org> wrote: >> > >> > >> > >> > > https://codereview.chromium.org/888193002/diff/60001/base/ > process/process_metrics_linux.cc > >> > File base/process/process_metrics_linux.cc (right): >> > >> > >> > > https://codereview.chromium.org/888193002/diff/60001/base/ > process/process_metrics_linux.cc#newcode416 > >> > base/process/process_metrics_linux.cc:416: if (input[j] != ' ' && >> > input[j - 1] == ' ') { >> > On 2015/02/04 01:52:54, danakj wrote: >> >> >> >> On 2015/02/04 01:49:46, oshima wrote: >> >> > I think he wanted to deal with two space case. I'm fine to use find >> >> > if you think it's not important. >> >> > >> >> > It's easier to understand if you flip j and j-1 if we want to keep >> > >> > this way. >> > >> >> Can just skip past contiguous spaces if that can happen in the input. >> > >> > Something >> >> >> >> like this, modulo off-by-ones >> > >> > >> >> while (...) { >> >> pos = std::find(" ", pos); >> >> if (pos == npos) break; >> >> while (*pos == ' ') ++pos; >> >> --num_spaces_remaining; >> > >> > >> > That's fine too. I don't have much preference. >> > >> > >> > >> > > https://codereview.chromium.org/888193002/diff/60001/base/ > process/process_metrics_linux.cc#newcode429 > >> > base/process/process_metrics_linux.cc:429: if >> > (sscanf(&input.data()[utime_start_index], "%d %d", &utime, &stime) != >> 2) >> > On 2015/02/04 01:29:18, danakj wrote: >> >> >> >> Why not StringToInt instead of sscanf? >> > >> > >> > Is there any issue with sscanf? >> > > I dont know but the old path used this instead of sscanf. >> > > It made sense because it used to tokenize them into std::string first. > sscanf does all of tokenizing, parsing and counting in one shot, so I think > this is fine unless there is a reason not to. ok i don't know of any reason and i do see some other use of it in similar code in base/ > > > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > > > > https://codereview.chromium.org/888193002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/04 18:21:05, danakj wrote: > On Wed, Feb 4, 2015 at 10:17 AM, <mailto:oshima@chromium.org> wrote: > > > On 2015/02/04 02:35:54, danakj wrote: > > > > On Feb 3, 2015 6:28 PM, <mailto:oshima@chromium.org> wrote: > >> > > >> > > >> > > >> > > > > https://codereview.chromium.org/888193002/diff/60001/base/ > > process/process_metrics_linux.cc > > > >> > File base/process/process_metrics_linux.cc (right): > >> > > >> > > >> > > > > https://codereview.chromium.org/888193002/diff/60001/base/ > > process/process_metrics_linux.cc#newcode416 > > > >> > base/process/process_metrics_linux.cc:416: if (input[j] != ' ' && > >> > input[j - 1] == ' ') { > >> > On 2015/02/04 01:52:54, danakj wrote: > >> >> > >> >> On 2015/02/04 01:49:46, oshima wrote: > >> >> > I think he wanted to deal with two space case. I'm fine to use find > >> >> > if you think it's not important. > >> >> > > >> >> > It's easier to understand if you flip j and j-1 if we want to keep > >> > > >> > this way. > >> > > >> >> Can just skip past contiguous spaces if that can happen in the input. > >> > > >> > Something > >> >> > >> >> like this, modulo off-by-ones > >> > > >> > > >> >> while (...) { > >> >> pos = std::find(" ", pos); > >> >> if (pos == npos) break; > >> >> while (*pos == ' ') ++pos; > >> >> --num_spaces_remaining; > >> > > >> > > >> > That's fine too. I don't have much preference. > >> > > >> > > >> > > >> > > > > https://codereview.chromium.org/888193002/diff/60001/base/ > > process/process_metrics_linux.cc#newcode429 > > > >> > base/process/process_metrics_linux.cc:429: if > >> > (sscanf(&input.data()[utime_start_index], "%d %d", &utime, &stime) != > >> 2) > >> > On 2015/02/04 01:29:18, danakj wrote: > >> >> > >> >> Why not StringToInt instead of sscanf? > >> > > >> > > >> > Is there any issue with sscanf? > >> > > > > I dont know but the old path used this instead of sscanf. > >> > > > > It made sense because it used to tokenize them into std::string first. > > sscanf does all of tokenizing, parsing and counting in one shot, so I think > > this is fine unless there is a reason not to. > > > ok i don't know of any reason and i do see some other use of it in similar > code in base/ > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > >> > > email > > > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > > > > > > > https://codereview.chromium.org/888193002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. How is the fifth patch?
https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... base/process/process_metrics_linux.cc:419: if (input[i - 1] != ' ' && --num_spaces_remaining == 0) { can you leave a comment like // Skips past contiguous spaces without counting them? btw none of the tests seem to have more than one space between values. can that actually happen? https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... base/process/process_metrics_linux.cc:425: if (num_spaces_remaining != 0) nit: maybe slightly more obvious to check utime_start_index == 0?
https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... base/process/process_metrics_linux.cc:419: if (input[i - 1] != ' ' && --num_spaces_remaining == 0) { On 2015/02/04 18:33:06, danakj wrote: > can you leave a comment like // Skips past contiguous spaces without counting > them? > > btw none of the tests seem to have more than one space between values. can that > actually happen? It's unlikey but it's better be safe. The original code used SplitString which took care of that. https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... base/process/process_metrics_linux.cc:425: if (num_spaces_remaining != 0) On 2015/02/04 18:33:06, danakj wrote: > nit: maybe slightly more obvious to check utime_start_index == 0? Done.
On 2015/02/04 19:05:21, afakhry wrote: > https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... > File base/process/process_metrics_linux.cc (right): > > https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... > base/process/process_metrics_linux.cc:419: if (input[i - 1] != ' ' && > --num_spaces_remaining == 0) { > On 2015/02/04 18:33:06, danakj wrote: > > can you leave a comment like // Skips past contiguous spaces without counting > > them? > > > > btw none of the tests seem to have more than one space between values. can > that > > actually happen? > > It's unlikey but it's better be safe. The original code used SplitString which > took care of that. Can you explain what you mean by unlikely? This is an established format is it not? I disagree in that adding complexity for cases that can not happen is a bad thing. > https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... > base/process/process_metrics_linux.cc:425: if (num_spaces_remaining != 0) > On 2015/02/04 18:33:06, danakj wrote: > > nit: maybe slightly more obvious to check utime_start_index == 0? > > Done.
On 2015/02/04 19:10:17, danakj wrote: > On 2015/02/04 19:05:21, afakhry wrote: > > > https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... > > File base/process/process_metrics_linux.cc (right): > > > > > https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... > > base/process/process_metrics_linux.cc:419: if (input[i - 1] != ' ' && > > --num_spaces_remaining == 0) { > > On 2015/02/04 18:33:06, danakj wrote: > > > can you leave a comment like // Skips past contiguous spaces without > counting > > > them? > > > > > > btw none of the tests seem to have more than one space between values. can > > that > > > actually happen? > > > > It's unlikey but it's better be safe. The original code used SplitString which > > took care of that. > > Can you explain what you mean by unlikely? This is an established format is it > not? > > I disagree in that adding complexity for cases that can not happen is a bad > thing. > > > > https://codereview.chromium.org/888193002/diff/80001/base/process/process_met... > > base/process/process_metrics_linux.cc:425: if (num_spaces_remaining != 0) > > On 2015/02/04 18:33:06, danakj wrote: > > > nit: maybe slightly more obvious to check utime_start_index == 0? > > > > Done. |input| is the contents of /proc/<pid>/stat so it's definitely an established format and that's why it's unlikely to happen. As I said, the original code took care of that (unnecessarily maybe) and I didn't want to change that behavior. If you agree to make it "garbage in, garbage out", then let's remove this check.
Yes, feel free to add a dcheck if youre worried this may change in the future
Agreed, Done!
https://codereview.chromium.org/888193002/diff/120001/base/process/process_me... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/120001/base/process/process_me... base/process/process_metrics_linux.cc:423: utime_start_index = i; cool thanks. one more comment, i think we could save a bunch of lines some branching, and a variable by just doing the sscanf here on |i|. wdyt?
https://codereview.chromium.org/888193002/diff/120001/base/process/process_me... File base/process/process_metrics_linux.cc (right): https://codereview.chromium.org/888193002/diff/120001/base/process/process_me... base/process/process_metrics_linux.cc:423: utime_start_index = i; On 2015/02/04 19:52:44, danakj wrote: > cool thanks. one more comment, i think we could save a bunch of lines some > branching, and a variable by just doing the sscanf here on |i|. wdyt? I like that! :) Done!
LGTM
lgtm
The CQ bit was checked by afakhry@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/888193002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/597a85175a34512b09b95cb241842ebadebd77ae Cr-Commit-Position: refs/heads/master@{#314691} |
