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

chrome/browser/memory/tab_manager.h

 // 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.
 
 #ifndef CHROME_BROWSER_MEMORY_TAB_MANAGER_H_
 #define CHROME_BROWSER_MEMORY_TAB_MANAGER_H_
 
 #include <stdint.h>
 
 #include <memory>
@@ skipping 141 matching lines @@
 
   // Returns true if |first| is considered less desirable to be killed than
   // |second|.
   static bool CompareTabStats(const TabStats& first, const TabStats& second);
 
   // Returns a unique ID for a WebContents. Do not cast back to a pointer, as
   // the WebContents could be deleted if the user closed the tab.
   static int64_t IdFromWebContents(content::WebContents* web_contents);
 
  private:
-  FRIEND_TEST_ALL_PREFIXES(TabManagerTest,
-                           ActivateTabResetPurgeAndSuspendState);
-  FRIEND_TEST_ALL_PREFIXES(TabManagerTest, NextPurgeAndSuspendState);
+  FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ActivateTabResetPurgeState);
+  FRIEND_TEST_ALL_PREFIXES(TabManagerTest, NextPurgeState);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, AutoDiscardable);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, CanOnlyDiscardOnce);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ChildProcessNotifications);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, Comparator);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, DiscardedTabKeepsLastActiveTime);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, DiscardWebContentsAt);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, InvalidOrEmptyURL);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, IsInternalPage);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, OomPressureListener);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ProtectPDFPages);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ProtectRecentlyUsedTabs);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ProtectVideoTabs);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, ReloadDiscardedTabContextMenu);
   FRIEND_TEST_ALL_PREFIXES(TabManagerTest, TabManagerBasics);
-  FRIEND_TEST_ALL_PREFIXES(TabManagerWebContentsDataTest, PurgeAndSuspendState);
+  FRIEND_TEST_ALL_PREFIXES(TabManagerWebContentsDataTest, PurgeState);
 
   // The time of the first purging after a renderer is backgrounded.
   // The initial value was chosen because most of users activate backgrounded
   // tabs within 30 minutes. (c.f. Tabs.StateTransfer.Time_Inactive_Active)
   static constexpr base::TimeDelta kDefaultTimeToFirstPurge =
       base::TimeDelta::FromMinutes(30);
 
   // This is needed so WebContentsData can call OnDiscardedStateChange, and
