1171994 - Improve RIL I/O code

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

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

The IPC code that transfers messages between RIL worker and I/O thread always sends messages through the main thread. We can probably improve this by transferring messages without the intermediate step.

Comment 1

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

Attachment: [01] Bug 1171994: Add |RilSocket| and |RilSocketConsumer|

Comment 2

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

Attachment: [02] Bug 1171994: Forward received RIL socket I/O via |WorkerCrossThreadDispatcher|

Comment 3

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

Attachment: [03] Bug 1171994: Separate RIL I/O interfaces

Comment 4

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

Attachment: [04] Bug 1171994: Store an instance of |RilWorker| for each RIL connection

Comment 5

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

Attachment: [05] Bug 1171994: Use |RilSocket| to handle RIL messages on the RIL worker

Comment 6

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

Attachment: [06] Bug 1171994: Cleanup JSAPI code of |RilConsumer|

Comment 7

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

It seems to me that the content of Ril.{cpp,h} is more about workers than IPC. Would it make sense to move the code to dom/system/gonk?

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e3d52895640

Comment 9

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thomas, I don't want to let you down but I will be occupied by product line resource allocation meeting this week. That means, I won't be able to start the review work until next week :( If this is urgent, feel free to ask other peer's review.

Comment 10

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

I see. But it's not urgent, so next week would also be OK.

It's actually hard to find other reviewers for this code. Maybe Kyle can help here as well.

Comment 11

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

Outside of saying your indentation is off in Patch 1's RilSocket.cpp (function name length changed but multi-line argument indents didn't :) ), I'd like to stay out of this if possible. I'm not on the FxOS team anymore, so the more eyes we can get on this from FxOS members who may be more directly involved, the better.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8616680 [details] [diff] [review]
[01] Bug 1171994: Add |RilSocket| and |RilSocketConsumer|

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

Looks good to me!

Comment 13

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

[Tracking Requested - why for this release]:

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8616681 [details] [diff] [review]
[02] Bug 1171994: Forward received RIL socket I/O via |WorkerCrossThreadDispatcher|

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

::: ipc/ril/RilSocket.cpp
@@ +206,4 @@
>    }
>  
>  private:
> +  RilSocketIO* mIO;

Can you explain that why we don't need it to be nsRefPtr?

Comment 15

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8616684 [details] [diff] [review]
[03] Bug 1171994: Separate RIL I/O interfaces

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

This part looks good to me.

Comment 16

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8616685 [details] [diff] [review]
[04] Bug 1171994: Store an instance of |RilWorker| for each RIL connection

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

::: ipc/ril/Ril.h
@@ +41,5 @@
> +  void     UnregisterConsumer(unsigned int aClientId);
> +
> +  static nsTArray<nsAutoPtr<RilWorker>> sRilWorkers;
> +
> +  mozilla::dom::workers::WorkerCrossThreadDispatcher* mDispatcher;

Could you explain why this doesn't need to be nsRefPtr?

Comment 17

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

Hi!

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #14)
> Comment on attachment 8616681 [details] [diff] [review]
> [02] Bug 1171994: Forward received RIL socket I/O via
> |WorkerCrossThreadDispatcher|
> 
> Review of attachment 8616681 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/ril/RilSocket.cpp
> @@ +206,4 @@
> >    }
> >  
> >  private:
> > +  RilSocketIO* mIO;
> 
> Can you explain that why we don't need it to be nsRefPtr?

This field is stored in |ReceiveTask|, which is send from the I/O thread to the consumer thread. Releasing the instance of |mIO| requires closing the socket. The closing is written to only happen *after* all other tasks have been processed, so any |ReceiveTask| would have been processed at this point. New tasks aren't created after the socket has been closed.

Comment 18

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

> ::: ipc/ril/Ril.h
> @@ +41,5 @@
> > +  void     UnregisterConsumer(unsigned int aClientId);
> > +
> > +  static nsTArray<nsAutoPtr<RilWorker>> sRilWorkers;
> > +
> > +  mozilla::dom::workers::WorkerCrossThreadDispatcher* mDispatcher;
> 
> Could you explain why this doesn't need to be nsRefPtr?

Using |nsRefPtr| sounds reasonable. Currently |RilConsumer| holds the reference, but that might lead to problems in some corner cases without an instance of this class. I'll fix the patch.

Comment 19

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8616681 [details] [diff] [review]
[02] Bug 1171994: Forward received RIL socket I/O via |WorkerCrossThreadDispatcher|

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

r+ after comment 17, thanks.

Comment 20

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8616687 [details] [diff] [review]
[05] Bug 1171994: Use |RilSocket| to handle RIL messages on the RIL worker

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

::: ipc/ril/Ril.cpp
@@ +353,5 @@
> +  }
> +
> +private:
> +  unsigned int mClientId;
> +  WorkerCrossThreadDispatcher* mDispatcher;

sorry again, why doesn't this need to be nsRefPtr?

Comment 21

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

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #20)
> Comment on attachment 8616687 [details] [diff] [review]
> [05] Bug 1171994: Use |RilSocket| to handle RIL messages on the RIL worker
> 
> Review of attachment 8616687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/ril/Ril.cpp
> @@ +353,5 @@
> > +  }
> > +
> > +private:
> > +  unsigned int mClientId;
> > +  WorkerCrossThreadDispatcher* mDispatcher;
> 
> sorry again, why doesn't this need to be nsRefPtr?

