Re: IPCStream landed in mozilla-central

[Ben Kelly]

On Sat, May 21, 2016 at 2:05 PM, Ben Kelly  wrote:

>
> On May 21, 2016 9:44 AM, "Honza Bambas"  wrote:
> > If it's nsPipeInputStream then it's definitely alright.  OTOH, we do
> copy the memory, right?  I was somehow hoping that you just expose the
> IPC-allocated buffers via your own implementation of nsIInputStream,
> avoiding coping to an XPCOM pipe.
>
> That would be nice, but no.  IPC doesn't even support passing dependent
> strings as far as I know.
>
> This first iteration is just an actor like any other.  I didn't change the
> IPC internals at all.
>
Oh, I think understand better now.  I wrote a bug to implement this idea in
the future:

https://bugzilla.mozilla.org/show_bug.cgi?id=1274815

Ben
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Ben Kelly]

On May 21, 2016 9:44 AM, "Honza Bambas"  wrote:
> If it's nsPipeInputStream then it's definitely alright.  OTOH, we do copy
the memory, right?  I was somehow hoping that you just expose the
IPC-allocated buffers via your own implementation of nsIInputStream,
avoiding coping to an XPCOM pipe.

That would be nice, but no.  IPC doesn't even support passing dependent
strings as far as I know.

This first iteration is just an actor like any other.  I didn't change the
IPC internals at all.

Ben
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Ben Kelly]

On May 21, 2016 7:45 AM, "Honza Bambas"  wrote:
> But that doesn't mean "a fixed length input stream" - actually I may not
even follow how you have translated this to you.

Sorry, I was thinking a single OnDataAvailable call for the one IPC call
just passing the stream.  Clearly that won't work, though.

> As I understand, your impl of Available() may return _different_ number
of bytes than the stream is then able to deliver when Read/ReadSegments on
it is called, right?  Can you explain why?

No.  Available() should work fine.  It's just an nsPipeInputStream.

So we should be able to loop and call ODA for each ReadSegments callback
after the IPC hop.

Sorry for my confusion.

Ben
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Honza Bambas]

On 5/21/2016 2:36, Ben Kelly wrote:

On Fri, May 20, 2016 at 8:10 PM, Ben Kelly  wrote:


On Fri, May 20, 2016 at 11:09 AM, Ben Kelly  wrote:


On Fri, May 20, 2016 at 7:37 AM, Honza Bambas 
wrote:


And I do! :)  Actually any parent necko channel, mainly HTTP, which
sends data to the child process.  We also have bug 1110596 which complains
about too much memory copying in that code.
Could your IPCStream be used for that?


Yes, I think that could work in general.

I think the main issue would be compat with existing nsIStreamListeners.
These listeners might be written such that they expect the nsIInputStream
passed in OnDataAvailable() to return their entire length from a single
Available() call.  This will not be true for a streamed pipe.


Actually, the nsIStreamListener interface explicitly requires a fixed
length nsIInputStream:

"The onDataAvailable impl must read exactly |aCount| bytes of data before
returning."


But that doesn't mean "a fixed length input stream" - actually I may not 
even follow how you have translated this to you.


(more below)



From:


https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIStreamListener.idl#18

I don't think we can use a pipe-oriented stream here without changing that
interface contract.


Is there a reason we can't just make necko code call OnDataAvailable()
multiple times with a different slice of the giant buffer?  It already has
the mechanism for chunked data.  It just needs to split the single buffer
into multiple callouts.

Ben


We probably can.  Normally it works this way:

- nsInputStreamPipe queries Available() of the input stream (count = 
Available())


- it calls (once) OnDataAvailable(inputStream, count)

- it assumes the implementation has read count from inputStream, as the 
contract suggest


- nsInputStreamPipe loops again, until Available() returns 
NS_BASE_STREAM_CLOSED - which means we have successfully read the stream 
and input stream pipe coverts it to NS_OK - or other error - which is an 
immediate abort/failure.


- when done, OnStopRequest(status)


As I understand, your impl of Available() may return _different_ number 
of bytes than the stream is then able to deliver when Read/ReadSegments 
on it is called, right?  Can you explain why?


