Make purge-and-suspend-time finch experiment parameter

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>
@@ skipping 203 matching lines @@
   if (monitor) {
     memory_pressure_listener_.reset(new base::MemoryPressureListener(
         base::Bind(&TabManager::OnMemoryPressure, base::Unretained(this))));
     base::MemoryPressureListener::MemoryPressureLevel level =
         monitor->GetCurrentPressureLevel();
     if (level == base::MemoryPressureListener::MEMORY_PRESSURE_LEVEL_CRITICAL) {
       OnMemoryPressure(level);
     }
   }
 #endif
+  std::string purge_and_suspend_time = variations::GetVariationParamValue(

[chrisha, 2016/11/24 17:13:09]

Should we have a default time that this is initialized to in the absence of a
variation param?

[tasak, 2016/11/25 01:15:24]

As far as I understand, base::TimeDelta() initializes delta_ as 0.
If purge-and-suspend-time is 0, purge-and-suspend is disabled (this is the same
behavior as the original purge-and-suspend-time).

Looking at minimum_protection_time_, it also depends on base::TimeDelta()'s
behavior (initializing delta_ as 0).
So do we need to explicitly initialize time_to_first_suspension_?

[chrisha, 2016/11/29 14:13:57]

Okay, that makes sense. I was thinking that there'd be a different on/off switch
for feature itself, and that only a non-zero purge-and-suspend delay would be
configurable via this variation. In which case having a non-zero default value
when the variation is not present would make sense.

This is fine, but I'd at least cleanly document that a zero purge and suspend
time indicates that the feature is disabled?

[tasak, 2016/12/02 02:49:24]

Yes... currently the zero means that the purge+suspend feature is disabled.

+      "PurgeAndSuspend", "purge-and-suspend-time");
+  if (!purge_and_suspend_time.empty()) {
+    int time_to_first_suspension_sec = 0;
+    if (base::StringToInt(purge_and_suspend_time,
+                          &time_to_first_suspension_sec)) {
+      if (time_to_first_suspension_sec > 0)
+        time_to_first_suspension_ =
+            base::TimeDelta::FromSeconds(time_to_first_suspension_sec);
+    }
+  }
 }
 
 void TabManager::Stop() {
   update_timer_.Stop();
   recent_tab_discard_timer_.Stop();
   memory_pressure_listener_.reset();
 }
 
 TabStatsList TabManager::GetTabStats() {
   TabStatsList stats_list(GetUnsortedTabStats());
@@ skipping 490 matching lines @@
       break;
     case SUSPENDED:
       if (time_passed > kDurationOfRendererSuspension)
         return RESUMED;
       break;
   }
   return state;
 }
 
 void TabManager::PurgeAndSuspendBackgroundedTabs() {
-  const base::CommandLine& command_line =
-      *base::CommandLine::ForCurrentProcess();
-  if (!command_line.HasSwitch(switches::kPurgeAndSuspendTime))
+  if (time_to_first_suspension_.InSeconds() <= 0)

[chrisha, 2016/11/24 17:13:09]

Do we need this check? This function simply shouldn't be called if
PurgeAndSuspend isn't enabled.

[tasak, 2016/11/25 01:15:24]

I would like to confirm. You mean, we should check
base::FeatureList::IsEnabled(kPurgeAndSuspend) before calling this function?

If so, I would like to explain again. I would like to do A/B testing. A is
"recording UMA when purge+suspend is really executed." and B is "recording UMA
at almost the same time when purge+suspend is executed but we don't
purge+suspend." (So B is "recording UMA when about purge-and-suspend time passes
after a renderer is backgrounded.)

If we don't call this function when
!base::FeatureList::IsEnabled(kPurgeAndSuspend), it is not possible to record B.

I'm not sure whether it is ok to always record B or not when we don't start any
finch experiment. So currently B's code (i.e.
RenderThreadImp::OnProcessPurgeAndSuspend and so on) is not called when
purge-and-suspend-time=0 (and 0 is default value).

[tasak, 2016/11/25 01:19:44]

So my first patch is trying to always record B.
I thought, if someone wants not to change chrome behavior when
!IsEnabled(kPurgeAndSuspend), I will keep purge-and-suspend-time.

[chrisha, 2016/11/29 14:13:57]

I'm not sure I follow this entire chain of logic? This function is only ever
called from UpdateTimerCallback. If time_to_first_suspension_ is <= 0, then it
immediately returns, without recording anything.

Where are these UMAs you speak of? Will you be adding them to this function?

Regardless, I'm fine with leaving the code as is but more comments in the code
would be very useful here. Something as simple as:

// A zero-valued |time_to_first_suspension_| means the feature is disabled, so
return immediately.

[tasak, 2016/12/02 02:49:24]

I see.... I'm trying to keep the existing behavior... But now we have
purge+suspend feature. We don't need to control whether purge+suspend is enabled
or not by purge-and-suspend-time.
I also added the code to make default purge-and-suspend-time 30min.
Done.

     return;
-  int purge_and_suspend_time = 0;
-  if (!base::StringToInt(
-          command_line.GetSwitchValueASCII(switches::kPurgeAndSuspendTime),
-          &purge_and_suspend_time)) {
-    return;
-  }
-  if (purge_and_suspend_time <= 0)
-    return;
+
   base::TimeTicks current_time = NowTicks();
-  base::TimeDelta time_to_first_suspension =
-      base::TimeDelta::FromSeconds(purge_and_suspend_time);
   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);
+        content, current_time, time_to_first_suspension_);
     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.
     GetWebContentsData(content)->SetPurgeAndSuspendState(next_state);
     switch (next_state) {
       case SUSPENDED:
@@ skipping 250 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