Skip to content

Commit

Permalink
Do not kill recently killed ARC processes again
Browse files Browse the repository at this point in the history 
ARC processes sometimes respawn right after being killed. In that case,
killing them every time is just a waste of resources.

BUG=706048
TEST=try
TEST=Follow crbug.com/706048#c76, confirm Chrome no longer kills the
 processes too often.

Review-Url: https://codereview.chromium.org/2850203003
Cr-Commit-Position: refs/heads/master@{#468835}
  • Loading branch information
@yusukes
yusukes authored and Commit bot committed May 3, 2017
1 parent 9c5225a commit b784d07
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 6 deletions.
17 changes: 16 additions & 1 deletion chrome/browser/memory/tab_manager_delegate_chromeos.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

#include <algorithm>
#include <map>
#include <string>
#include <vector>

#include "ash/shell.h"
Expand Down Expand Up @@ -51,6 +50,7 @@

using base::ProcessHandle;
using base::TimeDelta;
using base::TimeTicks;
using content::BrowserThread;

namespace memory {
Expand Down Expand Up @@ -554,6 +554,15 @@ TabManagerDelegate::GetSortedCandidates(
return candidates;
}

bool TabManagerDelegate::IsRecentlyKilledArcProcess(
const std::string& process_name,
const TimeTicks& now) {
const auto it = recently_killed_arc_processes_.find(process_name);
if (it == recently_killed_arc_processes_.end())
return false;
return (now - it->second) <= GetArcRespawnKillDelay();
}

bool TabManagerDelegate::KillArcProcess(const int nspid) {
auto* arc_service_manager = arc::ArcServiceManager::Get();
if (!arc_service_manager)
Expand Down Expand Up @@ -590,6 +599,7 @@ void TabManagerDelegate::LowMemoryKillImpl(
GetSortedCandidates(tab_list, arc_processes);

int target_memory_to_free_kb = mem_stat_->TargetMemoryToFreeKB();
const TimeTicks now = TimeTicks::Now();

// Kill processes until the estimated amount of freed memory is sufficient to
// bring the system memory back to a normal level.
Expand All @@ -610,9 +620,14 @@ void TabManagerDelegate::LowMemoryKillImpl(
continue;
}
if (it->app()) {
if (IsRecentlyKilledArcProcess(it->app()->process_name(), now)) {
MEMORY_LOG(ERROR) << "Avoided killing " << *it << " too often";
continue;
}
int estimated_memory_freed_kb =
mem_stat_->EstimatedMemoryFreedKB(it->app()->pid());
if (KillArcProcess(it->app()->nspid())) {
recently_killed_arc_processes_[it->app()->process_name()] = now;
target_memory_to_free_kb -= estimated_memory_freed_kb;
MemoryKillsMonitor::LogLowMemoryKill("APP", estimated_memory_freed_kb);
MEMORY_LOG(ERROR) << "Killed " << *it << ", estimated "
Expand Down
25 changes: 24 additions & 1 deletion chrome/browser/memory/tab_manager_delegate_chromeos.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#define CHROME_BROWSER_MEMORY_TAB_MANAGER_DELEGATE_CHROMEOS_H_

#include <memory>
#include <string>
#include <utility>
#include <vector>

Expand Down Expand Up @@ -78,6 +79,11 @@ class TabManagerDelegate : public aura::client::ActivationChangeObserver,
// Called when the timer fires, sets oom_adjust_score for all renderers.
void AdjustOomPriorities(const TabStatsList& tab_list);

// Returns true if the process has recently been killed.
// Virtual for unit testing.
virtual bool IsRecentlyKilledArcProcess(const std::string& process_name,
const base::TimeTicks& now);

protected:
// Kills an ARC process. Returns true if the kill request is successfully sent
// to Android. Virtual for unit testing.
Expand All @@ -87,11 +93,14 @@ class TabManagerDelegate : public aura::client::ActivationChangeObserver,
// Virtual for unit testing.
virtual bool KillTab(int64_t tab_id);

// Get debugd client instance.
// Get debugd client instance. Virtual for unit testing.
virtual chromeos::DebugDaemonClient* GetDebugDaemonClient();

private:
FRIEND_TEST_ALL_PREFIXES(TabManagerDelegateTest, CandidatesSorted);
FRIEND_TEST_ALL_PREFIXES(TabManagerDelegateTest, IsRecentlyKilledArcProcess);
FRIEND_TEST_ALL_PREFIXES(TabManagerDelegateTest,
DoNotKillRecentlyKilledArcProcesses);
FRIEND_TEST_ALL_PREFIXES(TabManagerDelegateTest, KillMultipleProcesses);
FRIEND_TEST_ALL_PREFIXES(TabManagerDelegateTest, SetOomScoreAdj);

Expand All @@ -112,6 +121,9 @@ class TabManagerDelegate : public aura::client::ActivationChangeObserver,
// Cache OOM scores in memory.
typedef base::hash_map<base::ProcessHandle, int> ProcessScoreMap;

// A map from an ARC process name to a monotonic timestamp when it's killed.
typedef base::hash_map<std::string, base::TimeTicks> KilledArcProcessesMap;

// Get the list of candidates to kill, sorted by descending importance.
static std::vector<Candidate> GetSortedCandidates(
const TabStatsList& tab_list,
Expand Down Expand Up @@ -149,6 +161,14 @@ class TabManagerDelegate : public aura::client::ActivationChangeObserver,
// Initiates an oom priority adjustment.
void ScheduleEarlyOomPrioritiesAdjustment();

// Returns a TimeDelta object that represents a minimum delay for killing
// the same ARC process again. ARC processes sometimes respawn right after
// being killed. In that case, killing them every time is just a waste of
// resources.
static constexpr base::TimeDelta GetArcRespawnKillDelay() {
return base::TimeDelta::FromSeconds(60);
}

// Holds a reference to the owning TabManager.
const base::WeakPtr<TabManager> tab_manager_;

Expand All @@ -165,6 +185,9 @@ class TabManagerDelegate : public aura::client::ActivationChangeObserver,
// Map maintaining the process handle - oom_score mapping.
ProcessScoreMap oom_score_map_;

// Map maintaing ARC process names and their last killed time.
KilledArcProcessesMap recently_killed_arc_processes_;

// Util for getting system memory status.
std::unique_ptr<TabManagerDelegate::MemoryStat> mem_stat_;

Expand Down
100 changes: 96 additions & 4 deletions chrome/browser/memory/tab_manager_delegate_chromeos_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,12 +102,13 @@ TEST_F(TabManagerDelegateTest, CandidatesSorted) {

class MockTabManagerDelegate : public TabManagerDelegate {
public:
MockTabManagerDelegate(): TabManagerDelegate(nullptr) {
}
MockTabManagerDelegate()
: TabManagerDelegate(nullptr),
always_return_true_from_is_recently_killed_(false) {}

explicit MockTabManagerDelegate(TabManagerDelegate::MemoryStat* mem_stat)
: TabManagerDelegate(nullptr, mem_stat) {
}
: TabManagerDelegate(nullptr, mem_stat),
always_return_true_from_is_recently_killed_(false) {}

// unit test.
std::vector<int> GetKilledArcProcesses() {
Expand All @@ -125,6 +126,20 @@ class MockTabManagerDelegate : public TabManagerDelegate {
killed_tabs_.clear();
}

// unit test.
void set_always_return_true_from_is_recently_killed(
bool always_return_true_from_is_recently_killed) {
always_return_true_from_is_recently_killed_ =
always_return_true_from_is_recently_killed;
}

bool IsRecentlyKilledArcProcess(const std::string& process_name,
const base::TimeTicks& now) override {
if (always_return_true_from_is_recently_killed_)
return true;
return TabManagerDelegate::IsRecentlyKilledArcProcess(process_name, now);
}

protected:
bool KillArcProcess(const int nspid) override {
killed_arc_processes_.push_back(nspid);
Expand All @@ -144,6 +159,7 @@ class MockTabManagerDelegate : public TabManagerDelegate {
chromeos::FakeDebugDaemonClient debugd_client_;
std::vector<int> killed_arc_processes_;
std::vector<int64_t> killed_tabs_;
bool always_return_true_from_is_recently_killed_;
};

class MockMemoryStat : public TabManagerDelegate::MemoryStat {
Expand Down Expand Up @@ -231,6 +247,75 @@ TEST_F(TabManagerDelegateTest, SetOomScoreAdj) {
EXPECT_EQ(650, oom_score_map[30]);
}

TEST_F(TabManagerDelegateTest, IsRecentlyKilledArcProcess) {
constexpr char kProcessName1[] = "org.chromium.arc.test1";
constexpr char kProcessName2[] = "org.chromium.arc.test2";

// Not owned.
MockMemoryStat* memory_stat = new MockMemoryStat();
// Instantiate the mock instance.
MockTabManagerDelegate tab_manager_delegate(memory_stat);

// When the process name is not in the map, IsRecentlyKilledArcProcess should
// return false.
const base::TimeTicks now = base::TimeTicks::Now();
EXPECT_FALSE(
tab_manager_delegate.IsRecentlyKilledArcProcess(kProcessName1, now));
EXPECT_FALSE(
tab_manager_delegate.IsRecentlyKilledArcProcess(kProcessName2, now));

// Update the map to tell the manager that the process was killed very
// recently.
tab_manager_delegate.recently_killed_arc_processes_[kProcessName1] = now;
EXPECT_TRUE(
tab_manager_delegate.IsRecentlyKilledArcProcess(kProcessName1, now));
EXPECT_FALSE(
tab_manager_delegate.IsRecentlyKilledArcProcess(kProcessName2, now));
tab_manager_delegate.recently_killed_arc_processes_[kProcessName1] =
now - base::TimeDelta::FromMicroseconds(1);
EXPECT_TRUE(
tab_manager_delegate.IsRecentlyKilledArcProcess(kProcessName1, now));
EXPECT_FALSE(
tab_manager_delegate.IsRecentlyKilledArcProcess(kProcessName2, now));
tab_manager_delegate.recently_killed_arc_processes_[kProcessName1] =
now - TabManagerDelegate::GetArcRespawnKillDelay();
EXPECT_TRUE(
tab_manager_delegate.IsRecentlyKilledArcProcess(kProcessName1, now));
EXPECT_FALSE(
tab_manager_delegate.IsRecentlyKilledArcProcess(kProcessName2, now));

// Update the map to tell the manager that the process was killed
// (GetArcRespawnKillDelay() + 1) seconds ago. In this case,
// IsRecentlyKilledArcProcess(kProcessName1) should return false.
tab_manager_delegate.recently_killed_arc_processes_[kProcessName1] =
now - TabManagerDelegate::GetArcRespawnKillDelay() -
base::TimeDelta::FromSeconds(1);
EXPECT_FALSE(
tab_manager_delegate.IsRecentlyKilledArcProcess(kProcessName1, now));
EXPECT_FALSE(
tab_manager_delegate.IsRecentlyKilledArcProcess(kProcessName2, now));
}

TEST_F(TabManagerDelegateTest, DoNotKillRecentlyKilledArcProcesses) {
// Not owned.
MockMemoryStat* memory_stat = new MockMemoryStat();

// Instantiate the mock instance.
MockTabManagerDelegate tab_manager_delegate(memory_stat);
tab_manager_delegate.set_always_return_true_from_is_recently_killed(true);

std::vector<arc::ArcProcess> arc_processes;
arc_processes.emplace_back(
1, 10, "service", arc::mojom::ProcessState::SERVICE, kNotFocused, 500);

memory_stat->SetTargetMemoryToFreeKB(250000);
memory_stat->SetProcessPss(30, 10000);
tab_manager_delegate.LowMemoryKillImpl({} /* tab_list */, arc_processes);

auto killed_arc_processes = tab_manager_delegate.GetKilledArcProcesses();
EXPECT_EQ(0U, killed_arc_processes.size());
}

TEST_F(TabManagerDelegateTest, KillMultipleProcesses) {
// Not owned.
MockMemoryStat* memory_stat = new MockMemoryStat();
Expand Down Expand Up @@ -313,6 +398,13 @@ TEST_F(TabManagerDelegateTest, KillMultipleProcesses) {
EXPECT_EQ(2, killed_tabs[0]);
EXPECT_EQ(5, killed_tabs[1]);
EXPECT_EQ(1, killed_tabs[2]);

// Check that killed apps are in the map.
const TabManagerDelegate::KilledArcProcessesMap& processes_map =
tab_manager_delegate.recently_killed_arc_processes_;
EXPECT_EQ(2U, processes_map.size());
EXPECT_EQ(1U, processes_map.count("service"));
EXPECT_EQ(1U, processes_map.count("not-visible"));
}

} // namespace memory

0 comments on commit b784d07

Please sign in to comment.