Added CacheEntry implementation.

net/disk_cache/flash/cache_entry.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 "base/logging.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/disk_cache/flash/cache_entry.h"
+#include "net/disk_cache/flash/format.h"
+#include "net/disk_cache/flash/log_structured_store.h"
+
+namespace disk_cache {
+
+CacheEntry::CacheEntry(LogStructuredStore* store)
+    : store_(store),
+      id_(-1),
+      init_(false),
+      closed_(false),
+      modified_(false) {
+  DCHECK(store);
+  for (int i = 0; i < kFlashCacheEntryNumStreams; ++i)
+    streams_[i].insync = true;
+}
+
+CacheEntry::CacheEntry(LogStructuredStore* store, int32 id)
+    : store_(store),
+      id_(id),
+      init_(false),
+      closed_(false),
+      modified_(false) {
+  DCHECK(store);
+}
+
+CacheEntry::~CacheEntry() {
+  DCHECK(!init_ || closed_);
+}
+
+bool CacheEntry::Init() {
+  DCHECK(!init_);
+
+  if (!OnDisk()) {
+    init_ = true;
+    return true;
+  }
+
+  int32 stream_sizes[kFlashCacheEntryNumStreams];
+  COMPILE_ASSERT(sizeof(stream_sizes) == kFlashCacheEntryHeaderSize,
+                 invalid_cache_entry_header_size);
+
+  if (!store_->OpenEntry(id_) ||
+      !store_->ReadData(id_, stream_sizes, kFlashCacheEntryHeaderSize, 0))

[rvargas (doing something else), 2012/11/28 03:12:56]

needs braces

[agayev, 2012/11/29 16:14:43]

Done.

+    return false;
+
+  for (int i = 0, offset = kFlashCacheEntryHeaderSize;
+       i < kFlashCacheEntryNumStreams; ++i) {
+    streams_[i].offset = offset;
+    streams_[i].data.resize(stream_sizes[i]);
+    offset += stream_sizes[i];
+  }
+
+  init_ = true;
+  return true;
+}
+
+bool CacheEntry::Close() {
+  DCHECK(init_ && !closed_);
+  if (OnDisk())
+    store_->CloseEntry(id_);
+
+  if (!modified_) {
+    closed_ = true;
+    return true;
+  }
+
+  if (!Save())
+    return false;
+
+  closed_ = true;
+  return true;
+}
+
+int32 CacheEntry::id() const {
+  DCHECK(init_);
+  return id_;
+}
+
+int32 CacheEntry::GetDataSize(int index) const {
+  DCHECK(init_&& ValidStream(index));

[rvargas (doing something else), 2012/11/28 03:12:56]

you should be careful here... it is not clear to me where index comes from, so
it may require run time verification.

[agayev, 2012/11/29 16:14:43]

Done.

+  return streams_[index].data.size();
+}
+
+int CacheEntry::ReadData(int index, int offset, net::IOBuffer* buf,
+                         int buf_len) {
+  DCHECK(init_ && ValidStream(index));

[rvargas (doing something else), 2012/11/28 03:12:56]

same here

[agayev, 2012/11/29 16:14:43]

Done.

+  const int stream_size = static_cast<int>(streams_[index].data.size());

[rvargas (doing something else), 2012/11/28 03:12:56]

drop the const and call GetDataSize

[agayev, 2012/11/29 16:14:43]

Done.  Why drop const?

[rvargas (doing something else), 2012/11/29 20:32:36]

Because it is a local variable that just stores the result of an operation.
Sure, it doesn't change during its lifetime, but there's very little benefit by
making it const, so it is mostly noise while reading the code... it belongs to
the "don't go crazy with const" on the style guide. It would be basically the
same as making |index| const just because it doesn't change during this method.

Note that having "const int kMyConstant = 125" is a different story because we
are now saying that we don't even need a variable.

+
+  if (offset >= stream_size || offset < 0 || buf_len == 0)
+    return 0;
+  if (offset + buf_len > stream_size)
+    buf_len = stream_size - offset;
+
+  if (OnDisk() && !LazyRead(index))
+    return net::ERR_FAILED;
+
+  memcpy(buf->data(), &streams_[index].data[offset], buf_len);

[rvargas (doing something else), 2012/11/28 03:12:56]

And extra copy here doesn't look right. What's the purpose of the extra buffer
at this layer?

[agayev, 2012/11/29 16:14:43]

Right now we are not buffering in the Segment layer, so it's the only buffering
we do, not extra.  Ideally we should buffer at the segment layer, but we need to
experiment to find out which is the best.

[rvargas (doing something else), 2012/11/29 20:32:36]

Well... it is an extra buffer because what we really need is buffering at the
Segment layer (even if it has not been implemented yet).

Another way to look at it: having a buffer here is an optimization that would be
fine if it helps the common use pattern. But the common pattern is Create ->
Write -> Close, or Open -> Read -> Close, with a much more uncommon Open -> Read
-> Overwrite(*) -> Close. None of these benefit by a read buffer here, and in
fact would just make things slower.

(*) Total or partial overwrite depending on 302 or 200 after revalidation, but
we don't really read again the same data.

Sure, the API allows multiple random reads before closing the entry, and we get
there  with multiple readers, or with a different user (other than the HTTP
cache), but that's not the main use case.

+  return buf_len;
+}
+
+int CacheEntry::WriteData(int index, int offset, net::IOBuffer* buf,
+                        int buf_len) {
+  DCHECK(init_ && !closed_ && ValidStream(index));
+  DCHECK(offset >= 0 && buf_len >= 0);

[rvargas (doing something else), 2012/11/28 03:12:56]

same caveat about who's the caller

[agayev, 2012/11/29 16:14:43]

Done.

[rvargas (doing something else), 2012/12/05 00:21:01]

done? does this come from the "user" ?

[agayev, 2012/12/05 16:53:19]

Yes it does.  HttpCache may call WriteData with an index on an entry and it
would travel down here.  So, I've added a runtime verification for that.

+  if (offset + buf_len == 0) {  // Truncate to 0.
+    streams_[index].data.clear();
+    streams_[index].insync = true;
+  } else if (offset == 0) {  // Overwrite and truncate if necessary.
+    streams_[index].data.resize(buf_len);
+    streams_[index].insync = true;
+    memcpy(&streams_[index].data[offset], buf->data(), buf_len);
+  } else {  // Append.
+    DCHECK(offset == static_cast<int>(streams_[index].data.size()));
+    streams_[index].data.resize(offset + buf_len);
+    if (!LazyRead(index))

[rvargas (doing something else), 2012/11/28 03:12:56]

what for?

[agayev, 2012/11/29 16:14:43]

The idea is to not to read a stream unless it is needed.  So, if we are doing
truncate, we do not read it, if we are doing append, we read what we have into
memory and then append to it.

[rvargas (doing something else), 2012/11/29 20:32:36]

But we don't have to read the first part in order to append... we just have to
remember that we are appending, no? Does that even make sense here? I mean, it
is possible to start writing with an offset, but I doubt that's what you have in
mind with this code :)

There's no code to handle updating an entry yet, and we know that doing that
means creating a new entry somewhere, so someone has to read the old entry,
receive writes from the user, and at the end create the new entry (deleting the
old one at some point). It's not clear to me if you are thinking about that use
case here, but if that's the case it seems better to wait until that code
appears.

I mean, I really don't know if this class will handle directly things coming
from the user or not... as in, if |index| is the value passed by the HTTP cache
and this class will grow code to handle the key and additional metadata, or if
that code will be handled by a layer above this one (so that we may have two
CacheEntry objects to deal with updates, for example).

+      return net::ERR_FAILED;
+    memcpy(&streams_[index].data[offset], buf->data(), buf_len);
+  }
+  modified_ = true;
+  return buf_len;
+}
+
+bool CacheEntry::OnDisk() const {
+  return id_ != -1;
+}
+
+bool CacheEntry::ValidStream(int stream_index) const {
+  DCHECK(init_);

[rvargas (doing something else), 2012/11/28 03:12:56]

I'd remove the dchecks for init that you have when calling this method.

[agayev, 2012/11/29 16:14:43]

Done.

+  return stream_index >= 0 && stream_index < kFlashCacheEntryNumStreams;
+}
+
+int32 CacheEntry::Size() const {
+  DCHECK(init_);
+  int32 size = kFlashCacheEntryHeaderSize;
+  for (int i = 0; i < kFlashCacheEntryNumStreams; ++i)
+    size += streams_[i].data.size();

[rvargas (doing something else), 2012/11/28 03:12:56]

GetDataSize

[agayev, 2012/11/29 16:14:43]

Done.

+  DCHECK(size > 0 && size <= kFlashSegmentFreeSpace);
+  return size;
+}
+
+bool CacheEntry::Save() {
+  DCHECK(init_ && !closed_);
+  int32 stream_sizes[kFlashCacheEntryNumStreams];
+  COMPILE_ASSERT(sizeof(stream_sizes) == kFlashCacheEntryHeaderSize,
+                 invalid_cache_entry_header_size);
+
+  for (int i = 0; i < kFlashCacheEntryNumStreams; ++i) {
+    if (!LazyRead(i))

[rvargas (doing something else), 2012/11/28 03:12:56]

why?

[agayev, 2012/11/29 16:14:43]

Say we open an existing entry and update stream 1 and then close it.  The above
makes sure that when we save the new copy, stream 0 and stream 2 are copied as
well, since we do not read a stream unless it is needed.

[rvargas (doing something else), 2012/11/29 20:32:36]

but that use case doesn't work with the rest of the code as is, so it feels out
of place.

+      return false;
+    stream_sizes[i] = streams_[i].data.size();
+  }
+
+  if (!store_->CreateEntry(Size(), &id_))
+    return false;
+  if (!store_->WriteData(stream_sizes, kFlashCacheEntryHeaderSize))
+    return false;
+  for (int i = 0; i < kFlashCacheEntryNumStreams; ++i)
+    if (!store_->WriteData(&streams_[i].data[0], streams_[i].data.size()))
+      return false;
+  store_->CloseEntry(id_);
+  return true;
+}
+
+bool CacheEntry::LazyRead(int index) {
+  DCHECK(init_ && ValidStream(index));
+  Stream& stream = streams_[index];
+  if (!stream.insync)
+    stream.insync = store_->ReadData(id_, &stream.data[0], stream.data.size(),
+                                     stream.offset);
+  return stream.insync;
+}
+
+}  // namespace disk_cache