HttpServerProperties - Implement MRU for AlternateProtocolMap. Persist

HttpServerProperties - Implement MRU for AlternateProtocolMap. Persist
only the most recently used 200 entries. In memory keep all the entries.

BUG=258189
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255323

[ramant (doing other things), 2014-03-04 23:59:21]

Hi Ryan,
  Used base::MRUCache instead of std::map for alternate_protocol_map_ in
HttpServerPropertiesImpl class.

  The cache is similar to CachedPipelineCapabilityMap. For AlternateProtocolMap,
used NO_AUTO_EVICT (thus had to make the typedef public) policy. While writing
to disk, persisted only most recently used 200 entries.

  In the Perferences file, verified that we write only 5 items by changing 
kMaxAlternateProtocolHostsToPersist to 5 and checking in the Preferences file
and checking in the net-internals UI (after browser startup) that we have loaded
only 5 entries.

[ramant (doing other things), 2014-03-05 00:01:30]

https://codereview.chromium.org/182383016/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/182383016/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:23:
CachedAlternateProtocolMap::NO_AUTO_EVICT)),
rch: Should we keep fewer entries in the memory? Added NO_AUTO_EVICT to keep it
same as the current system. Because we write only 200 entries to disk, pref's
file won't be big (200 times 300 bytes).

If this works, will change SpdyServer and SpdySettings to use MRUCache.

thanks

[Ryan Hamilton, 2014-03-05 00:19:20]

Ok, so I think I understand what's going on here.  The HttpServerProperitesImpl
wants to maintain an MRU map, but the external interface is just a map.  But I
think that when the MRU map is copied to a normal map, we lose the MRU ordering,
so I'm not sure this does the right thing, does it?

Instead, I wonder if we should just change the definition of
AlternateProtocolMap from map to MRUCache?

https://codereview.chromium.org/182383016/diff/20001/chrome/browser/net/http_...
File chrome/browser/net/http_server_properties_manager.cc (right):

