cobalt/metrics: Integrate granular memory tracking into memory instrumentation service#10019
cobalt/metrics: Integrate granular memory tracking into memory instrumentation service#10019Awallky wants to merge 5 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:
|
There was a problem hiding this comment.
Code Review
This pull request refactors memory instrumentation parsing logic to use std::string_view and base::SplitStringPiece, improving efficiency by reducing heap allocations. It also introduces a DetailedMetricsDelegate for project-specific metrics collection, with specific implementations for Cobalt. Review feedback identifies fragile path parsing logic in proc_maps_linux.cc and os_metrics_linux.cc that could be improved by using token offsets. Additionally, the buffer size for reading smaps lines should be increased to 4096 to handle maximum Linux path lengths, and the Cobalt implementation of PerformOSMemoryDump needs to accurately track and report the overall success status of the dump operation.
| 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 for finding the path position using find() on the device and inode tokens is fragile. If the device or inode strings appear earlier in the line (e.g., in the address or offset fields), find() might return the wrong position. Since tokens[4] is a std::string_view into lines[i], you can calculate the exact position of the inode and then find the path starting after it.
| 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)); | |
| } | |
| } | |
| if (tokens.size() >= 6) { | |
| // The path starts after the inode. Since tokens are views into the | |
| // original line, we can calculate the path position directly. | |
| size_t inode_end_offset = (tokens[4].data() + tokens[4].size()) - lines[i].data(); | |
| size_t path_offset = lines[i].find_first_not_of(' ', inode_end_offset); | |
| if (path_offset != std::string_view::npos) { | |
| region.path.assign(lines[i].substr(path_offset)); | |
| } | |
| } |
| if (tokens.size() >= 6) { | ||
| 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.
The logic for finding the path position using find() on the device and inode tokens is fragile. If the device or inode strings appear earlier in the line (e.g., in the address or offset fields), find() might return the wrong position. Since tokens[4] is a std::string_view into header_line, you can calculate the exact position of the inode and then find the path starting after it.
if (tokens.size() >= 6) {
// The path starts after the inode. Since tokens are views into the
// original line, we can calculate the path position directly.
size_t inode_end_offset = (tokens[4].data() + tokens[4].size()) - header_line.data();
size_t filename_offset = header_line.find_first_not_of(' ', inode_end_offset);
if (filename_offset != std::string_view::npos) {
region->mapped_file = std::string(header_line.substr(filename_offset));
base::TrimWhitespaceASCII(region->mapped_file, base::TRIM_ALL,
®ion->mapped_file);
}
}|
|
||
| if (!res) | ||
| return std::vector<VmRegionPtr>(); | ||
| char line[2048]; |
There was a problem hiding this comment.
The buffer size of 2048 is too small for Linux paths, which can be up to 4096 characters (PATH_MAX). If a line in /proc/self/smaps exceeds this buffer, fgets will return a partial line, which will likely break the parsing logic in the delegate. It is recommended to use at least 4096 bytes.
| char line[2048]; | |
| char line[4096]; |
| base::BindOnce( | ||
| [](RequestOSMemoryDumpCallback callback, | ||
| base::flat_map<base::ProcessId, mojom::RawOSMemDumpPtr> results) { | ||
| std::move(callback).Run(true, std::move(results)); |
There was a problem hiding this comment.
This commit wires the granular memory tracking delegate into the memory
instrumentation service and the UMA pipeline. It ensures that detailed
memory dumps correctly populate the custom categories.
cobalt_browser_main_parts.cc.MemoryInstrumentation.detailed_stats_kbinCreatePublicOSDump.Bug: 494004530