Blob Consolidation & Registry Hookup

content/child/blob_storage/blob_consolidation.h

+// 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.
+
+#ifndef CONTENT_CHILD_BLOB_STORAGE_BLOB_CONSOLIDATION_H_
+#define CONTENT_CHILD_BLOB_STORAGE_BLOB_CONSOLIDATION_H_
+
+#include <string>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "content/common/content_export.h"
+#include "storage/common/data_element.h"
+#include "third_party/WebKit/public/platform/WebBlobData.h"
+#include "third_party/WebKit/public/platform/WebString.h"
+#include "third_party/WebKit/public/platform/WebThreadSafeData.h"
+#include "third_party/WebKit/public/platform/WebURL.h"
+
+namespace content {
+
+// This class facilitates the consolidation of memory items in blobs. No memory
+// is copied to store items in this object. Instead, the memory is copied into
+// the external char* array given to the ReadMemory method.
+// Features:
+// * Add_Item methods for building the blob.
+// * consolidated_items for getting the consolidated items. This is
+//   used to describe the blob to the browser.
+// * total_memory to get the total memory size of the blob.
+// * ReadMemory for reading arbitrary memory from any consolidated item.
+// * RecordMemoryRead for recording that an amount of memory has been read from
+//   a given consolidated item. This allows this class to release memory
+//   resources once an item has been read.
+//
+// TODO(dmurph): Keep memory accounting per item instead of globally so we can
+//               free resources while reading instead of all at the end.
+class CONTENT_EXPORT BlobConsolidation {

[michaeln, 2015/06/17 03:17:55]

Seeing the heavy use of blink:WebTypes in here, i was initially concerned that
we'd be exposing more chromium code to the vagaries of WebStrings. But then I
saw this was really an impl detail of your new the Builder so that's not really
a concern.

You might consider nesting this class inside of WebBlobRegistryImpl since i
think that's the only expected consumer of it (modulo tests):
WebBlobRegistryImpl::Builder and WebBlobRegistryImpl::Consolidator.

[dmurph, 2015/06/17 18:29:50]

This class is going to be used elsewhere in later patches (in new files
blob_storage_dispatcher, blob_transport_temporary_holder, and the registry), I'd
rather leave it out.  Especially because it has non-trivial logic and tests.

[michaeln, 2015/06/18 00:10:48]

Got it, makes sense to leave it as a separate class.

But given it's broader use, it would be good to reduce its dependencies on the
non-thread-safe blink backed data types (WebString and WebUrl). In general, we
prefer to translate from WebSpeak into chrome data type speak as near the WebKep
api impl layer as possible for consistency throughout the codebase on what types
we use. And in this case in particular the non-thread-safe data types will
definitely be problematic (easy landmines to step on).

This ConsolidatedItem  struct is more or less a storage::DataElement with some
special casing for multiple fragmented TYPE_BYTES segments within a single
'item'. Wdyt of translating from to DataElement in BuilderImpls appendXXX
methods, with the exception of WebThreadSafeData handling. The consolidator
interface could look more like this...

struct ConsolidatedItem {
  DataElement element;
  std::vector<size_t> offsets;
  std::vector<blink::WebThreadSafeData> data;
}

void AddNonDataItem(const DataElement& data_not_of_type_bytes);
void AddDataItem(const blink::WebThreadSafeData& data);

wdyt?

[dmurph, 2015/06/22 18:17:46]

Whoops, missed this comment.

As discussed in person , I moved the storage to non-web types.

+ public:
+  enum class ReadStatus {
+    ERROR = 0,
+    ERROR_WRONG_TYPE,
+    ERROR_OUT_OF_BOUNDS,
+    BLOB_BYTES_PENDING,
+    DONE
+  };
+  struct ConsolidatedItem {
+    ConsolidatedItem();
+    ConsolidatedItem(storage::DataElement::Type type, uint64_t offset,
+                     uint64_t length);
+    ~ConsolidatedItem();
+
+    storage::DataElement::Type type;
+    uint64_t offset;
+    uint64_t length;
+
+    blink::WebString path;              // For TYPE_FILE.
+    blink::WebURL filesystem_url;       // For TYPE_FILE_FILESYSTEM.
+    double expected_modification_time;  // For TYPE_FILE, TYPE_FILE_FILESYSTEM.
+    blink::WebString blob_uuid;         // For TYPE_BLOB.
+    // Only populated if len(items) > 1.  Used for binary search.
+    // Since the offset of the first item is always 0, we exclude this.
+    std::vector<size_t> offsets;                 // For TYPE_BYTES.
+    std::vector<blink::WebThreadSafeData> data;  // For TYPE_BYTES.
+  };
+
+  BlobConsolidation();
+  ~BlobConsolidation();
+
+  void AddDataItem(const blink::WebThreadSafeData& data);
+  void AddFileItem(const blink::WebString& path, uint64_t offset,
+                   uint64_t length, double expected_modification_time);
+  void AddBlobItem(const blink::WebString& uuid, uint64_t offset,
+                   uint64_t length);
+  void AddFileSystemItem(const blink::WebURL& url, uint64_t offset,
+                         uint64_t length, double expected_modification_time);
+
+  // This gets the consolidated items constructed from the WebBlobData. This
+  // is used to describe the blob to the browser.
+  const std::vector<ConsolidatedItem>& consolidated_items() const {
+    return consolidated_items_;
+  }
+
+  const size_t total_memory() const { return total_memory_; }
+
+  // Reads memory from the given item into the given buffer. Returns:
+  // * ReadStatus::ERROR if the state or arguments are invalid (see error log),
+  // * ReadStatus::ERROR_WRONG_TYPE if the item at the index isn't memory,
+  // * ReadStatus::ERROR_OUT_OF_BOUNDS if index, offset, or size are invalid,
+  // * ReadStatus::DONE if the memory has been successfully read.
+  ReadStatus ReadMemory(size_t consolidated_item_index,
+                        size_t consolidated_offset, size_t consolidated_size,
+                        char* memory_out);

[michaeln, 2015/06/17 03:17:55]

maybe void* memory_out to avoid casting at callsites

[dmurph, 2015/06/17 18:29:50]

Done.  This has the side effect of me needing to cast it to a char* internally
so I can do pointer arithmetic, just FYI

[michaeln, 2015/06/18 00:10:48]

That's a fair trade, better to make life easier for he consumers.

+
+  // Records an amount of memory has been read from a given item and is no
+  // longer needed.
+  // * ReadStatus::ERROR if we have read more than the blob's total memory.
+  // * ReadStatus::ERROR_WRONG_TYPE if the item at the index isn't memory,
+  // * ReadStatus::ERROR_OUT_OF_BOUNDS if index or size are invalid,
+  // * ReadStatus::BLOB_BYTES_PENDING if there are still bytes to be read from
+  //   this blob (not only the given item).
+  // * ReadStatus::DONE if all bytes have been read from this blob.  This object
+  //   can now be destroyed, as all bytes have been read.
+  // TODO(dmurph): do per-item accounting, and error if we exceed per-item size.

[michaeln, 2015/06/17 03:17:55]

How do you anticipate using the accounting? I'm wondering if it's really needed?

[dmurph, 2015/06/17 18:29:50]

There are two cases where the memory is read:
1. on the 'shortcut' approach, where we send as much data as we can in the
initial RPC.  Here we only read, and we don't count that as 'using' memory
2. on a data request from the browser.  After we read we can then count the
memory as 'read', and discard it.  I'm implementing a trivial version of this
bookkeeping, with a TODO for better accounting if it's needed.

Finally, when we get DONE on this method (or a blobdone ipc), we can destruct
this object.

There are many ways to do this, this seemed like a decent way.

[michaeln, 2015/06/18 00:10:48]

Incremental cleanup might be "nice", but I'd vote to start simplest of all,
delete it when done. If/when it comes time to do incremental cleanup, it can be
added in then with a meaningful impl that actually performs incremental deletes.
When it does come time to do incremental cleanup would it make sense to delete
as it's being read, so the consolidated item is drained so to speak?

+  ReadStatus RecordMemoryRead(size_t consolidated_item_index,
+                              size_t memory_size);
+
+ private:
+  size_t total_memory_;
+  size_t total_memory_read_;
+  std::vector<ConsolidatedItem> consolidated_items_;
+
+  DISALLOW_COPY_AND_ASSIGN(BlobConsolidation);
+};
+
+}  // namespace content
+#endif  // CONTENT_CHILD_BLOB_STORAGE_BLOB_CONSOLIDATION_H_