Skip to content

cobalt/metrics: Add interfaces and stubs for granular memory tracking. #10017

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Awallky wants to merge 2 commits into
base: main
Choose a base branch
from
Open

cobalt/metrics: Add interfaces and stubs for granular memory tracking. #10017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e77d063
CL1: Add interfaces and stubs for granular memory tracking.
Awallky Apr 13, 2026
764f524
CL1: Update DetailedMetricsDelegate interface
Awallky Apr 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions services/resource_coordinator/public/cpp/memory_instrumentation/detailed_metrics_delegate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// Copyright 2026 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_MEMORY_INSTRUMENTATION_DETAILED_METRICS_DELEGATE_H_
#define SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_MEMORY_INSTRUMENTATION_DETAILED_METRICS_DELEGATE_H_

#include <cstdint>
#include <string>
#include <string_view>

#include "base/component_export.h"
#include "base/containers/flat_map.h"

#include "base/memory/weak_ptr.h"
#include "third_party/abseil-cpp/absl/strings/string_view.h"

namespace memory_instrumentation {

// Optimized structure for smaps metrics.
struct COMPONENT_EXPORT(RESOURCE_COORDINATOR_PUBLIC_MEMORY_INSTRUMENTATION)
SmapsMetrics {
uint64_t rss_kb = 0;
uint64_t pss_kb = 0;
uint64_t private_dirty_kb = 0;
uint64_t private_clean_kb = 0;
uint64_t shared_dirty_kb = 0;
uint64_t shared_clean_kb = 0;
uint64_t swap_kb = 0;
uint64_t swap_pss_kb = 0;
};

// Interface for project-specific detailed memory metrics collection.
class COMPONENT_EXPORT(RESOURCE_COORDINATOR_PUBLIC_MEMORY_INSTRUMENTATION)
DetailedMetricsDelegate {
public:
virtual ~DetailedMetricsDelegate() = default;

// Called for each parsed entry. |name| is transient and must be copied if
// needed. Implementations must be thread-safe.
virtual void OnSmapsEntry(absl::string_view name,
const SmapsMetrics& metrics) = 0;

// Swaps the internal results map with an empty one and returns it via |stats|.
virtual void GetAndResetStats(base::flat_map<std::string, uint64_t>& stats) = 0;

virtual base::WeakPtr<DetailedMetricsDelegate> GetWeakPtr() = 0;
};

} // namespace memory_instrumentation

#endif // SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_MEMORY_INSTRUMENTATION_DETAILED_METRICS_DELEGATE_H_
22 changes: 22 additions & 0 deletions services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,16 @@

#include "base/component_export.h"
#include "base/functional/callback_forward.h"
#include "base/memory/weak_ptr.h"
#include "base/trace_event/memory_dump_request_args.h"
#include "mojo/public/cpp/bindings/shared_remote.h"
#include "services/resource_coordinator/public/cpp/memory_instrumentation/global_memory_dump.h"
#include "services/resource_coordinator/public/mojom/memory_instrumentation/memory_instrumentation.mojom.h"

#if BUILDFLAG(IS_COBALT)
#include "base/synchronization/lock.h"
#endif

namespace memory_instrumentation {

// This a public API for the memory-infra service and allows any
Expand Down Expand Up @@ -100,6 +105,16 @@ class COMPONENT_EXPORT(RESOURCE_COORDINATOR_PUBLIC_MEMORY_INSTRUMENTATION)
MemoryDumpDeterminism,
RequestGlobalMemoryDumpAndAppendToTraceCallback);

#if BUILDFLAG(IS_COBALT)
// Set the delegate for detailed metrics collection.
void SetDetailedMetricsDelegate(class DetailedMetricsDelegate* delegate);

// Get the registered delegate.
class DetailedMetricsDelegate* GetDetailedMetricsDelegate() const;
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Returning a raw pointer to the delegate from GetDetailedMetricsDelegate() is potentially unsafe, even with the use of detailed_metrics_delegate_lock_. Once the lock is released, the caller holds a pointer that could be nullified or the underlying object destroyed by another thread (e.g., via a call to SetDetailedMetricsDelegate). Consider a safer access pattern, such as passing the delegate directly to the methods that require it, or using a callback/observer pattern if the lifetime cannot be strictly guaranteed.

#endif

base::WeakPtr<MemoryInstrumentation> GetWeakPtr();

private:
explicit MemoryInstrumentation(
mojo::PendingRemote<memory_instrumentation::mojom::Coordinator>
Expand All @@ -112,6 +127,13 @@ class COMPONENT_EXPORT(RESOURCE_COORDINATOR_PUBLIC_MEMORY_INSTRUMENTATION)

// Only browser process is allowed to request memory dumps.
const bool is_browser_process_;

#if BUILDFLAG(IS_COBALT)
mutable base::Lock detailed_metrics_delegate_lock_;
class DetailedMetricsDelegate* detailed_metrics_delegate_ = nullptr;
#endif

base::WeakPtrFactory<MemoryInstrumentation> weak_ptr_factory_{this};
};

} // namespace memory_instrumentation
Expand Down
22 changes: 21 additions & 1 deletion services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright 2017 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_MEMORY_INSTRUMENTATION_OS_METRICS_H_
#define SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_MEMORY_INSTRUMENTATION_OS_METRICS_H_

