Increase MessageAttachmentSet::kMaxDescriptorsPerMessage

Increase MessageAttachmentSet::kMaxDescriptorsPerMessage to 128

so that dynamically linked NaCl applications like ARC (App Runtime
for Chrome) start faster.

ARC is a dynamically linked NaCl application which lists more than
70 DSO files in its nmf file. crrev.com/319931 allowed Chrome to
pre-open up to 2 DSO files when it spawns a NaCl plugin process so
that the plugin process can open/read/map the DSO files without
issuing any Chrome IPC. This CL increase the number of DSO files
Chrome can pre-open from 2 to 120+.

BUG=348232
BUG=nativeclient:3802
TEST=ipc_tests, browser_tests (*PackagedAppTest.SuccessfulLoad)
TEST=manually tested non-SFI ARC apps

Committed: https://crrev.com/eaa3c5bf9ad0ac1730eec0bfe1f166f29fe20fc2
Cr-Commit-Position: refs/heads/master@{#320831}

[Yusuke Sato, 2015-03-12 17:56:12]

yusukes@chromium.org changed reviewers:
+ agl@chromium.org, dmichael@chromium.org, mseaborn@chromium.org

[Yusuke Sato, 2015-03-12 17:56:51]

Owners, could you take a look?

Adam:
ipc/ipc_message_attachment_set.cc
ipc/ipc_send_fds_test.cc

Dave:
ipc/ipc_channel_nacl.cc
ppapi/

Mark:
component/nacl/

Thanks!

https://codereview.chromium.org/998833002/diff/20001/ipc/ipc_send_fds_test.cc
File ipc/ipc_send_fds_test.cc (right):

https://codereview.chromium.org/998833002/diff/20001/ipc/ipc_send_fds_test.cc...
ipc/ipc_send_fds_test.cc:37: const unsigned kNumMessages = 3;
Changed this to 3 because some builders' RLIMIT_NOFILE (max open files) is set
to 512.

[agl, 2015-03-12 18:07:08]

https://codereview.chromium.org/998833002/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_host_message_filter.cc (right):

https://codereview.chromium.org/998833002/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_host_message_filter.cc:29: const size_t
kMaxPreOpenResourceFiles = 123;
123 is an odd number that requires a comment.

https://codereview.chromium.org/998833002/diff/20001/ppapi/tests/extensions/e...
File ppapi/tests/extensions/extensions.gyp (right):

https://codereview.chromium.org/998833002/diff/20001/ppapi/tests/extensions/e...
ppapi/tests/extensions/extensions.gyp:74: #
IPC::MessageAttachmentSet::kMaxDescriptorsPerMessage.
Don't you need to add more? If the point is to test numbers of resources that
exceed the max descriptors per message, shouldn't there be > 128 of them?

[Yusuke Sato, 2015-03-12 20:58:48]

Thanks for the review. Please take another look.

https://codereview.chromium.org/998833002/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_host_message_filter.cc (right):

https://codereview.chromium.org/998833002/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_host_message_filter.cc:29: const size_t
kMaxPreOpenResourceFiles = 123;
On 2015/03/12 18:07:07, agl wrote:
> 123 is an odd number that requires a comment.

Done.

https://codereview.chromium.org/998833002/diff/20001/ppapi/tests/extensions/e...
File ppapi/tests/extensions/extensions.gyp (right):

https://codereview.chromium.org/998833002/diff/20001/ppapi/tests/extensions/e...
ppapi/tests/extensions/extensions.gyp:74: #
IPC::MessageAttachmentSet::kMaxDescriptorsPerMessage.
On 2015/03/12 18:07:07, agl wrote:
> Don't you need to add more? If the point is to test numbers of resources that
> exceed the max descriptors per message, shouldn't there be > 128 of them?

Currently there are 129 resource files (the file number is 0 origin). Updated
the comment to make it clearer.

[dmichael (off chromium), 2015-03-12 22:04:28]

dmichael@chromium.org changed reviewers:
+ morrita@chromium.org

[dmichael (off chromium), 2015-03-12 22:04:29]

+morrita, as the person most familiar with MessageAttachmentSet.

[agl, 2015-03-12 22:05:45]

LGTM for ipc/

[dmichael (off chromium), 2015-03-12 22:07:16]

https://codereview.chromium.org/998833002/diff/40001/ipc/ipc_channel_nacl.cc
File ipc/ipc_channel_nacl.cc (right):

https://codereview.chromium.org/998833002/diff/40001/ipc/ipc_channel_nacl.cc#...
ipc/ipc_channel_nacl.cc:40: contents->fds.resize(NACL_ABI_IMC_DESC_MAX);
We want this number to be == kMaxDescriptorsPerMessage, right? Is it possible to
have a static_assert here so we know if one of them changes?

[Yusuke Sato, 2015-03-12 22:19:28]

https://codereview.chromium.org/998833002/diff/40001/ipc/ipc_channel_nacl.cc
File ipc/ipc_channel_nacl.cc (right):

https://codereview.chromium.org/998833002/diff/40001/ipc/ipc_channel_nacl.cc#...
ipc/ipc_channel_nacl.cc:40: contents->fds.resize(NACL_ABI_IMC_DESC_MAX);
On 2015/03/12 22:07:16, dmichael wrote:
> We want this number to be == kMaxDescriptorsPerMessage, right? Is it possible
to
> have a static_assert here so we know if one of them changes?

NACL_ABI_IMC_DESC_MAX defined in native_client/ is currently very small (8,
IIRC) and I think that's okay.

My understanding is that NACL_ABI_IPC_DESC_MAX defines the maximum number of FDs
that can be passed with one message from trusted to untrusted. Since there is no
IRT at this point that requires the trusted side to pass a large number of FDs
at once, I think keeping NACL_ABI_IMC_DESC_MAX small makes sense.

https://codereview.chromium.org/998833002/diff/40001/ipc/ipc_channel_nacl.cc#...
ipc/ipc_channel_nacl.cc:44: &iov, 1, &contents->fds[0], contents->fds.size()
.. and if we use 128 (MessageAttachmentSet::kMaxDescriptorsPerMessage) here,
code in native_client/ fails saying that the size is too large.

[dmichael (off chromium), 2015-03-12 22:42:02]

ppapi lgtm

[Hajime Morrita, 2015-03-12 23:32:43]

On 2015/03/12 22:42:02, dmichael wrote:
> ppapi lgtm

lgtm. The constant has been there and MessageAttachmentSet just got it through a
refactoring.

[Yusuke Sato, 2015-03-13 19:34:57]

Mark: ping?

[Mark Seaborn, 2015-03-16 17:06:50]

LGTM

https://codereview.chromium.org/998833002/diff/40001/ppapi/tests/extensions/p...
File ppapi/tests/extensions/packaged_app/test_packaged_app.cc (right):

https://codereview.chromium.org/998833002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:27: // TODO(yusukes):
Once all the NaCl toolchains support C++11, Add
Which NaCl toolchains does "ipc/file_descriptor_set_posix.h" not work with? 
Which toolchains does this file get compiled with?

If this is getting compiled with both the old nacl-gcc and the up-to-date Clang,
can you add a check conditionalised on "#if defined(__clang__)"?

[Yusuke Sato, 2015-03-16 21:27:28]

https://codereview.chromium.org/998833002/diff/40001/ppapi/tests/extensions/p...
File ppapi/tests/extensions/packaged_app/test_packaged_app.cc (right):

https://codereview.chromium.org/998833002/diff/40001/ppapi/tests/extensions/p...
ppapi/tests/extensions/packaged_app/test_packaged_app.cc:27: // TODO(yusukes):
Once all the NaCl toolchains support C++11, Add
On 2015/03/16 17:06:50, Mark Seaborn wrote:
> Which NaCl toolchains does "ipc/file_descriptor_set_posix.h" not work with? 
> Which toolchains does this file get compiled with?
> 
> If this is getting compiled with both the old nacl-gcc and the up-to-date
Clang,
> can you add a check conditionalised on "#if defined(__clang__)"?

At least '../../../native_client/build/build_nexe.py ...  --arch x86-64 --build
newlib_nexe' is not compatible with the header.

Adding __clang__ is a very good idea and I've confirmed it works as expected,
but it also requires a DEPS change. Let me do that in a subsequent change. I'll
send it to you today.

[Yusuke Sato, 2015-03-16 21:27:34]

The CQ bit was checked by yusukes@chromium.org

[commit-bot: I haz the power, 2015-03-16 21:28:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/998833002/40001

[commit-bot: I haz the power, 2015-03-17 00:34:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-17 00:34:47]

Patchset 3 (id:??) landed as
https://crrev.com/eaa3c5bf9ad0ac1730eec0bfe1f166f29fe20fc2
Cr-Commit-Position: refs/heads/master@{#320831}

[Tom Sepez, 2015-05-05 15:28:20]

Would like to ask for a revert pending the outcome of
https://codereview.chromium.org/1120343002/.  
Can you tell us how the performance scales vs. number of descriptors?  Would 12
- 16 still be acceptable?

[Mark Seaborn, 2015-05-05 19:18:10]

On 5 May 2015 at 08:28, <tsepez@chromium.org> wrote:

> Would like to ask for a revert pending the outcome of
> https://codereview.chromium.org/1120343002/.
> Can you tell us how the performance scales vs. number of descriptors?
> Would 12
> - 16 still be acceptable?


I would be OK with reverting this, since bloating IPC::Channel to ~100k is
quite bad and affects all users of Chrome, whereas increasing
kMaxDescriptorsPerMessage only benefits one use case that isn't very common.

You might find that there are conflicts when reverting, though, since the
code might have been refactored since.

I haven't thought about the proposed fix (in
https://codereview.chromium.org/1120343002/) very hard, but I can imagine
that accepting ~128 FDs while keeping recvmsg()'s buffer small could be
rather tricky, given that it potentially needs to handle receiving multiple
Chrome IPC messages.

The code path that wanted ~128 FDs per message could be changed to stream
the FDs across multiple messages (one FD per message).  I suggested that in
https://codereview.chromium.org/649603004#msg39.  I think that could be
worth implementing to avoid the problems with trying to allow ~128 FDs per
message.

Yusuke, what do you think?

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yusuke Sato, 2015-05-05 20:27:23]

What about changing ipc/ipc_message_attachment_set.h to something like

#if defined(OS_CHROME)
  static const size_t kMaxDescriptorsPerMessage = 128;
#else
  static const size_t kMaxDescriptorsPerMessage = 7;
#else

so that Chrome for Android won't be affected? The kMaxDescriptorsPerMessage
is primarily for ARC (App Runtime for Chrome) and ARC's production target
is only Chrome OS at this point. If this is acceptable for you, I'll
prepare a CL asap.

-Yusuke



On Tue, May 5, 2015 at 12:17 PM, Mark Seaborn <mseaborn@chromium.org> wrote:

> On 5 May 2015 at 08:28, <tsepez@chromium.org> wrote:
>
>> Would like to ask for a revert pending the outcome of
>> https://codereview.chromium.org/1120343002/.
>> Can you tell us how the performance scales vs. number of descriptors?
>> Would 12
>> - 16 still be acceptable?
>
>
> I would be OK with reverting this, since bloating IPC::Channel to ~100k is
> quite bad and affects all users of Chrome, whereas increasing
> kMaxDescriptorsPerMessage only benefits one use case that isn't very common.
>
> You might find that there are conflicts when reverting, though, since the
> code might have been refactored since.
>
> I haven't thought about the proposed fix (in
> https://codereview.chromium.org/1120343002/) very hard, but I can imagine
> that accepting ~128 FDs while keeping recvmsg()'s buffer small could be
> rather tricky, given that it potentially needs to handle receiving multiple
> Chrome IPC messages.
>
> The code path that wanted ~128 FDs per message could be changed to stream
> the FDs across multiple messages (one FD per message).  I suggested that in
> https://codereview.chromium.org/649603004#msg39.  I think that could be
> worth implementing to avoid the problems with trying to allow ~128 FDs per
> message.
>
> Yusuke, what do you think?
>
> Cheers,
> Mark
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2015-05-05 20:54:13]

On 5 May 2015 at 13:27, Yusuke Sato <yusukes@chromium.org> wrote:

> What about changing ipc/ipc_message_attachment_set.h to something like
>
> #if defined(OS_CHROME)
>   static const size_t kMaxDescriptorsPerMessage = 128;
> #else
>   static const size_t kMaxDescriptorsPerMessage = 7;
> #else
>
> so that Chrome for Android won't be affected?
> The kMaxDescriptorsPerMessage is primarily for ARC (App Runtime for Chrome)
> and ARC's production target is only Chrome OS at this point. If this is
> acceptable for you, I'll prepare a CL asap.
>

Please no, that's really hacky. :-(  Memory usage is still an issue on
Chrome OS -- probably more so than desktop Chrome because many Chromebooks
have limited memory.  Also it fragments test coverage because handling 128
FDs would only be tested in OS_CHROME builds.

Is there a reason you don't want to do streaming of FDs (i.e. one per
message)?  Obviously it's some extra work to implement, but it doesn't seem
like very much?

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yusuke Sato, 2015-05-05 22:23:50]

