[memory] Move process memory info to base [chromium/src : main]
0 views
Skip to first unread message
Etienne Pierre-Doray (Gerrit)
Nov 22, 2024, 12:02:54 PM11/22/24
to Gabriel Charette, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Zijie He, asvitki...@chromium.org, chromiumme...@microsoft.com, creis...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, yhanada+...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, chrome-gr...@chromium.org, fuchsia...@chromium.org, mac-r...@chromium.org
Attention needed from Gabriel Charette
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Related details
Attention is currently required from:
- Gabriel Charette
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Nov 22, 2024, 3:26:34 PM11/22/24
to Etienne Pierre-Doray, Gabriel Charette, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Zijie He, asvitki...@chromium.org, chromiumme...@microsoft.com, creis...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, yhanada+...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, chrome-gr...@chromium.org, fuchsia...@chromium.org, mac-r...@chromium.org
Attention needed from Etienne Pierre-Doray
Gabriel Charette added 10 comments![]( "Open in Gerrit")
Patchset-level comments
File base/process/process_metrics.h
Line 143, Patchset 18 (Latest):
// have platform-specific restrictions:Gabriel Charette . unresolved
has
Line 96, Patchset 18 (Latest):
uint64_t private_bytes;Gabriel Charette . unresolved
Can't we have PMF on all platforms?
Line 83, Patchset 18 (Latest):
uint64_t phys_footprint_bytes;Gabriel Charette . unresolved
physical? let's avoid abbreviations per style-guide
File base/process/process_metrics_linux.cc
Line 152, Patchset 18 (Latest):
ProcessMetrics::GetMemoryInfo() const {Gabriel Charette . unresolved
Document in the CL description that this changes Linux's FillOSMemoryDump from `GetResidentAndSharedPagesFromStatmFile()` to `ReadProcFileToTrimmedStringPairs()`. So someone can find this CL if it breaks some memory infra.
File chrome/browser/ash/arc/memory_pressure/container_app_killer.cc
Line 159, Patchset 18 (Latest):
if (!info.has_value()) {
return 0;
}
return (info->vm_swap_bytes + info->resident_set_bytes) / 1024;Gabriel Charette . unresolved
How about using a single ternary statement for such returns (ditto elsewhere)
```
return info.has_value() ?
(info->vm_swap_bytes + info->resident_set_bytes) / 1024 :
0;
```
File services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics.cc
Line 39, Patchset 18 (Latest):
auto process_metrics = [&]() {
if (handle == base::kNullProcessHandle) {
return base::ProcessMetrics::CreateCurrentProcessMetrics();
}
return base::ProcessMetrics::CreateProcessMetrics(handle);
}();Gabriel Charette . unresolved
ternary here as well?
File services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_mac.cc
Line 62, Patchset 18 (Latest):
"task_vm_info must be small enough for 10.14 MIG interfaces");Gabriel Charette . unresolved
Why does a copy of this+above also stick here?
Related details
Attention is currently required from:
- Etienne Pierre-Doray
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Nov 25, 2024, 11:17:04 AM11/25/24
to Gabriel Charette, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Zijie He, asvitki...@chromium.org, chromiumme...@microsoft.com, creis...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, yhanada+...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, chrome-gr...@chromium.org, fuchsia...@chromium.org, mac-r...@chromium.org
Attention needed from Gabriel Charette
Etienne Pierre-Doray added 10 comments![]( "Open in Gerrit")
Patchset-level comments
File base/process/process_metrics.h
Line 143, Patchset 18:
// have platform-specific restrictions:Gabriel Charette . resolved
Etienne Pierre-Dorayhas
Done
Line 96, Patchset 18:
uint64_t private_bytes;Gabriel Charette . unresolved
Can't we have PMF on all platforms?
Etienne Pierre-Doray
I intentionally didn't expose this as PMF, and exposed the platform specific bits required to compute PMF in memory_instrumentation. This is because:
- The caller forwards those bits through mojo (see https://source.chromium.org/chromium/chromium/src/+/main:services/resource_coordinator/public/mojom/memory_instrumentation/memory_instrumentation.mojom;l=128?q=PlatformPrivateFootprint%20-file:out&ss=chromium)
- I also want to expose the platform specific counters in traces.
physical? let's avoid abbreviations per style-guide
Etienne Pierre-Doray
Done
File base/process/process_metrics_linux.cc
Document in the CL description that this changes Linux's FillOSMemoryDump from `GetResidentAndSharedPagesFromStatmFile()` to `ReadProcFileToTrimmedStringPairs()`. So someone can find this CL if it breaks some memory infra.
Etienne Pierre-Doray
Done
File chrome/browser/ash/arc/memory_pressure/container_app_killer.cc
Line 159, Patchset 18:
if (!info.has_value()) {
return 0;
}
return (info->vm_swap_bytes + info->resident_set_bytes) / 1024;Gabriel Charette . resolved
How about using a single ternary statement for such returns (ditto elsewhere)
```
return info.has_value() ?
(info->vm_swap_bytes + info->resident_set_bytes) / 1024 :
0;
```
Etienne Pierre-Doray
Done
File services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics.cc
Line 39, Patchset 18:
auto process_metrics = [&]() {
if (handle == base::kNullProcessHandle) {
return base::ProcessMetrics::CreateCurrentProcessMetrics();
}
return base::ProcessMetrics::CreateProcessMetrics(handle);
}();Gabriel Charette . resolved
Etienne Pierre-Dorayternary here as well?
Done
File services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_mac.cc
Line 62, Patchset 18:
"task_vm_info must be small enough for 10.14 MIG interfaces");Gabriel Charette . resolved
Why does a copy of this+above also stick here?
Attention is currently required from:
- Gabriel Charette
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Nov 26, 2024, 5:19:35 PM11/26/24
to Etienne Pierre-Doray, Gabriel Charette, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Zijie He, asvitki...@chromium.org, chromiumme...@microsoft.com, creis...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, yhanada+...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, chrome-gr...@chromium.org, fuchsia...@chromium.org, mac-r...@chromium.org
Attention needed from Etienne Pierre-Doray
Gabriel Charette voted and added 2 comments![]( "Open in Gerrit")
Votes added by Gabriel Charette
| Code-Review | +1 |
2 comments
File base/process/process_metrics.h
Line 96, Patchset 18:
uint64_t private_bytes;Gabriel Charette . resolved
Etienne Pierre-DorayCan't we have PMF on all platforms?
I intentionally didn't expose this as PMF, and exposed the platform specific bits required to compute PMF in memory_instrumentation. This is because:
- The caller forwards those bits through mojo (see https://source.chromium.org/chromium/chromium/src/+/main:services/resource_coordinator/public/mojom/memory_instrumentation/memory_instrumentation.mojom;l=128?q=PlatformPrivateFootprint%20-file:out&ss=chromium)
- I also want to expose the platform specific counters in traces.
Gabriel Charette
Acknowledged
File services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_mac.cc
Line 280, Patchset 21 (Parent):
// The |phys_footprint| field was introduced in 10.11.Gabriel Charette . unresolved
Mention drop of support for this conditional in the CL description.
Where did you get the information about the min-supported MacOSX version? Refer to that too in the CL description (if it's public info)
Related details
Attention is currently required from:
- Etienne Pierre-Doray
Submit Requirements:
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Nov 27, 2024, 2:06:54 PM11/27/24
to Primiano Tucci, Gabriel Charette, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Zijie He, asvitki...@chromium.org, chromiumme...@microsoft.com, creis...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, yhanada+...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, chrome-gr...@chromium.org, fuchsia...@chromium.org, mac-r...@chromium.org
Attention needed from Primiano Tucci
Etienne Pierre-Doray added 2 comments![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 24 (Latest):
Etienne Pierre-Doray . resolved
+primiano@ for services/resource_coordinator
File services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_mac.cc
Line 280, Patchset 21 (Parent):
// The |phys_footprint| field was introduced in 10.11.Gabriel Charette . resolved
Mention drop of support for this conditional in the CL description.
Where did you get the information about the min-supported MacOSX version? Refer to that too in the CL description (if it's public info)
Etienne Pierre-Doray
Relevant CL:
https://chromium-review.googlesource.com/c/chromium/src/+/2025948
That CL removes support for 10.11, but for some reason kept the conditional.
I updated the CL description linking the bug.
Related details
Attention is currently required from:
- Primiano Tucci
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Primiano Tucci (Gerrit)
Dec 2, 2024, 5:00:24 AM12/2/24
to Etienne Pierre-Doray, Gabriel Charette, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Zijie He, asvitki...@chromium.org, chromiumme...@microsoft.com, creis...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, yhanada+...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, chrome-gr...@chromium.org, fuchsia...@chromium.org, mac-r...@chromium.org
Attention needed from Etienne Pierre-Doray
Primiano Tucci voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Etienne Pierre-Doray
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gabriel Charette (Gerrit)
Dec 2, 2024, 10:19:57 AM12/2/24
to Etienne Pierre-Doray, Gabriel Charette, Primiano Tucci, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Zijie He, asvitki...@chromium.org, chromiumme...@microsoft.com, creis...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, yhanada+...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, chrome-gr...@chromium.org, fuchsia...@chromium.org, mac-r...@chromium.org
Attention needed from Etienne Pierre-Doray
Gabriel Charette voted and added 1 comment![]( "Open in Gerrit")
Votes added by Gabriel Charette
| Code-Review | +1 |
| Owners-Override | +1 |
1 comment
Patchset-level comments
Gabriel Charette . resolved
O-O for side-effects outside //base and resource_coordinator/
Related details
Attention is currently required from:
- Etienne Pierre-Doray
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Dec 2, 2024, 10:21:19 AM12/2/24
to Gabriel Charette, Primiano Tucci, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, Zijie He, asvitki...@chromium.org, chromiumme...@microsoft.com, creis...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, yhanada+...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, chrome-gr...@chromium.org, fuchsia...@chromium.org, mac-r...@chromium.org
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Dec 2, 2024, 11:09:44 AM12/2/24
to Etienne Pierre-Doray, Gabriel Charette, Primiano Tucci, AyeAye, chromium...@chromium.org, Zijie He, asvitki...@chromium.org, chromiumme...@microsoft.com, creis...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, yhanada+...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, chrome-gr...@chromium.org, fuchsia...@chromium.org, mac-r...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
[memory] Move process memory info to base
This will be re-used in a follow-up in SystemMetricsSampler.
This should also replaced copied code in blink::MemoryUsageMonitor.
This changes Linux's FillOSMemoryDump from GetResidentAndSharedPagesFromStatmFile() (proc/pid/statm) to ReadProcFileToTrimmedStringPairs() (proc/pid/status)
This drops supports for pre MAC_OS_X_VERSION_10_11 missing
phys_footprint: https://issues.chromium.org/issues/40669926
Change-Id: I2002e69abbe852e82d0d1cb37017e77602538d6b
Reviewed-by: Primiano Tucci <prim...@chromium.org>
Reviewed-by: Gabriel Charette <g...@chromium.org>
Owners-Override: Gabriel Charette <g...@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etie...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1390345}
Files:
- M base/process/process_metrics.h
- M base/process/process_metrics_apple.cc
- M base/process/process_metrics_fuchsia.cc
- M base/process/process_metrics_linux.cc
- M base/process/process_metrics_win.cc
- M chrome/browser/ash/arc/memory_pressure/container_app_killer.cc
- M chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc
- M chrome/browser/resource_coordinator/utils.cc
- M chrome/browser/task_manager/sampling/task_group_sampler.cc
- M components/services/heap_profiling/json_exporter_unittest.cc
- M content/browser/renderer_host/render_process_host_impl.cc
- M services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc
- M services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics.cc
- M services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics.h
- M services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_fuchsia.cc
- M services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_linux.cc
- M services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_mac.cc
- M services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_unittest.cc
- M services/resource_coordinator/public/cpp/memory_instrumentation/os_metrics_win.cc
Change size: L
Delta: 19 files changed, 265 insertions(+), 192 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Primiano Tucci, +1 by Gabriel Charette
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Lukaszewicz (Gerrit)
Dec 2, 2024, 1:40:18 PM12/2/24
to Etienne Pierre-Doray, Chromium LUCI CQ, Gabriel Charette, Primiano Tucci, AyeAye, chromium...@chromium.org, Zijie He, asvitki...@chromium.org, chromiumme...@microsoft.com, creis...@chromium.org, alexmo...@chromium.org, navigation...@chromium.org, yhanada+...@chromium.org, arc-review...@google.com, hidehik...@chromium.org, chrome-gr...@chromium.org, fuchsia...@chromium.org, mac-r...@chromium.org
Thomas Lukaszewicz has created a revert of this change![]( "Open in Gerrit")
Related details
Attention set is empty
Submit Requirements:
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages