net/disk_cache/simple: Reduce size of EntrySet nodes.

net/disk_cache/simple: Reduce size of EntrySet nodes.

The EntrySet type was a base::hash_map that duplicated the hash_key
value in both the key and value types, wasting 8 bytes per node.

Changing the implementation to a base::hash_set has been experimented
with and introduces lots of non trivial code changes (some of them being
dependent on the exact C++ STL implementation).

This is a simpler optimization that simply puts the hash_key out of the
EntryMetadata structure, and updates all related code appropriately.

Note that this only changes the in-memory representation of the index,
the on-disk format is untouched.

BUG=

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

[gavinp, 2013-05-16 13:38:01]

+rdsmith  (FYI, as this might collide with some of your work)

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:241: InsertInEntrySet(
This is the only call site for this function now. Might as well elide it and
remove the method, I think?

[digit1, 2013-05-16 15:31:01]

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:241: InsertInEntrySet(
It's a public function that is also called from simple_index_file.cc and a unit
test I believe.

[pasko, 2013-05-16 18:13:21]

Thanks, David. LGTM with a few suggestions to help a less capable brain (like
mine?) understand the code faster when reading.

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.cc (left):

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:230: EntryMetadata metadata = it->second;
I would prefer to leave it with a named variable for the purpose of reading. If
you are concerned with copying, can make a const reference, although the
compiler should be able to eliminate the copying.

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:544: // merge the new data there with the
data loaded in the initialization.
while we are here, please delete this comment, it is too obvious.

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
net/disk_cache/simple/simple_index_file_unittest.cc:64: EntryMetadata
entries_array[] = {
Since we have hashes_array now, to avoid repeating the numbers it would be
better to create a vector of EntryMetadata in the loop below, and use that.

[pasko-google - do not use, 2013-05-16 18:24:41]

one nit:
net/disk_cache_simple -> net/disk_cache/simple
or just "Simple Cache: " as prefix

[digit1, 2013-05-17 08:45:38]

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.cc (left):

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:230: EntryMetadata metadata = it->second;
Sure, no problem. Done.

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
net/disk_cache/simple/simple_index_file_unittest.cc:64: EntryMetadata
entries_array[] = {
On 2013/05/16 18:13:21, pasko wrote:
> Since we have hashes_array now, to avoid repeating the numbers it would be
> better to create a vector of EntryMetadata in the loop below, and use that.

Done.

[digit1, 2013-05-17 08:46:59]

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
File net/disk_cache/simple/simple_index.cc (right):

https://chromiumcodereview.appspot.com/14746019/diff/6001/net/disk_cache/simp...
net/disk_cache/simple/simple_index.cc:544: // merge the new data there with the
data loaded in the initialization.
On 2013/05/16 18:13:21, pasko wrote:
> while we are here, please delete this comment, it is too obvious.

Done.

[pasko, 2013-05-17 09:21:33]

lgtm again, you'll also need an lg from Gavin or another net/ owner.

[felipeg, 2013-05-17 13:56:51]

On 2013/05/17 09:21:33, pasko wrote:
> lgtm again, you'll also need an lg from Gavin or another net/ owner.

lgtm

[gavinp, 2013-05-17 14:54:17]

lgtm.

Thanks for taking the effort here! Please keep these coming if you feel the
itch... :)

https://codereview.chromium.org/14746019/diff/22002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.cc (right):

https://codereview.chromium.org/14746019/diff/22002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.cc:545: EntryMetadata& current_entry =
ret.first->second;
This is more a comment, and less a request. I would have gone a step farther in
explicitness, and probably defined:

const bool did_insert = ret.second;

here. It's my experience that many good coders don't know the return type of
::insert methods on the associative containers.

https://codereview.chromium.org/14746019/diff/22002/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://codereview.chromium.org/14746019/diff/22002/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:63: static const uint64
kHashes[] = { 11, 22, 33 };
I don't think the static here is helpful.

[digit1, 2013-05-17 15:07:32]

https://chromiumcodereview.appspot.com/14746019/diff/22002/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://chromiumcodereview.appspot.com/14746019/diff/22002/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:63: static const uint64
kHashes[] = { 11, 22, 33 };
Without static, this actually creates a temporary array in the stack and
initializes it by storing each value individually there (I just re-checked that
on my workstation by disassembling the code).

Unfortunately, the compiler isn't very smart with constant local arrays. It does
better with integers though. Granted that's only a unit test, but good to know.

[gavinp, 2013-05-17 15:37:45]

lgtm still stands

https://chromiumcodereview.appspot.com/14746019/diff/22002/net/disk_cache/sim...
File net/disk_cache/simple/simple_index_file_unittest.cc (right):

https://chromiumcodereview.appspot.com/14746019/diff/22002/net/disk_cache/sim...
net/disk_cache/simple/simple_index_file_unittest.cc:63: static const uint64
kHashes[] = { 11, 22, 33 };
On 2013/05/17 15:07:33, digit1 wrote:
> Without static, this actually creates a temporary array in the stack and
> initializes it by storing each value individually there (I just re-checked
that
> on my workstation by disassembling the code).
> 
> Unfortunately, the compiler isn't very smart with constant local arrays. It
does
> better with integers though. Granted that's only a unit test, but good to
know.
> 

Wow, interesting.

What a mess compilers are; I know certainly that objects with constructors are
scary when static and const, and actually cheaper as automatics. So now, thanks
to your investigation, I don't know anything, and I have to disassemble
anything. Thanks, I think....

[gavinp, 2013-05-21 16:41:49]

Will we be landing this?

[digit1, 2013-05-30 12:51:41]

On 2013/05/21 16:41:49, gavinp wrote:
> Will we be landing this?

Yes, sorry, I was distracted by other issues. I've uploaded a simple rebase and
added Ricardo as a reviewer. Waiting for test completions and his approval now
:)
I'll do the same for the other patch too.

[commit-bot: I haz the power, 2013-05-30 13:26:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/14746019/38001

[commit-bot: I haz the power, 2013-05-30 17:04:09]

Change committed as 203166

[rvargas (doing something else), 2013-05-30 18:02:53]

Sounds good but I'm not familiar with this code to do a meaningful review so
instead of nitpicking I'll just defer to the other reviewers.

https://codereview.chromium.org/14746019/diff/38001/net/disk_cache/simple/sim...
File net/disk_cache/simple/simple_index.h (right):

https://codereview.chromium.org/14746019/diff/38001/net/disk_cache/simple/sim...
net/disk_cache/simple/simple_index.h:104: // TODO(felipeg): This way we are
storing the hash_key twice, as the
Remove this todo.