Commit 085c83e1 authored by Peiyong Lin's avatar Peiyong Lin Committed by Commit Bot

[GRC] Remove propagation code.

Previously, properties like CPU usage and expected task queueing duration are
propagated to PageCU from ProcessCU. At first we wrote the code this way
because we thought it could avoid duplicate calculation when multiple
components need to access the same property. However, this adds extra
complexity and currently we don't have properties shared by multiple
components. Thus, this patch removes the code and replace it with simple getter
APIs in PageCU.

BUG=775691

Change-Id: I441b01a68e8407f044b8b81d7d0753b52a363bf4
Reviewed-on: https://chromium-review.googlesource.com/747881Reviewed-by: default avatarZhen Wang <zhenw@chromium.org>
Commit-Queue: lpy <lpy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513312}
parent 69656abe
......@@ -120,7 +120,6 @@ void CoordinationUnitBase::SetProperty(mojom::PropertyType property_type,
// and propagated to the appropriate associated |CoordianationUnitBase|
// before |OnPropertyChanged| is invoked on all of the registered observers.
properties_[property_type] = value;
PropagateProperty(property_type, value);
OnPropertyChanged(property_type, value);
}
......
......@@ -32,9 +32,6 @@ class CoordinationUnitBase {
static void AssertNoActiveCoordinationUnits();
static void ClearAllCoordinationUnits();
// Recalculate property internally.
virtual void RecalculateProperty(const mojom::PropertyType property_type) {}
CoordinationUnitBase(const CoordinationUnitID& id);
virtual ~CoordinationUnitBase();
......@@ -64,9 +61,6 @@ class CoordinationUnitBase {
static std::vector<CoordinationUnitBase*> GetCoordinationUnitsOfType(
CoordinationUnitType cu_type);
// Propagate property change to relevant |CoordinationUnitBase| instances.
virtual void PropagateProperty(mojom::PropertyType property_type,
int64_t value) {}
virtual void OnEventReceived(mojom::Event event);
virtual void OnPropertyChanged(mojom::PropertyType property_type,
int64_t value);
......
......@@ -63,25 +63,6 @@ void PageCoordinationUnitImpl::OnMainFrameNavigationCommitted() {
SendEvent(mojom::Event::kNavigationCommitted);
}
void PageCoordinationUnitImpl::RecalculateProperty(
const mojom::PropertyType property_type) {
switch (property_type) {
case mojom::PropertyType::kCPUUsage: {
double cpu_usage = CalculateCPUUsage();
SetProperty(mojom::PropertyType::kCPUUsage, cpu_usage);
break;
}
case mojom::PropertyType::kExpectedTaskQueueingDuration: {
int64_t eqt;
if (CalculateExpectedTaskQueueingDuration(&eqt))
SetProperty(property_type, eqt);
break;
}
default:
NOTREACHED();
}
}
std::set<ProcessCoordinationUnitImpl*>
PageCoordinationUnitImpl::GetAssociatedProcessCoordinationUnits() const {
std::set<ProcessCoordinationUnitImpl*> process_cus;
......@@ -101,6 +82,38 @@ bool PageCoordinationUnitImpl::IsVisible() const {
return is_visible;
}
double PageCoordinationUnitImpl::GetCPUUsage() const {
double cpu_usage = 0.0;
for (auto* process_cu : GetAssociatedProcessCoordinationUnits()) {
size_t pages_in_process =
process_cu->GetAssociatedPageCoordinationUnits().size();
DCHECK_LE(1u, pages_in_process);
int64_t process_cpu_usage;
if (process_cu->GetProperty(mojom::PropertyType::kCPUUsage,
&process_cpu_usage)) {
cpu_usage += static_cast<double>(process_cpu_usage) / pages_in_process;
}
}
return cpu_usage / 1000;
}
bool PageCoordinationUnitImpl::GetExpectedTaskQueueingDuration(
int64_t* output) {
// Calculate the EQT for the process of the main frame only because
// the smoothness of the main frame may affect the users the most.
FrameCoordinationUnitImpl* main_frame_cu = GetMainFrameCoordinationUnit();
if (!main_frame_cu)
return false;
auto* process_cu = main_frame_cu->GetProcessCoordinationUnit();
if (!process_cu)
return false;
return process_cu->GetProperty(
mojom::PropertyType::kExpectedTaskQueueingDuration, output);
}
base::TimeDelta PageCoordinationUnitImpl::TimeSinceLastNavigation() const {
if (navigation_committed_time_.is_null())
return base::TimeDelta();
......@@ -153,36 +166,4 @@ PageCoordinationUnitImpl::GetMainFrameCoordinationUnit() {
return nullptr;
}
bool PageCoordinationUnitImpl::CalculateExpectedTaskQueueingDuration(
int64_t* output) {
// Calculate the EQT for the process of the main frame only because
// the smoothness of the main frame may affect the users the most.
FrameCoordinationUnitImpl* main_frame_cu = GetMainFrameCoordinationUnit();
if (!main_frame_cu)
return false;
auto* associated_process = main_frame_cu->GetProcessCoordinationUnit();
if (!associated_process)
return false;
return associated_process->GetProperty(
mojom::PropertyType::kExpectedTaskQueueingDuration, output);
}
double PageCoordinationUnitImpl::CalculateCPUUsage() {
double cpu_usage = 0.0;
for (auto* process_cu : GetAssociatedProcessCoordinationUnits()) {
size_t pages_in_process =
process_cu->GetAssociatedPageCoordinationUnits().size();
DCHECK_LE(1u, pages_in_process);
int64_t process_cpu_usage;
if (process_cu->GetProperty(mojom::PropertyType::kCPUUsage,
&process_cpu_usage)) {
cpu_usage += static_cast<double>(process_cpu_usage) / pages_in_process;
}
}
return cpu_usage;
}
} // namespace resource_coordinator
......@@ -36,15 +36,16 @@ class PageCoordinationUnitImpl
void OnTitleUpdated() override;
void OnMainFrameNavigationCommitted() override;
// CoordinationUnitBase implementation.
void RecalculateProperty(const mojom::PropertyType property_type) override;
// There is no direct relationship between processes and pages. However,
// frames are accessible by both processes and frames, so we find all of the
// processes that are reachable from the pages's accessible frames.
std::set<ProcessCoordinationUnitImpl*> GetAssociatedProcessCoordinationUnits()
const;
bool IsVisible() const;
double GetCPUUsage() const;
// Returns false if can't get an expected task queueing duration successfully.
bool GetExpectedTaskQueueingDuration(int64_t* duration);
// Returns 0 if no navigation has happened, otherwise returns the time since
// the last navigation commit.
......@@ -73,10 +74,6 @@ class PageCoordinationUnitImpl
bool AddFrame(FrameCoordinationUnitImpl* frame_cu);
bool RemoveFrame(FrameCoordinationUnitImpl* frame_cu);
// Returns true for a valid value. Returns false otherwise.
bool CalculateExpectedTaskQueueingDuration(int64_t* output);
double CalculateCPUUsage();
// Returns the main frame CU or nullptr if this page has no main frame.
FrameCoordinationUnitImpl* GetMainFrameCoordinationUnit();
......
......@@ -71,57 +71,33 @@ TEST_F(PageCoordinationUnitImplTest, RemoveFrame) {
TEST_F(PageCoordinationUnitImplTest,
CalculatePageCPUUsageForSinglePageInSingleProcess) {
MockSinglePageInSingleProcessCoordinationUnitGraph cu_graph;
cu_graph.process->SetCPUUsage(40);
int64_t cpu_usage;
EXPECT_TRUE(
cu_graph.page->GetProperty(mojom::PropertyType::kCPUUsage, &cpu_usage));
EXPECT_EQ(40, cpu_usage / 1000);
EXPECT_EQ(40, cu_graph.page->GetCPUUsage());
}
TEST_F(PageCoordinationUnitImplTest,
CalculatePageCPUUsageForMultiplePagesInSingleProcess) {
MockMultiplePagesInSingleProcessCoordinationUnitGraph cu_graph;
cu_graph.process->SetCPUUsage(40);
int64_t cpu_usage;
EXPECT_TRUE(
cu_graph.page->GetProperty(mojom::PropertyType::kCPUUsage, &cpu_usage));
EXPECT_EQ(20, cpu_usage / 1000);
EXPECT_TRUE(cu_graph.other_page->GetProperty(mojom::PropertyType::kCPUUsage,
&cpu_usage));
EXPECT_EQ(20, cpu_usage / 1000);
EXPECT_EQ(20, cu_graph.page->GetCPUUsage());
EXPECT_EQ(20, cu_graph.other_page->GetCPUUsage());
}
TEST_F(PageCoordinationUnitImplTest,
CalculatePageCPUUsageForSinglePageWithMultipleProcesses) {
MockSinglePageWithMultipleProcessesCoordinationUnitGraph cu_graph;
cu_graph.process->SetCPUUsage(40);
cu_graph.other_process->SetCPUUsage(30);
int64_t cpu_usage;
EXPECT_TRUE(
cu_graph.page->GetProperty(mojom::PropertyType::kCPUUsage, &cpu_usage));
EXPECT_EQ(70, cpu_usage / 1000);
EXPECT_EQ(70, cu_graph.page->GetCPUUsage());
}
TEST_F(PageCoordinationUnitImplTest,
CalculatePageCPUUsageForMultiplePagesWithMultipleProcesses) {
MockMultiplePagesWithMultipleProcessesCoordinationUnitGraph cu_graph;
cu_graph.process->SetCPUUsage(40);
cu_graph.other_process->SetCPUUsage(30);
int64_t cpu_usage;
EXPECT_TRUE(
cu_graph.page->GetProperty(mojom::PropertyType::kCPUUsage, &cpu_usage));
EXPECT_EQ(20, cpu_usage / 1000);
EXPECT_TRUE(cu_graph.other_page->GetProperty(mojom::PropertyType::kCPUUsage,
&cpu_usage));
EXPECT_EQ(50, cpu_usage / 1000);
EXPECT_EQ(20, cu_graph.page->GetCPUUsage());
EXPECT_EQ(50, cu_graph.other_page->GetCPUUsage());
}
TEST_F(PageCoordinationUnitImplTest,
......@@ -132,8 +108,7 @@ TEST_F(PageCoordinationUnitImplTest,
base::TimeDelta::FromMilliseconds(1));
int64_t eqt;
ASSERT_TRUE(cu_graph.page->GetProperty(
mojom::PropertyType::kExpectedTaskQueueingDuration, &eqt));
EXPECT_TRUE(cu_graph.page->GetExpectedTaskQueueingDuration(&eqt));
EXPECT_EQ(1, eqt);
}
......@@ -145,11 +120,10 @@ TEST_F(PageCoordinationUnitImplTest,
base::TimeDelta::FromMilliseconds(1));
int64_t eqt;
ASSERT_TRUE(cu_graph.page->GetProperty(
mojom::PropertyType::kExpectedTaskQueueingDuration, &eqt));
EXPECT_TRUE(cu_graph.page->GetExpectedTaskQueueingDuration(&eqt));
EXPECT_EQ(1, eqt);
ASSERT_TRUE(cu_graph.other_page->GetProperty(
mojom::PropertyType::kExpectedTaskQueueingDuration, &eqt));
eqt = 0;
EXPECT_TRUE(cu_graph.other_page->GetExpectedTaskQueueingDuration(&eqt));
EXPECT_EQ(1, eqt);
}
......
......@@ -72,6 +72,11 @@ void ProcessCoordinationUnitImpl::SetPID(int64_t pid) {
SetProperty(mojom::PropertyType::kPID, pid);
}
std::set<FrameCoordinationUnitImpl*>
ProcessCoordinationUnitImpl::GetFrameCoordinationUnits() const {
return frame_coordination_units_;
}
// There is currently not a direct relationship between processes and
// pages. However, frames are children of both processes and frames, so we
// find all of the pages that are reachable from the process's child
......@@ -86,33 +91,11 @@ ProcessCoordinationUnitImpl::GetAssociatedPageCoordinationUnits() const {
return page_cus;
}
void ProcessCoordinationUnitImpl::PropagateProperty(
mojom::PropertyType property_type,
void ProcessCoordinationUnitImpl::OnPropertyChanged(
const mojom::PropertyType property_type,
int64_t value) {
switch (property_type) {
// Trigger Page CU to recalculate their CPU usage.
case mojom::PropertyType::kCPUUsage: {
for (auto* page_cu : GetAssociatedPageCoordinationUnits()) {
page_cu->RecalculateProperty(mojom::PropertyType::kCPUUsage);
}
break;
}
case mojom::PropertyType::kExpectedTaskQueueingDuration: {
// Do not propagate if the associated frame is not the main frame.
for (auto* frame_cu : frame_coordination_units_) {
if (!frame_cu->IsMainFrame())
continue;
auto* page_cu = frame_cu->GetPageCoordinationUnit();
if (page_cu) {
page_cu->RecalculateProperty(
mojom::PropertyType::kExpectedTaskQueueingDuration);
}
}
break;
}
default:
break;
}
for (auto& observer : observers())
observer.OnProcessPropertyChanged(this, property_type, value);
}
bool ProcessCoordinationUnitImpl::AddFrame(
......
......@@ -35,6 +35,7 @@ class ProcessCoordinationUnitImpl
void SetLaunchTime(base::Time launch_time) override;
void SetPID(int64_t pid) override;
std::set<FrameCoordinationUnitImpl*> GetFrameCoordinationUnits() const;
std::set<PageCoordinationUnitImpl*> GetAssociatedPageCoordinationUnits()
const;
......@@ -42,7 +43,7 @@ class ProcessCoordinationUnitImpl
friend class FrameCoordinationUnitImpl;
// CoordinationUnitInterface implementation.
void PropagateProperty(mojom::PropertyType property_type,
void OnPropertyChanged(mojom::PropertyType property_type,
int64_t value) override;
bool AddFrame(FrameCoordinationUnitImpl* frame_cu);
......
......@@ -14,6 +14,7 @@ class CoordinationUnitBase;
class CoordinationUnitManager;
class FrameCoordinationUnitImpl;
class PageCoordinationUnitImpl;
class ProcessCoordinationUnitImpl;
// An observer API for the coordination unit graph maintained by GRC.
//
......@@ -62,6 +63,12 @@ class CoordinationUnitGraphObserver {
const mojom::PropertyType property_type,
int64_t value) {}
// Called whenever a property of the ProcessCoordinationUnit is changed.
virtual void OnProcessPropertyChanged(
const ProcessCoordinationUnitImpl* process_cu,
const mojom::PropertyType property_type,
int64_t value) {}
// Called whenever an event is received in |coordination_unit| if the
// |coordination_unit| doesn't implement its own EventReceived handler.
virtual void OnEventReceived(const CoordinationUnitBase* coordination_unit,
......
......@@ -38,15 +38,12 @@ const char kTabFromBackgroundedToFirstNonPersistentNotificationCreatedUMA[] =
// Gets the number of tabs that are co-resident in all of the render processes
// associated with a |CoordinationUnitType::kPage| coordination unit.
size_t GetNumCoresidentTabs(const CoordinationUnitBase* coordination_unit) {
DCHECK_EQ(CoordinationUnitType::kPage, coordination_unit->id().type);
auto* page_cu =
PageCoordinationUnitImpl::FromCoordinationUnitBase(coordination_unit);
size_t GetNumCoresidentTabs(const PageCoordinationUnitImpl* page_cu) {
std::set<CoordinationUnitBase*> coresident_tabs;
for (auto* process_cu : page_cu->GetAssociatedProcessCoordinationUnits()) {
for (auto* tab_coordination_unit :
for (auto* associated_page_cu :
process_cu->GetAssociatedPageCoordinationUnits()) {
coresident_tabs.insert(tab_coordination_unit);
coresident_tabs.insert(associated_page_cu);
}
}
// A tab cannot be co-resident with itself.
......@@ -64,7 +61,8 @@ MetricsCollector::~MetricsCollector() = default;
bool MetricsCollector::ShouldObserve(
const CoordinationUnitBase* coordination_unit) {
return coordination_unit->id().type == CoordinationUnitType::kFrame ||
coordination_unit->id().type == CoordinationUnitType::kPage;
coordination_unit->id().type == CoordinationUnitType::kPage ||
coordination_unit->id().type == CoordinationUnitType::kProcess;
}
void MetricsCollector::OnCoordinationUnitCreated(
......@@ -126,11 +124,6 @@ void MetricsCollector::OnPagePropertyChanged(
ResetMetricsReportRecord(page_cu_id);
return;
}
} else if (property_type == mojom::PropertyType::kCPUUsage) {
if (IsCollectingCPUUsageForUkm(page_cu_id)) {
RecordCPUUsageForUkm(page_cu_id, static_cast<double>(value) / 1000,
GetNumCoresidentTabs(page_cu));
}
} else if (property_type == mojom::PropertyType::kUKMSourceId) {
ukm::SourceId ukm_source_id = value;
UpdateUkmSourceIdForPage(page_cu_id, ukm_source_id);
......@@ -140,6 +133,20 @@ void MetricsCollector::OnPagePropertyChanged(
}
}
void MetricsCollector::OnProcessPropertyChanged(
const ProcessCoordinationUnitImpl* process_cu,
const mojom::PropertyType property_type,
int64_t value) {
if (property_type == mojom::PropertyType::kCPUUsage) {
for (auto* page_cu : process_cu->GetAssociatedPageCoordinationUnits()) {
if (IsCollectingCPUUsageForUkm(page_cu->id())) {
RecordCPUUsageForUkm(page_cu->id(), page_cu->GetCPUUsage(),
GetNumCoresidentTabs(page_cu));
}
}
}
}
void MetricsCollector::OnFrameEventReceived(
const FrameCoordinationUnitImpl* frame_cu,
const mojom::Event event) {
......
......@@ -48,6 +48,9 @@ class MetricsCollector : public CoordinationUnitGraphObserver {
void OnPagePropertyChanged(const PageCoordinationUnitImpl* page_cu,
const mojom::PropertyType property_type,
int64_t value) override;
void OnProcessPropertyChanged(const ProcessCoordinationUnitImpl* process_cu,
const mojom::PropertyType property_type,
int64_t value) override;
void OnFrameEventReceived(const FrameCoordinationUnitImpl* frame_cu,
const mojom::Event event) override;
void OnPageEventReceived(const PageCoordinationUnitImpl* page_cu,
......
......@@ -8,6 +8,7 @@
#include "services/resource_coordinator/coordination_unit/frame_coordination_unit_impl.h"
#include "services/resource_coordinator/coordination_unit/page_coordination_unit_impl.h"
#include "services/resource_coordinator/coordination_unit/process_coordination_unit_impl.h"
#include "services/service_manager/public/cpp/bind_source_info.h"
namespace resource_coordinator {
......@@ -27,9 +28,9 @@ void TabSignalGeneratorImpl::AddObserver(mojom::TabSignalObserverPtr observer) {
bool TabSignalGeneratorImpl::ShouldObserve(
const CoordinationUnitBase* coordination_unit) {
auto coordination_unit_type = coordination_unit->id().type;
return coordination_unit_type == CoordinationUnitType::kPage ||
coordination_unit_type == CoordinationUnitType::kFrame;
auto cu_type = coordination_unit->id().type;
return cu_type == CoordinationUnitType::kFrame ||
cu_type == CoordinationUnitType::kProcess;
}
void TabSignalGeneratorImpl::OnFramePropertyChanged(
......@@ -47,14 +48,22 @@ void TabSignalGeneratorImpl::OnFramePropertyChanged(
}
}
void TabSignalGeneratorImpl::OnPagePropertyChanged(
const PageCoordinationUnitImpl* page_cu,
void TabSignalGeneratorImpl::OnProcessPropertyChanged(
const ProcessCoordinationUnitImpl* process_cu,
const mojom::PropertyType property_type,
int64_t value) {
if (property_type == mojom::PropertyType::kExpectedTaskQueueingDuration) {
for (auto* frame_cu : process_cu->GetFrameCoordinationUnits()) {
if (!frame_cu->IsMainFrame())
continue;
auto* page_cu = frame_cu->GetPageCoordinationUnit();
int64_t duration;
if (!page_cu->GetExpectedTaskQueueingDuration(&duration))
continue;
DISPATCH_TAB_SIGNAL(observers_, SetExpectedTaskQueueingDuration,
page_cu->id(),
base::TimeDelta::FromMilliseconds(value));
base::TimeDelta::FromMilliseconds(duration));
}
}
}
......
......@@ -17,10 +17,6 @@ struct BindSourceInfo;
namespace resource_coordinator {
class CoordinationUnitBase;
class FrameCoordinationUnitImpl;
class PageCoordinationUnitImpl;
// The TabSignalGenerator is a dedicated |CoordinationUnitGraphObserver| for
// calculating and emitting tab-scoped signals. This observer observes Tab
// CoordinationUnits and Frame CoordinationUnits, utilize information from the
......@@ -39,7 +35,7 @@ class TabSignalGeneratorImpl : public CoordinationUnitGraphObserver,
void OnFramePropertyChanged(const FrameCoordinationUnitImpl* frame_cu,
const mojom::PropertyType property_type,
int64_t value) override;
void OnPagePropertyChanged(const PageCoordinationUnitImpl* page_cu,
void OnProcessPropertyChanged(const ProcessCoordinationUnitImpl* process_cu,
const mojom::PropertyType property_type,
int64_t value) override;
......
......@@ -15,7 +15,7 @@ namespace resource_coordinator {
class MockTabSignalGeneratorImpl : public TabSignalGeneratorImpl {
public:
// Overridden from TabSignalGeneratorImpl.
void OnPagePropertyChanged(const PageCoordinationUnitImpl* coordination_unit,
void OnProcessPropertyChanged(const ProcessCoordinationUnitImpl* process_cu,
const mojom::PropertyType property_type,
int64_t value) override {
if (property_type == mojom::PropertyType::kExpectedTaskQueueingDuration)
......@@ -41,7 +41,7 @@ class TabSignalGeneratorImplTest : public CoordinationUnitTestHarness {
TEST_F(TabSignalGeneratorImplTest,
CalculateTabEQTForSingleTabWithMultipleProcesses) {
MockSinglePageWithMultipleProcessesCoordinationUnitGraph cu_graph;
cu_graph.page->AddObserver(tab_signal_generator());
cu_graph.process->AddObserver(tab_signal_generator());
cu_graph.process->SetExpectedTaskQueueingDuration(
base::TimeDelta::FromMilliseconds(1));
......@@ -52,8 +52,7 @@ TEST_F(TabSignalGeneratorImplTest,
// propagate to the page.
EXPECT_EQ(1u, tab_signal_generator()->eqt_change_count());
int64_t eqt;
ASSERT_TRUE(cu_graph.page->GetProperty(
mojom::PropertyType::kExpectedTaskQueueingDuration, &eqt));
EXPECT_TRUE(cu_graph.page->GetExpectedTaskQueueingDuration(&eqt));
EXPECT_EQ(1, eqt);
}
......
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