Use shared memory for moving data over BlobChannel Mojo interface.

blimp/engine/renderer/blob_channel_sender_proxy.cc

Index: blimp/engine/renderer/blob_channel_sender_proxy.cc
diff --git a/blimp/engine/renderer/blob_channel_sender_proxy.cc b/blimp/engine/renderer/blob_channel_sender_proxy.cc
index 8b689d36c73dd840878105bb72a926845ebe97be..b5e15babd7696666d8f7fa44703875ac892bf1de 100644
--- a/blimp/engine/renderer/blob_channel_sender_proxy.cc
+++ b/blimp/engine/renderer/blob_channel_sender_proxy.cc
@@ -19,6 +19,56 @@ mojom::BlobChannelPtr GetConnectedBlobChannel() {
   return blob_channel_ptr;
 }
 
+// Manages the creation and lifetime of Mojo shared memory buffers for blobs.
+// Cleans up the shared memory state when deleted.
+// Caller is responsible for ensuring that |this| is not deleted until the
+// remote side has acknowledged that it is finished using the buffer.
+class SharedMemoryBlob {

[maniscalco, 2016/06/03 16:43:53]

I saw your note about adding a test for the proxy in another CL.  How about also
adding a simple unit test for this class that constructs/destroys an instance.

[Kevin M, 2016/06/03 17:14:57]

Good idea.

I'll add it in this CL, I just wanted to get the impl out there and vetted
before committing to writing tests.

+ public:
+  explicit SharedMemoryBlob(BlobDataPtr data) {
+    mojo::ScopedSharedBufferHandle local_handle;
+    MojoResult result =
+        mojo::CreateSharedBuffer(NULL, data->data.size(), &local_handle);
+    CHECK_EQ(MOJO_RESULT_OK, result)
+        << "Mojo error when creating shared buffer: " << result;
+
+    result = mojo::MapBuffer(local_handle.get(), 0, data->data.size(),
+                             &mapped_data_, MOJO_MAP_BUFFER_FLAG_NONE);
+    CHECK_EQ(MOJO_RESULT_OK, result)
+        << "Mojo error when memory mapping shared buffer: " << result;
+    memcpy(mapped_data_, data->data.data(), data->data.size());
+
+    // Create read-only handle for browser-side consumption.
+    MojoDuplicateBufferHandleOptions options{
+        sizeof(MojoDuplicateBufferHandleOptions),
+        MOJO_DUPLICATE_BUFFER_HANDLE_OPTIONS_FLAG_READ_ONLY};
+    result =
+        mojo::DuplicateBuffer(local_handle.get(), &options, &remote_handle_);
+    CHECK_EQ(MOJO_RESULT_OK, result)
+        << "Mojo error when creating read-only buffer handle.";
+    DCHECK(remote_handle_.is_valid());
+  }
+
+  ~SharedMemoryBlob() { mojo::UnmapBuffer(mapped_data_); }

[maniscalco, 2016/06/03 16:43:53]

Check result of UnmapBuffer?

[Kevin M, 2016/06/03 17:14:57]

Done.

+
+  mojo::ScopedSharedBufferHandle take_remote_handle() {
+    return std::move(remote_handle_);
+  }
+
+ private:
+  // Pointer to shared memory buffer.
+  void* mapped_data_;
+
+  // Handle to be passed to the remote end of the BlobChannel Mojo service.
+  mojo::ScopedSharedBufferHandle remote_handle_;
+
+  DISALLOW_COPY_AND_ASSIGN(SharedMemoryBlob);
+};
+
+void PutComplete(std::unique_ptr<SharedMemoryBlob>) {
+  // Allow the blob to go out of scope and be deleted.
+}
+
 }  // namespace
 
 BlobChannelSenderProxy::BlobChannelSenderProxy()
@@ -37,8 +87,13 @@ bool BlobChannelSenderProxy::IsInClientCache(const std::string& id) const {
 void BlobChannelSenderProxy::PutBlob(const BlobId& id, BlobDataPtr data) {
   DCHECK(!IsInEngineCache(id));
 
+  size_t size = data->data.size();

[maniscalco, 2016/06/03 16:43:53]

I'm guessing it doesn't really make sense to construct a ShareMemoryBlob of size
0, right?  Should we check data->data.size() or do we rely on mojo to do that?

[Kevin M, 2016/06/03 17:14:57]

Should we prevent that? IIRC zero-length arrays are valid, even if they are
wasteful. I think that we might want to enforce that constraint on the data
producing layer, since I would prefer to keep the layering somewhat orthogonal.

In our case, should a failure occur at any point during WebP transcoding, blob
creation will be aborted and this method will never be called.

[maniscalco, 2016/06/06 15:43:56]

I'm not real familiar with the applications that will use BlobChannel so I don't
have a strong opinion on whether we should allow or prevent zero-length blobs,
but I do think it's something that should be documented (probably where we
define BlobData?) and verified with tests.

Looking at the mojo implementation I got the impression that mojo doesn't allow
for zero-length maps:

https://cs.chromium.org/chromium/src/mojo/edk/embedder/platform_shared_buffer...

(probably worth checking to make sure I didn't miss something when reading the
code)

[Kevin M, 2016/06/09 00:08:32]

You're right, thank you very much for looking into that and correcting my
assumption. Done.

+  std::unique_ptr<SharedMemoryBlob> shared_mem_blob(
+      new SharedMemoryBlob(std::move(data)));
+  blob_channel_->PutBlob(
+      id, shared_mem_blob->take_remote_handle(), size,
+      base::Bind(&PutComplete, base::Passed(std::move(shared_mem_blob))));
   replication_state_[id] = false;

[maniscalco, 2016/06/03 16:43:53]

Question about ordering.  I noticed that the order of setting replication_state_
and PutBlob has changed.  Does that matter?  I ask because if in the future
PutComplete (or something it eventually calls) cares about replication_state_,
it could see the value true if PutBlob invokes its callback synchronously (I'm
not sure if it does or not).

[Kevin M, 2016/06/03 17:14:57]

Done. 

I put it there to emphasize the postcondition, but I don't feel strongly about
it.

I don't think PutBlob() is capable of returning synchronously *unless* I declare
the method as synchronous in the .mojom - that ability exists!

-  blob_channel_->PutBlob(id, data->data);
 }
 
 void BlobChannelSenderProxy::DeliverBlob(const std::string& id) {