Disk cache: Update Addr to handle file format version 3.

net/disk_cache/addr.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/disk_cache/addr.h"
 
 #include "base/logging.h"
 
+namespace {
+
+bool AreReservedBitsValid(uint32 value) {
+  // These two bits of a block address should not be set.
+  const uint32 kReservedBitsMask = 0x0c000000;
+  return !(value & kReservedBitsMask);
+}
+
+}  // namespace
+
 namespace disk_cache {
 
 int Addr::start_block() const {
   DCHECK(is_block_file());
   return value_ & kStartBlockMask;
 }
 
 int Addr::num_blocks() const {
   DCHECK(is_block_file() || !value_);
   return ((value_ & kNumBlocksMask) >> kNumBlocksOffset) + 1;
 }
 
 bool Addr::SetFileNumber(int file_number) {
   DCHECK(is_separate_file());
   if (file_number & ~kFileNameMask)
     return false;
   value_ = kInitializedMask | file_number;
   return true;
 }
 
-bool Addr::SanityCheck() const {
+Addr Addr::AsExternal() const {
+  DCHECK(file_type() == BLOCK_FILES);
+  CacheAddr new_value = value() & ~kFileTypeMask;

[gavinp, 2013/06/13 19:49:00]

Addr ret(*this);
ret.set_file_type(EXTERNAL);

??

What is AsExternal returning? Why was this addr wrong?

[rvargas (doing something else), 2013/06/13 22:46:29]

BLOCK_FILES is a type of file that holds a small marker about an external file.

So... I can have an entry with a stream pointing to 0x10040005 (f_040005), and
that also means that the fifth block of block_file 4 contains the marker for
this file. So the address for the marker would be 0xd0040005.

These methods translate between the two addresses.

I understand that the mechanism has to be explained somewhere, but I don't think
this file is the right place to do that. With that piece of knowledge, the
description on the header looks clear enough (in fact, it seems clear enough for
someone randomly going through the header without having the full picture of how
things work).

[gavinp, 2013/06/13 23:38:27]

What's a marker for a file?

[rvargas (doing something else), 2013/06/14 02:15:44]

The hash of the key (although I'm saving also EntryRecord.pad1 but I doubt the
hash will be ever increased to 64 bits).

https://docs.google.com/a/google.com/document/d/1UnBlsdJusO4pdgzC4Ut194YVki16...

[gavinp, 2013/06/14 15:15:23]

I don't understand this at all. Please remove Addr::AsExternal() and
Addr::AsBlockFile() and add them in a review in which they are not dead.

[rvargas (doing something else), 2013/06/14 19:25:17]

What is it that you don't understand?

----

Yes, I can remove the new methods (and in fact I'll upload a version doing that
shortly), but I want to point out to you that having callers is _not_ a
prerequisite to add a method when dealing with new functionality being added.
(especially when there is a design document that mentions the specific
functionality, and a public reference that show how the caller uses the new
methods). 

I really thought two trivial methods like these would pose no problem for a
review. I am wondering how am I going to be able to land the new code in a
reasonable timeframe :(

+  return Addr(new_value);
+}
+
+Addr Addr::AsBlockFile() const {
+  DCHECK(is_separate_file());
+  CacheAddr new_value = value() + (BLOCK_FILES << kFileTypeOffset);

[gavinp, 2013/06/13 19:49:00]

Addr ret(*this);
ret.set_file_type(BLOCK_FILES);

??

What is AsBlockFile returning? Why was this addr wrong?

[rvargas (doing something else), 2013/06/13 22:46:29]

The only thing we need is BLOCK_FILES (5) -> 0, and 0 -> 5.

Doing 0 << n to write a 0 somewhere bugs me (yes I know, in the big picture it
doesn't matter). On the other hand, a generic method to set the type has to deal
with xx -> yy, so it first has to clear, with the same pattern that I'm using,
and then set, with the second pattern... all the time. So we still have the same
two lines you don't like (now one after the other), more function calls (and
more code), and we do more work all the time.

but sure, slightly easier to read.

[gavinp, 2013/06/14 15:15:23]

They compile to the same thing. See:

#include <stdint.h>

const uint32_t mask = 0x000ff000;
const int shift = 12;

/*
  89 f8                	mov    %edi,%eax
  25 ff 0f f0 ff       	and    $0xfff00fff,%eax
  c3                   	retq   
*/
uint32_t foo(uint32_t i) {
  return i & ~mask;
}

/*
  89 f0                	mov    %esi,%eax
  81 e7 ff 0f f0 ff    	and    $0xfff00fff,%edi
  c1 e0 0c             	shl    $0xc,%eax
  09 f8                	or     %edi,%eax
  c3                   	retq   
*/
uint32_t quux(uint32_t i, int v) {
  return (v << shift) | (i & ~mask);
}

/*
  89 f8                	mov    %edi,%eax
  25 ff 0f f0 ff       	and    $0xfff00fff,%eax
  c3                   	retq   
*/
uint32_t frob(uint32_t i) {
  return quux(i, 0);
}

[rvargas (doing something else), 2013/06/14 19:25:17]

You are aware that I changed the code to what you wanted a while ago, right?

I fail to grasp what is your point. That a given compiler is able to inline a
function call and optimize part of it? Where did I said that an optimizer would
not be able to do that?

Do you expect me to paste compiled code showing that there may be a function
call (ergo not optimized part)?

I can write hundreds of lines and show you that the compiler optimizes
everything away, and that is not in itself a justification to write hundreds of
lines... as I said on my comment, just because in reality it doesn't matter
doesn't mean something is not going to bug me. And again, I did what you wanted
a while ago.

+  return Addr(new_value);
+}
+
+bool Addr::SanityCheckV2() const {
   if (!is_initialized())
     return !value_;
 
-  if (((value_ & kFileTypeMask) >> kFileTypeOffset) > 4)
+  if (file_type() > BLOCK_4K)
     return false;
 
   if (is_separate_file())
     return true;
 
-  const uint32 kReservedBitsMask = 0x0c000000;
-  return !(value_ & kReservedBitsMask);
+  return AreReservedBitsValid(value_);
+}
+
+bool Addr::SanityCheckV3() const {
+  if (!is_initialized())
+    return !value_;
+
+  // For actual entries, SanityCheckForEntryV3 should be used.
+  if (file_type() > BLOCK_FILES)
+    return false;
+
+  if (is_separate_file())
+    return true;
+
+  return AreReservedBitsValid(value_);
 }
 
 bool Addr::SanityCheckForEntryV2() const {
-  if (!SanityCheck() || !is_initialized())
+  if (!SanityCheckV2() || !is_initialized())
     return false;
 
   if (is_separate_file() || file_type() != BLOCK_256)
     return false;
 
   return true;
 }
 
+bool Addr::SanityCheckForEntryV3() const {
+  if (!is_initialized())
+    return false;
+
+  if (!AreReservedBitsValid(value_))
+    return false;
+
+  if (file_type() != BLOCK_ENTRIES && file_type() != BLOCK_EVICTED)
+    return false;
+
+  if (num_blocks() != 1)
+    return false;
+
+  return true;
+}
+
 bool Addr::SanityCheckForRankings() const {
-  if (!SanityCheck() || !is_initialized())
+  if (!SanityCheckV2() || !is_initialized())
     return false;
 
   if (is_separate_file() || file_type() != RANKINGS || num_blocks() != 1)
     return false;
 
   return true;
 }
 
 }  // namespace disk_cache