811683 - [b2g-bluetooth] Use given 'max packet size' instead of 255

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Eric Chou [:ericchou] [:echou]]

Most of the times, before a file transferring process starts, two devices would exchange information via CONNECT request and response. According to OBEX spec, we can get "maximum OBEX packet length" from the packet, and this value should be used to optimize the speed of sending file. In current codebase, we use the minimum packet size 255 bytes for one packet, apparently we need some improvement.

Comment 1

[Doug Turner (:dougt)]

Hey Eric,

It's hard to know if we would block shipping v.1 for this bug.  For that reason, we are minusing.  If you disagree, please renom and included rationale.  If you have patches that you'd like to uplift, you can request that on the patch itself.

Comment 2

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v1: used packet size as large as possible

Rationale for blocking+: From the result of my test, for current codebase, it took about 3 min and 45 seconds to send a 3.91MB music file to my HTC phone. On the other hand, when this patch was applied, it took only 38 seconds. In my opinion, this is a necessary optimization and the patch does not change the logic too much, which means it's a quite low-risk change.

* Kyle, please help to review UnixSocket.h. I increased the value of  MAX_DATA_SIZE because the original value (1KB) was not enough for OPP to send a MB-level file.

* Gina, please help to review BluetoothOppManager.cpp

Thanks in advance.

Comment 3

[Eric Chou [:ericchou] [:echou]]

Rationale for blocking+: From the result of my test, for current codebase, it took about 3 min and 45 seconds to send a 3.91MB music file to my HTC phone. On the other hand, when this patch was applied, it took only 38 seconds. In my opinion, this is a necessary optimization and the patch does not change the logic too much, which means it's a quite low-risk change.

Comment 4

[Lawrence Mandel [:lmandel] (use needinfo)]

During triage we agreed that we want to have performance fixes like this. However, slow transfer via bluetooth will not block the release. If this is indeed safe fix, please request Aurora uplift.

Comment 5

[Dave Hylands [:dhylands]]

Comment on attachment 681847 [details] [diff] [review]
patch 1: v1: used packet size as large as possible

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

::: ipc/unixsocket/UnixSocket.h
@@ +18,4 @@
>  
>  struct UnixSocketRawData
>  {
> +  static const size_t MAX_DATA_SIZE = 65535;

Wouldn't it be better to have this passed as an argument in a constructor. Otherwise this change will affect all users.

Comment 6

[Eric Chou [:ericchou] [:echou]]

(In reply to Dave Hylands [:dhylands] from comment #5)
> Comment on attachment 681847 [details] [diff] [review]
> patch 1: v1: used packet size as large as possible
> 
> Review of attachment 681847 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/unixsocket/UnixSocket.h
> @@ +18,4 @@
> >  
> >  struct UnixSocketRawData
> >  {
> > +  static const size_t MAX_DATA_SIZE = 65535;
> 
> Wouldn't it be better to have this passed as an argument in a constructor.
> Otherwise this change will affect all users.

It would be a little strange if we have to decide MAX_DATA_SIZE every time we need an UnixSocketRawData object. My suggestion would be using nsAutoPtr<uint8_t> to keep mData, however this part was implemented by Kyle, he must have better idea than me about how to implement. :)

Comment 7

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

Comment on attachment 681847 [details] [diff] [review]
patch 1: v1: used packet size as large as possible

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

I'm in agreement with dhylands on this one. MAX_DATA_SIZE is a holdover from the stuff I brought in when we did the RIL, which we knew would be small packets. For someone more variable that could generate TONS of packets, it'd be nice to have a variable max size, especially if we use this on the phone where we don't want to blow up the process with 64k blocks for 16 byte messages.

It /might/ be worth combining the UnixSocketRawData structure creation into the UnixSocket, so you could define the data size with the socket versus per packet? Lemme know your thoughts. If you agree with this, bug it to me unless you want to implement it. :)

Comment 8

[Eric Chou [:ericchou] [:echou]]

Hey Kyle,

Sorry for the late reply.

> 
> I'm in agreement with dhylands on this one. MAX_DATA_SIZE is a holdover from
> the stuff I brought in when we did the RIL, which we knew would be small
> packets. For someone more variable that could generate TONS of packets, it'd
> be nice to have a variable max size, especially if we use this on the phone
> where we don't want to blow up the process with 64k blocks for 16 byte
> messages.
> 

Yes, I know the point. I don't like the idea of creating 64k buffer for a small packet, either. However, do we really need to pass a value of 'MAX_DATA_SIZE' into ctor every time while creating an UnixSocketRawData object? MAX_DATA_SIZE sounds like to be a constant value indicating the limit of low-level implementation. :|

> It /might/ be worth combining the UnixSocketRawData structure creation into
> the UnixSocket, so you could define the data size with the socket versus per
> packet? Lemme know your thoughts. If you agree with this, bug it to me
> unless you want to implement it. :)

Sounds good. It would be nice if you could help on the implementation since you know more than me about how it should go. :)

I'm thinking that we can just use this bug since it's related, so re-assign it to you, and please assign it back to me once your part is done. Feel free to change it if it's not appropriate.

Eric

Comment 9

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #8)
> Sorry for the late reply.

