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

chrome/browser/memory/tab_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/memory/tab_manager.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <set>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/memory/memory_pressure_monitor.h"
 #include "base/metrics/field_trial.h"
 #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"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/tick_clock.h"
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/media/webrtc/media_capture_devices_dispatcher.h"
@@ skipping 44 matching lines @@
 // For each period of this length record a statistic to indicate whether or not
 // the user experienced a low memory event. If this interval is changed,
 // Tabs.Discard.DiscardInLastMinute must be replaced with a new statistic.
 const int kRecentTabDiscardIntervalSeconds = 60;
 #endif
 
 // If there has been no priority adjustment in this interval, assume the
 // 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.
 const int kAudioProtectionTimeSeconds = 60;
 
 int FindTabStripModelById(int64_t target_web_contents_id,
                           TabStripModel** model) {
   DCHECK(model);
   for (auto* browser : *BrowserList::GetInstance()) {
     TabStripModel* local_model = browser->tab_strip_model();
@@ skipping 120 matching lines @@
   }
 #endif
   // purge-and-suspend param is used for Purge+Suspend finch experiment
   // in the following way:
   // https://docs.google.com/document/d/1hPHkKtXXBTlsZx9s-9U17XC-ofEIzPo9FYbBEc7PPbk/edit?usp=sharing
   std::string purge_and_suspend_time = variations::GetVariationParamValue(
       "PurgeAndSuspend", "purge-and-suspend-time");
   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);
 }
 
 void TabManager::Stop() {
   update_timer_.Stop();
   recent_tab_discard_timer_.Stop();
   memory_pressure_listener_.reset();
 }
 
 TabStatsList TabManager::GetTabStats() const {
@@ skipping 433 matching lines @@
       stats.title = contents->GetTitle();
       stats.tab_contents_id = IdFromWebContents(contents);
       stats_list->push_back(stats);
     }
   }
 }
 
 // This function is called when |update_timer_| fires. It will adjust the clock
 // if needed (if it detects that the machine was asleep) and will fire the stats
 // updating on ChromeOS via the delegate. This function also tries to purge
 // cache memory and suspend tabs which becomes and keeps backgrounded for a

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

nit: Looks like this comment regarding purging is missing some words?

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

Done.

 // while.
 void TabManager::UpdateTimerCallback() {
   // If Chrome is shutting down, do not do anything.
   if (g_browser_process->IsShuttingDown())
     return;
 
   if (BrowserList::GetInstance()->empty())
     return;
 
   // Check for a discontinuity in time caused by the machine being suspended.
   if (!last_adjust_time_.is_null()) {
     TimeDelta suspend_time = NowTicks() - last_adjust_time_;
     if (suspend_time.InSeconds() > kSuspendThresholdSeconds) {
       // System was probably suspended, move the event timers forward in time so
       // when they get subtracted out later, "uptime" is being counted.
       start_time_ += suspend_time;
       if (!last_discard_time_.is_null())
         last_discard_time_ += suspend_time;
     }

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

Now we don't suspend any renderers. So we can remove this code.
However I'm not sure whether I should remove now or not...

I should just add TODO(tasak)?

   }
   last_adjust_time_ = NowTicks();
 
 #if defined(OS_CHROMEOS)
   TabStatsList stats_list = GetTabStats();
   // This starts the CrOS specific OOM adjustments in /proc/<pid>/oom_score_adj.
   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();
+  PurgeState state = GetWebContentsData(content)->GetPurgeState();
+  if (state == PURGED)
+    return state;
 
-  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;
-  }
+  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) {
     if (!tab.render_process_host->IsProcessBackgrounded())
       continue;
     if (!CanSuspendBackgroundedRenderer(tab.child_process_host_id))
       continue;
 
     WebContents* content = GetWebContentsById(tab.tab_contents_id);
     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;
 
     // TODO(hajimehoshi): Now calling PurgeAndSuspend is implemented without
     // 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();
   }
 }
 
 WebContents* TabManager::DiscardWebContentsAt(int index, TabStripModel* model) {
   // Can't discard active index.
   if (model->active_index() == index)
     return nullptr;
 
   WebContents* old_contents = model->GetWebContentsAt(index);
 
@@ skipping 80 matching lines @@
     data->SetRecentlyAudible(current_state);
     data->SetLastAudioChangeTime(NowTicks());
   }
 }
 
 void TabManager::ActiveTabChanged(content::WebContents* old_contents,
                                   content::WebContents* new_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)
     GetWebContentsData(old_contents)->SetLastInactiveTime(NowTicks());

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

Again, this is pre-existing code, but shouldn't this be SetLast*Active*Time()? 
Surely this is the last time the old tab was _active_?

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

As far as I understand, new_contents is a tab which has been just activated, and
old_contents is a tab which was active and has been just deactivated.

So old_contents's LastInactiveTime is updated here.

 }
 
 void TabManager::TabInsertedAt(TabStripModel* tab_strip_model,
                                content::WebContents* contents,
                                int index,
                                bool foreground) {
   // Only interested in background tabs, as foreground tabs get taken care of by
   // ActiveTabChanged.
   if (foreground)
     return;
@@ skipping 127 matching lines @@
   // platform.
   std::string allow_multiple_discards = variations::GetVariationParamValue(
       features::kAutomaticTabDiscarding.name, "AllowMultipleDiscards");
   return (allow_multiple_discards != "true");
 #else
   return false;
 #endif
 }
 
 }  // namespace memory