Zero copy writing of data

RESOLVED FIXED in Firefox 59

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jrmuizel, Assigned: jrmuizel)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Assignee

Description

2 years ago
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())
Assignee

Comment 1

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

Updated

2 years ago
Priority: -- → P3
Assignee

Comment 2

2 years ago
Posted patch Add AppendBuffer (obsolete) — Splinter Review
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.
Assignee: nobody → jmuizelaar
Assignee

Comment 3

2 years ago
Posted patch WIP (obsolete) — Splinter Review
I haven't tested this out yet, but in theory it seems like it puts all the pieces in place to work.
Assignee

Comment 4

2 years ago
Attachment #8916447 - Attachment is obsolete: true
Attachment #8919375 - Flags: review?(wmccloskey)
Assignee

Comment 5

2 years ago
Posted patch Add an ipc ByteBuf type (obsolete) — Splinter Review
Attachment #8919058 - Attachment is obsolete: true
Attachment #8919377 - Flags: review?(wmccloskey)
Assignee

Comment 6

2 years ago
I wrote the ipdl compiler patch by mostly copying & pasting so it could be full of errors.
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.
Assignee

Comment 9

2 years ago
(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?
Assignee

Updated

2 years ago
Flags: needinfo?(bkelly)
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.
Flags: needinfo?(bkelly)
(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.
Assignee

Comment 12

2 years ago
This fixes a missing return value
Attachment #8919375 - Attachment is obsolete: true
Attachment #8919375 - Flags: review?(wmccloskey)
Attachment #8919865 - Flags: review?(wmccloskey)
Assignee

Comment 13

2 years ago
Add a missing include for WriteParam
Attachment #8919377 - Attachment is obsolete: true
Attachment #8919377 - Flags: review?(wmccloskey)
Attachment #8919866 - Flags: review?(wmccloskey)
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.
Attachment #8919865 - Flags: review?(wmccloskey) → review+
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.
Attachment #8919866 - Flags: review?(wmccloskey) → review+
Assignee

Comment 17

2 years ago
(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.
(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.
Assignee

Comment 19

2 years ago
(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.
Assignee

Updated

2 years ago
Blocks: 1406507

Comment 20

2 years ago
Pushed by jmuizelaar@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/66ba728d4fe9
https://hg.mozilla.org/mozilla-central/rev/cbfc68a9fa54
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 22

2 years ago
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75dba1901e67
Revert changes that were accidentally landed as part the wrong bug. r=me
^ 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.
Assignee

Comment 24

2 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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

2 years ago
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4a1478b58f0
Move the displaylist ByteBuf into a Vec instead of copying. r=kats

Comment 27

2 years ago
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a31601120ad
Revert "Bug 1379680. Move the displaylist ByteBuf into a Vec instead of copying. r=kats"
You need to log in before you can comment on or make changes to this bug.