cobalt/metrics: Add browser tests for granular memory tracking by Awallky · Pull Request #10020 · youtube/cobalt

Awallky · 2026-04-14T04:58:48Z

This commit adds integration and browser tests to verify that the granular
memory tracking system works as expected in a browser environment.

Add GranularMemoryBrowsertest to verify dump triggering and propagation.
Include follow-up fixes to stabilize the tests.

Bug: 494004530

# Conflicts: # services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_linux.cc

…2 files.

…ervice.

…encies.

github-actions · 2026-04-14T04:59:16Z

🤖 Gemini Suggested Commit Message

cobalt: Add granular memory instrumentation

Introduce a detailed memory metrics delegation system to allow project-specific
categorization of memory regions. This enables more granular profiling and
verification of memory usage within Cobalt.

Refactor /proc/maps and /proc/smaps parsing to use string_view and safer
conversion functions, enhancing robustness. Move blocking I/O operations
for OS memory dumps to a background thread to prevent jank on the main thread.
Migrate the legacy libchrobalt memory tracking to this new, flexible delegate
infrastructure, providing consistent and extensible memory insights.
Include a new browser test for end-to-end validation of granular metrics.

Bug: 494004530

💡 Pro Tips for a Better Commit Message:

Influence the Result: Want to change the output? You can write custom prompts or instructions directly in the Pull Request description. The model uses that text to generate the message.
Re-run the Generator: Post a comment with: /generate-commit-message

gemini-code-assist

Code Review

This pull request implements granular memory reporting for Cobalt by introducing the DetailedMetricsDelegate interface and refactoring Linux memory parsing to use more efficient string utilities. Key changes include replacing sscanf with base::SplitStringPiece and updating OSMetrics to support project-specific memory categorization. Review feedback highlights opportunities to improve parsing robustness, such as using base::ReadFileToString to handle long lines in /proc/PID/smaps and employing pointer arithmetic for token extraction. Additionally, a suggestion was made to ensure that PSS validation failures do not prevent the reporting of basic memory metrics.

gemini-code-assist · 2026-04-14T05:00:34Z

services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_linux.cc

+  FILE* f = nullptr;
+  bool should_close = false;
+  {
+    base::AutoLock lock(GetTestingGlobalsLock());
+    if (g_proc_smaps_for_testing) {
+      f = g_proc_smaps_for_testing;
+      fseek(f, 0, SEEK_SET);
+    }
+  }
+
+  if (!f) {
    std::string file_name =
        "/proc/" +
        (handle == base::kNullProcessHandle ? "self"
                                            : base::NumberToString(handle)) +
        "/smaps";
-    base::ScopedFILE smaps_file(fopen(file_name.c_str(), "r"));
-    res = ReadLinuxProcSmapsFile(smaps_file.get(), &maps);
+    f = fopen(file_name.c_str(), "r");
+    if (!f) {
+      DPLOG(ERROR) << "Couldn't open " << file_name;
+      return false;
+    }
+    should_close = true;
  }

-  if (!res)
-    return std::vector<VmRegionPtr>();
+  char line[2048];
+  while (fgets(line, sizeof(line), f)) {
+    if (!delegate->OnSmapsBuffer(line)) {
+      if (should_close) {
+        fclose(f);
+      }
+      return false;
+    }
+  }

-  return maps;
+  if (should_close) {
+    fclose(f);
+  } else {
+    fseek(f, 0, SEEK_SET);
+  }
+
+  return true;


The current implementation uses fgets with a 2048-byte buffer. If a line in /proc/PID/smaps (e.g., a very long path) exceeds this size, it will be split into multiple calls to OnSmapsBuffer, which will likely break the delegate's parsing logic.

Additionally, the previous implementation of ReadLinuxProcSmapsFile used a 4096-byte buffer. It is recommended to use base::ReadFileToString and base::SplitStringPiece to handle lines of any length correctly and to be consistent with GetProcessMemoryMaps in this same file.

std::string smaps_content; { base::AutoLock lock(GetTestingGlobalsLock()); if (g_proc_smaps_for_testing) { fseek(g_proc_smaps_for_testing, 0, SEEK_SET); if (!base::ReadStreamToString(g_proc_smaps_for_testing, &smaps_content)) { return false; } } } if (smaps_content.empty()) { std::string file_name = "/proc/" + (handle == base::kNullProcessHandle ? "self" : base::NumberToString(handle)) + "/smaps"; if (!base::ReadFileToString(base::FilePath(file_name), &smaps_content)) { DPLOG(ERROR) << "Couldn't read " << file_name; return false; } } for (std::string_view line : base::SplitStringPiece( smaps_content, "\n", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL)) { if (line.empty()) continue; if (!delegate->OnSmapsBuffer(line)) return false; } return true;

