Last Comment Bug 699256 - Handle RIL parcels in a ChromeWorker
: Handle RIL parcels in a ChromeWorker
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla11
Assigned To: Philipp von Weitershausen [:philikon]
:
:
Mentors:
Depends on:
Blocks: b2g-telephony
  Show dependency treegraph
 
Reported: 2011-11-02 15:35 PDT by Kyle Machulis [:kmachulis] [:qdot]
Modified: 2011-12-16 13:52 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
ril manager in js (3.26 KB, application/x-javascript)
2011-11-09 17:58 PST, Andreas Gal :gal
no flags Details
Part 1 (v1): RIL parcel processing worker (53.26 KB, patch)
2011-12-02 14:15 PST, Philipp von Weitershausen [:philikon]
gal: review+
Details | Diff | Splinter Review
Part 2 (v1): Experimental navigator.mozTelephony API (14.01 KB, patch)
2011-12-02 14:17 PST, Philipp von Weitershausen [:philikon]
bent.mozilla: review+
Details | Diff | Splinter Review
Part 3 (v1): Hook it all up to the buildsystem (7.17 KB, patch)
2011-12-02 14:18 PST, Philipp von Weitershausen [:philikon]
gal: review+
Details | Diff | Splinter Review
Part 2 (v2): Experimental navigator.mozTelephony API (14.76 KB, patch)
2011-12-02 15:39 PST, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Splinter Review
Part 1 (v2): RIL parcel processing worker (54.84 KB, patch)
2011-12-04 21:48 PST, Philipp von Weitershausen [:philikon]
no flags Details | Diff | Splinter Review

Description Kyle Machulis [:kmachulis] [:qdot] 2011-11-02 15:35:58 PDT
We need to keep state of which RIL Parcels are waiting for return, as well as how to route them. Create a generalized RIL Manager class that deals with parcel transfer and state.
Comment 1 Andreas Gal :gal 2011-11-09 17:58:53 PST
Created attachment 573394 [details]
ril manager in js

This is a sketch of code that runs in a worker and processes input from RIL. There is also code showing how to send, but I think that should be in a separate file since it won't run on the worker. Worker and sender have to synchronize about the outstanding list, so I am less and less convinced that a worker is a good idea here. I think all of this should be on the main thread.
Comment 2 Philipp von Weitershausen [:philikon] 2011-12-02 13:32:08 PST
Morphing this bug to just implement the initial prototype of RIL parcel handling in a ChromeWorker.
Comment 3 Philipp von Weitershausen [:philikon] 2011-12-02 14:15:53 PST
Created attachment 578720 [details] [diff] [review]
Part 1 (v1): RIL parcel processing worker
Comment 4 Philipp von Weitershausen [:philikon] 2011-12-02 14:17:46 PST
Created attachment 578723 [details] [diff] [review]
Part 2 (v1): Experimental navigator.mozTelephony API
Comment 5 Philipp von Weitershausen [:philikon] 2011-12-02 14:18:27 PST
Created attachment 578725 [details] [diff] [review]
Part 3 (v1): Hook it all up to the buildsystem
Comment 6 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-02 14:44:36 PST
Comment on attachment 578723 [details] [diff] [review]
Part 2 (v1): Experimental navigator.mozTelephony API

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

r=me, we need to fix the below things at some point, now or with a followup.

::: dom/telephony/Telephony.js
@@ +88,5 @@
> +    }
> +    if (!this._listeners[type]) {
> +      this._listeners[type] = [];
> +    }
> +    this._listeners[type].push(handler);

This will add duplicates if addEventListener is called multiple times with the same listener, normal behavior is to remove the old one first.

@@ +99,5 @@
> +         (index = list.indexOf(handler) != -1)) {
> +       list.splice(index, 1);
> +       return;
> +     }
> +     throw "XXX TODO some error";

This doesn't match normal behavior. Not error should be thrown.