That seems - honestly - pretty weird, but whatever.  Does it at least 
change when you actually read?  Like avail() before read - actually read 
= avail() after read? Assuming no data has been put to it in the 
meantime, of course.


The implementation of ODA expects that reading from the stream is not 
going to fail, when reading no more than |count|.  If it fails, ODA 
usually just returns immediately with an error (unrecoverable) and the 
whole stream-listener contract ends with OnStop(that-read-error) - as 
described above.


What does your pipe return when reading past EOF?  I assume 
WOULD_BLOCK?  The thing is that the stream passed to ODA is not expected 
to be non-blocking, but blocking.  WOULD_BLOCK is something that Read() 
in ODA should never return.  And the stream should actually never block, 
regardless whether is blocking or non-blocking.


If we wrap your input stream and convert read errors to something else 
we still may have a problem.  Like returning BASE_STREAM_CLOSED from 
Read() or just bytes-read = 0 + NS_OK, which is a graceful EOF - 
something ODA also doesn't expect to have to handle, faulty impls will 
just indefinitely loop - that's why we have the stream-listener contract 
all for!


Also, not all stream-listener impls are capable to handle when Read() 
fails but later it gets instead of OnStop(failure) another ODA call.


-hb-

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Ben Kelly]

On Fri, May 20, 2016 at 8:10 PM, Ben Kelly  wrote:

> On Fri, May 20, 2016 at 11:09 AM, Ben Kelly  wrote:
>
>> On Fri, May 20, 2016 at 7:37 AM, Honza Bambas 
>> wrote:
>>
>>> And I do! :)  Actually any parent necko channel, mainly HTTP, which
>>> sends data to the child process.  We also have bug 1110596 which complains
>>> about too much memory copying in that code.
>>> Could your IPCStream be used for that?
>>>
>>
>> Yes, I think that could work in general.
>>
>> I think the main issue would be compat with existing nsIStreamListeners.
>> These listeners might be written such that they expect the nsIInputStream
>> passed in OnDataAvailable() to return their entire length from a single
>> Available() call.  This will not be true for a streamed pipe.
>>
>
> Actually, the nsIStreamListener interface explicitly requires a fixed
> length nsIInputStream:
>
> "The onDataAvailable impl must read exactly |aCount| bytes of data before
> returning."
>
> From:
>
>
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIStreamListener.idl#18
>
> I don't think we can use a pipe-oriented stream here without changing that
> interface contract.
>

Is there a reason we can't just make necko code call OnDataAvailable()
multiple times with a different slice of the giant buffer?  It already has
the mechanism for chunked data.  It just needs to split the single buffer
into multiple callouts.

Ben
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Ben Kelly]

On Fri, May 20, 2016 at 11:09 AM, Ben Kelly  wrote:

> On Fri, May 20, 2016 at 7:37 AM, Honza Bambas  wrote:
>
>> And I do! :)  Actually any parent necko channel, mainly HTTP, which sends
>> data to the child process.  We also have bug 1110596 which complains about
>> too much memory copying in that code.
>> Could your IPCStream be used for that?
>>
>
> Yes, I think that could work in general.
>
> I think the main issue would be compat with existing nsIStreamListeners.
> These listeners might be written such that they expect the nsIInputStream
> passed in OnDataAvailable() to return their entire length from a single
> Available() call.  This will not be true for a streamed pipe.
>

Actually, the nsIStreamListener interface explicitly requires a fixed
length nsIInputStream:

"The onDataAvailable impl must read exactly |aCount| bytes of data before
returning."

From:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIStreamListener.idl#18

I don't think we can use a pipe-oriented stream here without changing that
interface contract.

Ben
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Ben Kelly]

On Fri, May 20, 2016 at 7:37 AM, Honza Bambas  wrote:

> And I do! :)  Actually any parent necko channel, mainly HTTP, which sends
> data to the child process.  We also have bug 1110596 which complains about
> too much memory copying in that code.
> Could your IPCStream be used for that?
>

Yes, I think that could work in general.

