Revert "Update memory metrics immediately on loaded idle"

This reverts commit b471d6d. Reason for revert: Caused crashes https://bugs.chromium.org/p/chromium/issues/detail?id=1464503 Original change's description: > Update memory metrics immediately on loaded idle > > Currently the first metrics request happens after 2 minutes. We > are rolling out a feature to show memory usage in hovercards and it > is confusing to have the memory missing for the first 2 minutes of > a page load. > > This change will be behind a FeatureParam associated with the memory > usage in hovercards experiment. On kLoadedIdle we will > request metrics. This might still result in a slight delay before we have metrics but the data will be higher quality than if we request it immediately. > > (cherry picked from commit b04518d) > > Bug: 1459009 > Change-Id: Ib43f9689eca5233cdada8e599a5df6be89fe0779 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4656209 > Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com> > Reviewed-by: Francois Pierre Doray <fdoray@chromium.org> > Commit-Queue: Alison Gale <agale@chromium.org> > Cr-Original-Commit-Position: refs/heads/main@{#1168165} > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4679340 > Reviewed-by: Joe Mason <joenotcharles@google.com> > Cr-Commit-Position: refs/branch-heads/5845@{#425} > Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321} Bug: 1459009 Change-Id: Ibd43050534da70826c713d12272150dea8483ec8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4684629 Commit-Queue: Alison Gale <agale@chromium.org> Reviewed-by: Eshwar Stalin <estalin@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Reviewed-by: Patrick Monette <pmonette@chromium.org> Cr-Commit-Position: refs/branch-heads/5845@{#473} Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
chromium · Jul 13, 2023 · 2c1eb03 · 2c1eb03
1 parent d8d1fe0
commit 2c1eb03
Show file tree

Hide file tree

Showing 10 changed files with 11 additions and 86 deletions.
diff --git a/chrome/browser/about_flags.cc b/chrome/browser/about_flags.cc
@@ -3443,16 +3443,6 @@ const FeatureEntry::FeatureVariation kDiscardedTabTreatmentVariations[] = {
 
 };
 
