cobalt/metrics: Feature/granular memory cl2 engine#10018
cobalt/metrics: Feature/granular memory cl2 engine#10018Awallky wants to merge 3 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 Linux process map and smaps parsing to use std::string_view and base::SplitStringPiece, replacing sscanf for improved performance and safety. It also introduces a DetailedMetricsDelegate interface to support project-specific memory categorization. Review feedback highlights potential issues with the new parsing logic, including a risk of partial line reads due to a smaller buffer in ReadDetailedMetricsFile, fragility in the path-finding logic using find(), and remaining heap allocations from SplitStringPiece that could be further optimized using StringTokenizer.
| char line[2048]; | ||
| while (fgets(line, sizeof(line), f)) { |
There was a problem hiding this comment.
The buffer size for fgets has been reduced to 2048, and the code does not handle partial lines. If a line in /proc/self/smaps exceeds 2047 characters (which can happen with long paths or many flags), fgets will return the line in chunks. This will cause OnSmapsBuffer to be called with incomplete data, likely breaking the parsing logic in the delegate (e.g., if a keyword like "Pss:" or its value is split across chunks). Consider using a larger buffer (e.g., 4096 as used previously) and verifying that the line ends with a newline character, or using base::ReadFileToString to read the entire file into memory before processing, which would be more robust and consistent with GetProcessMemoryMaps.
| // Use SplitStringPiece to avoid heap allocations for every line. | ||
| std::vector<std::string_view> lines = base::SplitStringPiece( | ||
| input, "\n", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL); |
There was a problem hiding this comment.
The comment stating that SplitStringPiece avoids heap allocations for every line is slightly misleading. While it avoids copying the string content, base::SplitStringPiece still allocates a std::vector on the heap to return the pieces. While the allocation on line 113 happens once for the entire input, the subsequent calls to SplitStringPiece inside the loop (e.g., line 126, 136, 188) will perform heap allocations for every line processed. For a truly allocation-free approach in this tight loop, consider using base::StringTokenizer or manual searching with std::string_view::find.
| size_t dev_pos = lines[i].find(tokens[3]); | ||
| size_t inode_pos = lines[i].find(tokens[4], dev_pos + tokens[3].size()); |
There was a problem hiding this comment.
The logic for finding the start of the path using find() on the original line with token strings is potentially fragile. If the device string (tokens[3]) or inode string (tokens[4]) happens to appear earlier in the line (e.g., within the address range or offset fields), find() might return an incorrect position. While searching for the inode after the device position reduces the risk, it is less robust than the previous sscanf approach which used %n to get the exact offset. Consider if a more reliable way to determine the path start position can be used, perhaps by calculating the offset from the string_view positions if possible.
This commit implements the core engine for Cobalt memory tracking, including
efficient parsing of
/proc/self/smapsand categorization of memory regions.It includes fixes for streaming, thread safety, and concurrency tests that
were identified during development.
SmapsCategorizerandCobaltDetailedMetricsDelegate.Bug: 494004530