1221547 - copy less data for file transfers over bluetooth

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Nathan Froyd [:froydnj]]

Some of the copying that takes place can be eliminated by being more explicit
about ownership throughout the IPC socket code.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: make UnixSocketBuffer manage its own memory

UnixSocketBuffer and its subclasses have this unusual setup where the
subclasses are responsible for freeing internal data of
UnixSocketBuffer.  Let's make the ownership model clearer by having
UnixSocketBuffer always own its data.  This change also enables
transferring ownership of appropriately-allocated memory into
UnixSocketBuffer.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: remove GetBuffer from UnixSocketBuffer

This method was only there to make it possible for subclasses to free
the internal buffer of UnixSocketBuffer.  Now that UnixSocketBuffer is
managing the lifetime of the buffer itself, this method is no longer
necessary.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: enable UnixSocketRawData to take ownership of a passed-in-buffer

Now that we're using UniquePtr internally, it's a simple matter to
construct UnixSocketRawData such that we know we're assuming ownership
of a buffer.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: copy less data for file transfers over bluetooth

We have a number of places where we create a Bluetooth request with
allocated memory, then copy that memory into the raw data to be sent
over the socket.  We can do better by transferring ownership to the
socket data, rather than copying.

Comment 5

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

UnixSocket was rewritten by tzimmermann, so I'm deferring these reviews to him.

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Nathan,

I like the idea of moving buffers into the socket-buffer classes, but I have some general comments on the design.

|UnixSocketBuffer| provides an interface for reading and writing in arbitrary I/O buffers. The actual buffer is managed by the inherited class and might not event be allocated dynamically. |UnixSocketBuffer| only contains a pointer to this buffer to avoid calls to a virtual getter method in the |Read| and |Write| methods.

OTOH the current implementation in inconsistent as |UnixSocketBuffer::Append| reallocates buffer memory. It's the result of a long series of refactoring and cleanups of the I/O code. I guess I never got to the point of fully implementing the original idea.

Here's a suggestion for an updated patch set: The buffer ownership should remain in |UnixSocketRawBuffer| and |DaemonSocketPDU|. |UnixSocketBuffer| should get a virtual method for growing the buffer, which is called by |UnixSocketBuffer::Append| to make inherited classes increase the buffer size.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #6)
> Here's a suggestion for an updated patch set: The buffer ownership should
> remain in |UnixSocketRawBuffer| and |DaemonSocketPDU|. |UnixSocketBuffer|
> should get a virtual method for growing the buffer, which is called by
> |UnixSocketBuffer::Append| to make inherited classes increase the buffer
> size.

I don't understand the circumstances in which a subclass of UnixSocketBuffer would have more specialized knowledge about how to grow the buffer than UnixSocketBuffer itself.  Looking around, it doesn't seem like there are many opportunities to be able to apply this sort of specialized knowledge; most buffers appear to be dynamically allocated already or stored on the stack, which necessitate copying.  I guess maybe you're envisioning something like:

http://mxr.mozilla.org/mozilla-central/source/ipc/ril/Ril.cpp#186

(perhaps just the typed array case) where the memory is being kept alive by a JS object, and to grow the buffer, we should be reallocating the JS object?  That particular instance seems hard to do, since you have to manipulate the object on the object's thread for growing...

It seems like there are really three interesting cases:

1. The data for the buffer has to be copied.  Appending that would result in having to reallocate the buffer is not a big deal.  This is how we treat everything right now.

2. The data for the buffer has already been dynamically allocated, and we would like to assume ownership.  Appending that would result in having to reallocate the buffer is not a big deal.  This is what the patch set proposed here does.

3. The data for the buffer is owned by somebody else and will stay alive until it is consumed.  I think this is the case you would like to handle efficiently without copying.  I think that's reasonable, and you can do it with a type like Variant<const char*, UniquePtr<char[]>> for the storage of the data in UnixSocketBuffer.

The question then becomes what to do when Append() overflows the provided buffer, and I think this is where we would diverge in our implementation.  My sense is that we should simply move from the owned-by-somebody-else state to the dynamically-allocated-and-owned-by-us state, on the assumption that it wouldn't happen very often.  If I have understood you correctly, you would rather delegate this task of Append()'ing to the original owner, to see if they can do it more efficiently.

Is that a fair summary of the problem?  It's entirely possible I don't understand all the use cases in play here, so please do point out places where my assumptions are broken.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

> 
> http://mxr.mozilla.org/mozilla-central/source/ipc/ril/Ril.cpp#186
> 
> (perhaps just the typed array case) where the memory is being kept alive by
> a JS object, and to grow the buffer, we should be reallocating the JS
> object?  That particular instance seems hard to do, since you have to
> manipulate the object on the object's thread for growing...

True, and there's also |RilConsumer::Receive|, which copies data from a socket buffer into a JS array. Those functions run on a separate JS worker thread that processes inbound and outbound RIL messages. The I/O is then done on yet another thread.

I had some ideas for exposing the Unix sockets to JS and running I/O and JS worker on the same thread. This would allow to read and write directly from/to JS arrays. Most (all?) of the memcpy'ing could be removed. But it's complicated to get right and I didn't get to implement it yet. (bug 1191256, bug 760649)