Another case where holding a reference makes sense. Will be fixed in next version.

I tried to minimize references to keep the overhead low. In the current code base the clients are registered while the WCTD is around, so all this works without ref-counting. But now that I look at the patches, I can see the corner cases where it might fail. It's probably safer to use ref-counting everywhere.

Comment 22

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

Attachment: [01] Bug 1171994: Add |RilSocket| and |RilSocketConsumer| (v2)

Changes since v1:

  - fixed indention

Comment 23

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

Attachment: [02] Bug 1171994: Forward received RIL socket I/O via |WorkerCrossThreadDispatcher| (v2)

Changes since v1:

  - hold reference to WCTD in RIL socket classes

Comment 24

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

Attachment: [04] Bug 1171994: Store an instance of |RilWorker| for each RIL connection (v2)

Changes since v1:

  - hold reference to WCTD in RIL worker class

Comment 25

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

Attachment: [05] Bug 1171994: Use |RilSocket| to handle RIL messages on the RIL worker (v2)

Changes since v1:

  - hold reference to WCTD in |RegisterConsumerTask|

Comment 26

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

Attachment: [06] Bug 1171994: Cleanup JSAPI code of |RilConsumer| (v2)

Changes since v1:

  - rebased onto latest [01]-[05]

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8632668 [details] [diff] [review]
[04] Bug 1171994: Store an instance of |RilWorker| for each RIL connection (v2)

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

Looks good to me!

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8632670 [details] [diff] [review]
[05] Bug 1171994: Use |RilSocket| to handle RIL messages on the RIL worker (v2)

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

Thanks for the work, Thomas :)

Comment 29

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

Thanks Hsinyi!

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=46061230e77a

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/463528fcee4d
https://hg.mozilla.org/integration/b2g-inbound/rev/31cdbef9fe22
https://hg.mozilla.org/integration/b2g-inbound/rev/14ec8c87f973
https://hg.mozilla.org/integration/b2g-inbound/rev/2dc847e4650c
https://hg.mozilla.org/integration/b2g-inbound/rev/ea00769b70df

Comment 31

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

https://hg.mozilla.org/integration/b2g-inbound/rev/46061230e77a

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/463528fcee4d
https://hg.mozilla.org/mozilla-central/rev/31cdbef9fe22
https://hg.mozilla.org/mozilla-central/rev/14ec8c87f973
https://hg.mozilla.org/mozilla-central/rev/2dc847e4650c
https://hg.mozilla.org/mozilla-central/rev/ea00769b70df
https://hg.mozilla.org/mozilla-central/rev/46061230e77a

Comment 33

[Peter Bylenga [:PBylenga]]

This caused a sanity blocker "Unable to make calls" for Flame and Aries devices, need this backed out ASAP.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out at the request of QA and nightlies respun.
https://hg.mozilla.org/mozilla-central/rev/16e30af059b7

Comment 35

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

Attachment: [05] Bug 1171994: Use |RilSocket| to handle RIL messages on the RIL worker (v3)

Changes since v2:

  - store outgoing commands in send queue until connection to rild has been established

Comment 36

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=497cf32b408f

Comment 37

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

Hi Hsinyi,

I noticed that the RIL worker starts sending commands to rild before the connection is fully established. So the first few commands got lost and the RIL worker was forever waiting for their responses.

While it was trivial to fix, the RIL worker seems fragile when commands get lost. Is there a plan to only start sending after the connection has been established? Or to integrate the socket code more closely into the RIL worker?

Once there was bug 760649, but it was closed a few weeks ago.

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/abcbb5775c22
https://hg.mozilla.org/integration/b2g-inbound/rev/883f6b94c7d6
https://hg.mozilla.org/integration/b2g-inbound/rev/70c1efea545d
https://hg.mozilla.org/integration/b2g-inbound/rev/1cc8098ed0aa
https://hg.mozilla.org/integration/b2g-inbound/rev/325289d40058
https://hg.mozilla.org/integration/b2g-inbound/rev/919c11b60471

Comment 39

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

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=919c11b60471

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/abcbb5775c22
https://hg.mozilla.org/mozilla-central/rev/883f6b94c7d6
https://hg.mozilla.org/mozilla-central/rev/70c1efea545d
https://hg.mozilla.org/mozilla-central/rev/1cc8098ed0aa
https://hg.mozilla.org/mozilla-central/rev/325289d40058
https://hg.mozilla.org/mozilla-central/rev/919c11b60471

Comment 41

[Hsin-Yi Tsai (she/her) [:hsinyi]]

I'd like to make sure it's also on another module owner, Edgar's radar ;)

Comment 42

[Edgar Chen [:edgar]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #37)
> Hi Hsinyi,
> 
> I noticed that the RIL worker starts sending commands to rild before the
> connection is fully established. So the first few commands got lost and the
> RIL worker was forever waiting for their responses.
> 
> While it was trivial to fix, the RIL worker seems fragile when commands get
> lost. Is there a plan to only start sending after the connection has been
> established? Or to integrate the socket code more closely into the RIL
> worker?

Yes, we could have better handling in ril code. RIL worker now doesn't have too much information about the underlying status. We could expose more information to ril worker and have better handling. I have filed bug 1191256 for this. Thank you.

> 
> Once there was bug 760649, but it was closed a few weeks ago.