NaCl: Stop sending resource FDs with NaClProcessMsg_Start

components/nacl/browser/nacl_process_host.cc

Index: components/nacl/browser/nacl_process_host.cc
diff --git a/components/nacl/browser/nacl_process_host.cc b/components/nacl/browser/nacl_process_host.cc
index 08bf18d15d805d3325fb09a670abd661cec50241..1f793dcd6c069ef58f64fb499438eebec11197e7 100644
--- a/components/nacl/browser/nacl_process_host.cc
+++ b/components/nacl/browser/nacl_process_host.cc
@@ -45,6 +45,7 @@
 #include "content/public/common/process_type.h"
 #include "content/public/common/sandboxed_process_launcher_delegate.h"
 #include "ipc/ipc_channel.h"
+#include "ipc/ipc_message_attachment_set.h"
 #include "ipc/ipc_switches.h"
 #include "native_client/src/shared/imc/nacl_imc_c.h"
 #include "net/base/net_util.h"
@@ -70,6 +71,14 @@
 #include "content/public/common/sandbox_init.h"
 #endif
 
+static const size_t kMaxDescriptorsPerMessage = 7;
+
+#if defined(OS_POSIX)
+static_assert(kMaxDescriptorsPerMessage ==

[Mark Seaborn, 2015/05/08 00:35:36]

You could do:

#if defined(OS_POSIX)
static const size_t kMaxDescriptorsPerMessage =
IPC::MessageAttachmentSet::kMaxDescriptorsPerMessage;
#elif defined(OS_WIN)
// Windows imposes no limit...
static const size_t kMaxDescriptorsPerMessage = 100;
#endif

This way the OS_POSIX number isn't duplicated here.

[Yusuke Sato, 2015/05/08 15:12:34]

Removed the code. Done.

+              IPC::MessageAttachmentSet::kMaxDescriptorsPerMessage,
+              "kMaxDescriptorsPerMessage is not up to date");
+#endif
+
 using content::BrowserThread;
 using content::ChildProcessData;
 using content::ChildProcessHost;
@@ -873,8 +882,9 @@ bool NaClProcessHost::StartNaClExecution() {
   // Pass the pre-opened resource files to the loader. We do not have to reopen
   // resource files here even for SFI mode because the descriptors are not from
   // a renderer.
+  std::vector<NaClResourcePrefetchResult> prefetched_resource_files_to_send;
   for (size_t i = 0; i < prefetched_resource_files_.size(); ++i) {
-    params.prefetched_resource_files.push_back(
+    prefetched_resource_files_to_send.push_back(
         NaClResourcePrefetchResult(
             prefetched_resource_files_[i].file,
             // For the same reason as the comment below, always use an empty
@@ -882,6 +892,12 @@ bool NaClProcessHost::StartNaClExecution() {
             (uses_nonsfi_mode_ ? base::FilePath() :
              prefetched_resource_files_[i].file_path_metadata),
             prefetched_resource_files_[i].file_key));
+    if (prefetched_resource_files_to_send.size() >= kMaxDescriptorsPerMessage ||

[Mark Seaborn, 2015/05/08 00:35:36]

Can you check whether this batching of multiple FDs per message affects
performance at all?  If it doesn't, it would be slightly simpler to send one FD
per message.

[Yusuke Sato, 2015/05/08 15:12:34]

Done.

Checked performance, and didn't see any difference between 1 per message and 7
per message. Changed to 1.

[Mark Seaborn, 2015/05/08 17:18:24]

Great, thanks for checking!

Does this version perform as well as when Chromium was sending ~100 FDs in a
single IPC message?

[Yusuke Sato, 2015/05/08 18:41:17]

Yes it does.

+        i == prefetched_resource_files_.size() - 1) {
+      process_->Send(new NaClProcessMsg_SetPrefetchedResource(
+          prefetched_resource_files_to_send));
+      prefetched_resource_files_to_send.clear();
+    }
   }
   prefetched_resource_files_.clear();