Optimistically read from /proc for process stats. #3374
Conversation
|
Remaining:
|
aee-google
force-pushed
the
cpu-metric
branch
3 times, most recently
from
5cca730
to
194f29a
Compare
Pull Request is not mergeable
aee-google
force-pushed
the
cpu-metric
branch
2 times, most recently
from
6329fab
to
d27d65d
Compare
aee-google
force-pushed
the
cpu-metric
branch
2 times, most recently
from
7aa5b77
to
9a36b01
Compare
| if (open_parens_idx == std::string::npos || | ||
| close_parens_idx == std::string::npos || | ||
| open_parens_idx > close_parens_idx) { | ||
| size_t pid_end = stats_data.find(" ("); |
There was a problem hiding this comment.
Is the existing logic broken in upstream, or is it not a "hard broken" and its just that the upstream returns the thread name with the parentheses and you changed that for us to leave those out?
There was a problem hiding this comment.
The pid has an extra space at the end. Everything else is parsed correctly.
There was a problem hiding this comment.
It would be better to keep variable names the same where possible and keep our changes in STARBOARD/USE_COBALT_CUSTOMIZATIONS blocks, the only thing I see changing is comm_start
There was a problem hiding this comment.
We need the start and end positions for pid and comm, and we need the start position for state.
open_parens_idx was the first occurrence of " (. This is the pid end. pid start position is 0. open_parens_idx is incremented and the pid substring is off by 1.
Naming the positions we need avoids the modifying the value and using it to represent two different things.
stats_data.substr(start, end - start) returns the value we want.
After the last ") ", the rest of the fields are space delimited.
If I wanted to leave the code mostly the same, I would need to either move the open_parens_idx++ after getting the pid or removing the increment and fix how comm is retrieved.
There was a problem hiding this comment.
Another option is to parse the /proc/pid/stat in cobalt/base/process/process_metrics_helper.cc.
There was a problem hiding this comment.
Unless we upstream this code (the ideal solution) or guard it with ifdefs (an easier immediate solution), someone will eventually try to revert this back to the original code i.e. during an update as right now it looks like a mistaken diff from the upstream. Using the ifdefs we usually use makes it clear to readers that it's a conscious decision instead
Pull Request is not mergeable
| if (open_parens_idx == std::string::npos || | ||
| close_parens_idx == std::string::npos || | ||
| open_parens_idx > close_parens_idx) { | ||
| size_t pid_end = stats_data.find(" ("); |
There was a problem hiding this comment.
It would be better to keep variable names the same where possible and keep our changes in STARBOARD/USE_COBALT_CUSTOMIZATIONS blocks, the only thing I see changing is comm_start
These two tests failed due to the PR (youtube#3374), and they should be re-enabled once it fixes. b/346868673
These two tests failed due to the PR (youtube#3374), and they should be re-enabled once it fixes. b/346868673
These two tests failed due to the PR (#3374), and they should be re-enabled once it fixes. b/346868673
b/341774149 (cherry picked from commit 6424e50)
…s. (#3607) Refer to the original PR: #3374 b/341774149 Co-authored-by: aee <[email protected]>
b/341774149