-  // can use PurgeAndSuspendState.
+  // can use PurgeState.
   friend class WebContentsData;
 
   // Called by WebContentsData whenever the discard state of a WebContents
   // changes, so that observers can be informed.
   void OnDiscardedStateChange(content::WebContents* contents,
                               bool is_discarded);
 
   // Called by WebContentsData whenever the auto-discardable state of a
   // WebContents changes, so that observers can be informed.
   void OnAutoDiscardableStateChange(content::WebContents* contents,
@@ skipping 52 matching lines @@
   // If |active_model| is true, consider its first tab as being active.
   void AddTabStats(const TabStripModel* model,
                    bool is_app,
                    bool active_model,
                    TabStatsList* stats_list) const;
 
   // Callback for when |update_timer_| fires. Takes care of executing the tasks
   // that need to be run periodically (see comment in implementation).
   void UpdateTimerCallback();
 
-  // Initially PurgeAndSuspendState is RUNNING.
-  // RUNNING => SUSPENDED
+  // Initially PurgeState is NOT_PURGED.
+  // NOT_PURGED => PURGED
   // - A tab has been backgrounded for more than purge-and-suspend-time
   //   seconds.
-  // SUSPENDED => RESUMED
-  // - A suspended tab is still suspended (i.e. last active time < last
-  //   purge-and-suspend modified time), and
-  // - kMaxTimeRendererAllowedToBeSuspendedBeforeResume time passes since
-  //   since the tab was suspended.
-  // RESUMED => SUSPENDED
-  // - A resumed tab is still backgrounded (i.e. last active time < last
-  //   purge-and-suspend modified time), and
-  // - kSuspendedRendererLengthOfResumption time passes since the tab was
-  //   resumed.
-  // SUSPENDED, RESUMED, RUNNING => RUNNING
-  // - When ActiveTabChaged, the newly activated tab's state will be RUNNING.
-  enum PurgeAndSuspendState {
-    RUNNING,
-    RESUMED,
-    SUSPENDED,
+  // PURGED => NOT_PURGED
+  // - When ActiveTabChaged, the newly activated tab's state will be NOT_PURGED.

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

typo: ActiveTabChanged

The naming here implies that purging is a one-off operation, i.e. that we purge
one, at the point of switching between states - but I think we actually purge
every 20 minutes?

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

The new code doesn't purge every 20 minutes, because I think, ActiveTabChanged
is invoked when a user moves a background tab to foreground.

[Wez, 2017/03/02 02:28:32]

I thought the idea was to re-purge each background tab every 20 minutes or so,
though?

[tasak, 2017/03/02 09:22:47]

As far as I understand, we have a strong concern that re-purge occurs under
critical memory pressure situation.
I agree that 1 purge also occurs under the same situation, but next purge
doesn't.
So I would like to start from 1 purge implementation.

+  enum PurgeState {

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

Note that as per style-guide, this and the other enums and types defined in this
private section should preferably come before function and method definitions.

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

Removed.

+    NOT_PURGED,
+    PURGED,
   };
   // Returns WebContents whose contents id matches the given tab_contents_id.
   content::WebContents* GetWebContentsById(int64_t tab_contents_id) const;

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

Why is this getter placed randomly in-between the enum typedef and
GetNextPurgeState()?

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

Removed.

 
   // Returns the next state of the purge and suspend.

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

The function is called GetNextPurgeState() so the reader can see that it returns
the next state - this comment should clarify what that _means_, and how it is
used.

e.g. "Returns PURGED if |content| should be asked to purge memory at time
|current_time|".

Why do we need a |current_time| parameter - why not use Now()? If the point is
that we sometimes pass in something other than |current_time| then rename it to
just |time| or similar?

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

Replaced the method with ShouldPurgeAtTime.

On 2017/02/27 22:20:55, Wez wrote:
For testing. If we use Now() in the method, it is difficult to pass test_clock.

[Wez, 2017/03/02 02:28:32]

The class has a set_test_tick_clock() method, though, so surely we just set
that, and then call this function after having fast-forwarded the clock?

[tasak, 2017/03/02 09:22:46]

I see.
Done.

-  PurgeAndSuspendState GetNextPurgeAndSuspendState(
-      content::WebContents* content,
-      base::TimeTicks current_time,
-      const base::TimeDelta& time_to_first_suspension) const;
+  PurgeState GetNextPurgeState(content::WebContents* content,
+                               base::TimeTicks current_time) const;
 
-  // Purges and suspends renderers in backgrounded tabs.
-  void PurgeAndSuspendBackgroundedTabs();
+  // Purges renderers in backgrounded tabs.

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

Again, the reader can tell that from the name; what's important is what that
means, and how it happens, e.g. is this called periodically by some internal
timer? Or is it called whenever a tab state changes?

Also, the comment is not strictly accurate - this function only purges tabs
which are not already in the PURGED state.

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

Done.

+  void PurgeBackgroundedTabs();
 
   // Does the actual discard by destroying the WebContents in |model| at |index|
   // and replacing it by an empty one. Returns the new WebContents or NULL if
   // the operation fails (return value used only in testing).
   content::WebContents* DiscardWebContentsAt(int index, TabStripModel* model);
 
   // Called by the memory pressure listener when the memory pressure rises.
   void OnMemoryPressure(
       base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level);
 
@@ skipping 62 matching lines @@
   // used for statistics normalized by usage.
   bool recent_tab_discard_;
 
   // Whether a tab can only ever discarded once.
   bool discard_once_;
 
   // This allows protecting tabs for a certain amount of time after being
   // backgrounded.
   base::TimeDelta minimum_protection_time_;
 
-  // A backgrounded renderer will be suspended when this time passes.
-  base::TimeDelta time_to_first_suspension_;
+  // A backgrounded renderer will be purged when this time passes.
+  base::TimeDelta time_to_first_purge_;

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

This doesn't change over the lifetime of the TabManager, so you could initialize
it in the constructor and make the member const.

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

So is it possible to initialize the value by using GetVariationParamValue and
StringToUint in the constructor?

As far as I understand, TabManager::Start() is invoked at almost the same time
as TabManager is created. I think, it is ok to initialize the value in Start().

 
 #if defined(OS_CHROMEOS)
   std::unique_ptr<TabManagerDelegate> delegate_;
 #endif
 
   // Responsible for automatically registering this class as an observer of all
   // TabStripModels. Automatically tracks browsers as they come and go.
   BrowserTabStripTracker browser_tab_strip_tracker_;
 
   // Pointer to a test clock. If this is set, NowTicks() returns the value of
@@ skipping 39 matching lines @@
 
   // Weak pointer factory used for posting delayed tasks to task_runner_.
   base::WeakPtrFactory<TabManager> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(TabManager);
 };
 
 }  // namespace memory
 
 #endif  // CHROME_BROWSER_MEMORY_TAB_MANAGER_H_