gemini-code-assist · 2026-04-14T05:00:34Z

base/debug/proc_maps_linux.cc

+      // We find the inode token in the original line, but we must search
+      // after the device token to avoid matching the same string in earlier
+      // fields (e.g. if the offset is the same as the inode).
+      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));
+        }
+      }


Instead of using find() to locate the inode token in the line (which requires searching after the device token to avoid false positives), you can use pointer arithmetic since tokens[4] is a std::string_view pointing into lines[i]. This is more robust and efficient.

size_t inode_pos = tokens[4].data() - lines[i].data(); 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)); }

gemini-code-assist · 2026-04-14T05:00:35Z

services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_linux.cc

+  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,
+                                    &region->mapped_file);
+        }
+      }
+    }


Similar to the change in proc_maps_linux.cc, you can use pointer arithmetic to find the exact position of the inode token in header_line. This avoids the need for find() and the logic to search after dev_pos.

if (tokens.size() >= 6) { size_t inode_pos = tokens[4].data() - header_line.data(); 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, &region->mapped_file); } }

gemini-code-assist · 2026-04-14T05:00:35Z

services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_linux.cc

+  if (metrics.total_pss_kb > 0 && abs_diff > kPssValidationThresholdKb) {
+    LOG(WARNING) << "Detailed metrics PSS validation failed. "
+                 << "Detailed: " << metrics.total_pss_kb
+                 << " KiB, Rollup: " << rollup_pss / 1024 << " KiB";
+    return false;
+  }
+  dump->detailed_stats_kb = std::move(metrics.categories_kb);
+  dump->last_detailed_dump_time = base::TimeTicks::Now();


If the PSS validation against smaps_rollup fails, returning false here causes the entire memory dump for this PID to be discarded in client_process_impl.cc. It would be more robust to log the warning but still return true, simply skipping the population of detailed_stats_kb so that basic metrics are still reported.

Suggested change

if (metrics.total_pss_kb > 0 && abs_diff > kPssValidationThresholdKb) {

LOG(WARNING) << "Detailed metrics PSS validation failed. "

<< "Detailed: " << metrics.total_pss_kb

<< " KiB, Rollup: " << rollup_pss / 1024 << " KiB";

return false;

}

dump->detailed_stats_kb = std::move(metrics.categories_kb);

dump->last_detailed_dump_time = base::TimeTicks::Now();

if (metrics.total_pss_kb > 0 && abs_diff > kPssValidationThresholdKb) {

LOG(WARNING) << "Detailed metrics PSS validation failed. "

<< "Detailed: " << metrics.total_pss_kb

<< " KiB, Rollup: " << rollup_pss / 1024 << " KiB";

} else {

dump->detailed_stats_kb = std::move(metrics.categories_kb);

dump->last_detailed_dump_time = base::TimeTicks::Now();

}

return true;

Awallky added 8 commits April 13, 2026 20:35

CL1: Add interfaces and stubs for granular memory tracking.

e77d063

CL2: Implement high-performance smaps parsing and OS metrics utilities.

7e04cda

# Conflicts: # services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_linux.cc

Apply fixes for streaming, thread safety, and concurrency tests to CL…

3cfced7

…2 files.

CL3: Integrate granular memory tracking into memory instrumentation s…

f6cb2ff

…ervice.

CL3: Fix move of detailed_stats_kb in CreatePublicOSDump

6a3ce93

CL4: Add browser tests for granular memory tracking.

d3c2e1b

Apply fixes to granular_memory_browsertest.cc.

dd02f9e

Restore original granular_memory_browsertest.cc without Cobalt depend…

315cee2

…encies.

Awallky requested a review from a team as a code owner April 14, 2026 04:58

Awallky requested a review from y4vor April 14, 2026 04:58

gemini-code-assist bot reviewed Apr 14, 2026

View reviewed changes

Awallky changed the title ~~Feature/granular memory cl4 verification~~ cobalt/metrics: Add browser tests for granular memory tracking Apr 14, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cobalt/metrics: Add browser tests for granular memory tracking#10020

cobalt/metrics: Add browser tests for granular memory tracking#10020
Awallky wants to merge 8 commits intoyoutube:mainfrom
Awallky:feature/granular-memory-cl4-verification

Awallky commented Apr 14, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Apr 14, 2026

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

gemini-code-assist bot Apr 14, 2026

Uh oh!

gemini-code-assist bot Apr 14, 2026

Uh oh!

gemini-code-assist bot Apr 14, 2026

Uh oh!

gemini-code-assist bot Apr 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

Awallky commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 14, 2026

🤖 Gemini Suggested Commit Message

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Awallky commented Apr 14, 2026 •

edited

Loading