1379680 - Zero copy writing of data

Status: RESOLVED FIXED

(Core :: IPC, enhancement, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

It would be nice if there was a way to pass ownership of data to the io thread to avoid having to make it a copy of it (i.e. WriteBytes())

Comment 1

[Jeff Muizelaar [:jrmuizel]]

It looks like BufferList needs some kind of concatenate method for this to work.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add AppendBuffer

This adds a AppendBuffer method that move ownership of a buffer into a BufferList. However, it's not useful because all of the ParamTraits Write methods take a const reference. I'd be interested in thoughts on how to fix this.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: WIP

I haven't tested this out yet, but in theory it seems like it puts all the pieces in place to work.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add a way to append buffers to a BufferList

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add an ipc ByteBuf type

Comment 6

[Jeff Muizelaar [:jrmuizel]]

I wrote the ipdl compiler patch by mostly copying & pasting so it could be full of errors.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Jeff, if I understand correctly we need to hand write serializers to take advantage of this?  Is that correct?  Just curious if we can use this in our stream serializations, etc.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: use ByteBuf to send display lists

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Ben Kelly [:bkelly] from comment #7)
> Jeff, if I understand correctly we need to hand write serializers to take
> advantage of this?  Is that correct?  Just curious if we can use this in our
> stream serializations, etc.

I don't quite understand your question. This is intended to help to be used when you already have a buffer bytes and just want to get it to the other side. What's the problem you want solved?

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Looking at patch 8, maybe we can just replace uses of ByteBuffer here:

https://searchfox.org/mozilla-central/source/ipc/glue/PParentToChildStream.ipdl#23

In the past I think we even used nsCString for this sort of thing.  Sorry for my confusion.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Ben Kelly [:bkelly] from comment #10)
> In the past I think we even used nsCString for this sort of thing.  Sorry
> for my confusion.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1351488#c54 onwards for context around why PParentToChildStream was changed from nsCString to wr:ByteBuffer.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add a way to append buffers to a BufferList

This fixes a missing return value

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add an ipc ByteBuf type

Add a missing include for WriteParam

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Attachment: Move the displaylist ByteBuf into a Vec instead of copying

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8919865 [details] [diff] [review]
Add a way to append buffers to a BufferList

Review of attachment 8919865 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/base/pickle.cc
@@ +626,5 @@
>  #endif
>    return WriteBytes(&value, sizeof(value));
>  }
>  
> +bool Pickle::AppendBuffer(const void* data, uint32_t data_len, uint32_t capacity) {

I'd rather call this WriteBytesZeroCopy as well (see below).

Is it necessary to pass |data| as const? It clearly isn't. Even if some caller requires the conversion, can't we do it there?

::: mfbt/BufferList.h
@@ +341,5 @@
>      MOZ_ASSERT(start.IsIn(*this) && end.IsIn(*this));
>      return start.BytesUntil(*this, end);
>    }
>  
> +  void* AppendSegment(char *aData, size_t aSize, size_t aCapacity)

Can we call this WriteBytesZeroCopy? I think it's a clearer name. Also, please comment that this method takes ownership of aData.

@@ +343,5 @@
>    }
>  
> +  void* AppendSegment(char *aData, size_t aSize, size_t aCapacity)
> +  {
> +    MOZ_ASSERT(aCapacity != 0);

I think we should probably assert mOwning here. Otherwise, it's unclear whether AppendSegment actually takes ownership of aData.

If you decide to leave it this way, then the free_ call below should be conditional on mOwning.

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8919866 [details] [diff] [review]
Add an ipc ByteBuf type

Review of attachment 8919866 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/ByteBuf.h
@@ +5,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* A type that can be sent without needing to make a copy during
> + * serialization. In addition the receiver can take ownership of the
> + * data to avoid having to make a copy. */

It seems like we do copy on the receiving side.

@@ +33,5 @@
> +    mCapacity = aLength;
> +    return true;
> +  }
> +
> +  ByteBuf() :

Can you do these constructors in this style?

Ctor()
: foo()
, bar()

@@ +64,5 @@
> +  }
> +
> +  uint8_t* mData;
> +  size_t mLen;
> +  size_t mCapacity;