> It seems like there are really three interesting cases:
> 
> 1. The data for the buffer has to be copied.  Appending that would result in
> having to reallocate the buffer is not a big deal.  This is how we treat
> everything right now.
> 
> 2. The data for the buffer has already been dynamically allocated, and we
> would like to assume ownership.  Appending that would result in having to
> reallocate the buffer is not a big deal.  This is what the patch set
> proposed here does.
> 
> 3. The data for the buffer is owned by somebody else and will stay alive
> until it is consumed.  I think this is the case you would like to handle
> efficiently without copying.  I think that's reasonable, and you can do it
> with a type like Variant<const char*, UniquePtr<char[]>> for the storage of
> the data in UnixSocketBuffer.
>
> The question then becomes what to do when Append() overflows the provided
> buffer, and I think this is where we would diverge in our implementation. 
> My sense is that we should simply move from the owned-by-somebody-else state
> to the dynamically-allocated-and-owned-by-us state, on the assumption that
> it wouldn't happen very often.  If I have understood you correctly, you
> would rather delegate this task of Append()'ing to the original owner, to
> see if they can do it more efficiently.

I'd like to delegate the buffer (re-)allocation to the original owner. |Append| returns a pointer to the end of the current buffer and ensures that there are at least 'len' bytes available for writing new data. If the buffer is to small, it reallocates the memory. The reallocation should probably be handled by the buffer's owner.  If the reallocation fails (OOM, static buffers), nullptr would be returned and reading/writing would fail.


> Is that a fair summary of the problem?  It's entirely possible I don't
> understand all the use cases in play here, so please do point out places
> where my assumptions are broken.

You're assumptions are correct, I think. Sorry that I don't have an actual use case ATM, or any numbers where the current implementation is more efficient. The separation of buffer management and buffer access still is nice property that I wouldn't like to give up easily.

The buffer ownership could be more clear in the code; or maybe |UnixSocketBuffer| shouldn't even be a base class, but a wrapper around buffer classes.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> I had some ideas for exposing the Unix sockets to JS and running I/O and JS
> worker on the same thread. This would allow to read and write directly
> from/to JS arrays. Most (all?) of the memcpy'ing could be removed. But it's
> complicated to get right and I didn't get to implement it yet. (bug 1191256,
> bug 760649)

"complicated". :)

> > The question then becomes what to do when Append() overflows the provided
> > buffer, and I think this is where we would diverge in our implementation. 
> > My sense is that we should simply move from the owned-by-somebody-else state
> > to the dynamically-allocated-and-owned-by-us state, on the assumption that
> > it wouldn't happen very often.  If I have understood you correctly, you
> > would rather delegate this task of Append()'ing to the original owner, to
> > see if they can do it more efficiently.
> 
> I'd like to delegate the buffer (re-)allocation to the original owner.
> |Append| returns a pointer to the end of the current buffer and ensures that
> there are at least 'len' bytes available for writing new data. If the buffer
> is to small, it reallocates the memory. The reallocation should probably be
> handled by the buffer's owner.  If the reallocation fails (OOM, static
> buffers), nullptr would be returned and reading/writing would fail.
> 
> > Is that a fair summary of the problem?  It's entirely possible I don't
> > understand all the use cases in play here, so please do point out places
> > where my assumptions are broken.
> 
> You're assumptions are correct, I think. Sorry that I don't have an actual
> use case ATM, or any numbers where the current implementation is more
> efficient. The separation of buffer management and buffer access still is
> nice property that I wouldn't like to give up easily.

OK, how about this as a compromise: drop parts 1 and 2, so the current ownership scheme whereby UnixSocketBuffer subclasses manage their own buffers stays, but we just modify part 3 to have UnixSocketBuffer assume ownership of the raw pointer stored in the UniquePtr?  Append() stays the same as it is today.  That way we can at least avoid the copying.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Nathan Froyd [:froydnj] from comment #9)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> > I had some ideas for exposing the Unix sockets to JS and running I/O and JS
> > worker on the same thread. This would allow to read and write directly
> > from/to JS arrays. Most (all?) of the memcpy'ing could be removed. But it's
> > complicated to get right and I didn't get to implement it yet. (bug 1191256,
> > bug 760649)
> 
> "complicated". :)

Is there a joke here that I don't get?

The memcpy is of course not complicated, but the I/O code runs by default on a thread without a JS context. In most of the cases, that's the right thing to do and let's us share a single I/O thread for a number of modules (BT, NFC, RIL). Changing the internal socket interfaces to *optionally* support JS and workers and run the I/O code within that environment is not trivial. All the existing non-JS use cases need to be continued to be supported. BT doesn't use JS internally and NFC only to some extend; they shouldn't start to depend on JS workers. Maybe "complex" would have been a better term.


> OK, how about this as a compromise: drop parts 1 and 2, so the current
> ownership scheme whereby UnixSocketBuffer subclasses manage their own
> buffers stays, but we just modify part 3 to have UnixSocketBuffer assume
> ownership of the raw pointer stored in the UniquePtr?  Append() stays the
> same as it is today.  That way we can at least avoid the copying.

This sounds worse than the original proposal.

What's wrong with keeping the ownership in the sub-classes, |Move|'ing external buffers into the sub-classes, and only updating the buffer pointer in |UnixSocketBuffer|? I don't think I understood your motivation for this change. Clarifying buffer ownership?

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: enable UnixSocketRawData to take ownership of a passed-in-buffer

This implements the scheme described in comment 9.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8684904 [details] [diff] [review]
enable UnixSocketRawData to take ownership of a passed-in-buffer

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

Thank you!

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8683087 [details] [diff] [review]
copy less data for file transfers over bluetooth

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

LGTM. We need a follow-up bug to fix MAP in bug 1211769 as well.

Comment 14

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #13)
> LGTM. We need a follow-up bug to fix MAP in bug 1211769 as well.

Opened follow-up bug 1223650.

Comment 15

[Nathan Froyd [:froydnj]]

Attachment: part 1 - enable UnixSocketRawData to take ownership of a passed-in-buffer

Missed an #include, carrying r+.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/309cf6675d4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/756632e20aa4

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/309cf6675d4e
https://hg.mozilla.org/mozilla-central/rev/756632e20aa4