Adjusting ProactiveTabDiscardParams struct so tabs / gb memory logic is not exposed to caller

We do not want callers (TabManager) to have to implement any of the logic related to determining the thresholds, but simply consume them. These thresholds will be used to determine timeouts until discard for proactive tab discarding.

Bug: 775644
Change-Id: I09b3f54cf3acb44ac3724ef4a08ae57d4ce42255
Reviewed-on: https://chromium-review.googlesource.com/1012797Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551221}

chrome/browser/resource_coordinator/tab_manager_features.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/resource_coordinator/tab_manager_features.h"
 
 #include "base/metrics/field_trial_params.h"
+#include "base/numerics/ranges.h"
 #include "chrome/common/chrome_features.h"
 
 namespace {
@@ -42,6 +43,30 @@ const base::Feature kStaggeredBackgroundTabOpeningExperiment{
 
 namespace resource_coordinator {
 
+namespace {
+
+// Determines the moderate threshold for tab discarding based on system memory,
+// and enforces the constraint that it must be in the interval
+// [low_loaded_tab_count, high_loaded_tab_count].
+int GetModerateThresholdTabCountBasedOnSystemMemory(
+    ProactiveTabDiscardParams* params,
+    int memory_in_gb) {
+  int moderate_loaded_tab_count_per_gb = base::GetFieldTrialParamByFeatureAsInt(
+      features::kProactiveTabDiscarding,
+      kProactiveTabDiscard_ModerateLoadedTabsPerGbRamParam,
+      kProactiveTabDiscard_ModerateLoadedTabsPerGbRamDefault);
+
+  int moderate_level = moderate_loaded_tab_count_per_gb * memory_in_gb;
+
+  moderate_level =
+      base::ClampToRange(moderate_level, params->low_loaded_tab_count,
+                         params->high_loaded_tab_count);
+
+  return moderate_level;
+}
+
+}  // namespace
+
 // Field-trial parameter names for proactive tab discarding.
 const char kProactiveTabDiscard_LowLoadedTabCountParam[] = "LowLoadedTabCount";
 const char kProactiveTabDiscard_ModerateLoadedTabsPerGbRamParam[] =
@@ -80,23 +105,23 @@ const base::TimeDelta kProactiveTabDiscard_ModerateOccludedTimeoutDefault =
 const base::TimeDelta kProactiveTabDiscard_HighOccludedTimeoutDefault =
     base::TimeDelta::FromMinutes(10);
 
-void GetProactiveTabDiscardParams(ProactiveTabDiscardParams* params) {
+void GetProactiveTabDiscardParams(ProactiveTabDiscardParams* params,
+                                  int memory_in_gb) {
   params->low_loaded_tab_count = base::GetFieldTrialParamByFeatureAsInt(
       features::kProactiveTabDiscarding,
       kProactiveTabDiscard_LowLoadedTabCountParam,
       kProactiveTabDiscard_LowLoadedTabCountDefault);
 
-  params->moderate_loaded_tab_count_per_gb =
-      base::GetFieldTrialParamByFeatureAsInt(
-          features::kProactiveTabDiscarding,
-          kProactiveTabDiscard_ModerateLoadedTabsPerGbRamParam,
-          kProactiveTabDiscard_ModerateLoadedTabsPerGbRamDefault);
-
   params->high_loaded_tab_count = base::GetFieldTrialParamByFeatureAsInt(
       features::kProactiveTabDiscarding,
       kProactiveTabDiscard_HighLoadedTabCountParam,
       kProactiveTabDiscard_HighLoadedTabCountDefault);
 
+  // |moderate_loaded_tab_count| determined after |high_loaded_tab_count| so it
+  // can be enforced that it is lower than |high_loaded_tab_count|.
+  params->moderate_loaded_tab_count =
+      GetModerateThresholdTabCountBasedOnSystemMemory(params, memory_in_gb);
+
   params->low_occluded_timeout =
       base::TimeDelta::FromSeconds(base::GetFieldTrialParamByFeatureAsInt(
           features::kProactiveTabDiscarding,

chrome/browser/resource_coordinator/tab_manager_features.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_RESOURCE_COORDINATOR_TAB_MANAGER_FEATURES_H_
 
 #include "base/feature_list.h"
+#include "base/sys_info.h"
 #include "base/time/time.h"
 
 namespace features {
@@ -79,11 +80,12 @@ struct ProactiveTabDiscardParams {
   // it is very unlikely that memory pressure will be encountered with this many
   // tabs loaded.
   int low_loaded_tab_count;
-  // Tab count (inclusive) beyond which the state transitions to HIGH, expressed
-  // relative to system memory.
-  int moderate_loaded_tab_count_per_gb;
+  // Tab count (inclusive) beyond which the state transitions to HIGH. This
+  // value is determined based on the available system memory, and is ensured to
+  // be in the interval [low_loaded_tab_count, high_loaded_tab_count].
+  int moderate_loaded_tab_count;
   // Tab count (inclusive) beyond which the state transitions to EXCESSIVE.
-  // Not expressed relative to system memory, as its intended to be a hard cap
+  // Not relative to system memory, as its intended to be a hard cap
   // more akin to a maximum mental model size.
   int high_loaded_tab_count;
   // Amount of time a tab must be occluded before eligible for proactive
@@ -100,7 +102,10 @@ struct ProactiveTabDiscardParams {
 // Gets parameters for the proactive tab discarding feature. This does no
 // parameter validation, and sets the default values if the feature is not
 // enabled.
-void GetProactiveTabDiscardParams(ProactiveTabDiscardParams* params);
+void GetProactiveTabDiscardParams(
+    ProactiveTabDiscardParams* params,
+    int memory_in_gb = base::SysInfo::AmountOfPhysicalMemory() /
+                       (1024 * 1024 * 1024));
 
 base::TimeDelta GetTabLoadTimeout(const base::TimeDelta& default_timeout);
 

chrome/browser/resource_coordinator/tab_manager_features_unittest.cc

@@ -31,16 +31,23 @@ class TabManagerFeaturesTest : public testing::Test {
 
   void ExpectProactiveTabDiscardingParams(
       int low_loaded_tab_count,
-      int moderate_loaded_tab_count_per_gb,
+      int moderate_loaded_tab_count,
       int high_loaded_tab_count,
+      int memory_in_gb,
       base::TimeDelta low_occluded_timeout,
       base::TimeDelta moderate_occluded_timeout,
       base::TimeDelta high_occluded_timeout) {
     ProactiveTabDiscardParams params = {};
-    GetProactiveTabDiscardParams(&params);
+    GetProactiveTabDiscardParams(&params, memory_in_gb);
+
     EXPECT_EQ(low_loaded_tab_count, params.low_loaded_tab_count);
-    EXPECT_EQ(moderate_loaded_tab_count_per_gb,
-              params.moderate_loaded_tab_count_per_gb);
+    EXPECT_EQ(moderate_loaded_tab_count, params.moderate_loaded_tab_count);
+
+    // Enforce that |moderate_loaded_tab_count| is within [low_loaded_tab_count,
+    // high_loaded_tab_count].
+    EXPECT_GE(params.moderate_loaded_tab_count, params.low_loaded_tab_count);
+    EXPECT_LE(params.moderate_loaded_tab_count, params.high_loaded_tab_count);
+
     EXPECT_EQ(high_loaded_tab_count, params.high_loaded_tab_count);
     EXPECT_EQ(low_occluded_timeout, params.low_occluded_timeout);
     EXPECT_EQ(moderate_occluded_timeout, params.moderate_occluded_timeout);
@@ -48,10 +55,11 @@ class TabManagerFeaturesTest : public testing::Test {
   }
 
   void ExpectDefaultProactiveTabDiscardParams() {
+    int memory_in_gb = 4;
     ExpectProactiveTabDiscardingParams(
         kProactiveTabDiscard_LowLoadedTabCountDefault,
-        kProactiveTabDiscard_ModerateLoadedTabsPerGbRamDefault,
-        kProactiveTabDiscard_HighLoadedTabCountDefault,
+        kProactiveTabDiscard_ModerateLoadedTabsPerGbRamDefault * memory_in_gb,
+        kProactiveTabDiscard_HighLoadedTabCountDefault, memory_in_gb,
         kProactiveTabDiscard_LowOccludedTimeoutDefault,
         kProactiveTabDiscard_ModerateOccludedTimeoutDefault,
         kProactiveTabDiscard_HighOccludedTimeoutDefault);
@@ -89,15 +97,35 @@ TEST_F(TabManagerFeaturesTest,
 
 TEST_F(TabManagerFeaturesTest, GetProactiveTabDiscardParams) {
   SetParam(kProactiveTabDiscard_LowLoadedTabCountParam, "7");
-  SetParam(kProactiveTabDiscard_ModerateLoadedTabsPerGbRamParam, "19");
+  SetParam(kProactiveTabDiscard_ModerateLoadedTabsPerGbRamParam, "4");
   SetParam(kProactiveTabDiscard_HighLoadedTabCountParam, "42");
   // These are expressed in seconds.
   SetParam(kProactiveTabDiscard_LowOccludedTimeoutParam, "60");
   SetParam(kProactiveTabDiscard_ModerateOccludedTimeoutParam, "120");
   SetParam(kProactiveTabDiscard_HighOccludedTimeoutParam, "247");
   EnableProactiveTabDiscarding();
+
+  // Should snap |moderate_loaded_tab_count| to |low_loaded_tab_count|, when the
+  // amount of physical memory is so low that (|memory_in_gb| *
+  // |moderate_tab_count_per_gb_ram|) < |low_loaded_tab_count|).
+  int memory_in_gb_low = 1;
+  ExpectProactiveTabDiscardingParams(
+      7, 7, 42, memory_in_gb_low, base::TimeDelta::FromSeconds(60),
+      base::TimeDelta::FromSeconds(120), base::TimeDelta::FromSeconds(247));
+
+  // Should snap |moderate_loaded_tab_count| to |high_loaded_tab_count|, when
+  // the amount of physical memory is so high that (|memory_in_gb| *
+  // |moderate_tab_count_per_gb_ram|) > |high_loaded_tab_count|).
+  int memory_in_gb_high = 100;
+  ExpectProactiveTabDiscardingParams(
+      7, 42, 42, memory_in_gb_high, base::TimeDelta::FromSeconds(60),
+      base::TimeDelta::FromSeconds(120), base::TimeDelta::FromSeconds(247));
+
+  // Tests normal case where |memory_in gb| * |moderate_tab_count_per_gb_ram| is
+  // in the interval [low_loaded_tab_count, high_loaded_tab_count].
+  int memory_in_gb_normal = 4;
   ExpectProactiveTabDiscardingParams(
-      7, 19, 42, base::TimeDelta::FromSeconds(60),
+      7, 16, 42, memory_in_gb_normal, base::TimeDelta::FromSeconds(60),
       base::TimeDelta::FromSeconds(120), base::TimeDelta::FromSeconds(247));
 }