Expand Down Expand Up @@ -52,7 +53,8 @@ class COMPONENT_EXPORT(
// the current process is used
static bool FillOSMemoryDump(base::ProcessHandle handle,
const MemDumpFlagSet& flags,
mojom::RawOSMemDump* dump);
mojom::RawOSMemDump* dump,
class DetailedMetricsDelegate* delegate = nullptr);
#if BUILDFLAG(IS_APPLE)
static bool FillOSMemoryDump(base::ProcessHandle handle,
const MemDumpFlagSet& flags,
Expand All @@ -64,13 +66,31 @@ class COMPONENT_EXPORT(
mojom::RawOSMemDump*);
static std::vector<mojom::VmRegionPtr> GetProcessMemoryMaps(
base::ProcessHandle);
static std::vector<mojom::VmRegionPtr> GetProcessMemoryMaps(
const std::string& smaps_content);

#if BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS) || BUILDFLAG(IS_ANDROID)
static void SetProcSmapsForTesting(FILE*);
#if BUILDFLAG(IS_COBALT)
static void SetSmapsRollupForTesting(FILE*);

// Set the delegate for detailed metrics collection. This must be called
// before FillOSMemoryDump with MEM_DUMP_DETAILED_STATS.
static void SetDetailedMetricsDelegate(
class DetailedMetricsDelegate* delegate);
Comment on lines +79 to +80
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The addition of a static SetDetailedMetricsDelegate method introduces global state into the OSMetrics utility class. Since FillOSMemoryDump has been updated to accept a DetailedMetricsDelegate* as a parameter (line 57), this static setter is redundant and creates ambiguity regarding which delegate is used. It is better to pass the delegate explicitly through the call chain to keep OSMetrics stateless and easier to test, consistent with Chromium's preference for avoiding global state in utility classes.

References
  1. Follow Chromium best practices by avoiding global state in utility classes and preferring explicit dependency injection via parameters. (link)

#endif
#endif // BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS) ||
// BUILDFLAG(IS_ANDROID)

private:
#if BUILDFLAG(IS_COBALT)
static bool FillDetailedMetrics(base::ProcessHandle handle,
const MemDumpFlagSet& flags,
mojom::RawOSMemDump* dump,
class DetailedMetricsDelegate* delegate);
static bool ReadDetailedMetricsFile(base::ProcessHandle handle,
class DetailedMetricsDelegate* delegate);
#endif
FRIEND_TEST_ALL_PREFIXES(OSMetricsTest, ParseProcSmaps);
FRIEND_TEST_ALL_PREFIXES(OSMetricsTest, TestWinModuleReading);
FRIEND_TEST_ALL_PREFIXES(OSMetricsTest, TestMachOReading);
Expand Down
4 changes: 3 additions & 1 deletion services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_linux.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#endif

#include "services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics.h"
#include "services/resource_coordinator/public/cpp/memory_instrumentation/detailed_metrics_delegate.h"

#include <dlfcn.h>
#include <fcntl.h>
Expand Down Expand Up @@ -387,7 +388,8 @@ void OSMetrics::SetProcSmapsForTesting(FILE* f) {
// static
bool OSMetrics::FillOSMemoryDump(base::ProcessHandle handle,
const MemDumpFlagSet& flags,
mojom::RawOSMemDump* dump) {
mojom::RawOSMemDump* dump,
DetailedMetricsDelegate* /*delegate*/) {
auto info = GetMemoryInfo(handle);
if (!info.has_value()) {
return false;
Expand Down
4 changes: 4 additions & 0 deletions ...ces/resource_coordinator/public/mojom/memory_instrumentation/memory_instrumentation.mojom
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,9 +226,11 @@ struct RawOSMemDump {
uint32 stacks_rss_kb = 0;

// Generic map for detailed memory metrics.
[EnableIf=is_cobalt_with_libchrobalt_metrics]
map<string, uint32>? detailed_stats_kb;

// Timestamp of the last successful detailed memory dump.
[EnableIf=is_cobalt_with_libchrobalt_metrics]
mojo_base.mojom.TimeTicks last_detailed_dump_time;
};

Expand Down Expand Up @@ -321,9 +323,11 @@ struct OSMemDump {
uint32 stacks_rss_kb = 0;

// Generic map for detailed memory metrics.
[EnableIf=is_cobalt_with_libchrobalt_metrics]
map<string, uint32>? detailed_stats_kb;

// Timestamp of the last successful detailed memory dump.
[EnableIf=is_cobalt_with_libchrobalt_metrics]
mojo_base.mojom.TimeTicks last_detailed_dump_time;
};

Expand Down
Loading