The logic is added in https://codereview.chromium.org/2132313002, to reduce the avoid Chrome being killed on Android.

I think your proposal is reasonable. At the time of making that CL, we didn't take into account such a probleme.

Thanks msisov@. Which one is more reasonable?

Now I prefer solution 2, because timeout idle sockets are cleared everytime new socket request comes in. I think it's also safe to apply the approach to all the platforms.

I don't think it's really up to net/ to know the different memory characteristics of Chromecast vs Android, so I'm reluctant to implement different logic in net/ for this.  If we move the calls over to the embedder, I'm fine with the embedder logic being different for different platforms.

Also, not sure if we'd want to have different behavior on Android and iOS.

I don't think the proposal is to have net understand those memory characteristics (e.g. of Chromecast vs Android).  The proposal is just for net/ to do something less drastic for moderate memory pressure than for critical (for all platforms).

I thought proposal 2) was specifically to have net do something different on Android?  Proposal 1) was for all platforms.

Sorry for the confusion, what I mean is to move idle sockets flushing to critical memory pressure, for all the platforms.

OK, to make it clear, I have 3 proposals:

1. Flush timeout idle sockets when memory pressure moderate, for all the platforms.
2. Flush all idle sockets when memory pressure moderate, only for Android. I believe we can have a flag in HttpNetworkSession::Params. In this way net/ doesn't need to know about memory characters on different platforms.
3. Flush all idle sockets when memory pressure critical, don't do anything when moderate. This is also for all the platforms.

3) is preferred.

Just to toss in another two options:

4.  On moderate, flush all sockets that have been idle for >= X seconds when moderate (X could be, say, 30).
5.  Reconsider when moderate low memory notifications are sent on Chromecast, or if they're sent.  What should we be doing on moderate notifications on Chromecast, anyways?

I don't have a great understanding of when low memory notifications are sent on either platform, or what other kinds of things we should do in response, so don't have strong opinions on what we should actually be doing when we get one.

Marking this as available, just to get it off of the network bug triager's radar.

Yeah, unfortunately the definition of 'moderate' is pretty fuzzy/ill-defined.  In practice, changing it can have very wide-ranging effects on memory usage patterns, and needs to be done with great care.  In particular, we depend on it to free up memory in other systems (e.g. image cache).  So I would prefer not to do (5) unless we've really exhausted all other options :)

I think (4) is the same as (1)? Currently, timeout isn't checked when flushing idle sockets on memory pressure moderate.

I thought 1) was the current behavior, so perhaps I was misunderstanding it?

IIRC, timeout is ignored when flushing idle sockets on moderate memory pressure. It is respected in other cases, e.g. clear before requesting new socket.

Oh, you meant flushing timed out idle sockets...  Sorry, completely misunderstood.

So yea, option 4 would be closing those, and sockets that had been idle for 30 seconds (The normal timeout is something like 5 minutes, I believe).

I think it's worth to start with solution 3, a.k.a, only flush idle sockets on critical memory pressure. If we observer any memory issue due to the change, we can move on solution 2.

WDYT?

I'm not sure what metrics we monitor, so can't comment intelligently.  I've been hoping mariakhomenko would chime in.

I am ok with changing logic to flush on critical pressure only. Please monitor Memory.Browser.PrivateMemoryFootprint for any changes (and specifically filtered to low-end devices) after your change lands.

Actually, one more thought: have you verified that the current memory pressure sent that clears the sockets too often on chromecast is actually moderate rather than critical? It's possible that you won't see any difference if moderate signals are not the issue here.

Yes, most of them are due to moderate memory pressure. Removing idle sockets flushing from moderate pressure will solve the problem.

Great!  Let's try that and see how things go, then.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9462d916a795eee44483d0fa695b1fca516e0aa1

commit 9462d916a795eee44483d0fa695b1fca516e0aa1
Author: yucliu <yucliu@chromium.org>
Date: Wed Nov 22 02:41:23 2017

Close idle sockets only on memory pressure critical

Closing idle sockets on memory pressure moderate is too aggressive. In
some cases (e.g. media streaming on Cast), the socket connecting to
the same server is closed and re-opened multiple times, if we close
idle sockets when moderate memory pressure happens.

Make it less aggressive by keeping the logic only for memory pressure
critical.

BUG= 786585 
TEST=Stream on Cast