Do you actually use capacity? I don't really see any purpose for it unless clients allocate more data than necessary so that IPDL could use it for later parts of a message. Is that likely?

@@ +79,5 @@
> +struct ParamTraits<mozilla::ipc::ByteBuf>
> +{
> +  typedef mozilla::ipc::ByteBuf paramType;
> +
> +  // this is where the magic happens

This comment could be more specific. Maybe "This is where we transfer the memory from the ByteBuf to IPDL, avoiding a copy."?

@@ +95,5 @@
> +  {
> +    size_t length;
> +    return ReadParam(aMsg, aIter, &length)
> +      && aResult->Allocate(length)
> +      && aMsg->ReadBytesInto(aIter, aResult->mData, length);

Please make it clear that you're copying here. If you're willing to deal with non-contiguous data, you could simply use a BufferList instead of ByteBuf and then use something like Pickle::ExtractBuffers.

Alternatively, we used to have a BufferList method (FlattenBytes, removed in bug 1297981) that would make the bytes in a certain range contiguous, possibly requiring reallocation and copying. Then you could borrow bytes out of the BufferList. This works if you just want to pass them into RecvXYZ methods. It doesn't work for other cases, like return values from sync messages. If this would be useful to you, we could add the FlattenBytes method back.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(In reply to Bill McCloskey (:billm) from comment #16)
> Please make it clear that you're copying here. If you're willing to deal
> with non-contiguous data, you could simply use a BufferList instead of
> ByteBuf and then use something like Pickle::ExtractBuffers.

I think I'm actually going to look at doing this patch over and see if I can make it so you can just send and receive a BufferList and let the producer/consumer worry about getting the bytes in and out as they please.

> Do you actually use capacity? I don't really see any purpose for it
> unless clients allocate more data than necessary so that IPDL could use it
> for later parts of a message. Is that likely?

Yes, the buffer that we build our display list into comes from a Rust Vec so it's very likely that it will have extra space which the parameters that come after the ByteBuf can be serialized into.

Comment 18

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #17)
> (In reply to Bill McCloskey (:billm) from comment #16)
> > Please make it clear that you're copying here. If you're willing to deal
> > with non-contiguous data, you could simply use a BufferList instead of
> > ByteBuf and then use something like Pickle::ExtractBuffers.
> 
> I think I'm actually going to look at doing this patch over and see if I can
> make it so you can just send and receive a BufferList and let the
> producer/consumer worry about getting the bytes in and out as they please.

OK, that sounds good to me, as long as you can make the Rust code deal with that.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #17)
> (In reply to Bill McCloskey (:billm) from comment #16)
> > Please make it clear that you're copying here. If you're willing to deal
> > with non-contiguous data, you could simply use a BufferList instead of
> > ByteBuf and then use something like Pickle::ExtractBuffers.
> 
> I think I'm actually going to look at doing this patch over and see if I can
> make it so you can just send and receive a BufferList and let the
> producer/consumer worry about getting the bytes in and out as they please.

I changed my mind on this. Right now we still need a contiguous buffer of bytes on the rust receiving side and handing out an extracted BufferList and then building the contiguous buffer from that will be less efficient, so I'll just go forward with this for now.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66ba728d4fe9
Add a way to append buffers to a BufferList. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbfc68a9fa54
Add an ipc ByteBuf type. r=billm

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/66ba728d4fe9
https://hg.mozilla.org/mozilla-central/rev/cbfc68a9fa54

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75dba1901e67
Revert changes that were accidentally landed as part the wrong bug. r=me

Comment 23

[Kartikaya Gupta (email:[e-mail])]

^ This stuff should have been on bug 1406507, so I backed it out here and relanded it in that bug to get more useful blame/history.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> ^ This stuff should have been on bug 1406507, so I backed it out here and
> relanded it in that bug to get more useful blame/history.

Thanks.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a1478b58f0
Move the displaylist ByteBuf into a Vec instead of copying. r=kats

Comment 26

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/75dba1901e67

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a31601120ad
Revert "Bug 1379680. Move the displaylist ByteBuf into a Vec instead of copying. r=kats"

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d4a1478b58f0

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4a31601120ad