https://codereview.chromium.org/182383016/diff/20001/chrome/browser/net/http_...
chrome/browser/net/http_server_properties_manager.cc:606: for
(net::AlternateProtocolMap::const_iterator map_it =
What guarantee do we have that this iterator is going to go in LRU order?

https://codereview.chromium.org/182383016/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/182383016/diff/20001/net/http/http_server_pro...
net/http/http_server_properties.h:118: virtual AlternateProtocolMap
GetAlternateProtocolMap() const = 0;
I'm sure that this is somehow related to the change to MRUCache, but I don't
understand why?

https://codereview.chromium.org/182383016/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/182383016/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.h:165:
scoped_ptr<CachedAlternateProtocolMap> alternate_protocol_map_;
Is the idea here that the actual map we're keeping is of a different type than
AlternateProtocolMap?

[ramant (doing other things), 2014-03-05 15:11:10]

>Instead, I wonder if we should just change the definition of
>AlternateProtocolMap from map to MRUCache?

Hi Ryan,
  Many thanks for the above suggestion. Made the above changes. Would like to
investigate why Pipeline code didn't use the above approach.

  Tested this code by changing #of persisting entries to 5 and verified only 5
MRU's are persisted.

  Still investigating if I missing something obvious.

thanks
raman

https://codereview.chromium.org/182383016/diff/20001/chrome/browser/net/http_...
File chrome/browser/net/http_server_properties_manager.cc (right):

https://codereview.chromium.org/182383016/diff/20001/chrome/browser/net/http_...
chrome/browser/net/http_server_properties_manager.cc:606: for
(net::AlternateProtocolMap::const_iterator map_it =
On 2014/03/05 00:19:20, Ryan Hamilton wrote:
> What guarantee do we have that this iterator is going to go in LRU order?

Many thanks for the catch. My fault (had only 200 for memory cache ala PipeLine
code).

Changed the code to use MruCache.
 
Done.

https://codereview.chromium.org/182383016/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties.h (right):

https://codereview.chromium.org/182383016/diff/20001/net/http/http_server_pro...
net/http/http_server_properties.h:118: virtual AlternateProtocolMap
GetAlternateProtocolMap() const = 0;
On 2014/03/05 00:19:20, Ryan Hamilton wrote:
> I'm sure that this is somehow related to the change to MRUCache, but I don't
> understand why?

Undid this change. Changed code to return MRUCache (not sure why  from Pipeline
code didn't want to give const version of MRUCache).

Verified callers of alternate_protocol_map() are not modifying the data.

https://codereview.chromium.org/182383016/diff/20001/net/http/http_server_pro...
File net/http/http_server_properties_impl.h (right):

https://codereview.chromium.org/182383016/diff/20001/net/http/http_server_pro...
net/http/http_server_properties_impl.h:165:
scoped_ptr<CachedAlternateProtocolMap> alternate_protocol_map_;
On 2014/03/05 00:19:20, Ryan Hamilton wrote:
> Is the idea here that the actual map we're keeping is of a different type than
> AlternateProtocolMap?

Used the MRUCache for AlternateProtocolMap. 

Will dig up Pipeline code's review to see why the actual map
CachedPipelineCapabilityMap is different from PipelineCapabilityMap.

[Ryan Hamilton, 2014-03-05 17:51:22]

lgtm

https://codereview.chromium.org/182383016/diff/40001/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/182383016/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:22: :
alternate_protocol_map_(AlternateProtocolMap::NO_AUTO_EVICT),
With NO_AUTO_EVICT, this means the in-memory map will be unbounded, right?  (I
think you mentioned that in your mail).  This seems fine for this CL, but just
confirming.

nit: Should we add a test of the MRU behavior of Has|GetAlternateProtocol?

https://codereview.chromium.org/182383016/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:58: }
alternatively:

AlternateProtocolMap::iterator old_it = it;
++it;
if (it->second.protocol != ALTERNATE_PROTOCOL_BROKEN) {
  alternate_protocol_map_.Erase(old_it);
}

It's shorter, but not sure if that's clearer.  What do you think?

https://codereview.chromium.org/182383016/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:59: }
I take it there is no swap() method in MRUCache?

[ramant (doing other things), 2014-03-05 22:43:22]

Thanks very much Ryan for your comments. PTAL.

Would like to migrate Spdy* properties to use MRUCache. Would like to migrate
HttpServerProperties code to disk cache after talking with you/jar/wtc.

thanks
raman

https://codereview.chromium.org/182383016/diff/40001/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/182383016/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:22: :
alternate_protocol_map_(AlternateProtocolMap::NO_AUTO_EVICT),
On 2014/03/05 17:51:22, Ryan Hamilton wrote:
> With NO_AUTO_EVICT, this means the in-memory map will be unbounded, right?  (I
> think you mentioned that in your mail).  This seems fine for this CL, but just
> confirming.

I went back and forth on the above. The current implementation has no bounds.
Thus made the MRUCache to be unbounded.

Tested the current code with a limit 10 and verified in net-internals that
MRUCache is evicting the right server.

A small change to set the limit: use a different const from
AlternateProtocolMap::NO_AUTO_EVICT. 

> 
> nit: Should we add a test of the MRU behavior of 
> Has|GetAlternateProtocol?

Thanks much for the above suggestion. Added the unit tests.

https://codereview.chromium.org/182383016/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:58: }
On 2014/03/05 17:51:22, Ryan Hamilton wrote:
> alternatively:
> 
> AlternateProtocolMap::iterator old_it = it;
> ++it;
> if (it->second.protocol != ALTERNATE_PROTOCOL_BROKEN) {
>   alternate_protocol_map_.Erase(old_it);
> }
> 
> It's shorter, but not sure if that's clearer.  What do you think?

Done.

https://codereview.chromium.org/182383016/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:59: }
On 2014/03/05 17:51:22, Ryan Hamilton wrote:
> I take it there is no swap() method in MRUCache?

Correct.

[Ryan Hamilton, 2014-03-05 23:10:20]

lgtm

Thanks, Raman.  Looks great.  LGTM

https://codereview.chromium.org/182383016/diff/40001/net/http/http_server_pro...
File net/http/http_server_properties_impl.cc (right):

https://codereview.chromium.org/182383016/diff/40001/net/http/http_server_pro...
net/http/http_server_properties_impl.cc:22: :
alternate_protocol_map_(AlternateProtocolMap::NO_AUTO_EVICT),
On 2014/03/05 22:43:22, ramant wrote:
> On 2014/03/05 17:51:22, Ryan Hamilton wrote:
> > With NO_AUTO_EVICT, this means the in-memory map will be unbounded, right? 
(I
> > think you mentioned that in your mail).  This seems fine for this CL, but
just
> > confirming.
> 
> I went back and forth on the above. The current implementation has no bounds.
> Thus made the MRUCache to be unbounded.

SGTM.  We can revisit this at any point.

> Tested the current code with a limit 10 and verified in net-internals that
> MRUCache is evicting the right server.
> 
> A small change to set the limit: use a different const from
> AlternateProtocolMap::NO_AUTO_EVICT. 
> 
> > 
> > nit: Should we add a test of the MRU behavior of 
> > Has|GetAlternateProtocol?
> 
> Thanks much for the above suggestion. Added the unit tests.

[ramant (doing other things), 2014-03-05 23:15:46]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-05 23:17:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/182383016/100001

[ramant (doing other things), 2014-03-05 23:18:08]

The CQ bit was unchecked by rtenneti@chromium.org

[ramant (doing other things), 2014-03-06 00:21:08]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-06 00:25:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/182383016/100001

[commit-bot: I haz the power, 2014-03-06 00:51:46]

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

[commit-bot: I haz the power, 2014-03-06 00:51:46]

Try jobs failed on following builders: mac_chromium_compile_dbg

[ramant (doing other things), 2014-03-06 01:07:04]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-06 01:09:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/182383016/120001

[commit-bot: I haz the power, 2014-03-06 02:06:05]

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

[commit-bot: I haz the power, 2014-03-06 02:06:06]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[ramant (doing other things), 2014-03-06 02:27:18]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-06 02:28:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/182383016/140001

[commit-bot: I haz the power, 2014-03-06 03:10:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/182383016/140001

[commit-bot: I haz the power, 2014-03-06 04:52:12]

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

[commit-bot: I haz the power, 2014-03-06 04:52:12]

Try jobs failed on following builders: mac_chromium_rel

[ramant (doing other things), 2014-03-06 05:47:54]

The CQ bit was checked by rtenneti@chromium.org

[commit-bot: I haz the power, 2014-03-06 05:50:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/182383016/140001

[commit-bot: I haz the power, 2014-03-06 13:09:08]

Change committed as 255323

[rmistry1, 2014-03-06 13:39:51]

A revert of this CL has been created in
https://codereview.chromium.org/188663002/ by rmistry@chromium.org.

The reason for reverting is: Broke compile on a bunch of bots:
Chromium Linux Builder (valgrind)
Chromium OS Builder (valgrind)
Chromium Windows Builder (DrMemory)
Linux Builder (TSan v2)

Log:
http://build.chromium.org/p/chromium.memory.fyi/builders/Chromium%20Linux%20B...
.