Purge once random minutes(between 30min and 60min) after backgrounded.

chrome/browser/memory/tab_manager.cc

Index: chrome/browser/memory/tab_manager.cc
diff --git a/chrome/browser/memory/tab_manager.cc b/chrome/browser/memory/tab_manager.cc
index 959ebdcdaefb72159122a6bb1babe1247113d23f..31d95b3b151ab12dc1f16261477b7d7308618ace 100644
--- a/chrome/browser/memory/tab_manager.cc
+++ b/chrome/browser/memory/tab_manager.cc
@@ -20,6 +20,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/observer_list.h"
 #include "base/process/process.h"
+#include "base/rand_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -84,13 +85,10 @@ const int kRecentTabDiscardIntervalSeconds = 60;
 // machine was suspended and correct the timing statistics.
 const int kSuspendThresholdSeconds = kAdjustmentIntervalSeconds * 4;
 
-// A suspended renderer is suspended for this duration.
-constexpr base::TimeDelta kDurationOfRendererSuspension =
-    base::TimeDelta::FromSeconds(1200);
-
-// A resumed renderer is resumed for this duration.
-constexpr base::TimeDelta kDurationOfRendererResumption =
-    base::TimeDelta::FromSeconds(10);
+// Within the following range after the tab is backgrounded and
+// time_to_first_purge_ passes, purge the tab's cache if the tab is still
+// backgrounded and in-active.

[Wez, 2017/02/27 22:20:55]

This comment is pretty confusingly worded!

Would it be clearer to express this as a "maximum" time-to-first-purge, and the
existing time-to-first-purge as a minimum? Rather than as a minimum + range?

Also, do you need this value to be visible to tests? Surely tests need to know
the maximum time-to-first-suspend, so they can fast-forward time and be sure to
reach the suspended state?

[tasak, 2017/02/28 09:45:37]

Done.

+const unsigned int kRangeOfTimeToFirstPurgeInMinutes = 30;

[chrisha, 2017/02/27 13:31:06]

How is this a range? Or is it implicitly lower bounded by zero?

[tasak, 2017/02/28 09:45:37]

Done.

 
 // The time during which a tab is protected from discarding after it stops being
 // audible.
@@ -231,9 +229,9 @@ void TabManager::Start() {
   unsigned int time_to_first_purge_sec = 0;
   if (purge_and_suspend_time.empty() ||
       !base::StringToUint(purge_and_suspend_time, &time_to_first_purge_sec))
-    time_to_first_suspension_ = kDefaultTimeToFirstPurge;
+    time_to_first_purge_ = kDefaultTimeToFirstPurge;
   else
-    time_to_first_suspension_ =
+    time_to_first_purge_ =
         base::TimeDelta::FromSeconds(time_to_first_purge_sec);
 }
 
@@ -716,37 +714,26 @@ void TabManager::UpdateTimerCallback() {
   delegate_->AdjustOomPriorities(stats_list);
 #endif
 
-  PurgeAndSuspendBackgroundedTabs();
+  PurgeBackgroundedTabs();
 }
 
