Handle RIL parcels in a ChromeWorker

RESOLVED FIXED in mozilla11

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: qdot, Assigned: philikon)

Tracking

unspecified
mozilla11
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 3 obsolete attachments)

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.
Blocks: 699235

Comment 1

6 years ago
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.
Morphing this bug to just implement the initial prototype of RIL parcel handling in a ChromeWorker.
Assignee: nobody → philipp
Summary: Create JS RIL Packet State Manager class for B2G RIL data → Handle RIL parcels in a ChromeWorker
Created attachment 578720 [details] [diff] [review]
Part 1 (v1): RIL parcel processing worker
Attachment #573394 - Attachment is obsolete: true
Attachment #578720 - Flags: review?(jones.chris.g)
Created attachment 578723 [details] [diff] [review]
Part 2 (v1): Experimental navigator.mozTelephony API
Attachment #578723 - Flags: review?(bent.mozilla)
Created attachment 578725 [details] [diff] [review]
Part 3 (v1): Hook it all up to the buildsystem
Attachment #578725 - Flags: review?(jones.chris.g)
(Assignee)

Updated

6 years ago
Attachment #578720 - Flags: review?(jones.chris.g) → review?(gal)
(Assignee)

Updated

6 years ago
Attachment #578725 - Flags: review?(jones.chris.g) → review?(gal)
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.
Attachment #578723 - Flags: review?(bent.mozilla) → review+
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.
Attachment #578723 - Attachment is obsolete: true

Comment 8

6 years ago
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.
Attachment #578720 - Flags: review?(gal) → review+

Updated

6 years ago
Attachment #578725 - Flags: review?(gal) → review+
(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?
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.)
Attachment #578720 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/a95b61c88f65
https://hg.mozilla.org/integration/mozilla-inbound/rev/4452fc1a3cad
https://hg.mozilla.org/integration/mozilla-inbound/rev/431962a6ac34
https://hg.mozilla.org/mozilla-central/rev/a95b61c88f65
https://hg.mozilla.org/mozilla-central/rev/4452fc1a3cad
https://hg.mozilla.org/mozilla-central/rev/431962a6ac34
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
(Assignee)

Updated

6 years ago
Component: DOM → DOM: Device Interfaces
QA Contact: general → device-interfaces
You need to log in before you can comment on or make changes to this bug.