Record histograms when an urgent discard request is received.

This CL removes the following histograms which were recorded before
each individual tab discard:
- Tabs.Discard.TabCount
- Tabs.Discard.InitialTime2
- Tabs.Discard.IntervalTime2

And replaces them with histograms recorded when an urgent discard
request is received, before any tab is discarded:
- Discarding.Urgent.NumAliveTabs
- Discarding.Urgent.TimeSinceStartup
- Discarding.Urgent.TimeSinceLastUrgent

Also, metrics::RecordMemoryStats() is called when an urgent discard
request is received rather than before each individual tab discard.

The reason for this change is that we are interested in knowing
how often urgent discard requests occur and what leads to them. We
don't care how often we proactively discard tabs and we don't
need multiple histogram samples when we discard multiple tabs in
response to a single discard request.

Bug: 775644
Change-Id: Ic603a904dc9635b12b3204c1e12bc293c4dcbaf7
Reviewed-on: https://chromium-review.googlesource.com/786295Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521389}

chrome/browser/resource_coordinator/tab_manager.cc

@@ -264,7 +264,6 @@ void TabManager::Start() {
 // MemoryPressureMonitor is not implemented on Linux so far and tabs are never
 // discarded.
 #if defined(OS_WIN) || defined(OS_MACOSX) || defined(OS_CHROMEOS)
-  start_time_ = NowTicks();
   // Create a |MemoryPressureListener| to listen for memory events when
   // MemoryCoordinator is disabled. When MemoryCoordinator is enabled
   // it asks TabManager to do tab discarding.
@@ -411,6 +410,9 @@ bool TabManager::CanDiscardTab(const TabStats& tab_stats,
 }
 
 void TabManager::DiscardTab(DiscardReason reason) {
+  if (reason == DiscardReason::kUrgent)
+    stats_collector_->RecordWillDiscardUrgently(GetNumAliveTabs());
+
 #if defined(OS_CHROMEOS)
   // Call Chrome OS specific low memory handling process.
   if (base::FeatureList::IsEnabled(features::kArcMemoryManagement)) {
@@ -637,41 +639,6 @@ bool TabManager::IsInternalPage(const GURL& url) {
   return false;
 }
 
-void TabManager::RecordDiscardStatistics() {
-  discard_count_++;
-
-  // TODO(jamescook): Maybe incorporate extension count?
-  UMA_HISTOGRAM_CUSTOM_COUNTS("Tabs.Discard.TabCount", GetTabCount(), 1, 100,
-                              50);
-  // TODO(jamescook): If the time stats prove too noisy, then divide up users
-  // based on how heavily they use Chrome using tab count as a proxy.
-  // Bin into <= 1, <= 2, <= 4, <= 8, etc.
-  if (last_discard_time_.is_null()) {
-    // This is the first discard this session.
-    TimeDelta interval = NowTicks() - start_time_;
-    int interval_seconds = static_cast<int>(interval.InSeconds());
-    // Record time in seconds over an interval of approximately 1 day.
-    UMA_HISTOGRAM_CUSTOM_COUNTS("Tabs.Discard.InitialTime2", interval_seconds,
-                                1, 100000, 50);
-  } else {
-    // Not the first discard, so compute time since last discard.
-    TimeDelta interval = NowTicks() - last_discard_time_;
-    int interval_ms = static_cast<int>(interval.InMilliseconds());
-    // Record time in milliseconds over an interval of approximately 1 day.
-    // Start at 100 ms to get extra resolution in the target 750 ms range.
-    UMA_HISTOGRAM_CUSTOM_COUNTS("Tabs.Discard.IntervalTime2", interval_ms, 100,
-                                100000 * 1000, 50);
-  }
-// TODO(georgesak): Remove this #if when RecordMemoryStats is implemented for
-// all platforms.
-#if defined(OS_WIN) || defined(OS_CHROMEOS)
-  // Record system memory usage at the time of the discard.
-  metrics::RecordMemoryStats(metrics::RECORD_MEMORY_STATS_TAB_DISCARDED);
-#endif
-  // Set up to record the next interval.
-  last_discard_time_ = NowTicks();
-}
-
 void TabManager::PurgeBrowserMemory() {
   // Based on experimental evidence, attempts to free memory from renderers
   // have been too slow to use in OOM situations (V8 garbage collection) or
@@ -809,9 +776,7 @@ WebContents* TabManager::DiscardWebContentsAt(int index,
   if (GetWebContentsData(old_contents)->IsDiscarded())
     return nullptr;
 
-  // Record statistics before discarding to capture the memory state that leads
-  // to the discard.
-  RecordDiscardStatistics();
+  ++discard_count_;
 
   UMA_HISTOGRAM_BOOLEAN(
       "TabManager.Discarding.DiscardedTabHasBeforeUnloadHandler",
@@ -1311,6 +1276,23 @@ bool TabManager::ComparePendingNavigations(
   return false;
 }
 
+int TabManager::GetNumAliveTabs() const {
+  int tab_count = 0;
+  for (auto* browser : *BrowserList::GetInstance()) {
+    TabStripModel* tab_strip_model = browser->tab_strip_model();
+    for (int index = 0; index < tab_strip_model->count(); ++index) {
+      content::WebContents* contents = tab_strip_model->GetWebContentsAt(index);
+      if (!IsTabDiscarded(contents))
+        ++tab_count;
+    }
+  }
+
+  tab_count -= pending_navigations_.size();
+  DCHECK_GE(tab_count, 0);
+
+  return tab_count;
+}
+
 bool TabManager::IsTabLoadingForTest(content::WebContents* contents) const {
   if (loading_contents_.count(contents) == 1) {
     DCHECK_EQ(TAB_IS_LOADING,

chrome/browser/resource_coordinator/tab_manager.h

@@ -321,11 +321,6 @@ class TabManager : public TabStripModelObserver,
   // can be easily reloaded and hence makes a good choice to discard.
   static bool IsInternalPage(const GURL& url);
 
-  // Records UMA histogram statistics for a tab discard. Record statistics for
-  // user triggered discards via chrome://discards/ because that allows to
-  // manually test the system.
-  void RecordDiscardStatistics();
-
   // Purges data structures in the browser that can be easily recomputed.
   void PurgeBrowserMemory();
 
@@ -462,6 +457,9 @@ class TabManager : public TabStripModelObserver,
       const BackgroundTabNavigationThrottle* first,
       const BackgroundTabNavigationThrottle* second);
 
+  // Returns the number of tabs that are not pending load or discarded.
+  int GetNumAliveTabs() const;
+
   // Check if the tab is loading. Use only in tests.
   bool IsTabLoadingForTest(content::WebContents* contents) const;
 
@@ -488,13 +486,6 @@ class TabManager : public TabStripModelObserver,
   // A listener to global memory pressure events.
   std::unique_ptr<base::MemoryPressureListener> memory_pressure_listener_;
 
-  // Wall-clock time when the priority manager started running.
-  base::TimeTicks start_time_;
-
-  // Wall-clock time of last tab discard during this browsing session, or 0 if
-  // no discard has happened yet.
-  base::TimeTicks last_discard_time_;
-
   // Number of times a tab has been discarded, for statistics.
   int discard_count_;
 

chrome/browser/resource_coordinator/tab_manager_stats_collector.cc

@@ -16,11 +16,13 @@
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/time/time.h"
+#include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/resource_coordinator/resource_coordinator_web_contents_observer.h"
 #include "chrome/browser/resource_coordinator/tab_manager_web_contents_data.h"
 #include "chrome/browser/resource_coordinator/time.h"
 #include "chrome/browser/sessions/session_restore.h"
+#include "components/metrics/system_memory_stats_recorder.h"
 #include "content/public/browser/swap_metrics_driver.h"
 #include "services/metrics/public/cpp/ukm_builders.h"
 #include "services/metrics/public/cpp/ukm_recorder.h"
@@ -115,11 +117,7 @@ class TabManagerStatsCollector::SwapMetricsDelegate
   const SessionType session_type_;
 };
 
-TabManagerStatsCollector::TabManagerStatsCollector()
-    : is_session_restore_loading_tabs_(false),
-      is_in_background_tab_opening_session_(false),
-      is_overlapping_session_restore_(false),
-      is_overlapping_background_tab_opening_(false) {
+TabManagerStatsCollector::TabManagerStatsCollector() {
   SessionRestore::AddObserver(this);
 }
 
@@ -127,6 +125,34 @@ TabManagerStatsCollector::~TabManagerStatsCollector() {
   SessionRestore::RemoveObserver(this);
 }
 
+void TabManagerStatsCollector::RecordWillDiscardUrgently(int num_alive_tabs) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  base::TimeTicks discard_time = NowTicks();
+
+  UMA_HISTOGRAM_COUNTS_100("Discarding.Urgent.NumAliveTabs", num_alive_tabs);
+
+  if (last_urgent_discard_time_.is_null()) {
+    UMA_HISTOGRAM_CUSTOM_TIMES(
+        "Discarding.Urgent.TimeSinceStartup", discard_time - start_time_,
+        base::TimeDelta::FromSeconds(1), base::TimeDelta::FromDays(1), 50);
+  } else {
+    UMA_HISTOGRAM_CUSTOM_TIMES("Discarding.Urgent.TimeSinceLastUrgent",
+                               discard_time - last_urgent_discard_time_,
+                               base::TimeDelta::FromMilliseconds(100),
+                               base::TimeDelta::FromDays(1), 50);
+  }
+
+// TODO(fdoray): Remove this #if when RecordMemoryStats is implemented for all
+// platforms.
+#if defined(OS_WIN) || defined(OS_CHROMEOS)
+  // Record system memory usage at the time of the discard.
+  metrics::RecordMemoryStats(metrics::RECORD_MEMORY_STATS_TAB_DISCARDED);
+#endif
+
+  last_urgent_discard_time_ = discard_time;
+}
+
 void TabManagerStatsCollector::RecordSwitchToTab(
     content::WebContents* old_contents,
     content::WebContents* new_contents) {
@@ -165,6 +191,8 @@ void TabManagerStatsCollector::RecordSwitchToTab(
 void TabManagerStatsCollector::RecordExpectedTaskQueueingDuration(
     content::WebContents* contents,
     base::TimeDelta queueing_time) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (!contents->IsVisible())
     return;
 
@@ -185,7 +213,7 @@ void TabManagerStatsCollector::RecordExpectedTaskQueueingDuration(
           TabManager_SessionRestore_ForegroundTab_ExpectedTaskQueueingDurationInfo(
               ukm_source_id)
               .SetExpectedTaskQueueingDuration(queueing_time.InMilliseconds())
-              .SetSequenceId(sequence_->GetNext())
+              .SetSequenceId(sequence_++)
               .SetSessionRestoreSessionId(session_id_)
               .SetSessionRestoreTabCount(restored_tab_count)
               .SetSystemTabCount(
@@ -213,7 +241,7 @@ void TabManagerStatsCollector::RecordExpectedTaskQueueingDuration(
               .SetBackgroundTabOpeningSessionId(session_id_)
               .SetBackgroundTabPendingCount(background_tab_pending_count)
               .SetExpectedTaskQueueingDuration(queueing_time.InMilliseconds())
-              .SetSequenceId(sequence_->GetNext())
+              .SetSequenceId(sequence_++)
               .SetSystemTabCount(
                   g_browser_process->GetTabManager()->GetTabCount())
               .Record(ukm::UkmRecorder::Get());
@@ -329,6 +357,8 @@ void TabManagerStatsCollector::OnWillLoadNextBackgroundTab(bool timeout) {
 }
 
 void TabManagerStatsCollector::OnTabIsLoaded(content::WebContents* contents) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   if (!base::ContainsKey(foreground_contents_switched_to_times_, contents))
     return;
 
@@ -343,7 +373,7 @@ void TabManagerStatsCollector::OnTabIsLoaded(content::WebContents* contents) {
       ukm::builders::
           TabManager_Experimental_SessionRestore_TabSwitchLoadStopped(
               ukm_source_id)
-              .SetSequenceId(sequence_->GetNext())
+              .SetSequenceId(sequence_++)
               .SetSessionRestoreSessionId(session_id_)
               .SetSessionRestoreTabCount(
                   g_browser_process->GetTabManager()->restored_tab_count())
@@ -368,7 +398,7 @@ void TabManagerStatsCollector::OnTabIsLoaded(content::WebContents* contents) {
               .SetBackgroundTabPendingCount(
                   g_browser_process->GetTabManager()
                       ->GetBackgroundTabPendingCount())
-              .SetSequenceId(sequence_->GetNext())
+              .SetSequenceId(sequence_++)
               .SetSystemTabCount(
                   g_browser_process->GetTabManager()->GetTabCount())
               .SetTabSwitchLoadTime(switch_load_time.InMilliseconds())
@@ -402,11 +432,12 @@ void TabManagerStatsCollector::ClearStatsWhenInOverlappedSession() {
 }
 
 void TabManagerStatsCollector::UpdateSessionAndSequence() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
   // This function is used by both SessionRestore and BackgroundTabOpening. This
   // is fine because we do not report any metric when those two overlap.
-  static base::AtomicSequenceNumber session_seq;
-  session_id_ = session_seq.GetNext();
-  sequence_.reset(new base::AtomicSequenceNumber());
+  ++session_id_;
+  sequence_ = 0;
 }
 
 // static

chrome/browser/resource_coordinator/tab_manager_stats_collector.h

@@ -11,15 +11,12 @@
 #include <string>
 #include <unordered_map>
 
-#include "base/atomic_sequence_num.h"
 #include "base/gtest_prod_util.h"
+#include "base/sequence_checker.h"
 #include "base/time/time.h"
+#include "chrome/browser/resource_coordinator/time.h"
 #include "chrome/browser/sessions/session_restore_observer.h"
 
-namespace base {
-class TimeDelta;
-}
-
 namespace content {
 class SwapMetricsDriver;
 class WebContents;
@@ -86,6 +83,11 @@ class TabManagerStatsCollector final : public SessionRestoreObserver {
   TabManagerStatsCollector();
   ~TabManagerStatsCollector();
 
+  // Records histograms *before* starting to urgently discard LifecycleUnits.
+  // |num_alive_tabs| is the number of tabs that are not pending load or
+  // discarded.
+  void RecordWillDiscardUrgently(int num_alive_tabs);
+
   // Records UMA histograms for the tab state when switching to a different tab
   // during session restore.
   void RecordSwitchToTab(content::WebContents* old_contents,
@@ -204,14 +206,23 @@ class TabManagerStatsCollector final : public SessionRestoreObserver {
   static const char kHistogramSessionOverlapSessionRestore[];
   static const char kHistogramSessionOverlapBackgroundTabOpening[];
 
-  int session_id_;
-  std::unique_ptr<base::AtomicSequenceNumber> sequence_;
+  // TabManagerStatsCollector should be used from a single sequence.
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  // Time at which the TabManagerStatsCollector was created.
+  const base::TimeTicks start_time_ = NowTicks();
+
+  // Last time at which TabManager had to urgently discard LifecycleUnits.
+  base::TimeTicks last_urgent_discard_time_;
+
+  int session_id_ = -1;
+  int sequence_ = 0;
 
-  bool is_session_restore_loading_tabs_;
-  bool is_in_background_tab_opening_session_;
+  bool is_session_restore_loading_tabs_ = false;
+  bool is_in_background_tab_opening_session_ = false;
 
-  bool is_overlapping_session_restore_;
-  bool is_overlapping_background_tab_opening_;
+  bool is_overlapping_session_restore_ = false;
+  bool is_overlapping_background_tab_opening_ = false;
 
   // This is shared between SessionRestore and BackgroundTabOpening because we
   // do not report metrics when those two overlap.

tools/metrics/histograms/histograms.xml

@@ -14932,6 +14932,32 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
   </summary>
 </histogram>
 
+<histogram name="Discarding.Urgent.NumAliveTabs" units="tabs">
+  <owner>fdoray@chromium.org</owner>
+  <summary>
+    Number of tabs that are not pending load or discarded when an urgent discard
+    request is received.
+  </summary>
+</histogram>
+
+<histogram name="Discarding.Urgent.TimeSinceLastUrgent" units="ms">
+  <owner>fdoray@chromium.org</owner>
+  <summary>
+    Time between two consecutive urgent discard requests. Urgent discards are
+    undesirable; it is better to reduce memory usage before the system is in a
+    bad state.
+  </summary>
+</histogram>
+
+<histogram name="Discarding.Urgent.TimeSinceStartup" units="ms">
+  <owner>fdoray@chromium.org</owner>
+  <summary>
+    Time between Chrome startup and the first urgent discard request. Urgent
+    discards are undesirable; it is better to reduce memory usage before the
+    system is in a bad state.
+  </summary>
+</histogram>
+
 <histogram name="DiskBasedCertCache.CertIo" enum="CertificateChainPosition">
   <obsolete>
     Deprecated as of 01/2016. CertCacheTrial has been removed.
@@ -87924,6 +87950,11 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 </histogram>
 
 <histogram name="Tabs.Discard.InitialTime2" units="seconds">
+  <obsolete>
+    Deprecated 11/2017. Replaced with Discarding.Urgent.TimeSinceStartup which
+    is recorded when Chrome has to discard tabs or apps urgently (instead of
+    before an individual tab is urgently or proactively discarded).
+  </obsolete>
   <owner>jamescook@chromium.org</owner>
   <summary>
     Time in seconds between system startup and when the first tab is discarded
@@ -87945,6 +87976,11 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 </histogram>
 
 <histogram name="Tabs.Discard.IntervalTime2" units="ms">
+  <obsolete>
+    Deprecated 11/2017. Replaced with Discarding.Urgent.TimeSinceLastUrgent
+    which is recorded before a set of tabs/apps are urgently discarded (instead
+    of before an individual tab is urgently or proactively discarded).
+  </obsolete>
   <owner>jamescook@chromium.org</owner>
   <summary>
     Time in milliseconds between tab discard events after the first one,
@@ -88016,6 +88052,12 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
 </histogram>
 
 <histogram name="Tabs.Discard.TabCount" units="tabs">
+  <obsolete>
+    Deprecated 11/2017. Replaced with Discarding.Urgent.NumAliveTabs which
+    records the number of tabs that are not pending load or discarded when an
+    urgent discard request is received (vs. this that records the total number
+    of tabs every time a tab is discarded, no matter the reason).
+  </obsolete>
   <owner>jamescook@chromium.org</owner>
   <summary>
     The number of tabs open across all browser windows when a tab was discarded