@@ +109,5 @@
> +    if (!list) {
> +      return;
> +    }
> +    event.target = this;
> +    list.forEach(function (handler) {

This doesn't handle mutations during event firing. Correct behavior is to not call any listeners that were added during firing and to not call any listeners that were removed during firing. Not much way to do this except with stack guards to track mutation from when you start dispatching. Oh, and you have to worry about recursion.
Comment 7 Philipp von Weitershausen [:philikon] 2011-12-02 15:39:35 PST
Created attachment 578755 [details] [diff] [review]
Part 2 (v2): Experimental navigator.mozTelephony API

Address bent's review comments, except for the recursion stuff. This needs to be scrutinized and tested for edge cases before it goes into production code anyway.
Comment 8 Andreas Gal :gal 2011-12-04 05:51:12 PST
Comment on attachment 578720 [details] [diff] [review]
Part 1 (v1): RIL parcel processing worker

>+const DEBUG = true; // set to false to suppress debug messages

DEBUG code should be removed once this is more stable, or make it less verbose and add a pref.

>+  //TODO review these values
>+  INCOMING_BUFFER_LENGTH: 4096,
>+  OUTGOING_BUFFER_LENGTH: 4096,

Make this smaller and make sure you grow the buffer when tail catches head.

>+    // Track where incoming data is read from and written to.
>+    //XXX I think we could fold this into one index just like we do it
>+    // with outgoingIndex.

Yeah, file a follow-up, this looks wonky.

>+    // Maps tokens we send out with requests to the request type, so that
>+    // when we get a response parcel back, we know what request it was for.
>+    this.tokenRequestMap = {};

Object.create(null) is safer

>+  readUint8: function readUint8() {
>+    let value = this.incomingBytes[this.incomingReadIndex];
>+    this.incomingReadIndex = (this.incomingReadIndex + 1) %
>+                             this.INCOMING_BUFFER_LENGTH;

Some sort of & would be nicer and faster I think.

>+  readUint32List: function readUint32List() {
>+    let length = this.readUint32();
>+    let ints = [];
>+    for (let i = 0; i < length; i++) {
>+      ints.push(this.readUint32());
>+    }
>+    return ints;
>+  },

This is a bit allocation happy.

>+    if (!(string_len % 2)) {
>+      delimiter += this.readUint16();
>+    }

& ...

>+  writeUint8: function writeUint8(value) {
>+    this.outgoingBytes[this.outgoingIndex] = value;
>+    this.outgoingIndex += 1;
>+  },

++ !

>+    if (!(value.length % 2)) {

...

>+  writeToIncoming: function writeToIncoming(incoming) {
>+    // We don't have to worry about the head catching the tail since
>+    // we process any backlog in parcels immediately, before writing
>+    // new data to the buffer. So the only edge case we need to handle
>+    // is when the incoming data is larger than the buffer size.
>+    if (incoming.length > this.INCOMING_BUFFER_LENGTH) {
>+      if (DEBUG) {
>+        debug("Current buffer of " + this.INCOMING_BUFFER_LENGTH +
>+              " can't handle incoming " + incoming.length + " bytes ");
>+      }
>+      let oldBytes = this.incomingBytes;
>+      while (this.INCOMING_BUFFER_LENGTH < incoming.length) {
>+        this.INCOMING_BUFFER_LENGTH *= 2;
>+      }

There is probably some math with log you can do here. Make sure an overflow doesn't make this an endless loop (been there, done that).

>+    //TODO XXX this assumes that postRILMessage can eat a ArrayBufferView!
>+    // It also assumes that it will make a copy of the ArrayBufferView right
>+    // away.

Ugly as sin.

Good starting point. Lets land it so we can unblock parallel work.
Comment 9 Philipp von Weitershausen [:philikon] 2011-12-04 06:05:06 PST
(In reply to Andreas Gal :gal from comment #8)
> >+const DEBUG = true; // set to false to suppress debug messages
> 
> DEBUG code should be removed once this is more stable, or make it less
> verbose and add a pref.

Yup. For now it's incredibly useful, though.

> >+  //TODO review these values
> >+  INCOMING_BUFFER_LENGTH: 4096,
> >+  OUTGOING_BUFFER_LENGTH: 4096,
> 
> Make this smaller and make sure you grow the buffer when tail catches head.

Ok, will decrease the values. The tail-catching-head scenario is already taken care of in Buf.writeToIncoming()!

> >+    // Track where incoming data is read from and written to.
> >+    //XXX I think we could fold this into one index just like we do it
> >+    // with outgoingIndex.
> 
> Yeah, file a follow-up, this looks wonky.

Actually, that comment is outdated, I will remove it. We will battle-test this code with RIL packets all week. I definitely have some suspicions that the indexing counting has a few bugs, so we will file follow-ups as we see them.

> >+    // Maps tokens we send out with requests to the request type, so that
> >+    // when we get a response parcel back, we know what request it was for.
> >+    this.tokenRequestMap = {};
> 
> Object.create(null) is safer

Huh? That throws a TypeError for me. Also, I thought we preferred literals over explicit constructors wherever they're possible...

> >+  readUint32List: function readUint32List() {
> >+    let length = this.readUint32();
> >+    let ints = [];
> >+    for (let i = 0; i < length; i++) {
> >+      ints.push(this.readUint32());
> >+    }
> >+    return ints;
> >+  },
> 
> This is a bit allocation happy.

Well, yes. Both read{String|List}List() functions have that problem, but unfortunately I see no way around the one array and N string/number allocations.

> >+    //TODO XXX this assumes that postRILMessage can eat a ArrayBufferView!
> >+    // It also assumes that it will make a copy of the ArrayBufferView right
> >+    // away.
> 
> Ugly as sin.

Yes, though if it's well documented, I see no problem. Any alternative that has us deal with this problem in JS before we hand the data off to postRILMessage would mean more object allocations, and I know how you feel about those ;). A memcpy in C land is cheaper, no?
Comment 10 Philipp von Weitershausen [:philikon] 2011-12-04 21:48:27 PST
Created attachment 578982 [details] [diff] [review]
Part 1 (v2): RIL parcel processing worker

Address gal's review comments (and also fix a hideous off-by-one error.)

Note You need to log in before you can comment on or make changes to this bug.