Hah, I spent some time trying to find this bug today, so thanks for making it come up in my bug mail. :)

> > 
> > I'm in agreement with dhylands on this one. MAX_DATA_SIZE is a holdover from
> > the stuff I brought in when we did the RIL, which we knew would be small
> > packets. For someone more variable that could generate TONS of packets, it'd
> > be nice to have a variable max size, especially if we use this on the phone
> > where we don't want to blow up the process with 64k blocks for 16 byte
> > messages.
> > 
> 
> Yes, I know the point. I don't like the idea of creating 64k buffer for a
> small packet, either. However, do we really need to pass a value of
> 'MAX_DATA_SIZE' into ctor every time while creating an UnixSocketRawData
> object? MAX_DATA_SIZE sounds like to be a constant value indicating the
> limit of low-level implementation. :|


Yeah, honestly the "single size for everything" idea is a bit obtuse, and I haven't really seen anyone else do it (nor can I remember why I kept thinking we should, but I haven't looked at the code in a couple of weeks). We should be able to just read and write as much as you want per RawData object, just means the RawData object gets a bit more complex, but nothing we can't deal with.
 
> Sounds good. It would be nice if you could help on the implementation since
> you know more than me about how it should go. :)

Cool, was why I was trying to find this bug in the first place. I'll go ahead and handle this. :)

Comment 10

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

Attachment: Patch 2 (V1) - Make sockets take variable sizes, fix opp types

Tagging cjones on this for UnixSocket review, echou for OPP review.

Comment 11

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

Attachment: Patch 2 (V2) - Make sockets take variable sizes, fix opp types

Comment 12

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

New patch version because I forgot to set UnixSocketRawData's data to delete[] properly. :(

Comment 13

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

Attachment: Patch 2 (V3) - Make sockets take variable sizes, fix opp types

In which I /actually/ change that type I meant to last time but forgot my -a and just did git commit --amend. Go me.

Comment 14

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 689579 [details] [diff] [review]
Patch 2 (V3) - Make sockets take variable sizes, fix opp types

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

The part of OPP looks good to me. Thanks for fixing it.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 689579 [details] [diff] [review]
Patch 2 (V3) - Make sockets take variable sizes, fix opp types

>diff --git a/ipc/unixsocket/UnixSocket.h b/ipc/unixsocket/UnixSocket.h

>+  static const size_t MAX_DATA_SIZE = 0xFFFF;

ITYM 1 << 16?

>+  nsAutoArrayPtr<uint8_t> mData;

Use nsAutoTArray<int8_t, DEFAULT> with a well-chosen default (1024?)
if possible.  This makes the common case faster the and the uncommon
case safer.

I don't understand this patch too well but lmk if this makes sense.

Comment 16

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

Attachment: Patch 2 (V4) - Make sockets take variable sizes, fix opp types

I removed the default no-arguments constructor so that size input is required. Main reason for doing this is so we can support bigger packet sizes for bluetooth.

And changed hex to bit shift. Habit. :)

Comment 17

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

Kicking this back to echou since patch 1 needs to be updated (remove unixsocket change) and r+'d by gyeh.

Comment 18

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v2: use max packet size given by remote device for file sharing

* Increased the speed of file sharing, not used 255 (the minimum packet size) anymore.

Comment 19

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 690192 [details] [diff] [review]
patch 1: v2: use max packet size given by remote device for file sharing

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

Looks good, r=me.

Comment 20

[Eric Chou [:ericchou] [:echou]]

try:

https://tbpl.mozilla.org/?tree=Try&rev=647142bfb91a
https://tbpl.mozilla.org/?tree=Try&rev=7d09ac918dd0

Comment 21

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/216d556025d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a7b73eb421

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/216d556025d1
https://hg.mozilla.org/mozilla-central/rev/14a7b73eb421

Comment 23

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 689808 [details] [diff] [review]
Patch 2 (V4) - Make sockets take variable sizes, fix opp types

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no
User impact if declined: very slow file-sending process
Testing completed (on m-c, etc.): on m-c and by myself
Risk to taking this patch (and alternatives if risky): No. I've tested file transferring for several days and can confirm that no regression is caused by both patches.
String or UUID changes made by this patch: no

Please see comment 3 for more details about the impact if declined.

Comment 24

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 690192 [details] [diff] [review]
patch 1: v2: use max packet size given by remote device for file sharing

[Approval Request Comment]
Bug caused by (feature/regressing bug #): no
User impact if declined: very slow file-sending process
Testing completed (on m-c, etc.): on m-c and by myself
Risk to taking this patch (and alternatives if risky): No. I've tested file transferring for several days and can confirm that no regression is caused by both patches.
String or UUID changes made by this patch: no

Please see comment 3 for more details about the impact if declined.

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 689808 [details] [diff] [review]
Patch 2 (V4) - Make sockets take variable sizes, fix opp types

low risk change for a perf win, based on bug comment#3 #4.Approving on aurora/b2g18 .

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fb5bfb0c3d1d
https://hg.mozilla.org/releases/mozilla-aurora/rev/c044401043f5

https://hg.mozilla.org/releases/mozilla-b2g18/rev/e3664edb30cf
https://hg.mozilla.org/releases/mozilla-b2g18/rev/eacd80a0e9a7