Add an eviction mechanism for SDCH dictionaries.

Add an eviction mechanism for SDCH dictionaries.

Implemented policy:
* If space is needed for a newly loaded dictionary, evict any dictionaries
  that haven't been used in a day to make room, oldest first.
* If the system signals memory pressure, flush all dictionaries.

Also moved chrome_sdch_policy.* -> net/sdch/sdch_owner.*.

BUG=387883
BUG=374916
Committed: https://crrev.com/a8a4e3c851d3e12ad27ac25493b6b690b867e39c
Cr-Commit-Position: refs/heads/master@{#310584}

[Randy Smith (Not in Mondays), 2015-01-08 03:23:04]

rdsmith@chromium.org changed reviewers:
+ bnc@chromium.org

[Randy Smith (Not in Mondays), 2015-01-08 03:23:04]

rdsmith@chromium.org changed required reviewers:
+ bnc@chromium.org

[Randy Smith (Not in Mondays), 2015-01-08 03:23:55]

Bence: Would you take a look?  I may be making minor tweaks on my own (I've been
pushing hard to get this reviewable to possibly make the branch cut this week),
but I'll flag any of those I do in messages to you.  And I think this is
basically ready.

[Bence, 2015-01-08 14:37:17]

https://codereview.chromium.org/841883002/diff/20001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/841883002/diff/20001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:268: while (!dictionaries_.empty()) {
What was wrong with dictionaries_.clear()?

https://codereview.chromium.org/841883002/diff/20001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/841883002/diff/20001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:278: // If |server_hash| is non-null, returns the server
has that may be
s/server has/server hash/

https://codereview.chromium.org/841883002/diff/20001/net/sdch/sdch_owner.cc
File net/sdch/sdch_owner.cc (right):

https://codereview.chromium.org/841883002/diff/20001/net/sdch/sdch_owner.cc#n...
net/sdch/sdch_owner.cc:93: // for the lifetime // of
|memory_pressure_listener_|.
Remove extra // from middle of line.

https://codereview.chromium.org/841883002/diff/20001/net/sdch/sdch_owner.cc#n...
net/sdch/sdch_owner.cc:134: DictionaryItem(const DictionaryItem& rhs) {
Copy constructor is generated when needed, why do you define it explicitly? 
Same with assigment operator.  I was able to compile it locally without these
two definitions, and all tests passed.

[Randy Smith (Not in Mondays), 2015-01-08 15:48:23]

rdsmith@chromium.org changed reviewers:
+ asvitkine@chromium.org, mmenke@chromium.org

[Randy Smith (Not in Mondays), 2015-01-08 15:48:23]

rdsmith@chromium.org changed required reviewers:
+ asvitkine@chromium.org, mmenke@chromium.org

[Randy Smith (Not in Mondays), 2015-01-08 15:48:23]

Comments incorporated.  PTAL.

Matt, Alexei: Given that Bence isn't calling out major areas for rewrite, I'd
like to pull you in to review your sections in parallel:

mmenke@: chrome/browser/profiles
asvitkine@: tools/metrics/histograms

Thanks!

https://codereview.chromium.org/841883002/diff/20001/net/base/sdch_manager.cc
File net/base/sdch_manager.cc (right):

https://codereview.chromium.org/841883002/diff/20001/net/base/sdch_manager.cc...
net/base/sdch_manager.cc:268: while (!dictionaries_.empty()) {
On 2015/01/08 14:37:17, Bence wrote:
> What was wrong with dictionaries_.clear()?

Huh.  Good question.  I think this is from an earlier version that provided a
"DictionaryRemoved" notification to the observer.  Reverted (left the comment
out, though, as it's no longer accurate).

https://codereview.chromium.org/841883002/diff/20001/net/base/sdch_manager.h
File net/base/sdch_manager.h (right):

https://codereview.chromium.org/841883002/diff/20001/net/base/sdch_manager.h#...
net/base/sdch_manager.h:278: // If |server_hash| is non-null, returns the server
has that may be
On 2015/01/08 14:37:17, Bence wrote:
> s/server has/server hash/

Done.

https://codereview.chromium.org/841883002/diff/20001/net/sdch/sdch_owner.cc
File net/sdch/sdch_owner.cc (right):

https://codereview.chromium.org/841883002/diff/20001/net/sdch/sdch_owner.cc#n...
net/sdch/sdch_owner.cc:93: // for the lifetime // of
|memory_pressure_listener_|.
On 2015/01/08 14:37:17, Bence wrote:
> Remove extra // from middle of line.

Ooops.  Done.

https://codereview.chromium.org/841883002/diff/20001/net/sdch/sdch_owner.cc#n...
net/sdch/sdch_owner.cc:134: DictionaryItem(const DictionaryItem& rhs) {
On 2015/01/08 14:37:17, Bence wrote:
> Copy constructor is generated when needed, why do you define it explicitly? 
> Same with assigment operator.  I was able to compile it locally without these
> two definitions, and all tests passed.

So I interpreted
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mova...
as indicating that if a type is made copyable, it should have explicit
constructors.  ("If you choose to make it copyable, define both copy operations
(constructor and assignment).")  However, reading it more carefully, it nudges
towards using =default, which is a new feature I'm not used to.  That'll save a
lot of space, so I'll use it.  Let me know if you think I should leave it out. 
(I guess I should do the same for DictionaryInfo :-J.)

[Bence, 2015-01-08 15:56:38]

LGTM.

https://codereview.chromium.org/841883002/diff/20001/net/sdch/sdch_owner.cc
File net/sdch/sdch_owner.cc (right):

https://codereview.chromium.org/841883002/diff/20001/net/sdch/sdch_owner.cc#n...
net/sdch/sdch_owner.cc:134: DictionaryItem(const DictionaryItem& rhs) {
On 2015/01/08 15:48:23, rdsmith wrote:
> On 2015/01/08 14:37:17, Bence wrote:
> > Copy constructor is generated when needed, why do you define it explicitly? 
> > Same with assigment operator.  I was able to compile it locally without
these
> > two definitions, and all tests passed.
> 
> So I interpreted
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mova...
> as indicating that if a type is made copyable, it should have explicit
> constructors.  ("If you choose to make it copyable, define both copy
operations
> (constructor and assignment).")  However, reading it more carefully, it nudges
> towards using =default, which is a new feature I'm not used to.  That'll save
a
> lot of space, so I'll use it.  Let me know if you think I should leave it out.

> (I guess I should do the same for DictionaryInfo :-J.)

Excellent.

[Alexei Svitkine (slow), 2015-01-08 16:47:14]

histograms lgtm, but I found some C++ nits - also I didn't look very closely, so
there may be others of the same type that I missed

https://codereview.chromium.org/841883002/diff/40001/net/base/sdch_manager_un...
File net/base/sdch_manager_unittest.cc (right):

https://codereview.chromium.org/841883002/diff/40001/net/base/sdch_manager_un...
net/base/sdch_manager_unittest.cc:77: };
Nit: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/841883002/diff/40001/net/filter/sdch_filter_u...
File net/filter/sdch_filter_unittest.cc (right):

https://codereview.chromium.org/841883002/diff/40001/net/filter/sdch_filter_u...
net/filter/sdch_filter_unittest.cc:1229: SimpleSdchObserver(SdchManager*
manager)
Nit: explicit

https://codereview.chromium.org/841883002/diff/40001/net/filter/sdch_filter_u...
net/filter/sdch_filter_unittest.cc:1242: int dictionary_used_calls() { return
dictionary_used_; }
Nit: const, same below

https://codereview.chromium.org/841883002/diff/40001/net/filter/sdch_filter_u...
net/filter/sdch_filter_unittest.cc:1254: };
Nit: DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner.h
File net/sdch/sdch_owner.h (right):

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner.h#ne...
net/sdch/sdch_owner.h:91: };
Nit: DISALLOW_COPY_AND_ASSIGN()?

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner_unit...
File net/sdch/sdch_owner_unittest.cc (right):

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner_unit...
net/sdch/sdch_owner_unittest.cc:22: static const char* generic_domain =
"www.example.com";
Nit: const char kGeneralUrl[] =

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner_unit...
net/sdch/sdch_owner_unittest.cc:24: static std::string NewSdchDictionary(size_t
dictionary_size) {
Nit: No need for static if in anon namespace.

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner_unit...
net/sdch/sdch_owner_unittest.cc:77: };
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/841883002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/841883002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:55442: +  <int value="44"
label="DICTIONARY_NO_ROOM"/>
You don't have to do it in this CL, but histograms using this enum should
probably use UMA_HISTOGRAM_SPARSE_SLOWLY to avoid pre-allocating all these
buckets.

[mmenke, 2015-01-08 17:46:27]

profiles/ LGTM

[Randy Smith (Not in Mondays), 2015-01-08 18:00:12]

I'm going to take a separate pass through and look for nits like the ones Alexei
found (oops), but I'll assume the LGTMs stand unless someone tells me otherwise.
 So if anyone wants to take another nit pass along those lines, let me know, and
I'll hold landing for you.

Thanks all!

https://codereview.chromium.org/841883002/diff/40001/net/base/sdch_manager_un...
File net/base/sdch_manager_unittest.cc (right):

https://codereview.chromium.org/841883002/diff/40001/net/base/sdch_manager_un...
net/base/sdch_manager_unittest.cc:77: };
On 2015/01/08 16:47:14, Alexei Svitkine wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/841883002/diff/40001/net/filter/sdch_filter_u...
File net/filter/sdch_filter_unittest.cc (right):

https://codereview.chromium.org/841883002/diff/40001/net/filter/sdch_filter_u...
net/filter/sdch_filter_unittest.cc:1229: SimpleSdchObserver(SdchManager*
manager)
On 2015/01/08 16:47:14, Alexei Svitkine wrote:
> Nit: explicit

Done.

https://codereview.chromium.org/841883002/diff/40001/net/filter/sdch_filter_u...
net/filter/sdch_filter_unittest.cc:1242: int dictionary_used_calls() { return
dictionary_used_; }
On 2015/01/08 16:47:14, Alexei Svitkine wrote:
> Nit: const, same below

Done.

https://codereview.chromium.org/841883002/diff/40001/net/filter/sdch_filter_u...
net/filter/sdch_filter_unittest.cc:1254: };
On 2015/01/08 16:47:14, Alexei Svitkine wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner.h
File net/sdch/sdch_owner.h (right):

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner.h#ne...
net/sdch/sdch_owner.h:91: };
On 2015/01/08 16:47:14, Alexei Svitkine wrote:
> Nit: DISALLOW_COPY_AND_ASSIGN()?

Done.

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner_unit...
File net/sdch/sdch_owner_unittest.cc (right):

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner_unit...
net/sdch/sdch_owner_unittest.cc:22: static const char* generic_domain =
"www.example.com";
On 2015/01/08 16:47:14, Alexei Svitkine wrote:
> Nit: const char kGeneralUrl[] =

Done.

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner_unit...
net/sdch/sdch_owner_unittest.cc:24: static std::string NewSdchDictionary(size_t
dictionary_size) {
On 2015/01/08 16:47:14, Alexei Svitkine wrote:
> Nit: No need for static if in anon namespace.

Meaning it should be in an anon namespace?  It isn't now.  (Though I'll note
that I haven't generally worried too much about this type of namespace issue in
tests.)

https://codereview.chromium.org/841883002/diff/40001/net/sdch/sdch_owner_unit...
net/sdch/sdch_owner_unittest.cc:77: };
On 2015/01/08 16:47:14, Alexei Svitkine wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/841883002/diff/40001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/841883002/diff/40001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:55442: +  <int value="44"
label="DICTIONARY_NO_ROOM"/>
On 2015/01/08 16:47:14, Alexei Svitkine wrote:
> You don't have to do it in this CL, but histograms using this enum should
> probably use UMA_HISTOGRAM_SPARSE_SLOWLY to avoid pre-allocating all these
> buckets.

Will do, but not in this CL (actually, I want to completely revamp how SDCH does
histograms :-J).

[mmenke, 2015-01-08 18:19:12]

You should fix the grammar in your description "Add an eviction mechanisms for
SDCH dictionaries." - either make "mechanisms" singular, or remove "an".

[Randy Smith (Not in Mondays), 2015-01-08 18:25:31]

On 2015/01/08 18:19:12, mmenke wrote:
> You should fix the grammar in your description "Add an eviction mechanisms for
> SDCH dictionaries." - either make "mechanisms" singular, or remove "an".

Done; thanks for the catch.

[Randy Smith (Not in Mondays), 2015-01-08 19:15:24]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2015-01-08 19:16:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/841883002/120001

[commit-bot: I haz the power, 2015-01-08 20:18:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-01-08 20:20:19]

Patchset 7 (id:??) landed as
https://crrev.com/a8a4e3c851d3e12ad27ac25493b6b690b867e39c
Cr-Commit-Position: refs/heads/master@{#310584}