-TabManager::PurgeAndSuspendState TabManager::GetNextPurgeAndSuspendState(
+TabManager::PurgeState TabManager::GetNextPurgeState(
     content::WebContents* content,
-    base::TimeTicks current_time,
-    const base::TimeDelta& time_to_first_suspension) const {
+    base::TimeTicks current_time) const {
   DCHECK(content);

[Wez, 2017/02/27 22:20:55]

You don't need a DCHECK here, since you're about to de-reference |content|,
which will crash cleanly if it's null.

GetWebContentsData() will already DCHECK(contents) (twice!) in any case; those
should probably be removed as well (not in this CL, though) since they're
similarly redundant.

[tasak, 2017/02/28 09:45:37]

Done.

-  PurgeAndSuspendState state =
-      GetWebContentsData(content)->GetPurgeAndSuspendState();
-
-  auto time_passed = current_time -
-      GetWebContentsData(content)->LastPurgeAndSuspendModifiedTime();
-  switch (state) {
-    case RUNNING:
-      if (time_passed > time_to_first_suspension)
-        return SUSPENDED;
-      break;
-    case RESUMED:
-      if (time_passed > kDurationOfRendererResumption)
-        return SUSPENDED;
-      break;
-    case SUSPENDED:
-      if (time_passed > kDurationOfRendererSuspension)
-        return RESUMED;
-      break;
-  }
+  PurgeState state = GetWebContentsData(content)->GetPurgeState();
+  if (state == PURGED)
+    return state;
+
+  DCHECK(state == NOT_PURGED);

[Wez, 2017/02/27 22:20:55]

This is a tautologous DCHECK, since there are only two possible states; you
should change this code to use a switch(state) so that later addition of some
new PurgeState value will be caught at compile-time if this code isn't also
updated.

[tasak, 2017/02/28 09:45:37]

Done.

+  auto time_passed =

[Wez, 2017/02/27 22:20:55]

There's no need to use |auto| here; it makes the code less readable since the
reader has to either know, or guess, that the value will probably be
base::TimeDelta, rather than e.g. an int holding a number of milliseconds.

[tasak, 2017/02/28 09:45:37]

Done.

+      current_time - GetWebContentsData(content)->LastInactiveTime();
+  if (time_passed > GetWebContentsData(content)->TimeToFirstPurge())
+    return PURGED;
   return state;
 }
 
-void TabManager::PurgeAndSuspendBackgroundedTabs() {
+void TabManager::PurgeBackgroundedTabs() {
   base::TimeTicks current_time = NowTicks();
   auto tab_stats = GetUnsortedTabStats();
   for (auto& tab : tab_stats) {
@@ -759,16 +746,8 @@ void TabManager::PurgeAndSuspendBackgroundedTabs() {
     if (!content)
       continue;
 
-    PurgeAndSuspendState current_state =
-        GetWebContentsData(content)->GetPurgeAndSuspendState();
-    // If the tab's purge-and-suspend state is not RUNNING, the tab should be
-    // backgrounded. Since tab.last_hidden is updated everytime the tab is
-    // hidden, we should see tab.last_hidden < last_modified_time.
-    DCHECK(current_state == RUNNING ||
-           tab.last_hidden <
-               GetWebContentsData(content)->LastPurgeAndSuspendModifiedTime());
-    PurgeAndSuspendState next_state = GetNextPurgeAndSuspendState(
-        content, current_time, time_to_first_suspension_);
+    PurgeState current_state = GetWebContentsData(content)->GetPurgeState();
+    PurgeState next_state = GetNextPurgeState(content, current_time);
     if (current_state == next_state)
       continue;
 
@@ -776,17 +755,11 @@ void TabManager::PurgeAndSuspendBackgroundedTabs() {
     // timers for simplicity, so PurgeAndSuspend is called even after the
     // renderer is purged and suspended once. This should be replaced with
     // timers if we want necessary and sufficient signals.

[Wez, 2017/02/27 22:20:55]

It's not clear what this (pre-existing) comment is trying to express, but it
seems to conflict with the |continue| immediately preceding it - should the if()
continue be removed, or the comment be updated?

[tasak, 2017/02/28 09:45:37]

Done.

-    GetWebContentsData(content)->SetPurgeAndSuspendState(next_state);
-    switch (next_state) {
-      case SUSPENDED:
-        tab.render_process_host->PurgeAndSuspend();
-        break;
-      case RESUMED:
-        tab.render_process_host->Resume();
-        break;
-      case RUNNING:
-        NOTREACHED();
-    }
+    GetWebContentsData(content)->SetPurgeState(next_state);
+    DCHECK(next_state == PURGED);

[Wez, 2017/02/27 22:20:55]

As noted in the previous function, you can replace the if()continue with a
switch(next_state) with the two cases, to get compile-time checking that you
must be in the correct state (though see my comment re the comment above vs the
if() being inconsistent).

However, since GetNextPurgeState() never transitions from PURGED->NOT_PURGED
(that happens separately in response to activity) and since GetNextPurgeState()
already fetches the current state to shortcut the PURGED case, I'd suggest
change it to something like:

bool purge_now = ShouldPurgeAtTime(current_time);
if (!purge_now)
  continue

// Do purge stuff.
SetPurgeState(PURGED)
PurgeAndSuspend()

which would at least avoid making duplicate calls to GetPurgeState().

[tasak, 2017/02/28 09:45:37]

Done.

+    // TODO(tasak): rename PurgeAndSuspend with a better name, e.g.
+    // RequestPurgeCache, because we don't suspend any renderers.
+    tab.render_process_host->PurgeAndSuspend();
   }
 }
 
@@ -887,7 +860,15 @@ void TabManager::ActiveTabChanged(content::WebContents* old_contents,
                                   int index,
                                   int reason) {
   GetWebContentsData(new_contents)->SetDiscardState(false);
-  GetWebContentsData(new_contents)->SetPurgeAndSuspendState(RUNNING);
+  // To avoid purging many processes' cache at the same time, we will use
+  // time_to_first_purge_ + random value with in the range
+  // kRangeOfTimeToFirstPurgeInMinutes to invoke purge.

[Wez, 2017/02/27 22:20:55]

I think the more important thing to get across in this comment is that we are
re-setting the time-to-purge to a new random value every time a tab becomes
active (otherwise it's a little surprising to see us setting teh time-to-suspend
when the tab becomes active, rather than when it becomes inactive)

[tasak, 2017/02/28 09:45:37]

Moved the code into the statement: if(old_contents) { ... }, and added the code
to TabInsertedAt.

+  base::TimeDelta time_to_first_purge =
+      time_to_first_purge_ +
+      base::TimeDelta::FromMinutes(
+          base::RandGenerator(kRangeOfTimeToFirstPurgeInMinutes));
+  GetWebContentsData(new_contents)->SetTimeToFirstPurge(time_to_first_purge);

[Wez, 2017/02/27 22:20:55]

nit: I'd suggest calling this SetTimeToPurge(), so you can re-use it if you want
to do periodic purges; even if purge is a one-shot operation, it's still a
reasonable name.

[tasak, 2017/02/28 09:45:37]

Done.

+  GetWebContentsData(new_contents)->SetPurgeState(NOT_PURGED);
   // If |old_contents| is set, that tab has switched from being active to
   // inactive, so record the time of that transition.
   if (old_contents)