memory_coordinator: Discard tabs under critical memory condition

memory_coordinator: Discard tabs under critical memory condition

When the next memory condition is CRITICAL, ask the delegate
(TabManager) to discard a tab.

BUG=689363
Review-Url: https://codereview.chromium.org/2735993002
Cr-Commit-Position: refs/heads/master@{#456314}
Committed: https://chromium.googlesource.com/chromium/src/+/2f0045d8cd5a0555c96cb1844953cb81b6f13bdc

[bashi, 2017-03-07 07:51:00]

bashi@chromium.org changed reviewers:
+ chrisha@chromium.org, haraken@chromium.org

[bashi, 2017-03-07 07:51:01]

This is a simple approach to connect tab discarding with memory coordinator.
What do you think about the approach? Suggestions and other ideas are welcomed
:)

[haraken, 2017-03-07 08:04:57]

LGTM with comments.

https://codereview.chromium.org/2735993002/diff/1/chrome/browser/memory/tab_m...
File chrome/browser/memory/tab_manager.h (right):

https://codereview.chromium.org/2735993002/diff/1/chrome/browser/memory/tab_m...
chrome/browser/memory/tab_manager.h:103: // a tab is actually discarded.

the tab was

https://codereview.chromium.org/2735993002/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator_impl.cc (right):

https://codereview.chromium.org/2735993002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl.cc:452: if (memory_condition_ !=
MemoryCondition::CRITICAL)

Don't we need to update the memory condition before reading memory_condition_?

https://codereview.chromium.org/2735993002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl.cc:455: // Keep scheduling tab
discarding task while memory condition is CRITICAL.

a tab discarding task while the memory condition is CRITICAL

https://codereview.chromium.org/2735993002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl.cc:458:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

Maybe it's nicer to check memory_condition_ again before posting a new task.

[bashi, 2017-03-07 08:14:08]

(I will address grammatical errors later)

https://codereview.chromium.org/2735993002/diff/1/content/browser/memory/memo...
File content/browser/memory/memory_coordinator_impl.cc (right):

https://codereview.chromium.org/2735993002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl.cc:452: if (memory_condition_ !=
MemoryCondition::CRITICAL)
On 2017/03/07 08:04:57, haraken wrote:
> 
> Don't we need to update the memory condition before reading memory_condition_?

Yeah, if we want to use up-to-date condition we would move this logic into
UpdateConditionIfNeeded().

https://codereview.chromium.org/2735993002/diff/1/content/browser/memory/memo...
content/browser/memory/memory_coordinator_impl.cc:458:
base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
On 2017/03/07 08:04:57, haraken wrote:
> 
> Maybe it's nicer to check memory_condition_ again before posting a new task.

Maybe, but I guess the cost wouldn't be so different (updating condition
involves syscalls).

[haraken, 2017-03-07 08:22:23]

On 2017/03/07 08:14:08, bashi wrote:
> (I will address grammatical errors later)
> 
>
https://codereview.chromium.org/2735993002/diff/1/content/browser/memory/memo...
> File content/browser/memory/memory_coordinator_impl.cc (right):
> 
>
https://codereview.chromium.org/2735993002/diff/1/content/browser/memory/memo...
> content/browser/memory/memory_coordinator_impl.cc:452: if (memory_condition_
!=
> MemoryCondition::CRITICAL)
> On 2017/03/07 08:04:57, haraken wrote:
> > 
> > Don't we need to update the memory condition before reading
memory_condition_?
> 
> Yeah, if we want to use up-to-date condition we would move this logic into
> UpdateConditionIfNeeded().

I'm not sure if we need to know the latest condition but we'll need to make sure
that memory_condition_ is updated before the next tab discarding task is fired.

> 
>
https://codereview.chromium.org/2735993002/diff/1/content/browser/memory/memo...
> content/browser/memory/memory_coordinator_impl.cc:458:
> base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
> On 2017/03/07 08:04:57, haraken wrote:
> > 
> > Maybe it's nicer to check memory_condition_ again before posting a new task.
> 
> Maybe, but I guess the cost wouldn't be so different (updating condition
> involves syscalls).

Makes sense.

[bashi, 2017-03-10 04:46:45]

The CQ bit was checked by bashi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 04:47:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bashi, 2017-03-10 04:53:20]

Simplified the CL. Instead of scheduling another task, just call DiscardTab() in
UpdateConditionIfNeeded() when the next condition is CRITICAL. This way we don't
have to worry about stale condition.

If there is no objection I'll submit this CL. WDYT?

[bashi, 2017-03-10 04:56:48]

Description was changed from

==========
WIP: memory_coordinator: Discard tabs under critical memory condition

Basic idea:
- When |memory_condition_| becomes CRITICAL, post a task to try to discard
one tab
- Keep scheduling tab discarding with an interval while |memory_condition_|
  is CRITICAL
- Stop tab discarding when there is no tab we can discard

BUG=689363
==========

to

==========
memory_coordinator: Discard tabs under critical memory condition

When the next memory condition is CRITICAL, ask the delegate
(TabManager) to discard a tab.

BUG=689363
==========

[commit-bot: I haz the power, 2017-03-10 05:35:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 05:35:59]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-03-10 07:50:05]

On 2017/03/10 04:53:20, bashi wrote:
> Simplified the CL. Instead of scheduling another task, just call DiscardTab()
in
> UpdateConditionIfNeeded() when the next condition is CRITICAL. This way we
don't
> have to worry about stale condition.
> 
> If there is no objection I'll submit this CL. WDYT?

LGTM

[bashi, 2017-03-13 00:18:00]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2017-03-13 00:18:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 01:09:30]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489364280655560,
"parent_rev": "ebefd65d9e7285db1b4f85ef732df331f3f23fd1", "commit_rev":
"2f0045d8cd5a0555c96cb1844953cb81b6f13bdc"}

[commit-bot: I haz the power, 2017-03-13 01:10:32]

Description was changed from

==========
memory_coordinator: Discard tabs under critical memory condition

When the next memory condition is CRITICAL, ask the delegate
(TabManager) to discard a tab.

BUG=689363
==========

to

==========
memory_coordinator: Discard tabs under critical memory condition

When the next memory condition is CRITICAL, ask the delegate
(TabManager) to discard a tab.

BUG=689363

Review-Url: https://codereview.chromium.org/2735993002
Cr-Commit-Position: refs/heads/master@{#456314}
Committed:
https://chromium.googlesource.com/chromium/src/+/2f0045d8cd5a0555c96cb1844953...
==========

[commit-bot: I haz the power, 2017-03-13 01:10:33]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2f0045d8cd5a0555c96cb1844953...