-const FeatureEntry::FeatureParam
-    kMemoryUsageInHovercardsUpdateOnNavigationVariations[] = {
-        {"update_memory_on_navigation", "true"},
-};
-const FeatureEntry::FeatureVariation kMemoryUsageInHovercardsVariations[] = {
-    {"With Update on Navigation",
-     kMemoryUsageInHovercardsUpdateOnNavigationVariations,
-     std::size(kMemoryUsageInHovercardsUpdateOnNavigationVariations), nullptr},
-};
-
 const FeatureEntry::FeatureParam kMemorySavingsReportingFrequent[] = {
     // 100 * 1024 * 1024
     {"expanded_high_efficiency_chip_threshold_bytes", "104857600"},
@@ -9963,10 +9953,8 @@ const FeatureEntry kFeatureEntries[] = {
      flag_descriptions::kHighEfficiencyMemoryUsageInHovercardsName,
      flag_descriptions::kHighEfficiencyMemoryUsageInHovercardsDescription,
      kOsDesktop,
-     FEATURE_WITH_PARAMS_VALUE_TYPE(
-         performance_manager::features::kMemoryUsageInHovercards,
-         kMemoryUsageInHovercardsVariations,
-         "MemoryUsageInHovercards")},
+     FEATURE_VALUE_TYPE(
+         performance_manager::features::kMemoryUsageInHovercards)},
 
     {"memory-saver-discard-exceptions-improvements",
      flag_descriptions::kHighEfficiencyDiscardExceptionsImprovementsName,

diff --git a/chrome/browser/performance_manager/user_tuning/resource_usage_tab_helper_browsertest.cc b/chrome/browser/performance_manager/user_tuning/resource_usage_tab_helper_browsertest.cc
@@ -81,7 +81,7 @@ class ResourceUsageTabHelperTest : public InteractiveBrowserTest {
           base::BindLambdaForTesting([](performance_manager::Graph* graph) {
             auto* metrics_decorator = graph->GetRegisteredObjectAs<
                 performance_manager::ProcessMetricsDecorator>();
-            metrics_decorator->RequestImmediateMetrics();
+            metrics_decorator->RefreshMetricsForTesting();
           }));
 
       run_loop.Run();

diff --git a/chrome/browser/performance_manager/user_tuning/user_performance_tuning_notifier.cc b/chrome/browser/performance_manager/user_tuning/user_performance_tuning_notifier.cc
@@ -7,8 +7,6 @@
 #include <utility>
 #include <vector>
 
-#include "components/performance_manager/public/features.h"
-#include "components/performance_manager/public/graph/page_node.h"
 #include "components/performance_manager/public/graph/process_node.h"
 
 namespace performance_manager::user_tuning {
@@ -67,20 +65,6 @@ void UserPerformanceTuningNotifier::OnTypeChanged(const PageNode* page_node,
   }
 }
 
-void UserPerformanceTuningNotifier::OnLoadingStateChanged(
-    const PageNode* page_node,
-    PageNode::LoadingState previous_state) {
-  if (features::kMemoryUsageInHovercardsUpdateOnNavigation.Get() &&
-      page_node->GetType() == PageType::kTab &&
-      page_node->GetLoadingState() == PageNode::LoadingState::kLoadedIdle) {
-    auto* metrics_decorator =
-        page_node->GetGraph()
-            ->GetRegisteredObjectAs<
-                performance_manager::ProcessMetricsDecorator>();
-    metrics_decorator->RequestImmediateMetrics();
-  }
-}
-
 void UserPerformanceTuningNotifier::OnProcessMemoryMetricsAvailable(
     const SystemNode* system_node) {
   uint64_t total_rss = 0;

diff --git a/chrome/browser/performance_manager/user_tuning/user_performance_tuning_notifier.h b/chrome/browser/performance_manager/user_tuning/user_performance_tuning_notifier.h
@@ -63,8 +63,6 @@ class UserPerformanceTuningNotifier : public performance_manager::GraphOwned,
   void OnBeforePageNodeRemoved(const PageNode* page_node) override;
   void OnTypeChanged(const PageNode* page_node,
                      PageType previous_type) override;
-  void OnLoadingStateChanged(const PageNode* page_node,
-                             PageNode::LoadingState previous_state) override;
 
   // SystemNode::ObserverDefaultImpl:
   void OnProcessMemoryMetricsAvailable(const SystemNode* system_node) override;

diff --git a/chrome/browser/performance_manager/user_tuning/user_performance_tuning_notifier_unittest.cc b/chrome/browser/performance_manager/user_tuning/user_performance_tuning_notifier_unittest.cc
@@ -11,12 +11,10 @@
 #include <vector>
 
 #include "base/memory/raw_ptr.h"
-#include "base/test/scoped_feature_list.h"
 #include "components/performance_manager/graph/frame_node_impl.h"
 #include "components/performance_manager/graph/page_node_impl.h"
 #include "components/performance_manager/graph/system_node_impl.h"
 #include "components/performance_manager/public/decorators/process_metrics_decorator.h"
-#include "components/performance_manager/public/features.h"
 #include "components/performance_manager/public/graph/graph.h"
 #include "components/performance_manager/test_support/graph_test_harness.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -55,22 +53,11 @@ class UserPerformanceTuningNotifierTest : public GraphTestHarness {
     std::vector<uint64_t> pages_pmf_kb_;
   };
 
-  class TestProcessMetricsDecorator
-      : public performance_manager::ProcessMetricsDecorator {
-   public:
-    void RequestImmediateMetrics() override {
-      ++request_immediate_metrics_count_;
-    }
-
-    int request_immediate_metrics_count_ = 0;
-  };
-
   void SetUp() override {
     GraphTestHarness::SetUp();
 
-    auto decorator = std::make_unique<TestProcessMetricsDecorator>();
-    decorator_ = decorator.get();
-    graph()->PassToGraph(std::move(decorator));
+    graph()->PassToGraph(
+        std::make_unique<performance_manager::ProcessMetricsDecorator>());
 
     auto receiver = std::make_unique<TestReceiver>();
     receiver_ = receiver.get();
@@ -81,7 +68,6 @@ class UserPerformanceTuningNotifierTest : public GraphTestHarness {
     graph()->PassToGraph(std::move(notifier));
   }
 
-  raw_ptr<TestProcessMetricsDecorator> decorator_;
   raw_ptr<TestReceiver> receiver_;
 };
 
@@ -171,27 +157,4 @@ TEST_F(UserPerformanceTuningNotifierTest, TestMemoryAvailableTriggered) {
   EXPECT_EQ(2, receiver_->memory_refreshed_count_);
 }
 
-TEST_F(UserPerformanceTuningNotifierTest,
-       TestRequestImmediateMetricsTriggered) {
-  base::test::ScopedFeatureList feature_list;
-  feature_list.InitAndEnableFeatureWithParameters(
-      features::kMemoryUsageInHovercards,
-      {{"update_memory_on_navigation", "true"}});
-
-  // Memory Metrics are available
-  auto process = CreateNode<ProcessNodeImpl>();
-  auto page = CreateNode<PageNodeImpl>();
-  page->SetType(PageType::kTab);
-  auto frame = CreateFrameNodeAutoId(process.get(), page.get());
-  frame->SetPrivateFootprintKbEstimate(30);
-
-  // No memory refresh should occur while loading.
-  page->SetLoadingState(PageNode::LoadingState::kLoading);
-  EXPECT_EQ(0, decorator_->request_immediate_metrics_count_);
-
-  // Memory refresh should occur after MainFrameDocumentCommitted.
-  page->SetLoadingState(PageNode::LoadingState::kLoadedIdle);
-  EXPECT_EQ(1, decorator_->request_immediate_metrics_count_);
-}
-
 }  // namespace performance_manager::user_tuning