On Tue, May 5, 2015 at 1:53 PM, Mark Seaborn <mseaborn@chromium.org> wrote:

> On 5 May 2015 at 13:27, Yusuke Sato <yusukes@chromium.org> wrote:
>
>> What about changing ipc/ipc_message_attachment_set.h to something like
>>
>> #if defined(OS_CHROME)
>>   static const size_t kMaxDescriptorsPerMessage = 128;
>> #else
>>   static const size_t kMaxDescriptorsPerMessage = 7;
>> #else
>>
>> so that Chrome for Android won't be affected?
>> The kMaxDescriptorsPerMessage is primarily for ARC (App Runtime for Chrome)
>> and ARC's production target is only Chrome OS at this point. If this is
>> acceptable for you, I'll prepare a CL asap.
>>
>
> Please no, that's really hacky. :-(  Memory usage is still an issue on
> Chrome OS -- probably more so than desktop Chrome because many Chromebooks
> have limited memory.  Also it fragments test coverage because handling 128
> FDs would only be tested in OS_CHROME builds.
>

Ok.. if the #ifdef hack above is not acceptable, please revert the
kMaxDescriptorsPerMessage = 128 change.

Is there a reason you don't want to do streaming of FDs (i.e. one per
> message)?  Obviously it's some extra work to implement, but it doesn't seem
> like very much?
>

