Persist QUIC Broken State 0: change RecentlyBrokenAlternativeServies

Persist QUIC Broken State 0: change RecentlyBrokenAlternativeServies to
base::MRUCache instead of std::map. No functional change.

BUG=705029
Review-Url: https://codereview.chromium.org/2801533002
Cr-Commit-Position: refs/heads/master@{#462760}
Committed: https://chromium.googlesource.com/chromium/src/+/c3af7fa4c410ee0ca5c6b4607487475dd8e32ab8

[Zhongyi Shi, 2017-04-04 23:42:21]

zhongyi@chromium.org changed reviewers:
+ rch@chromium.org

[Zhongyi Shi, 2017-04-04 23:42:36]

Description was changed from

==========
Persist QUIC Broken State 0: change RecentlyBrokenAlternativeServies to
base::MRUCache instead of std::map. No functional change.

BUG=705029
==========

to

==========
Persist QUIC Broken State 0: change RecentlyBrokenAlternativeServies to
base::MRUCache instead of std::map. No functional change.

BUG=705029
==========

[Ryan Hamilton, 2017-04-05 03:34:35]

I suspect this change is good, but what's the motivation for this?

https://codereview.chromium.org/2801533002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/2801533002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:465: it->second++;
Can you say:

 shift = it->second++;

https://codereview.chromium.org/2801533002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:507: return !(it ==
recently_broken_alternative_services_.end());
can you simplify this by:

return foo.Get() != foo.end()?

(So annoying that MRUCache does not work with base::ContainsKey

[Zhongyi Shi, 2017-04-06 21:36:30]

On 2017/04/05 03:34:35, Ryan Hamilton wrote:
> I suspect this change is good, but what's the motivation for this?
> 

To persist the recently_broken_alternative_services_ to disk, I think we don't
want to persist an unbounded map,
rather we will persist it with a limited size applying MRU rules?

[Zhongyi Shi, 2017-04-06 21:37:14]

https://codereview.chromium.org/2801533002/diff/1/net/http/http_server_proper...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/2801533002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:465: it->second++;
On 2017/04/05 03:34:34, Ryan Hamilton wrote:
> Can you say:
> 
>  shift = it->second++;

Done.

https://codereview.chromium.org/2801533002/diff/1/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:507: return !(it ==
recently_broken_alternative_services_.end());
On 2017/04/05 03:34:34, Ryan Hamilton wrote:
> can you simplify this by:
> 
> return foo.Get() != foo.end()?
> 
> (So annoying that MRUCache does not work with base::ContainsKey

Done.

[Ryan Hamilton, 2017-04-06 21:53:06]

On 2017/04/06 21:36:30, Zhongyi Shi wrote:
> On 2017/04/05 03:34:35, Ryan Hamilton wrote:
> > I suspect this change is good, but what's the motivation for this?
> > 
> 
> To persist the recently_broken_alternative_services_ to disk, I think we don't
> want to persist an unbounded map,
> rather we will persist it with a limited size applying MRU rules?

Hm. Interesting. It's a bit curious that we don't limit the size in memory but
we do on disk. *Boggle* Anyway, this seems plausible.

LGTM

[Zhongyi Shi, 2017-04-06 22:43:38]

On 2017/04/06 21:53:06, Ryan Hamilton wrote:
> On 2017/04/06 21:36:30, Zhongyi Shi wrote:
> > On 2017/04/05 03:34:35, Ryan Hamilton wrote:
> > > I suspect this change is good, but what's the motivation for this?
> > > 
> > 
> > To persist the recently_broken_alternative_services_ to disk, I think we
don't
> > want to persist an unbounded map,
> > rather we will persist it with a limited size applying MRU rules?
> 
> Hm. Interesting. It's a bit curious that we don't limit the size in memory but
> we do on disk. *Boggle* Anyway, this seems plausible.
> 
> LGTM

That's also surprising to me. We actually persist an unlimited (NO_AUTO_EVICT)
in memory, but when the memory updates, we schedule update to disk, which go
through the data persisted in memory, add those data to a limited size of MRU,
then persist the MRU to disk. It is a waste of time to iterate through unlimited
memory data. ServerNetworkStatsMap is an example.

[Zhongyi Shi, 2017-04-07 02:07:04]

The CQ bit was checked by zhongyi@chromium.org

[commit-bot: I haz the power, 2017-04-07 02:07:46]

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-04-07 05:09:53]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491530824976090,
"parent_rev": "169982cfc8c9619b413f133606cc832845e5d069", "commit_rev":
"c3af7fa4c410ee0ca5c6b4607487475dd8e32ab8"}

[commit-bot: I haz the power, 2017-04-07 05:11:29]

Description was changed from

==========
Persist QUIC Broken State 0: change RecentlyBrokenAlternativeServies to
base::MRUCache instead of std::map. No functional change.

BUG=705029
==========

to

==========
Persist QUIC Broken State 0: change RecentlyBrokenAlternativeServies to
base::MRUCache instead of std::map. No functional change.

BUG=705029

Review-Url: https://codereview.chromium.org/2801533002
Cr-Commit-Position: refs/heads/master@{#462760}
Committed:
https://chromium.googlesource.com/chromium/src/+/c3af7fa4c410ee0ca5c6b4607487...
==========

[commit-bot: I haz the power, 2017-04-07 05:11:30]

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