Skip to content

GitLab

T
tangled
Commit b006d855 authored by Marc Treib's avatar Marc Treib Committed by Commit Bot
Browse files
Options

Revert "RC: Add memory measurement to render process probe." 

This reverts commit 6c3cc295.

Reason for revert: ResourceCoordinatorRenderProcessProbeBrowserTest.TrackAndMeasureActiveRenderProcesses fails on Mac Asan 64:
https://ci.chromium.org/buildbot/chromium.memory/Mac%20ASan%2064%20Tests%20%281%29/40456
https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.memory%2FMac_ASan_64_Tests__1_%2F40456%2F%2B%2Frecipes%2Fsteps%2Fbrowser_tests%2F0%2Flogs%2FResourceCoordinatorRenderProcessProbeBrowserTest.TrackAndMeasureActiveRenderProcesses%2F0

../../chrome/browser/resource_coordinator/resource_coordinator_render_process_probe_browsertest.cc:171: Failure
Expected: (0u) != (measurement->private_footprint_kb), actual: 0 vs 0

Original change's description:
> RC: Add memory measurement to render process probe.
> 
> Bug: 755840
> Change-Id: I96af63e817abe9bdefb0916da4d652e97c620a2c
> Reviewed-on: https://chromium-review.googlesource.com/1037631
> Commit-Queue: Chris Hamilton <chrisha@chromium.org>
> Reviewed-by: Chris Hamilton <chrisha@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#555844}

TBR=chrisha@chromium.org,fdoray@chromium.org,erikchen@chromium.org,siggi@chromium.org