diff --git a/chrome/browser/ui/views/performance_controls/high_efficiency_interactive_ui_test.cc b/chrome/browser/ui/views/performance_controls/high_efficiency_interactive_ui_test.cc
@@ -150,7 +150,7 @@ class HighEfficiencyInteractiveTest : public InteractiveBrowserTest {
           base::BindLambdaForTesting([](performance_manager::Graph* graph) {
             auto* metrics_decorator = graph->GetRegisteredObjectAs<
                 performance_manager::ProcessMetricsDecorator>();
-            metrics_decorator->RequestImmediateMetrics();
+            metrics_decorator->RefreshMetricsForTesting();
           }));
 
       run_loop.Run();

diff --git a/components/performance_manager/decorators/process_metrics_decorator.cc b/components/performance_manager/decorators/process_metrics_decorator.cc
@@ -148,9 +148,10 @@ void ProcessMetricsDecorator::RefreshMetrics() {
       &ProcessMetricsDecorator::DidGetMemoryUsage, weak_factory_.GetWeakPtr()));
 }
 
-void ProcessMetricsDecorator::RequestImmediateMetrics() {
-  // Stop the timer so the next metrics sample will be 2 minutes after this to
-  // avoid re-sampling shortly after updating the metrics.
+void ProcessMetricsDecorator::RefreshMetricsForTesting() {
+  // Tests may refresh the metrics outside the normal schedule. Make sure the
+  // timer isn't running so that RefreshMetrics() can call StartTimer() to
+  // schedule the next refresh.
   StopTimer();
   RefreshMetrics();
 }

diff --git a/components/performance_manager/features.cc b/components/performance_manager/features.cc
@@ -136,9 +136,6 @@ const base::FeatureParam<int> kMemoryUsageInHovercardsHighThresholdBytes{
     &kMemoryUsageInHovercards,
     "memory_usage_in_hovercards_high_threshold_bytes", 800 * 1024 * 1024};
 
-const base::FeatureParam<bool> kMemoryUsageInHovercardsUpdateOnNavigation{
-    &kMemoryUsageInHovercards, "update_memory_on_navigation", false};
-
 #endif
 
 BASE_FEATURE(kBFCachePerformanceManagerPolicy,

diff --git a/components/performance_manager/public/decorators/process_metrics_decorator.h b/components/performance_manager/public/decorators/process_metrics_decorator.h
@@ -72,8 +72,7 @@ class ProcessMetricsDecorator
     return refresh_timer_.GetCurrentDelay();
   }
 
-  // Immediately refreshes the metrics for all the process nodes.
-  virtual void RequestImmediateMetrics();
+  void RefreshMetricsForTesting();
 
  protected:
   class ScopedMetricsInterestTokenImpl;

diff --git a/components/performance_manager/public/features.h b/components/performance_manager/public/features.h
@@ -193,11 +193,6 @@ extern const base::FeatureParam<int> kDiscardedTabTreatmentOption;
 // Threshold for when memory usage is labeled as "high".
 extern const base::FeatureParam<int> kMemoryUsageInHovercardsHighThresholdBytes;
 
-// When true, memory usage metrics will be fetched after a navigation is idle
-// in addition to every 2 minutes.
-extern const base::FeatureParam<bool>
-    kMemoryUsageInHovercardsUpdateOnNavigation;
-
 enum class DiscardTabTreatmentOptions {
   kNone = 0,
   kFadeFullsizedFavicon = 1,