cobalt/metrics: Complete Cobalt-specific granular memory tracking implementation#10014
cobalt/metrics: Complete Cobalt-specific granular memory tracking implementation#10014Awallky wants to merge 11 commits intoyoutube:mainfrom
Conversation
# Conflicts: # services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_linux.cc
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
…based on code review findings
78dd42e to
45bd7e3
Compare
There was a problem hiding this comment.
Code Review
This pull request implements a granular memory metrics collection system for Cobalt, introducing a DetailedMetricsDelegate to categorize memory mappings from /proc/self/smaps. Key changes include refactoring proc_maps_linux.cc to use std::string_view and base::SplitStringPiece for more efficient parsing, and updating the memory instrumentation service to support project-specific detailed metrics. Review feedback highlights opportunities to improve the robustness of the parsing logic by using pointer arithmetic instead of string searching for path extraction, handling potential numeric overflows by saturating to the maximum size_t value, and maintaining larger buffer sizes to prevent truncation of long smaps lines.
| size_t dev_pos = lines[i].find(tokens[3]); | ||
| size_t inode_pos = lines[i].find(tokens[4], dev_pos + tokens[3].size()); | ||
| if (inode_pos != std::string_view::npos) { | ||
| size_t path_pos = | ||
| lines[i].find_first_not_of(' ', inode_pos + tokens[4].size()); | ||
| if (path_pos != std::string_view::npos) { | ||
| region.path.assign(lines[i].substr(path_pos)); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to find the path position using find() on token strings is fragile. It can return incorrect indices if the device or inode strings appear earlier in the line (e.g., as part of the hex address range or offset). Since tokens[4] is a std::string_view that points directly into the lines[i] buffer, you can calculate the exact position using pointer arithmetic, which is safer and more efficient.
const char* line_start = lines[i].data();
const char* inode_end = tokens[4].data() + tokens[4].size();
size_t inode_end_pos = static_cast<size_t>(inode_end - line_start);
size_t path_pos = lines[i].find_first_not_of(' ', inode_end_pos);
if (path_pos != std::string_view::npos) {
region.path.assign(lines[i].substr(path_pos));
}| size_t dev_pos = header_line.find(tokens[3]); | ||
| if (dev_pos != std::string_view::npos) { | ||
| size_t inode_pos = | ||
| header_line.find(tokens[4], dev_pos + tokens[3].size()); | ||
| if (inode_pos != std::string_view::npos) { | ||
| size_t filename_pos = | ||
| header_line.find_first_not_of(' ', inode_pos + tokens[4].size()); | ||
| if (filename_pos != std::string_view::npos) { | ||
| region->mapped_file = std::string(header_line.substr(filename_pos)); | ||
| base::TrimWhitespaceASCII(region->mapped_file, base::TRIM_ALL, | ||
| ®ion->mapped_file); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar to the issue in base/debug/proc_maps_linux.cc, using find() to locate the filename position is unreliable because the device or inode strings might match substrings in earlier fields like the address range. Use pointer arithmetic with the std::string_view tokens to determine the correct offset.
const char* line_start = header_line.data();
const char* inode_end = tokens[4].data() + tokens[4].size();
size_t inode_end_pos = static_cast<size_t>(inode_end - line_start);
size_t filename_pos = header_line.find_first_not_of(' ', inode_end_pos);
if (filename_pos != std::string_view::npos) {
region->mapped_file = std::string(header_line.substr(filename_pos));
base::TrimWhitespaceASCII(region->mapped_file, base::TRIM_ALL,
®ion->mapped_file);
}| if (base::StringToSizeT(tokens[1], &val)) { | ||
| base::CheckedNumeric<size_t> val_bytes = val; | ||
| val_bytes *= 1024; | ||
| tmp[key] = val_bytes.ValueOrDefault(0); |
There was a problem hiding this comment.
Using ValueOrDefault(0) on overflow can lead to incorrect metrics reporting if the value exceeds the capacity of size_t (which is possible on 32-bit platforms). It is safer to saturate to the maximum possible value.
| tmp[key] = val_bytes.ValueOrDefault(0); | |
| tmp[key] = val_bytes.ValueOrDefault(std::numeric_limits<size_t>::max()); |
|
|
||
| if (!res) | ||
| return std::vector<VmRegionPtr>(); | ||
| char line[2048]; |
There was a problem hiding this comment.
The buffer size for reading smaps lines has been reduced from 4096 to 2048. While 2048 is often enough, smaps lines containing very long paths can exceed this limit. It is safer to maintain the previous limit of 4096 to avoid truncation issues during parsing.
| char line[2048]; | |
| char line[4096]; |
This commit finalizes the granular memory tracking implementation for Cobalt.
It resolves merge conflicts, adds specific coverage for Cobalt libraries in
browser tests, and adds robustness tests based on code review feedback.
granular_memory_browsertest.ccandos_metrics.content/test/BUILD.gn.CobaltSpecificMetricstest to verify actual library categorization.Bug: 494004530