Change-Id: If2e102252f43c0b62b6a539e0df7793ee05281bc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 755840
Reviewed-on: https://chromium-review.googlesource.com/1043805Reviewed-by: default avatarMarc Treib <treib@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556009}
parent d29602fc
Hide whitespace changes
Inline Side-by-side
Showing with 22 additions and 65 deletions
chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.cc
View file @ b006d855
......@@ -7,6 +7,7 @@
#include <vector>
#include "base/bind.h"
#include "base/values.h"
#include "build/build_config.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/render_process_host.h"
......@@ -26,6 +27,9 @@ namespace {
constexpr base::TimeDelta kDefaultMeasurementInterval =
base::TimeDelta::FromMinutes(10);
base::LazyInstance<ResourceCoordinatorRenderProcessProbe>::DestructorAtExit
g_probe = LAZY_INSTANCE_INITIALIZER;
} // namespace
ResourceCoordinatorRenderProcessProbe::RenderProcessInfo::RenderProcessInfo() =
......@@ -45,8 +49,7 @@ ResourceCoordinatorRenderProcessProbe::
// static
ResourceCoordinatorRenderProcessProbe*
ResourceCoordinatorRenderProcessProbe::GetInstance() {
static base::NoDestructor<ResourceCoordinatorRenderProcessProbe> probe;
return probe.get();
return g_probe.Pointer();
}
// static
......@@ -129,24 +132,16 @@ void ResourceCoordinatorRenderProcessProbe::
content::BrowserThread::PostTask(
content::BrowserThread::IO, FROM_HERE,
base::BindOnce(
&ResourceCoordinatorRenderProcessProbe::
CollectRenderProcessMetricsAndStartMemoryDumpOnIOThread,
base::Unretained(this)));
base::BindOnce(&ResourceCoordinatorRenderProcessProbe::
CollectAndDispatchRenderProcessMetricsOnIOThread,
base::Unretained(this)));
}
void ResourceCoordinatorRenderProcessProbe::
CollectRenderProcessMetricsAndStartMemoryDumpOnIOThread() {
CollectAndDispatchRenderProcessMetricsOnIOThread() {
DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
DCHECK(is_gathering_);
// Dispatch the memory collection request.
memory_instrumentation::MemoryInstrumentation::GetInstance()
->RequestGlobalDump(
base::BindRepeating(&ResourceCoordinatorRenderProcessProbe::
ProcessGlobalMemoryDumpAndDispatchOnIOThread,
base::Unretained(this)));
RenderProcessInfoMap::iterator iter = render_process_info_map_.begin();
while (iter != render_process_info_map_.end()) {
auto& render_process_info = iter->second;
......@@ -162,21 +157,12 @@ void ResourceCoordinatorRenderProcessProbe::
continue;
}
}
}
void ResourceCoordinatorRenderProcessProbe::
ProcessGlobalMemoryDumpAndDispatchOnIOThread(
bool global_success,
std::unique_ptr<memory_instrumentation::GlobalMemoryDump> dump) {
// Create the measurement batch.
mojom::ProcessResourceMeasurementBatchPtr batch =
mojom::ProcessResourceMeasurementBatch::New();
// TODO(siggi): Add start/end times. This will need some support from
// the memory_instrumentation code.
// Start by adding the render process hosts we know about to the batch.
for (const auto& render_process_info_map_entry : render_process_info_map_) {
for (auto& render_process_info_map_entry : render_process_info_map_) {
auto& render_process_info = render_process_info_map_entry.second;
// TODO(oysteine): Move the multiplier used to avoid precision loss
// into a shared location, when this property gets used.
......@@ -185,35 +171,13 @@ void ResourceCoordinatorRenderProcessProbe::
measurement->pid = render_process_info.process.Pid();
measurement->cpu_usage = render_process_info.cpu_usage;
// TODO(siggi): Add the private footprint.
batch->measurements.push_back(std::move(measurement));
}
if (dump) {
// Then amend the ones we have memory metrics for with their private
// footprint. The global dump may contain non-renderer processes, it may
// contain renderer processes we didn't capture at the start of the cycle,
// and it may not contain all the renderer processes we know about.
// This may happen due to the inherent race between the request and
// starting/stopping renderers, or because of other failures
// This may therefore provide incomplete information.
for (const auto& dump_entry : dump->process_dumps()) {
base::ProcessId pid = dump_entry.pid();
for (const auto& measurement : batch->measurements) {
if (measurement->pid == pid) {
measurement->private_footprint_kb =
dump_entry.os_dump().private_footprint_kb;
break;
}
}
}
} else {
// We should only get a nullptr in case of failure.
DCHECK(!global_success);
}
bool should_restart = DispatchMetrics(std::move(batch));
content::BrowserThread::PostTask(
content::BrowserThread::UI, FROM_HERE,
base::BindOnce(
......
chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.h
View file @ b006d855
......@@ -9,12 +9,11 @@
#include <memory>
#include <utility>
#include "base/lazy_instance.h"
#include "base/macros.h"
#include "base/no_destructor.h"
#include "base/process/process.h"
#include "base/process/process_metrics.h"
#include "base/timer/timer.h"
#include "services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h"
#include "services/resource_coordinator/public/cpp/system_resource_coordinator.h"
namespace resource_coordinator {
......@@ -42,7 +41,6 @@ class ResourceCoordinatorRenderProcessProbe {
void StartSingleGather();
protected:
// Internal state protected for testing.
struct RenderProcessInfo {
RenderProcessInfo();
~RenderProcessInfo();
......@@ -53,7 +51,9 @@ class ResourceCoordinatorRenderProcessProbe {
};
using RenderProcessInfoMap = std::map<int, RenderProcessInfo>;
friend class base::NoDestructor<ResourceCoordinatorRenderProcessProbe>;
// Internal state protected for testing.
friend struct base::LazyInstanceTraitsBase<
ResourceCoordinatorRenderProcessProbe>;
ResourceCoordinatorRenderProcessProbe();
virtual ~ResourceCoordinatorRenderProcessProbe();
......@@ -61,13 +61,10 @@ class ResourceCoordinatorRenderProcessProbe {
// (1) Identify all of the render processes that are active to measure.
// Child render processes can only be discovered in the browser's UI thread.
void RegisterAliveRenderProcessesOnUIThread();
// (2) Collect the render process CPU metrics and initiate a memory dump.
void CollectRenderProcessMetricsAndStartMemoryDumpOnIOThread();
// (3) Process the results of the memory dump and dispatch the results.
void ProcessGlobalMemoryDumpAndDispatchOnIOThread(
bool success,
std::unique_ptr<memory_instrumentation::GlobalMemoryDump> dump);
// (4) Initiate the next render process metrics collection cycle if the
// (2) Collect and dispatch the render process metrics to the system
// coordination unit.
void CollectAndDispatchRenderProcessMetricsOnIOThread();
// (3) Initiate the next render process metrics collection cycle if the
// cycle has been started and |restart_cycle| is true, which consists of a
// delayed call to perform (1) via a timer.
// Virtual for testing.
......
chrome/browser/resource_coordinator/resource_coordinator_render_process_probe_browsertest.cc
View file @ b006d855
......@@ -135,10 +135,8 @@ IN_PROC_BROWSER_TEST_F(ResourceCoordinatorRenderProcessProbeBrowserTest,
EXPECT_EQ(initial_size, probe.last_measurement_batch()->measurements.size());
// A quirk of the process_metrics implementation is that the first CPU
// measurement returns zero.
for (const auto& measurement : probe.last_measurement_batch()->measurements) {
for (const auto& measurement : probe.last_measurement_batch()->measurements)
EXPECT_EQ(0.0, measurement->cpu_usage);
EXPECT_NE(0u, measurement->private_footprint_kb);
}
// Open a second tab and complete a navigation.
ui_test_utils::NavigateToURLWithDisposition(
......@@ -166,10 +164,8 @@ IN_PROC_BROWSER_TEST_F(ResourceCoordinatorRenderProcessProbeBrowserTest,
size_t info_map_size = info_map.size();
probe.StartGatherCycleAndWait();
// The second and subsequent CPU measurements should return some data.
for (const auto& measurement : probe.last_measurement_batch()->measurements) {
for (const auto& measurement : probe.last_measurement_batch()->measurements)
EXPECT_LE(0.0, measurement->cpu_usage);
EXPECT_NE(0u, measurement->private_footprint_kb);
}
EXPECT_EQ(info_map_size, info_map.size());
for (const auto& entry : probe.render_process_info_map()) {
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment