[BlobAsync] Patch 4: Browser Classes & Logic.

storage/browser/blob/blob_async_transport_strategy.cc

Index: storage/browser/blob/blob_async_transport_strategy.cc
diff --git a/storage/browser/blob/blob_async_transport_strategy.cc b/storage/browser/blob/blob_async_transport_strategy.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0a9abd42842d3063c6a8066f0a74f1268c696e33
--- /dev/null
+++ b/storage/browser/blob/blob_async_transport_strategy.cc
@@ -0,0 +1,311 @@
+// Copyright 2015 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 <storage/browser/blob/blob_async_transport_strategy.h>

[kinuko, 2015/11/17 14:51:42]

nit: use "" instead of <> and have empty line per our usual style

[dmurph, 2015/11/17 20:40:19]

Done. Eclipse artifact.

+#include <algorithm>
+
+#include "storage/common/blob_storage/blob_storage_constants.h"
+
+namespace storage {
+
+namespace {
+
+const char* kTemporaryFileName = "TODO(dmurph) Replace with real filename";

[michaeln, 2015/11/17 21:55:28]

what about this? make the TODO a comment and the filename something else.

[dmurph, 2015/11/19 02:06:18]

That only adds an extra line of code.  This is deleted and redone a bit for the
blob paging (this is never used now, and should never be used).

+
+bool IsBytes(DataElement::Type type) {
+  return type == DataElement::TYPE_BYTES ||
+         type == DataElement::TYPE_BYTES_DESCRIPTION;
+}
+
+// This class handles the logic of how transported memory is going to be
+// represented as storage in the browser.  The main idea is that all the memory
+// is now packed into file chunks, and the browser items will just reference
+// the file with offsets and sizes.
+class FileStorageStrategy
+    : public BlobAsyncTransportStrategy::BlobSegmentVisitor<uint64_t> {
+ public:
+  typedef uint64_t SizeType;
+
+  FileStorageStrategy(
+      std::vector<BlobAsyncTransportStrategy::MemoryItemRequest>* requests,
+      BlobDataBuilder* builder)
+      : requests(requests), builder(builder), current_storage_item(0) {}
+
+  ~FileStorageStrategy() override {}
+
+  void VisitSegment(const DataElement& element,
+                    size_t element_index,
+                    SizeType element_offset,
+                    size_t segment_index,
+                    SizeType segment_offset,
+                    SizeType size) override {
+    if (!IsBytes(element.type())) {
+      builder->AppendDataElement(element);
+      current_storage_item++;
+      return;
+    }
+    BlobAsyncTransportStrategy::MemoryItemRequest request;
+    request.browser_item_index = current_storage_item;
+    request.browser_item_offset = 0;
+    request.message.request_number = requests->size();
+    request.message.transport_strategy = IPCBlobItemRequestStrategy::FILE;
+    request.message.renderer_item_index = element_index;
+    request.message.renderer_item_offset = element_offset;
+    request.message.size = size;
+    request.message.handle_index = segment_index;
+    request.message.handle_offset = segment_offset;
+
+    requests->push_back(request);
+    builder->AppendFile(
+        base::FilePath::FromUTF8Unsafe(std::string(kTemporaryFileName)),
+        segment_offset, size, base::Time::FromDoubleT(0));
+    current_storage_item++;
+  };
+
+  void Done() override {}
+
+  std::vector<BlobAsyncTransportStrategy::MemoryItemRequest>* requests;
+  BlobDataBuilder* builder;
+
+  size_t current_storage_item;
+};
+
+// This class handles the logic of storing memory that is transported as
+// consolidated shared memory.  The main hurdle is to re-separate memory blocks
+// that cross file or blob boundaries.
+class SharedMemoryStorageStrategy
+    : public BlobAsyncTransportStrategy::BlobSegmentVisitor<size_t> {
+ public:
+  typedef size_t SizeType;
+
+  SharedMemoryStorageStrategy(
+      SizeType max_segment_size,
+      std::vector<BlobAsyncTransportStrategy::MemoryItemRequest>* requests,
+      BlobDataBuilder* builder)
+      : requests(requests),
+        max_segment_size(max_segment_size),
+        storage_element_offset(0),
+        builder(builder),
+        current_item_size(0),
+        current_storage_item(0) {}
+  ~SharedMemoryStorageStrategy() override {}
+
+  void VisitSegment(const DataElement& element,
+                    size_t element_index,
+                    SizeType element_offset,
+                    size_t segment_index,
+                    SizeType segment_offset,
+                    SizeType size) override {
+    if (!IsBytes(element.type())) {
+      builder->AppendFutureData(current_item_size);
+      current_storage_item++;
+      builder->AppendDataElement(element);
+      current_storage_item++;
+      storage_element_offset = 0;
+      current_item_size = 0;
+      return;
+    }
+    if (storage_element_offset + size > max_segment_size) {
+      builder->AppendFutureData(current_item_size);
+      current_storage_item++;
+      current_item_size = 0;
+      storage_element_offset = 0;
+    }
+    BlobAsyncTransportStrategy::MemoryItemRequest request;
+    request.browser_item_index = current_storage_item;
+    request.browser_item_offset = storage_element_offset;
+    request.message.request_number = requests->size();
+    request.message.transport_strategy =
+        IPCBlobItemRequestStrategy::SHARED_MEMORY;
+    request.message.renderer_item_index = element_index;
+    request.message.renderer_item_offset = element_offset;
+    request.message.size = size;
+    request.message.handle_index = segment_index;
+    request.message.handle_offset = segment_offset;
+
+    requests->push_back(request);
+    storage_element_offset += size;
+    current_item_size += size;
+  };
+
+  void Done() override {
+    if (current_item_size != 0) {
+      builder->AppendFutureData(current_item_size);
+    }
+  }
+
+  std::vector<BlobAsyncTransportStrategy::MemoryItemRequest>* requests;
+
+  SizeType max_segment_size;
+  SizeType storage_element_offset;
+  BlobDataBuilder* builder;
+  SizeType current_item_size;
+  size_t current_storage_item;
+};
+}  // namespace
+
+BlobAsyncTransportStrategy::MemoryItemRequest::MemoryItemRequest()
+    : browser_item_index(0), browser_item_offset(0), received(false) {}
+
+BlobAsyncTransportStrategy::BlobAsyncTransportStrategy()
+    : error_(BlobAsyncTransportStrategy::ERROR_NONE), total_bytes_size_(0) {}
+
+BlobAsyncTransportStrategy::~BlobAsyncTransportStrategy() {}
+

[michaeln, 2015/11/17 21:55:28]

A comment like the one you made in the review log would help in here...

if total_blob_size > kMaxBlobSize (say 400MB)
  // Request all data in files
  // (Segment all of the existing data into
  // file blocks, of <= kMaxFileSize)
else if total_blob_size > kMaxIPCSize (this is 150KB)
  // Request all data in shared memory
  // (Segment all of the existing data into
  // shared memory blocks, of <= kMaxSharedMemorySize)
else
  // Request all data to be sent over IPC

[dmurph, 2015/11/19 02:06:18]

Done.

+void BlobAsyncTransportStrategy::Initialize(
+    BlobAsyncTransportStrategy::Mode mode,

[michaeln, 2015/11/17 21:55:28]

Is MODE_NO_DISK functionally equivalent to MODE_DISK with disk_space_left and
max_file_size zero?

[dmurph, 2015/11/19 02:06:17]

Not quite, but I changed it so it is.

After doing the disk part, I can simplify this part and reduce it's
responsibilities a bit.

+    size_t max_ipc_memory_size,
+    size_t max_shared_memory_size,
+    uint64_t max_file_size,
+    size_t max_blob_in_memory_size,
+    uint64_t disk_space_left,
+    size_t memory_left,
+    const std::string& uuid,
+    const std::vector<DataElement>& blob_item_infos) {
+  file_handles_.clear();
+  shared_memory_handles_.clear();
+  requests_.clear();
+  builder_.reset(new BlobDataBuilder(uuid));
+  error_ = BlobAsyncTransportStrategy::ERROR_NONE;
+
+  size_t memory_items = 0;
+  total_bytes_size_ = 0;
+  for (const auto& info : blob_item_infos) {
+    if (!IsBytes(info.type())) {
+      continue;
+    }
+    total_bytes_size_ += info.length();
+    ++memory_items;
+  }
+
+  switch (mode) {
+    case BlobAsyncTransportStrategy::MODE_DISK:
+      if (total_bytes_size_ >
+          disk_space_left + static_cast<uint64_t>(memory_left)) {
+        error_ = BlobAsyncTransportStrategy::ERROR_TOO_LARGE;
+        return;
+      }
+      if (total_bytes_size_ > max_blob_in_memory_size) {
+        if (total_bytes_size_ > disk_space_left) {
+          error_ = BlobAsyncTransportStrategy::ERROR_TOO_LARGE;
+          return;
+        }
+        CreateHandleSizes(total_bytes_size_, max_file_size, &file_handles_);
+        FileStorageStrategy strategy(&requests_, builder_.get());
+        ForEachWithSegment(blob_item_infos, max_file_size, &strategy);
+        return;
+      }
+      break;
+    case BlobAsyncTransportStrategy::MODE_NO_DISK:
+      if (total_bytes_size_ > memory_left) {

[michaeln, 2015/11/17 21:55:28]

It looks like it is provided |memory_left| is less than 
|max_blob_in_memory_size|. It might be nice to not have the two modes if
possible. Do you think disk_space_left = 0 can be used to distinguish between
the two?

[dmurph, 2015/11/19 02:06:17]

Done.

+        error_ = BlobAsyncTransportStrategy::ERROR_TOO_LARGE;
+        return;
+      }
+      break;
+    default:
+      NOTREACHED() << "Invalid mode " << mode;
+      error_ = BlobAsyncTransportStrategy::ERROR_INVALID_PARAMS;
+      return;
+  }
+
+  if (total_bytes_size_ > max_ipc_memory_size) {
+    CHECK_LE(total_bytes_size_, std::numeric_limits<size_t>::max());
+    CreateHandleSizes(static_cast<size_t>(total_bytes_size_),
+                      max_shared_memory_size, &shared_memory_handles_);
+    SharedMemoryStorageStrategy strategy(max_shared_memory_size, &requests_,
+                                         builder_.get());
+    ForEachWithSegment(blob_item_infos, max_shared_memory_size, &strategy);
+    return;
+  }
+  // Since they can all fit in IPC memory, we don't need to segment anything,
+  // and just request them straight in IPC.
+  size_t items_length = blob_item_infos.size();
+  for (size_t i = 0; i < items_length; i++) {
+    const auto& info = blob_item_infos.at(i);
+    if (!IsBytes(info.type())) {
+      builder_->AppendDataElement(info);
+      continue;
+    }
+    BlobAsyncTransportStrategy::MemoryItemRequest request;
+    request.browser_item_index = i;
+    request.browser_item_offset = 0;
+    request.message.request_number = requests_.size();
+    request.message.transport_strategy = IPCBlobItemRequestStrategy::IPC;
+    request.message.renderer_item_index = i;
+    request.message.renderer_item_offset = 0;
+    request.message.size = info.length();
+    requests_.push_back(request);
+    builder_->AppendFutureData(info.length());
+  }
+}
+

[michaeln, 2015/11/17 21:55:28]

This was the note i made at some point to help me understand what what was going
on. A comment like this might help.

Splits each |element| into one or more |segments| of a max_size, invokes the
strategy to determine the request to make for each |segment| produced.

[dmurph, 2015/11/19 02:06:17]

Done, also added a bit to clarify that a segment can span multiple elements.

+template <typename SizeType>
+/* static */ void BlobAsyncTransportStrategy::ForEachWithSegment(
+    const std::vector<DataElement>& items,

[michaeln, 2015/11/17 21:55:28]

it'd be good to use consistent local names for the vector<DataElement> and
vector<DataRequest> in this file. i'd vote for |elements| and |requests| and the
corresponding singular.

[dmurph, 2015/11/19 02:06:18]

Done.

+    SizeType max_segment_size,
+    BlobSegmentVisitor<SizeType>* visitor) {
+  DCHECK_GT(max_segment_size, 0ull);
+  size_t segment_index = 0;
+  SizeType segment_offset = 0;
+  size_t items_length = items.size();
+  for (size_t element_index = 0; element_index < items_length;
+       ++element_index) {
+    const auto& element = items.at(element_index);
+    DataElement::Type type = element.type();
+    if (!IsBytes(type)) {
+      visitor->VisitSegment(element, element_index, 0, 0, 0, 0);

[michaeln, 2015/11/17 21:55:28]

The caller and the callee both have special cases for TYPE_BYTES. Seems like one
or the other of these should not be needed? Maybe add a second virtual method
for VisitNonBytesSegment(e, i)

[dmurph, 2015/11/19 02:06:17]

Done.

+      continue;
+    }
+    SizeType element_memory_left = element.length();
+    SizeType element_offset = 0;
+    while (element_memory_left > 0) {
+      if (segment_offset == max_segment_size) {
+        ++segment_index;
+        segment_offset = 0;
+      }
+      SizeType memory_writing =
+          std::min(max_segment_size - segment_offset, element_memory_left);
+      visitor->VisitSegment(element, element_index, element_offset,
+                            segment_index, segment_offset, memory_writing);
+      element_memory_left -= memory_writing;
+      segment_offset += memory_writing;
+      element_offset += memory_writing;
+    }
+  }
+  visitor->Done();
+}
+
+/* static */ bool BlobAsyncTransportStrategy::ShouldBeShortcut(

[michaeln, 2015/11/17 21:55:28]

style nit: this kind of comment in front of the return value is not used in the
codebase, there are some place where it's annotated like...

// static
void Foo() {
}

/* static */
void Foo() {
}

[dmurph, 2015/11/19 02:06:17]

ok.

+    const std::vector<DataElement>& items,
+    size_t memory_available) {
+  size_t items_length = items.size();
+  size_t shortcut_bytes = 0;
+  for (size_t element_index = 0; element_index < items_length;

[michaeln, 2015/11/17 21:55:28]

would a simpler loop construct work here and also in the ForEach method? You
might be able to get rid of some locals.

for (const DataElement& element : elements)

[dmurph, 2015/11/19 02:06:17]

Yes in this one, but in the ForEach I need the index for a lot of places.

+       ++element_index) {
+    const auto& element = items.at(element_index);
+    DataElement::Type type = element.type();
+    if (type == DataElement::TYPE_BYTES_DESCRIPTION) {
+      return false;
+    }
+    if (type == DataElement::TYPE_BYTES) {
+      shortcut_bytes += element.length();
+    }
+  }
+  return shortcut_bytes <= memory_available;
+}
+
+template <typename SizeType>
+/* static */ void BlobAsyncTransportStrategy::CreateHandleSizes(
+    SizeType total_memory_size,
+    SizeType max_segment_size,
+    std::vector<SizeType>* segment_sizes) {
+  SizeType memory_left = total_memory_size;
+  SizeType segment_size;
+  for (; memory_left > 0; memory_left -= segment_size) {

[michaeln, 2015/11/17 21:55:28]

can this be done with modulo math w/o looping?

 v.insert(v.begin(), total_memory_size / max_segment_size, max_segment_size);
 v.push_back(total_memory_size % max_segment_size);

[dmurph, 2015/11/19 02:06:17]

Sure.

+    segment_size = std::min(max_segment_size, memory_left);
+    segment_sizes->push_back(segment_size);
+  }
+}
+
+}  // namespace storage