Use shared memory for moving data over BlobChannel Mojo interface.

blimp/engine/renderer/blob_channel_sender_proxy.cc

 // Copyright 2016 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 "blimp/engine/renderer/blob_channel_sender_proxy.h"
 
+#include "blimp/common/blob_cache/id_util.h"
 #include "content/public/common/service_registry.h"
 #include "content/public/renderer/render_thread.h"
 
 namespace blimp {
 namespace engine {
 namespace {
 
 mojom::BlobChannelPtr GetConnectedBlobChannel() {
   mojom::BlobChannelPtr blob_channel_ptr;
   content::RenderThread::Get()->GetServiceRegistry()->ConnectToRemoteService(
       mojo::GetProxy(&blob_channel_ptr));
   CHECK(blob_channel_ptr) << "Could not connect to BlobChannel Mojo service.";
   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

[Wez, 2016/06/11 00:16:38]

From the caller's PoV, |this| is itself - I think you are saying don't delete
the SharedMemoryBlob until the Mojo service has ACK'd it..?

[Kevin M, 2016/06/16 21:00:38]

Done.

+// remote side has acknowledged that it is finished using the buffer.
+class SharedMemoryBlob {
+ public:
+  explicit SharedMemoryBlob(BlobDataPtr data) {

[Wez, 2016/06/11 00:16:38]

As per style-guide, don't inline methods unless they're trivial setter/getters,
plz!

[Kevin M, 2016/06/16 21:00:38]

Done.

+    mojo::ScopedSharedBufferHandle local_handle;
+    MojoResult result =
+        mojo::CreateSharedBuffer(NULL, data->data.size(), &local_handle);

[Wez, 2016/06/11 00:16:38]

nit: nullptr

[Kevin M, 2016/06/16 21:00:39]

Done.

+    CHECK_EQ(MOJO_RESULT_OK, result)
+        << "Mojo error when creating shared buffer: " << result;

[Wez, 2016/06/11 00:16:38]

CHECK_EQ will already log |result| (and the expected value) in Debug builds, so
do you really need the additional log text?  If you do then could it simply be:

CHECK_EQ(...) << "mojo::CreateSharedBuffer failed: " << result;

and similarly, below?

[Kevin M, 2016/06/16 21:00:39]

Done.

+
+    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.

[Wez, 2016/06/11 00:16:37]

Why not do this on-the-fly in take_remote_handle()?

[Kevin M, 2016/06/16 21:00:38]

Uh, why not, indeed! Done.

+    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() {
+    MojoResult result = mojo::UnmapBuffer(mapped_data_);
+    CHECK_EQ(MOJO_RESULT_OK, result);

[Wez, 2016/06/11 00:16:37]

Looks like you can move the un-mapping into the ctor, immediately after the
memcpy()?

[Kevin M, 2016/06/16 21:00:39]

Done.

+  }
+
+  mojo::ScopedSharedBufferHandle take_remote_handle() {
+    return std::move(remote_handle_);
+  }
+
+ private:
+  // Pointer to shared memory buffer.
+  void* mapped_data_;

[Wez, 2016/06/11 00:16:37]

Why do we store this? It doesn't look like anything can/does access it?

[Kevin M, 2016/06/16 21:00:38]

Done.

+
+  // 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()
-    : blob_channel_(GetConnectedBlobChannel()) {}
+    : BlobChannelSenderProxy(GetConnectedBlobChannel()) {}
 
 BlobChannelSenderProxy::~BlobChannelSenderProxy() {}
 
+BlobChannelSenderProxy::BlobChannelSenderProxy(
+    mojom::BlobChannelPtr blob_channel)
+    : blob_channel_(std::move(blob_channel)) {}
+
 bool BlobChannelSenderProxy::IsInEngineCache(const std::string& id) const {
   return replication_state_.find(id) != replication_state_.end();
 }
 
 bool BlobChannelSenderProxy::IsInClientCache(const std::string& id) const {
-  return replication_state_.find(id)->second;
+  auto found = replication_state_.find(id);
+  return found != replication_state_.end() && found->second;
 }
 
 void BlobChannelSenderProxy::PutBlob(const BlobId& id, BlobDataPtr data) {
-  DCHECK(!IsInEngineCache(id));
+  if (IsInEngineCache(id)) {
+    DLOG(ERROR) << "Redundant blob put attempted: " << BlobIdToString(id);

[Wez, 2016/06/11 00:16:38]

nit: If this is a coding error, consider DLOG(FATAL), otherwise make it a
DVLOG(1)?

[Kevin M, 2016/06/16 21:00:38]

Done.

+    return;
+  }
+
+  size_t size = data->data.size();
+  if (size == 0) {
+    DLOG(FATAL) << "Zero length blob requested: " << BlobIdToString(id);

[Wez, 2016/06/11 00:16:38]

nit: This is PutBlob - surely the zero-length blob was being sent, not
requested?

[Kevin M, 2016/06/16 21:00:38]

Done.

+    return;
+  }
 
   replication_state_[id] = false;
-  blob_channel_->PutBlob(id, data->data);
+  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))));
 }
 
 void BlobChannelSenderProxy::DeliverBlob(const std::string& id) {
-  DCHECK(!IsInClientCache(id));
+  if (!IsInEngineCache(id)) {
+    DLOG(ERROR) << "Attempted to deliver an invalid blob: "
+                << BlobIdToString(id);

[Wez, 2016/06/11 00:16:38]

This seems like a serious coding error - caller has violated the
PutBlob->DeliverBlob contract?

[Kevin M, 2016/06/16 21:00:39]

Done.

+    return;
+  }
+  if (IsInClientCache(id)) {
+    DLOG(ERROR) << "Blob is already in the remote cache:" << BlobIdToString(id);

[Wez, 2016/06/11 00:16:37]

nit: As noted above, is it worth having this be an error, or can it be DVLOG(1)?

[Kevin M, 2016/06/16 21:00:38]

Done.

+    return;
+  }
 
   // We assume that the client will have the blob if we push it.
   // TODO(kmarshall): Revisit this assumption when asynchronous blob transport
   // is supported.
   replication_state_[id] = true;
 
   blob_channel_->DeliverBlob(id);
 }
 
 }  // namespace engine
 }  // namespace blimp