Reland: Refactor and shorten in-memory cache.

Reland: Refactor and shorten in-memory cache.

Previously this code was maintaining a complex doubly-linked list
manually, rather than using base::linked_list, which does the same thing.

A number of uses of vector were strictly speaking violating the C++98
specification, also fixed.

Too many functions were short redirectors into other functions. Now fixed.

The originally landed version contained a significant double free bug
which caused reversion, now fixed.

R=mmenke
BUG=581791, 586440
Committed: https://crrev.com/4ffdb91a6e68f0f20fa40a2b224e98ce5da8f629
Cr-Commit-Position: refs/heads/master@{#377124}

[gavinp, 2016-02-19 18:51:39]

Matt，

The crash on our first attempt to land this was that the eviction was
re-entering and double-dooming. Really scary stuff.

The fix is basically copied from the original code; it had a method to add to
the size of a backend, and a second to shrink it. The difference was checking
for eviction. Oh boy.

[gavinp, 2016-02-19 18:52:57]

The new test, btw, runs really slowly in simple cache, which is clearly a bug.
I'm working on that now as a new issue, crbug.com/588184 .

[mmenke, 2016-02-19 19:08:32]

https://codereview.chromium.org/1715833002/diff/60001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_entry_impl.cc (right):

https://codereview.chromium.org/1715833002/diff/60001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_entry_impl.cc:300: it.second->Doom();
So the problem was the child calling backend_->ModifyStorageSize(...) resulting
in evicting another child?  How does the check you added avoid that?  Populated
children don't report a 0 size...do they?

[gavinp, 2016-02-19 19:18:01]

https://codereview.chromium.org/1715833002/diff/60001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_entry_impl.cc (right):

https://codereview.chromium.org/1715833002/diff/60001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_entry_impl.cc:300: it.second->Doom();
On 2016/02/19 19:08:32, mmenke wrote:
> So the problem was the child calling backend_->ModifyStorageSize(...)
resulting
> in evicting another child?  How does the check you added avoid that? 
Populated
> children don't report a 0 size...do they?

No child should report a zero size. Nor is that very important; take a look at
the check again: it's checking if the modification of the size is shrinking or
enlarging the current backend, not on the current size of any particular entry.

The check I added prevents eviction from recursing; tons of double destructions
and double deletions came from that as the eviction list was modified mid
eviction.

[mmenke, 2016-02-19 19:20:43]

https://codereview.chromium.org/1715833002/diff/60001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_entry_impl.cc (right):

https://codereview.chromium.org/1715833002/diff/60001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_entry_impl.cc:300: it.second->Doom();
On 2016/02/19 19:18:01, gavinp wrote:
> On 2016/02/19 19:08:32, mmenke wrote:
> > So the problem was the child calling backend_->ModifyStorageSize(...)
> resulting
> > in evicting another child?  How does the check you added avoid that? 
> Populated
> > children don't report a 0 size...do they?
> 
> No child should report a zero size. Nor is that very important; take a look at
> the check again: it's checking if the modification of the size is shrinking or
> enlarging the current backend, not on the current size of any particular
entry.
> 
> The check I added prevents eviction from recursing; tons of double
destructions
> and double deletions came from that as the eviction list was modified mid
> eviction.

Oh!  For some reason I read it (twice) as "== 0", not "> 0"....which did seem
rather odd.

[mmenke, 2016-02-19 19:36:53]

LGTM, but wondering about one other thing

https://codereview.chromium.org/1715833002/diff/60001/net/disk_cache/memory/m...
File net/disk_cache/memory/mem_entry_impl.cc (right):

https://codereview.chromium.org/1715833002/diff/60001/net/disk_cache/memory/m...
net/disk_cache/memory/mem_entry_impl.cc:357:
backend_->ModifyStorageSize(data_[index].size() - old_data_size);
Hrm...Wait...So if we're a child entry, we are not in use.  We're relying on the
fact that we're no the stalest entry in the backend for this not to result in
our destruction?  But we haven't updated when we were last used yet.

And even if we had, there's nothing to prevent all other entries from being in
use, and this entry ending up being deleted by this call...Is there?

[gavinp, 2016-02-23 19:52:36]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2016-02-23 19:57:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1715833002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1715833002/60001

[commit-bot: I haz the power, 2016-02-23 23:28:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-23 23:30:46]

Description was changed from

==========
Reland: Refactor and shorten in-memory cache.

Previously this code was maintaining a complex doubly-linked list
manually, rather than using base::linked_list, which does the same thing.

A number of uses of vector were strictly speaking violating the C++98
specification, also fixed.

Too many functions were short redirectors into other functions. Now fixed.

The originally landed version contained a significant double free bug
which caused reversion, now fixed.

R=mmenke
BUG=581791,586440
==========

to

==========
Reland: Refactor and shorten in-memory cache.

Previously this code was maintaining a complex doubly-linked list
manually, rather than using base::linked_list, which does the same thing.

A number of uses of vector were strictly speaking violating the C++98
specification, also fixed.

Too many functions were short redirectors into other functions. Now fixed.

The originally landed version contained a significant double free bug
which caused reversion, now fixed.

R=mmenke
BUG=581791,586440

Committed: https://crrev.com/4ffdb91a6e68f0f20fa40a2b224e98ce5da8f629
Cr-Commit-Position: refs/heads/master@{#377124}
==========

[commit-bot: I haz the power, 2016-02-23 23:30:47]

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