I think the main issue would be compat with existing nsIStreamListeners.
These listeners might be written such that they expect the nsIInputStream
passed in OnDataAvailable() to return their entire length from a single
Available() call.  This will not be true for a streamed pipe.

I'll try to get some patches ready to test with in the next couple weeks
and we can see.  Worst case we could do the accumulation into a string in
the child process to avoid spiking memory in the parent process.

Ben
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Honza Bambas]

On 5/19/2016 17:04, Ben Kelly wrote:

Hi all,

FYI, I've landed a new IPDL type in bug 1093357 called IPCStream.  This is
intended to make it easier to serialize nsIInputStreams across IPC.

In short, IPCStream:

1) Supports our existing serializable nsInputStreams.
2) Also automatically handles send file descriptors using the
PFileDescriptorSet actor
3) Supports async pipe streams using a new PSendStream actor from
*child-to-parent*.  I have plans to add support for parent-to-child, but I
don't have a consumer yet


And I do! :)  Actually any parent necko channel, mainly HTTP, which 
sends data to the child process.  We also have bug 1110596 which 
complains about too much memory copying in that code.

Could your IPCStream be used for that?

-hb-

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Ting-Yu Chou]

On Fri, May 20, 2016 at 2:19 AM, Ben Kelly  wrote:

> On Thu, May 19, 2016 at 1:41 PM, Andrew McCreight 
> wrote:
>
> > On Thu, May 19, 2016 at 8:04 AM, Ben Kelly  wrote:
> >
> > > 3) Supports async pipe streams using a new PSendStream actor from
> > > *child-to-parent*.  I have plans to add support for parent-to-child,
> but
> > I
> > > don't have a consumer yet and we need to figure out some issues with
> > > PBackground targeting worker threads.
> > >
> >
> > One place that this would be very useful would be for networking. Right
> > now, the various networking protocols send data to the child by calling
> > NS_ReadInputStreamToString(), then copying the string into an IPC
> message,
> > which is bad because it requires a lot of contiguous memory addresses,
> and
> > also has a fair bit of bloat from all of the copies (bug 1110596, bug
> > 1263028).
> >
>
> That would be a good thing to try.  I wonder how many consumers assume
> nsIChannel::OnDataAvailable() provides a fixed length stream, though.
>

The copying we want to avoid in bug 1110596 is from a stream to a nsCString
buffer, which to let a stream write to the IPC Pickle buffer directly.

But I checked SendStreamChildImpl::DoRead() [1], it still reads to a
nsCString buffer at first. The good thing is it sends the stream's data in
chunks of 32k at maximum, which can avoid the bloat.

[1]
https://dxr.mozilla.org/mozilla-central/rev/c4449eab07d39e20ea315603f1b1863eeed7dcfe/ipc/glue/SendStreamChild.cpp#276-282

Ting


> Here is the bug to track parent-to-child pipe streaming:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=1274343
>
> Thanks.
>
> Ben
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Ben Kelly]

On Thu, May 19, 2016 at 1:41 PM, Andrew McCreight 
wrote:

> On Thu, May 19, 2016 at 8:04 AM, Ben Kelly  wrote:
>
> > 3) Supports async pipe streams using a new PSendStream actor from
> > *child-to-parent*.  I have plans to add support for parent-to-child, but
> I
> > don't have a consumer yet and we need to figure out some issues with
> > PBackground targeting worker threads.
> >
>
> One place that this would be very useful would be for networking. Right
> now, the various networking protocols send data to the child by calling
> NS_ReadInputStreamToString(), then copying the string into an IPC message,
> which is bad because it requires a lot of contiguous memory addresses, and
> also has a fair bit of bloat from all of the copies (bug 1110596, bug
> 1263028).
>

That would be a good thing to try.  I wonder how many consumers assume
nsIChannel::OnDataAvailable() provides a fixed length stream, though.

Here is the bug to track parent-to-child pipe streaming:

https://bugzilla.mozilla.org/show_bug.cgi?id=1274343

Thanks.

Ben
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Ben Kelly]