Change-Id: I56fe7f53aa974125e9acada43f853ed6c28fd0fe
Reviewed-on: https://chromium-review.googlesource.com/780779
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Yuchen Liu <yucliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#518508}
[modify] https://crrev.com/9462d916a795eee44483d0fa695b1fca516e0aa1/net/http/http_network_session.cc

The situation is much better, but we still see lots of socket being closed due to critical memory pressure.

One more proposal I have is to close idle sockets only when HttpNetworkSession receives 5 (number is configurable) consecutive critical memory pressures. This will allow the other components to clear their memory usage first.

Any comments on this? We can apply this logic to cast only.

It sort of sounds like you don't want to close idle sockets at all even with memory pressure on Cast. Perhaps if we have cast-specific logic, we should just not clear sockets for cast only?

Or re-calibrate when memory pressure notifications are sent on cast.  Ignoring "critical" memory warnings when we have idle sockets seems not great.

Yes, that's a much easier option for us. Given browser is the only app we're running and streaming is the most important feature we need to support, avoid closing sockets on memory pressure works for us.

I don't think recalibrating that is practical right now.  We are a super low memory device and are regularly low on memory during video playback.  Without emitting these notifications, various Chromium systems won't free up necessary memory and we'll OOM soon after.

Fixing this would be dependent on completely revamping how many parts of Chromium handle memory - there are efforts in this direction, but I don't think it will change that soon.

That works for me.  The way to do it is probably to add yet another element to HttpNetworkSession::Params, and set it in cast when setting up the URLRequestContext.

We do rely on memory pressure notification to free memories in other parts. I prefer to ignoring critical memory pressure for network only.

Re #31, that's where I plan to add the flag.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/48f235dbd388f669547b5da912f38c6ee7f49497

commit 48f235dbd388f669547b5da912f38c6ee7f49497
Author: yucliu <yucliu@chromium.org>
Date: Thu Jan 11 00:59:55 2018

Add flag to not close idle sockets on memory pressure

This patch:
1. Adds a flag to disable idle sockets closing on memory pressure.
2. Enable the flag for Chromecast.
3. Restore the old logic to close idle socket on moderate memory
pressure.

BUG= 786585 
TEST=Number of connections is kept as a reasonable level when streaming
on Chromecast.

Change-Id: I6035060b0880fe04d78e08afd556ccbdd31383d9
Reviewed-on: https://chromium-review.googlesource.com/860897
Commit-Queue: Yuchen Liu <yucliu@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528511}
[modify] https://crrev.com/48f235dbd388f669547b5da912f38c6ee7f49497/chromecast/browser/url_request_context_factory.cc
[modify] https://crrev.com/48f235dbd388f669547b5da912f38c6ee7f49497/net/http/http_network_session.cc
[modify] https://crrev.com/48f235dbd388f669547b5da912f38c6ee7f49497/net/http/http_network_session.h
[modify] https://crrev.com/48f235dbd388f669547b5da912f38c6ee7f49497/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/48f235dbd388f669547b5da912f38c6ee7f49497/net/spdy/chromium/spdy_test_util_common.cc
[modify] https://crrev.com/48f235dbd388f669547b5da912f38c6ee7f49497/net/spdy/chromium/spdy_test_util_common.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/759a1fb4b1b1e18d4537f63eba52cb9931a87073

commit 759a1fb4b1b1e18d4537f63eba52cb9931a87073
Author: yucliu <yucliu@chromium.org>
Date: Fri Feb 16 03:00:19 2018

[Chromecast] Feature flag to disable idle socket close

Add new feature flag disable_idle_socket_close_on_memory_pressure. If
the flag is turned on, cast_shell won't close idle sockets when memory
pressure happens.

BUG= 786585 , internal b/73351220, internal b/73495670
TEST=Lauch cast_shell with enable-features=disable_... and watch logs

Change-Id: I5aa22a20103624c3114bc8de34412a9ace0dfa29
Reviewed-on: https://chromium-review.googlesource.com/922902
Reviewed-by: Sergey Volk <servolk@chromium.org>
Commit-Queue: Yuchen Liu <yucliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537161}
[modify] https://crrev.com/759a1fb4b1b1e18d4537f63eba52cb9931a87073/chromecast/base/cast_features.cc
[modify] https://crrev.com/759a1fb4b1b1e18d4537f63eba52cb9931a87073/chromecast/base/cast_features.h
[modify] https://crrev.com/759a1fb4b1b1e18d4537f63eba52cb9931a87073/chromecast/browser/url_request_context_factory.cc