My concern was performance. Since Lollipop version of ARC has 110+ small
DSOs, I think streaming FDs one by one would require 110+ extra Chrome IPCs
before sending NaClProcessMsg_Start. Since the intention of
the kMaxDescriptorsPerMessage change was to reduce ARC's startup time by a
few hundred milliseconds by reducing the total number of Chrome IPCs,
adding 110+ extra IPC is kind of opposite of the original intention of the
change.. and will probably increase the ARC startup time.

However, with kMaxDescriptorsPerMessage = 7, we can actually send (stream)
7 FDs per message instead of 1, So the number of extra IPCs is ~16. I'll
check if this is acceptable for ARC. Please feel free to revert the change
for now (I can do it if needed.)

-Yusuke

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2015-05-05 22:29:46]

On 5 May 2015 at 15:23, Yusuke Sato <yusukes@chromium.org> wrote:

> On Tue, May 5, 2015 at 1:53 PM, Mark Seaborn <mseaborn@chromium.org>
> wrote:
>
>> On 5 May 2015 at 13:27, Yusuke Sato <yusukes@chromium.org> wrote:
>>
>>> What about changing ipc/ipc_message_attachment_set.h to something like
>>>
>>> #if defined(OS_CHROME)
>>>   static const size_t kMaxDescriptorsPerMessage = 128;
>>> #else
>>>   static const size_t kMaxDescriptorsPerMessage = 7;
>>> #else
>>>
>>> so that Chrome for Android won't be affected?
>>> The kMaxDescriptorsPerMessage is primarily for ARC (App Runtime for Chrome)
>>> and ARC's production target is only Chrome OS at this point. If this is
>>> acceptable for you, I'll prepare a CL asap.
>>>
>>
>> Please no, that's really hacky. :-(  Memory usage is still an issue on
>> Chrome OS -- probably more so than desktop Chrome because many Chromebooks
>> have limited memory.  Also it fragments test coverage because handling 128
>> FDs would only be tested in OS_CHROME builds.
>>
>
> Ok.. if the #ifdef hack above is not acceptable, please revert the
> kMaxDescriptorsPerMessage = 128 change.
>
> Is there a reason you don't want to do streaming of FDs (i.e. one per
>> message)?  Obviously it's some extra work to implement, but it doesn't seem
>> like very much?
>>
>
> My concern was performance. Since Lollipop version of ARC has 110+ small
> DSOs, I think streaming FDs one by one would require 110+ extra Chrome IPCs
> before sending NaClProcessMsg_Start. Since the intention of
> the kMaxDescriptorsPerMessage change was to reduce ARC's startup time by a
> few hundred milliseconds by reducing the total number of Chrome IPCs,
> adding 110+ extra IPC is kind of opposite of the original intention of the
> change.. and will probably increase the ARC startup time.
>

