Add read support to PNaClTranslationCache

chrome/browser/nacl_host/pnacl_translation_cache.cc

Index: chrome/browser/nacl_host/pnacl_translation_cache.cc
diff --git a/chrome/browser/nacl_host/pnacl_translation_cache.cc b/chrome/browser/nacl_host/pnacl_translation_cache.cc
index 8316ba7052137881205682015efe67f1ba7f657e..b7f8a366fa31d980265698ffd566402a1efaf904 100644
--- a/chrome/browser/nacl_host/pnacl_translation_cache.cc
+++ b/chrome/browser/nacl_host/pnacl_translation_cache.cc
@@ -33,80 +33,99 @@ const int kMaxDiskCacheSize = 1000 * 1024 * 1024;
 const int kMaxMemCacheSize = 100 * 1024 * 1024;
 
 //////////////////////////////////////////////////////////////////////
-// Handle Storing to Cache.
+// Handle Reading/Writing to Cache.
 
-// PNaClTranslationCacheWriteEntry is a shim that provides storage for the
+// PNaClTranslationCacheEntry is a shim that provides storage for the
 // 'key' and 'data' strings as the disk_cache is performing various async
 // operations. It also tracks the open disk_cache::Entry
 // and ensures that the entry is closed.
-class PNaClTranslationCacheWriteEntry
-    : public base::RefCounted<PNaClTranslationCacheWriteEntry> {
+class PNaClTranslationCacheEntry
+    : public base::RefCounted<PNaClTranslationCacheEntry> {
  public:
-  PNaClTranslationCacheWriteEntry(base::WeakPtr<PNaClTranslationCache> cache,
-                                  const std::string& key,
-                                  const std::string& nexe,
-                                  const net::CompletionCallback& callback);
+  PNaClTranslationCacheEntry(base::WeakPtr<PNaClTranslationCache> cache,
+                             const std::string& key,
+                             std::string* nexe,
+                             const CompletionCallback& callback,
+                             bool is_read);
 
-  void Cache();
+  void Start();
 
-  //                                        ---
+  // Writes:                                ---
   //                                        v  |
-  // Cache -> Open Existing --------------> Write ---> Close
+  // Start -> Open Existing --------------> Write ---> Close
   //                          \              ^
   //                           \             /
   //                            --> Create --
+  // Reads:
+  // Start -> Open --------Read ----> Close
+  //                       |  ^
+  //                       |__|
   enum CacheStep {
     UNINITIALIZED,
     OPEN_ENTRY,
     CREATE_ENTRY,
-    WRITE_ENTRY,
+    TRANSFER_ENTRY,
     CLOSE_ENTRY
   };
 
  private:
-  friend class base::RefCounted<PNaClTranslationCacheWriteEntry>;
-  ~PNaClTranslationCacheWriteEntry();
+  friend class base::RefCounted<PNaClTranslationCacheEntry>;
+  ~PNaClTranslationCacheEntry();
 
   void CreateEntry();
 
   void OpenEntry();
 
-  void WriteEntry(int bytes_to_skip);
+  void WriteEntry(int offset, int len);
+  void ReadEntry(int offset, int len);

[jvoung (off chromium), 2013/06/05 00:42:28]

extra space between functions to be consistent

[Derek Schuff, 2013/06/05 05:01:59]

even better; made consistent and added comments.

 
   void CloseEntry(int rv);
 
+  // Handles state transitions, tracks bytes transferred
   void DispatchNext(int rv);
 
+  // Get the total transfer size. For reads, must be called after the entry
+  // has been info has been fetched from the backend.
+  int GetTransferSize();
+
   base::WeakPtr<PNaClTranslationCache> cache_;
 
   std::string key_;
-  std::string nexe_;
+  std::string* nexe_;
   disk_cache::Entry* entry_;
   CacheStep step_;
+  bool is_read_;
+  int bytes_transferred_;
+  int bytes_to_transfer_;
+  scoped_refptr<net::IOBufferWithSize> read_buf_;
   CompletionCallback finish_callback_;
   base::ThreadChecker thread_checker_;
-  DISALLOW_COPY_AND_ASSIGN(PNaClTranslationCacheWriteEntry);
+  DISALLOW_COPY_AND_ASSIGN(PNaClTranslationCacheEntry);
 };
 
-PNaClTranslationCacheWriteEntry::PNaClTranslationCacheWriteEntry(
+PNaClTranslationCacheEntry::PNaClTranslationCacheEntry(
     base::WeakPtr<PNaClTranslationCache> cache,
     const std::string& key,
-    const std::string& nexe,
-    const net::CompletionCallback& callback)
+    std::string* nexe,
+    const CompletionCallback& callback,
+    bool is_read)
     : cache_(cache),
       key_(key),
       nexe_(nexe),
       entry_(NULL),
       step_(UNINITIALIZED),
+      is_read_(is_read),
+      bytes_transferred_(0),
+      bytes_to_transfer_(-1),
       finish_callback_(callback) {}
 
-PNaClTranslationCacheWriteEntry::~PNaClTranslationCacheWriteEntry() {
+PNaClTranslationCacheEntry::~PNaClTranslationCacheEntry() {
   if (entry_)
     BrowserThread::PostTask(
         BrowserThread::IO, FROM_HERE, base::Bind(&CloseDiskCacheEntry, entry_));
 }
 
-void PNaClTranslationCacheWriteEntry::Cache() {
+void PNaClTranslationCacheEntry::Start() {
   DCHECK(thread_checker_.CalledOnValidThread());
   step_ = OPEN_ENTRY;
   OpenEntry();
@@ -114,49 +133,71 @@ void PNaClTranslationCacheWriteEntry::Cache() {
 
 // OpenEntry, CreateEntry, WriteEntry, and CloseEntry are only called from
 // DispatchNext, so they know that cache_ is still valid.
-void PNaClTranslationCacheWriteEntry::OpenEntry() {
-  int rv = cache_->backend()->OpenEntry(
-      key_,
-      &entry_,
-      base::Bind(&PNaClTranslationCacheWriteEntry::DispatchNext, this));
+void PNaClTranslationCacheEntry::OpenEntry() {
+  int rv = cache_->backend()
+      ->OpenEntry(key_,
+                  &entry_,
+                  base::Bind(&PNaClTranslationCacheEntry::DispatchNext, this));
   if (rv != net::ERR_IO_PENDING)
     DispatchNext(rv);
 }
 
-void PNaClTranslationCacheWriteEntry::CreateEntry() {
+void PNaClTranslationCacheEntry::CreateEntry() {
   int rv = cache_->backend()->CreateEntry(
       key_,
       &entry_,
-      base::Bind(&PNaClTranslationCacheWriteEntry::DispatchNext, this));
+      base::Bind(&PNaClTranslationCacheEntry::DispatchNext, this));
   if (rv != net::ERR_IO_PENDING)
     DispatchNext(rv);
 }
 
-void PNaClTranslationCacheWriteEntry::WriteEntry(int bytes_to_skip) {
-  nexe_ = nexe_.substr(bytes_to_skip);
-  scoped_refptr<net::StringIOBuffer> io_buf = new net::StringIOBuffer(nexe_);
+void PNaClTranslationCacheEntry::WriteEntry(int offset, int len) {
+  scoped_refptr<net::StringIOBuffer> io_buf =
+      new net::StringIOBuffer(nexe_->substr(offset, len));
   int rv = entry_->WriteData(
       1,
-      0,
+      offset,
       io_buf,
-      nexe_.length(),
-      base::Bind(&PNaClTranslationCacheWriteEntry::DispatchNext, this),
+      len,
+      base::Bind(&PNaClTranslationCacheEntry::DispatchNext, this),
       false);
   if (rv != net::ERR_IO_PENDING)
     DispatchNext(rv);
 }
 
-void PNaClTranslationCacheWriteEntry::CloseEntry(int rv) {
-  if (rv < 0)
+void PNaClTranslationCacheEntry::ReadEntry(int offset, int len) {
+  read_buf_ = new net::IOBufferWithSize(len);
+  int rv = entry_->ReadData(
+      1,
+      offset,
+      read_buf_,
+      len,
+      base::Bind(&PNaClTranslationCacheEntry::DispatchNext, this));
+  if (rv != net::ERR_IO_PENDING)
+    DispatchNext(rv);
+}
+
+int PNaClTranslationCacheEntry::GetTransferSize() {
+  if (is_read_) {
+    DCHECK(entry_);
+    return entry_->GetDataSize(1);
+  }
+  return nexe_->size();
+}
+
+void PNaClTranslationCacheEntry::CloseEntry(int rv) {
+  if (entry_ && rv < 0)
     entry_->Doom();
   if (!finish_callback_.is_null()) {
     finish_callback_.Run(rv);
     finish_callback_.Reset();
   }
-  cache_->WriteComplete(this);
+  if (!is_read_)
+    delete nexe_;

[jvoung (off chromium), 2013/06/05 00:42:28]

Perhaps the nexe shouldn't be the same field for both the write operation and
the read operation.  Stuff like GetTransferSize() already do different things
depending on whether it's a read or a write.

The way it is now, to prevent memory leaks during writes you would have to be
sure to call CloseEntry().  Then again, you would also need to remember to call
CloseEntry to return control flow to the user...

[Derek Schuff, 2013/06/05 05:01:59]

Yeah, we need to call CloseEntry anyway. Actually I don't like that we do all
the stuff in CloseEntry but don't actually close the entry until the destructor,
so I fixed that. I'm not sure using a separate field makes it much simpler,
since it complicates construction (you have to have 2 different constructors or
static factories) without simplifying the other logic, since it has to have
different behavior for reads and writes either way. It does make it the data
usage a little clearer, and it means you don't have to manually delete anything.
I dunno, I tried it that way, see what you think. 

[jvoung (off chromium), 2013/06/05 17:52:29]

Well, I think static factories might be better so that the public interface will
be a bit cleaner (no memory ownership transfer, no need for two nexe arguments),
but the code that implements them may look messy.

Anyway, this is probably okay for now. The public interface it will probably
change again once you start working with files.

+  cache_->OpComplete(this);
 }
 
-void PNaClTranslationCacheWriteEntry::DispatchNext(int rv) {
+void PNaClTranslationCacheEntry::DispatchNext(int rv) {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!cache_)
     return;
@@ -168,9 +209,17 @@ void PNaClTranslationCacheWriteEntry::DispatchNext(int rv) {
 
     case OPEN_ENTRY:
       if (rv == net::OK) {
-        step_ = WRITE_ENTRY;
-        WriteEntry(0);
+        step_ = TRANSFER_ENTRY;
+        bytes_to_transfer_ = GetTransferSize();
+        is_read_ ? ReadEntry(0, bytes_to_transfer_)
+                 : WriteEntry(0, bytes_to_transfer_);
       } else {
+        if (is_read_) {
+          // Just a cache miss, not necessarily an error.
+          entry_ = NULL;

[jvoung (off chromium), 2013/06/05 00:42:28]

It looks a bit odd to set entry_ to NULL then call a function that's named
"CloseEntry".  Perhaps the CloseEntry step should be split into two.  One that
closes, and another that's the end of everything, deleting stuff and calls the
callback?

[Derek Schuff, 2013/06/05 05:01:59]

Done.

+          CloseEntry(rv);
+          break;
+        }
         step_ = CREATE_ENTRY;
         CreateEntry();
       }
@@ -178,15 +227,16 @@ void PNaClTranslationCacheWriteEntry::DispatchNext(int rv) {
 
     case CREATE_ENTRY:
       if (rv == net::OK) {
-        step_ = WRITE_ENTRY;
-        WriteEntry(0);
+        step_ = TRANSFER_ENTRY;
+        bytes_to_transfer_ = GetTransferSize();
+        WriteEntry(bytes_transferred_, bytes_to_transfer_ - bytes_transferred_);
       } else {
-        LOG(ERROR) << "Failed to Open/Create a PNaCl Translation Cache Entry";
+        LOG(ERROR) << "Failed to Create a PNaCl Translation Cache Entry";
         CloseEntry(rv);
       }
       break;
 
-    case WRITE_ENTRY:
+    case TRANSFER_ENTRY:
       if (rv < 0) {
         // We do not call DispatchNext directly if WriteEntry returns

[jvoung (off chromium), 2013/06/05 00:42:28]

WriteEntry or ReadEntry now.

[Derek Schuff, 2013/06/05 05:01:59]

Done.

         // ERR_IO_PENDING, and the callback should not return that value either.
@@ -196,15 +246,21 @@ void PNaClTranslationCacheWriteEntry::DispatchNext(int rv) {
         step_ = CLOSE_ENTRY;
         CloseEntry(rv);
         break;
+      } else if (rv > 0) {
+        // For reads, copy the data that was just returned
+        if (is_read_)
+          nexe_->append(read_buf_->data(), rv);
+        bytes_transferred_ += rv;
+        if (bytes_transferred_ < bytes_to_transfer_) {
+          int len = bytes_to_transfer_ - bytes_transferred_;
+          is_read_ ? ReadEntry(bytes_transferred_, len)
+                   : WriteEntry(bytes_transferred_, len);
+          break;
+        }
       }
-      if (rv == 0) {
-        step_ = CLOSE_ENTRY;
-        CloseEntry(rv);
-        break;
-      }
-      // rv bytes were written; call WriteEntry again to skip them and try to
-      // write the rest.
-      WriteEntry(rv);
+      // rv == 0 or we fell through (i.e. we have transferred all the bytes)
+      step_ = CLOSE_ENTRY;
+      CloseEntry(0);
       break;
 
     case CLOSE_ENTRY:
@@ -214,8 +270,7 @@ void PNaClTranslationCacheWriteEntry::DispatchNext(int rv) {
 }
 
 //////////////////////////////////////////////////////////////////////
-void PNaClTranslationCache::WriteComplete(
-    PNaClTranslationCacheWriteEntry* entry) {
+void PNaClTranslationCache::OpComplete(PNaClTranslationCacheEntry* entry) {
   write_entries_.erase(entry);
 }
 
@@ -224,27 +279,25 @@ void PNaClTranslationCache::WriteComplete(
 PNaClTranslationCache::PNaClTranslationCache()
     : disk_cache_(NULL), in_memory_(false) {}
 
-PNaClTranslationCache::~PNaClTranslationCache() {
-  delete disk_cache_;
-}
+PNaClTranslationCache::~PNaClTranslationCache() { delete disk_cache_; }
 
 int PNaClTranslationCache::InitWithDiskBackend(
     const base::FilePath& cache_dir,
     int cache_size,
-    const net::CompletionCallback& callback) {
+    const CompletionCallback& callback) {
   return Init(net::DISK_CACHE, cache_dir, cache_size, callback);
 }
 
 int PNaClTranslationCache::InitWithMemBackend(
     int cache_size,
-    const net::CompletionCallback& callback) {
+    const CompletionCallback& callback) {
   return Init(net::MEMORY_CACHE, base::FilePath(), cache_size, callback);
 }
 
 int PNaClTranslationCache::Init(net::CacheType cache_type,
                                 const base::FilePath& cache_dir,
                                 int cache_size,
-                                const net::CompletionCallback& callback) {
+                                const CompletionCallback& callback) {
   int rv = disk_cache::CreateCacheBackend(
       cache_type,
       net::CACHE_BACKEND_DEFAULT,
@@ -273,35 +326,32 @@ void PNaClTranslationCache::OnCreateBackendComplete(int rv) {
 //////////////////////////////////////////////////////////////////////
 // High-level API
 
-// TODO(dschuff): Surely there must be a way to just create a null callback?
-static void NullCallback(int ignored) {}
-
 void PNaClTranslationCache::StoreNexe(const std::string& key,
                                       const std::string& nexe) {
-  StoreNexe(key, nexe, base::Bind(NullCallback));
+  StoreNexe(key, nexe, CompletionCallback());
 }
 
 void PNaClTranslationCache::StoreNexe(const std::string& key,
                                       const std::string& nexe,
-                                      const net::CompletionCallback& callback) {
-  PNaClTranslationCacheWriteEntry* entry =
-      new PNaClTranslationCacheWriteEntry(AsWeakPtr(), key, nexe, callback);
+                                      const CompletionCallback& callback) {
+  PNaClTranslationCacheEntry* entry = new PNaClTranslationCacheEntry(
+      AsWeakPtr(), key, new std::string(nexe), callback, false);
   write_entries_[entry] = entry;
-  entry->Cache();
+  entry->Start();
 }
 
-int PNaClTranslationCache::GetNexe(const std::string& key,
-                                   std::string* nexe,
-                                   const net::CompletionCallback& callback) {
-  // TODO(dschuff): Actually find the entry, and do the right thing.
-  // Shader cache ended up making a separate ReadHelper, analogous
-  // to the PNaClTranslationCacheWriteEntry.
-  return net::OK;
+void PNaClTranslationCache::GetNexe(const std::string& key,
+                                    std::string* nexe,
+                                    const CompletionCallback& callback) {
+  PNaClTranslationCacheEntry* entry =
+      new PNaClTranslationCacheEntry(AsWeakPtr(), key, nexe, callback, true);
+  write_entries_[entry] = entry;
+  entry->Start();
 }
 
 int PNaClTranslationCache::InitCache(const base::FilePath& cache_directory,
                                      bool in_memory,
-                                     const net::CompletionCallback& callback) {
+                                     const CompletionCallback& callback) {
   int rv;
   in_memory_ = in_memory;
   if (in_memory_) {