cobalt/metrics: Add interfaces and stubs for granular memory tracking.#10017
cobalt/metrics: Add interfaces and stubs for granular memory tracking.#10017Awallky wants to merge 1 commit intoyoutube:mainfrom
Conversation
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
There was a problem hiding this comment.
Code Review
This pull request introduces the DetailedMetricsDelegate interface and DetailedMetrics structure to enable project-specific memory metrics collection. It updates MemoryInstrumentation and OSMetrics to integrate this delegate and adds conditional Mojo fields for detailed stats. Key feedback includes providing definitions for the DetailedMetrics lifecycle methods to prevent linker errors, removing redundant global state in OSMetrics in favor of explicit parameter passing, and addressing thread-safety concerns regarding raw pointer access to the delegate.
| DetailedMetrics(); | ||
| ~DetailedMetrics(); | ||
| DetailedMetrics(DetailedMetrics&&); | ||
| DetailedMetrics& operator=(DetailedMetrics&&); |
There was a problem hiding this comment.
The DetailedMetrics struct declares a constructor, destructor, and move operations but does not provide definitions. Since this is a new header and no corresponding .cc file is included in the PR, this will result in linker errors. If these are intended to be default implementations, they should be defined as = default in the header. Additionally, per Chromium style, move operations should be marked noexcept to allow for optimizations when used in containers.
DetailedMetrics() = default;
~DetailedMetrics() = default;
DetailedMetrics(DetailedMetrics&&) noexcept = default;
DetailedMetrics& operator=(DetailedMetrics&&) noexcept = default;References
- Adhere to established upstream Chromium style guides for C++ code, including the use of
= defaultandnoexceptfor move operations. (link)
| static void SetDetailedMetricsDelegate( | ||
| class DetailedMetricsDelegate* delegate); |
There was a problem hiding this comment.
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
- Follow Chromium best practices by avoiding global state in utility classes and preferring explicit dependency injection via parameters. (link)
| void SetDetailedMetricsDelegate(class DetailedMetricsDelegate* delegate); | ||
|
|
||
| // Get the registered delegate. | ||
| class DetailedMetricsDelegate* GetDetailedMetricsDelegate() const; |
There was a problem hiding this comment.
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.
This commit introduces the
DetailedMetricsDelegateinterface and necessarystubs in
OSMetricsto support custom memory categorization in Cobalt.It also includes a Starboard wrapper for
prctlto support VMA naming.DetailedMetricsDelegateinterface.OSMetricsfor delegate interaction.prctl(VMA naming).Bug: 494004530