Have you checked whether sending many small Chrome IPC messages is slow, or
are you guessing?  The messages would be async and pipelined, so there's no
round trip between processes per message.  At the syscall level, sendmsg()
will just be adding the messages to an in-kernel buffer, and recvmsg() may
well remove all the messages from the buffer in one go.

Cheers,
Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yusuke Sato, 2015-05-06 01:34:56]

On Tue, May 5, 2015 at 3:29 PM, Mark Seaborn <mseaborn@chromium.org> wrote:

> On 5 May 2015 at 15:23, Yusuke Sato <yusukes@chromium.org> wrote:
>
>> On Tue, May 5, 2015 at 1:53 PM, Mark Seaborn <mseaborn@chromium.org>
>> wrote:
>>
>>> On 5 May 2015 at 13:27, Yusuke Sato <yusukes@chromium.org> wrote:
>>>
>>>> What about changing ipc/ipc_message_attachment_set.h to something like
>>>>
>>>> #if defined(OS_CHROME)
>>>>   static const size_t kMaxDescriptorsPerMessage = 128;
>>>> #else
>>>>   static const size_t kMaxDescriptorsPerMessage = 7;
>>>> #else
>>>>
>>>> so that Chrome for Android won't be affected?
>>>> The kMaxDescriptorsPerMessage is primarily for ARC (App Runtime for
Chrome)
>>>> and ARC's production target is only Chrome OS at this point. If this is
>>>> acceptable for you, I'll prepare a CL asap.
>>>>
>>>
>>> Please no, that's really hacky. :-(  Memory usage is still an issue on
>>> Chrome OS -- probably more so than desktop Chrome because many Chromebooks
>>> have limited memory.  Also it fragments test coverage because handling 128
>>> FDs would only be tested in OS_CHROME builds.
>>>
>>
>> Ok.. if the #ifdef hack above is not acceptable, please revert the
>> kMaxDescriptorsPerMessage = 128 change.
>>
>> Is there a reason you don't want to do streaming of FDs (i.e. one per
>>> message)?  Obviously it's some extra work to implement, but it doesn't seem
>>> like very much?
>>>
>>
>> My concern was performance. Since Lollipop version of ARC has 110+ small
>> DSOs, I think streaming FDs one by one would require 110+ extra Chrome IPCs
>> before sending NaClProcessMsg_Start. Since the intention of
>> the kMaxDescriptorsPerMessage change was to reduce ARC's startup time by a
>> few hundred milliseconds by reducing the total number of Chrome IPCs,
>> adding 110+ extra IPC is kind of opposite of the original intention of the
>> change.. and will probably increase the ARC startup time.
>>
>
> Have you checked whether sending many small Chrome IPC messages is slow,
> or are you guessing?  The messages would be async and pipelined, so there's
> no round trip between processes per message.  At the syscall level,
> sendmsg() will just be adding the messages to an in-kernel buffer, and
> recvmsg() may well remove all the messages from the buffer in one go.
>

No I haven't, I just guessed. I'll report the result to you tomorrow (I
just finished writing a CL for streaming FDs.)

tsepez:
Re reverting the kMaxDescriptorsPerMessage change, here is a CL:
https://codereview.chromium.org/1127913002/
I'll start code review with Mark and Adam shortly.

-Yusuke


>
> Cheers,
> Mark
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.