[BlobAsync] Patch 4: Browser Classes & Logic.

storage/browser/blob/blob_transport_strategy.cc

+// 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_transport_strategy.h"
+
+#include <algorithm>
+
+namespace storage {
+
+namespace {
+
+// 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.
+struct FileStorageStrategy {
+  typedef uint64_t SizeType;
+
+  FileStorageStrategy(
+      SizeType total_memory_size, SizeType max_segment_size,
+      std::vector<SizeType>* segment_sizes,
+      std::vector<BlobTransportStrategy::MemoryItemRequest>* requests)

[kinuko, 2015/04/30 15:04:21]

nit: one parameter on each line (here and below)

[dmurph, 2015/05/07 02:07:39]

Done.

+      : segment_sizes(segment_sizes),
+        requests(requests),
+        storage_element_index(0) {
+    SizeType memory_left = total_memory_size;
+    SizeType segment_size = 0;
+    for (; memory_left > 0; memory_left -= segment_size) {
+      segment_size = std::min(max_segment_size, memory_left);
+      segment_sizes->push_back(segment_size);
+    }

[kinuko, 2015/04/30 15:04:21]

What is this segment_sizes for? (Not used?) In the current code it doesn't look
it makes much sense to have this same code in ctors of *Strategy.

[kinuko, 2015/04/30 15:04:21]

nit: I'd keep segment_size within the loop (reads 0.2 sec faster)

while (memory_left > 0) {
   SizeType segment_size = std::min(...);
   segment_sizes->push_back(segment_size);
   memory_left -= segment_size;
}

[dmurph, 2015/05/07 02:07:38]

Done.

+  }
+
+  void operator()(DataElement::Type type, size_t element_index,

[kinuko, 2015/04/30 15:04:21]

nit: in chromium code I've hardly seen operator() usage (I have in blink), the
most common way to express closure-like type is to have a method Run().  Not
sure if it's explicitly discouraged though (while operator overloading is
discouraged)

[dmurph, 2015/05/07 02:07:38]

I could turn these into lambdas as well.  All the for_each std library methods
(and other algorithm stuff) takes functors with the () operator overloaded, so I
was following this pattern.  But I can change it to whatever.

+                  SizeType element_offset, size_t segment_index,
+                  SizeType segment_offset, SizeType size) {
+    if (type != DataElement::TYPE_BYTES) {
+      storage_element_index++;
+      return;
+    }
+    BlobTransportStrategy::MemoryItemRequest request;
+    request.browser_item_index = storage_element_index;

[kinuko, 2015/04/30 15:04:21]

is storage_element_index == element_index always true for this one?

[dmurph, 2015/05/07 02:07:39]

No, as the element index can stay the same for multiple callbacks (where we need
to split the item into chunks).

+    request.browser_item_offset = 0;
+    request.message.request_number = requests->size();
+    request.message.transport_strategy = IPC_BLOB_ITEM_REQUEST_STRATEGY_FILE;
+    request.message.renderer_item_index = element_index;
+    request.message.renderer_item_offset = element_offset;
+    request.message.size = size;

[kinuko, 2015/04/30 15:04:21]

nit: why the order of indices/sizes different in function params and in
BlobItemBytesRequest?  (i.e. Having 'size' between two <index, offset> pairs or
after the paris)

[dmurph, 2015/05/07 02:07:38]

No reason, which way do you prefer?

+    request.message.handle_index = segment_index;
+    request.message.handle_offset = segment_offset;

[kinuko, 2015/04/30 15:04:21]

nit: Intentionally not using BlobItemBytesRequest::CreateFileRequest(...) here?

[dmurph, 2015/05/07 02:07:38]

Yes, it's a little more readable, as you can read the name of the property
that's being set.

+
+    requests->push_back(request);
+    storage_element_index++;
+  };
+
+  std::vector<SizeType>* segment_sizes;
+  std::vector<BlobTransportStrategy::MemoryItemRequest>* requests;
+
+  size_t storage_element_index;
+};
+
+// 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.
+struct SharedMemoryStorageStrategy {
+  typedef size_t SizeType;
+
+  SharedMemoryStorageStrategy(
+      SizeType total_memory_size, SizeType max_segment_size,
+      std::vector<SizeType>* segment_sizes,
+      std::vector<BlobTransportStrategy::MemoryItemRequest>* requests)
+      : segment_sizes(segment_sizes),
+        requests(requests),
+        max_segment_size(max_segment_size),
+        storage_element_index(0),
+        storage_element_offset(0) {
+    SizeType memory_left = total_memory_size;
+    SizeType segment_size = 0;
+    for (; memory_left > 0; memory_left -= segment_size) {
+      segment_size = std::min(max_segment_size, memory_left);
+      segment_sizes->push_back(segment_size);
+    }
+  }
+
+  void operator()(DataElement::Type type, size_t element_index,
+                  SizeType element_offset, size_t segment_index,
+                  SizeType segment_offset, SizeType size) {
+    if (type != DataElement::TYPE_BYTES) {
+      storage_element_index += 2;

[kinuko, 2015/04/30 15:04:21]

I think I'm lost here; why do we add 2?

[dmurph, 2015/05/07 02:07:38]

Added comment.

+      storage_element_offset = 0;
+      return;
+    }
+    if (storage_element_offset + size > max_segment_size) {
+      storage_element_index++;
+      storage_element_offset = 0;
+    }
+    BlobTransportStrategy::MemoryItemRequest request;
+    request.browser_item_index = storage_element_index;
+    request.browser_item_offset = storage_element_offset;
+    request.message.request_number = requests->size();
+    request.message.transport_strategy =
+        IPC_BLOB_ITEM_REQUEST_STRATEGY_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;
+  };
+
+  std::vector<SizeType>* segment_sizes;
+  std::vector<BlobTransportStrategy::MemoryItemRequest>* requests;
+
+  SizeType max_segment_size;
+  size_t storage_element_index;
+  SizeType storage_element_offset;
+};
+}  // namespace
+
+template <typename ForEachFunctor, typename SizeType>
+/* static */ void BlobTransportStrategy::ForEachWithSegment(

[kinuko, 2015/04/30 15:04:21]

nit: same method order in .h and in .cc

[dmurph, 2015/05/07 02:07:38]

Done.

+    const std::vector<DataElement> items, SizeType max_segment_size,
+    ForEachFunctor function) {
+  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);
+    if (element.type() != DataElement::TYPE_BYTES) {
+      function(element.type(), element_index, 0, 0, 0, 0);
+      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);
+      function(DataElement::TYPE_BYTES, element_index, element_offset,
+               segment_index, segment_offset, memory_writing);
+      element_memory_left -= memory_writing;
+      segment_offset += memory_writing;
+      element_offset += memory_writing;
+    }
+  }
+}
+
+BlobTransportStrategy::BlobTransportStrategy()
+    : error_(BlobTransportStrategy::ERROR_NONE) {}
+
+BlobTransportStrategy::~BlobTransportStrategy() {}
+
+void BlobTransportStrategy::Initialize(
+    BlobTransportStrategy::Mode mode, 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::vector<DataElement> blob_item_infos) {
+  file_handles_.clear();
+  shared_memory_handles_.clear();
+  requests_.clear();
+  error_ = BlobTransportStrategy::ERROR_NONE;

[kinuko, 2015/04/30 15:04:21]

Could Initialize() be called multiple times? Is there a reason we don't do this
in ctor?

[dmurph, 2015/05/07 02:07:38]

Yes it can, it's reusable.

+
+  uint64_t total_memory_size = 0;
+  size_t memory_items = 0;

[kinuko, 2015/04/30 15:04:21]

Not used after line 177?

If it's for the # of memory items let's name it num_memory_items or
memory_items_count or sth like that.

[dmurph, 2015/05/07 02:07:39]

Removed.

+  for (const auto& info : blob_item_infos) {
+    if (info.type() != DataElement::TYPE_BYTES) {
+      continue;
+    }
+    ++memory_items;
+    total_memory_size += info.length();
+  }
+
+  switch (mode) {
+    case BlobTransportStrategy::MODE_DISK:
+      if (total_memory_size >
+          disk_space_left + static_cast<uint64_t>(memory_left)) {
+        error_ = BlobTransportStrategy::ERROR_TOO_LARGE;
+        return;
+      }
+      if (total_memory_size > max_blob_in_memory_size) {

[kinuko, 2015/04/30 15:04:21]

max_blob_in_memory_size == kMaxBlobSize (e.g. 400MB) in your comment?

[dmurph, 2015/05/07 02:07:39]

Yes, here we know that we need to save to disk.

+        if (total_memory_size > disk_space_left) {
+          error_ = BlobTransportStrategy::ERROR_TOO_LARGE;
+          return;
+        }
+        FileStorageStrategy strategy(total_memory_size, max_file_size,
+                                     &file_handles_, &requests_);
+        BlobTransportStrategy::ForEachWithSegment(blob_item_infos,
+                                                  max_file_size, strategy);
+        return;
+      }
+      break;
+    case BlobTransportStrategy::MODE_NO_DISK:
+      if (total_memory_size > memory_left) {
+        error_ = BlobTransportStrategy::ERROR_TOO_LARGE;
+        return;
+      }
+      break;
+    default:
+      NOTREACHED() << "Invalid mode " << mode;
+      error_ = BlobTransportStrategy::ERROR_INVALID_PARAMS;
+      return;
+  }
+  if (total_memory_size > max_ipc_memory_size) {
+    DCHECK_LE(total_memory_size, std::numeric_limits<size_t>::max());

[kinuko, 2015/04/30 15:04:21]

If blob_item_infos comes over IPC this cannot be guaranteed, DCHECK_LE might not
be enough

[dmurph, 2015/05/07 02:07:38]

Good catch, changed to CHECK.  This should still never hit though, as we're
checking for maximums above.

+    SharedMemoryStorageStrategy strategy(total_memory_size,
+                                         max_shared_memory_size,
+                                         &shared_memory_handles_, &requests_);
+    BlobTransportStrategy::ForEachWithSegment(blob_item_infos,
+                                              max_shared_memory_size, strategy);
+    return;
+  }
+  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 (info.type() != DataElement::TYPE_BYTES) {
+      continue;
+    }
+    BlobTransportStrategy::MemoryItemRequest request;
+    request.browser_item_index = i;
+    request.browser_item_offset = 0;
+    request.message.request_number = requests_.size();
+    request.message.transport_strategy =
+        storage::IPC_BLOB_ITEM_REQUEST_STRATEGY_IPC;
+    request.message.renderer_item_index = i;
+    request.message.renderer_item_offset = 0;
+    request.message.size = info.length();
+    requests_.push_back(request);
+  }
+}
+
+}  // namespace storage