On Thu, May 19, 2016 at 1:41 PM, Andrew McCreight 
wrote:

> On Thu, May 19, 2016 at 8:04 AM, Ben Kelly  wrote:
>
> > 3) Supports async pipe streams using a new PSendStream actor from
> > *child-to-parent*.  I have plans to add support for parent-to-child, but
> I
> > don't have a consumer yet and we need to figure out some issues with
> > PBackground targeting worker threads.
> >
>
> One place that this would be very useful would be for networking. Right
> now, the various networking protocols send data to the child by calling
> NS_ReadInputStreamToString(), then copying the string into an IPC message,
> which is bad because it requires a lot of contiguous memory addresses, and
> also has a fair bit of bloat from all of the copies (bug 1110596, bug
> 1263028).
>
> Andrew
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: IPCStream landed in mozilla-central

[Andrew McCreight]

On Thu, May 19, 2016 at 8:04 AM, Ben Kelly  wrote:

> 3) Supports async pipe streams using a new PSendStream actor from
> *child-to-parent*.  I have plans to add support for parent-to-child, but I
> don't have a consumer yet and we need to figure out some issues with
> PBackground targeting worker threads.
>

One place that this would be very useful would be for networking. Right
now, the various networking protocols send data to the child by calling
NS_ReadInputStreamToString(), then copying the string into an IPC message,
which is bad because it requires a lot of contiguous memory addresses, and
also has a fair bit of bloat from all of the copies (bug 1110596, bug
1263028).

Andrew
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

IPCStream landed in mozilla-central

[Ben Kelly]

Hi all,

FYI, I've landed a new IPDL type in bug 1093357 called IPCStream.  This is
intended to make it easier to serialize nsIInputStreams across IPC.

In short, IPCStream:

1) Supports our existing serializable nsInputStreams.
2) Also automatically handles send file descriptors using the
PFileDescriptorSet actor
3) Supports async pipe streams using a new PSendStream actor from
*child-to-parent*.  I have plans to add support for parent-to-child, but I
don't have a consumer yet and we need to figure out some issues with
PBackground targeting worker threads.

Because the actors require special care there is also a new RAII type
called AutoIPCStream.

A short example of using these types:

  // ipdl
  protocol PMyStuff
  {
  parent:
async DoStuff(IPCStream aStream);
  }

  // in the child
  void
  CallDoStuff(PMyStuffChild* aActor, nsIInputStream* aStream)
  {
AutoIPCstream autoStream;
autoStream.Serialize(aStream, aActor->Manager());
aActor->SendDoStuff(autoStream.TakeValue());
  }

  // in the parent
  bool
  MyStuffParent::RecvDoStuff(const IPCStream& aIPCStream)
  {
nsCOMPtr stream = DeserializeIPCStream(aIPCStream);
// do something with the stream
  }

This example and more documentation can be found in the comments in
IPCStreamUtils.h:

https://dxr.mozilla.org/mozilla-central/source/ipc/glue/IPCStreamUtils.h#31

For the most part you should be able to replace ipdl like:

  InputStreamParams streamParams;
  FileDescriptorSet streamFds;

With:

  IPCStream stream;

Note, however, some code will assume it gets a fixed length stream from IPC
because that is all we have supported in the past.  You should audit the
code to make sure it supports variable length streams.

For example, necko expects to be able to call Available() on the stream in
the parent in order to set content-length.  In order to support the
IPCStream pipe mechanism necko needs to implement chunked uploads or
accumulate the stream before setting the content-length.

Right now the AutoIPCStream::Serialize() method will automatically try
serialization in this order:

1) Fixed length serialization
2) Variable length serialization

We could expand the API to favor one over the other or restrict to only one
kind of serialization.

There is one consumer using IPCStream at the moment; dom/cache.  Its a bit
of a complex example, though, since Cache API needs to send arrays of
streams in a single IPC call.  I tried to include adequate examples in
IPCStreamUtils.h to compensate for this.

Anyway, I hope this helps people dealing with nsInputStreams and ipdl
interfaces.  Please let me know if you run into any problems or have
questions.

Thanks.

Ben
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform