960894 - Utilize one single RIL worker in DSDS

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

+++ This bug was initially created as a clone of Bug #864927 +++

From bug 864927 comment 9, we'd like to have a research about the possibility to merge multiple ril_workers in DSDS for saving more memory.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

One possible way is to wrap most lines in ril_worker into a huge function.  This way minimizes LOC modified while preserves validness of most xpcshell tests with a few changes.  Worker instantiation in RIL.js and onRILMessage in ril_worker.js have to be modified as well.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer : WIP

This patch basically adds an additional data member in |class DispatchRILEvent|.

|class DispatchRILEvent| will pass the received data to the corresponding worker by calling |onRILMessage|.  Because there may be several RilConsumer instances mapped to one single worker, we need something, the CLIENT_ID, to identify the received data at calling |onRILMessage|.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/4: refactor WorkerMessenger : WIP

Refactor WorkerMessenger and make it RadioInterface-neutral.

WorkerMessenger instances was originally created by RadioInterface.  This patch moves the instantiation into RadioInterfaceLayer instead.  Also introduce a new method |registerClient| to assign CLIENT_ID in the worker global.

Comment 4

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer : WIP2

Move onRILMessage change (add an additional argument 'clientId') to this part.

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.a/4: move ril_worker init code out of RadioInterfaceLayer ctor

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.b/4: allow sharing WorkerMessenger between RadioInterface instances

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.a/4: Move PENDING_NETWORK_TYPE into RIL and rename to pendingNetworkType

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.b/4: don't modify shared global variables after initialized

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.c/4: refactor object literals to object ctors

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Marionette test cases look fine, have to fix ICC/STK related ones on emulator-jb because they assume there is only one rild.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.d/4: look up foreign objects through 'this.context' : WIP

Almost there ...

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.d/4: look up foreign objects through 'this.context'

Add a 'context' property to global helpers, RIL and Buf.  Access each other through, for example, |this.context.RIL|.

Comment 13

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.e/4: multiple contexts in one RIL worker

1) Create one |Context| object for each registered client
2) Define a |ContextPool| object for dispatching messages from chrome and rild, instantiating new contexts upon receiving 'registerClient' messages.
3) Switch CURRENT_CLIENT_ID correctly for printing debug messages.  Note that call paths originated from 'setTimeout' calls have to be handled specially.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.f/4: really merge all RIL worker instances into one

Relatively simple.  Just move WorkerMessenger instantiation outside RadioInterface instantiation loop.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Emulator-Jellybean-9-SIMS-Merged.zip

** THIS IS MEMORY-REPORT IS COLLECTED FROM emulator-jb WITH 9 SIM CARDS **

│   ├──5.30 MB (10.46%) -- worker(resource://gre/modules/ril_worker.js, 0x437d4c00)
│   │  ├──1.95 MB (03.85%) -- gc-heap
│   │  │  ├──1.91 MB (03.76%) ── unused-arenas
│   │  │  └──0.05 MB (00.09%) ++ (2 tiny)
│   │  ├──1.77 MB (03.50%) -- runtime
│   │  │  ├──0.94 MB (01.86%) ── script-sources
│   │  │  └──0.83 MB (01.64%) ++ (11 tiny)
│   │  ├──0.98 MB (01.93%) -- zone(0x44873800)
│   │  │  ├──0.65 MB (01.28%) ++ compartment(web-worker)
│   │  │  └──0.33 MB (00.65%) ++ (4 tiny)
│   │  └──0.60 MB (01.18%) ++ (2 tiny)

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Emulator-Jellybean-9-SIMS-Separated (original).zip

** THIS IS MEMORY-REPORT IS COLLECTED FROM emulator-jb WITH 9 SIM CARDS **

│  ├───2.88 MB (04.00%) -- worker(resource://gre/modules/ril_worker.js, 0x4486fc00)
│  │   ├──1.66 MB (02.31%) -- runtime
│  │   │  ├──0.90 MB (01.25%) ── script-sources
│  │   │  └──0.76 MB (01.06%) ++ (11 tiny)
│  │   └──1.21 MB (01.69%) ++ (4 tiny)
│  ├───2.88 MB (04.00%) -- worker(resource://gre/modules/ril_worker.js, 0x44e17800)
│  │   ├──1.66 MB (02.31%) -- runtime
│  │   │  ├──0.90 MB (01.25%) ── script-sources
│  │   │  └──0.76 MB (01.06%) ++ (11 tiny)
│  │   └──1.21 MB (01.69%) ++ (4 tiny)
│  ├───2.88 MB (04.00%) -- worker(resource://gre/modules/ril_worker.js, 0x44e1a000)
│  │   ├──1.66 MB (02.31%) -- runtime
│  │   │  ├──0.90 MB (01.25%) ── script-sources
│  │   │  └──0.76 MB (01.06%) ++ (11 tiny)
│  │   └──1.21 MB (01.69%) ++ (4 tiny)
│  ├───2.88 MB (04.00%) -- worker(resource://gre/modules/ril_worker.js, 0x45664400)
│  │   ├──1.66 MB (02.31%) -- runtime
│  │   │  ├──0.90 MB (01.25%) ── script-sources
│  │   │  └──0.76 MB (01.06%) ++ (11 tiny)
│  │   └──1.21 MB (01.69%) ++ (4 tiny)
│  ├───2.88 MB (04.00%) -- worker(resource://gre/modules/ril_worker.js, 0x45667c00)
│  │   ├──1.66 MB (02.31%) -- runtime
│  │   │  ├──0.90 MB (01.25%) ── script-sources
│  │   │  └──0.76 MB (01.06%) ++ (11 tiny)
│  │   └──1.21 MB (01.69%) ++ (4 tiny)
│  ├───2.88 MB (04.00%) -- worker(resource://gre/modules/ril_worker.js, 0x45f54000)
│  │   ├──1.66 MB (02.31%) -- runtime
│  │   │  ├──0.90 MB (01.25%) ── script-sources
│  │   │  └──0.76 MB (01.06%) ++ (11 tiny)
│  │   └──1.21 MB (01.69%) ++ (4 tiny)
│  ├───2.88 MB (04.00%) -- worker(resource://gre/modules/ril_worker.js, 0x45fe1000)
│  │   ├──1.66 MB (02.31%) -- runtime
│  │   │  ├──0.90 MB (01.25%) ── script-sources
│  │   │  └──0.76 MB (01.06%) ++ (11 tiny)
│  │   └──1.21 MB (01.69%) ++ (4 tiny)
│  ├───2.87 MB (04.00%) -- worker(resource://gre/modules/ril_worker.js, 0x45fe5800)
│  │   ├──1.66 MB (02.31%) -- runtime
│  │   │  ├──0.90 MB (01.25%) ── script-sources
│  │   │  └──0.76 MB (01.06%) ++ (11 tiny)
│  │   └──1.21 MB (01.68%) ++ (4 tiny)
│  ├───2.83 MB (03.94%) -- worker(resource://gre/modules/ril_worker.js, 0x437d4c00)
│  │   ├──1.60 MB (02.23%) -- runtime
│  │   │  ├──0.90 MB (01.25%) ── script-sources
│  │   │  └──0.70 MB (00.97%) ++ (11 tiny)
│  │   └──1.23 MB (01.71%) ++ (4 tiny)

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

** THIS IS MEMORY-REPORT IS COLLECTED FROM emulator-jb WITH 9 SIM CARDS **

(Diff)

├──-20.52 MB (96.87%) -- workers/workers()/worker(resource://gre/modules/ril_worker.js, 0xNNN)
│  ├──-13.12 MB (61.92%) -- runtime
│  │  ├───-7.15 MB (33.74%) ── script-sources [9]
│  │  ├───-2.56 MB (12.08%) ── script-data [9]
│  │  ├───-1.88 MB (08.85%) -- code
│  │  │   ├──-0.82 MB (03.88%) ── unused [9]
│  │  │   ├──-0.73 MB (03.47%) ── baseline [9]
│  │  │   └──-0.32 MB (01.51%) ── other [9]
│  │  ├───-1.00 MB (04.72%) ── atoms-table [9]
│  │  ├───-0.31 MB (01.47%) ── runtime-object [9]
│  │  └───-0.22 MB (01.06%) ++ (5 tiny)
│  └───-7.41 MB (34.95%) ++ (31 tiny)

Will find a Tarako and re-correct memory-reports on it tomorrow.

Comment 18

[Vicamo Yang [:vicamo][:vyang]]

Above patches are based on those in bug 960961.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

TODO: fix existing xpcshell test cases as part 4.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Emulator-Jellybean-2-SIMS-Separated (original).zip

Re-do with only 2 SIM cards, and invoke get_about_memory.py with --minimize.

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Emulator-Jellybean-9-SIMS-Merged.zip

Re-do with only 2 SIM cards, and invoke get_about_memory.py with --minimize.

Comment 22

[ying.xu]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> Created attachment 8364878 [details]
> Emulator-Jellybean-9-SIMS-Merged.zip
> 
> Re-do with only 2 SIM cards, and invoke get_about_memory.py with --minimize.

can't view the report , with firefox nightly 26

Invalid memory report(s): non-sentence other description: mem/processes/process(\init, pid=1)/other-files/init/[r-xp], /init [r-xp]

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Emulator-Jellybean-2-SIMS-Merged.zip

Wrong file attached.

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

(In reply to ying.xu from comment #22)
> can't view the report , with firefox nightly 26

26 is not nightly.  It's many-nights-beforely. :)

> Invalid memory report(s): non-sentence other description:
> mem/processes/process(\init, pid=1)/other-files/init/[r-xp], /init [r-xp]

In summary, originally each ril_worker instance takes 2.67MB, and the merged one takes 2.80MB.  2.54MB saved here on emulator-jb with 2 SIM cards.

In that increased ~130KB per instance memory footprint, ~50KB comes from around additional 600 lines of code, ~10KB comes from additional strings, and gc-heap/unused-gc-things account for the remaining ~60KB.

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #17)
> Will find a Tarako and re-correct memory-reports on it tomorrow.

Have to rebase onto v1.3 to have a test on Tarako, and that will take some time...

Comment 26

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> Above patches are based on those in bug 960961.

And bug 958773 (remove systemlib.js from ril_worker), bug 959503 (follow-up).

Comment 27

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0001-Bug-958773-Don-t-use-js-ctypes-in-the-ril-worker-r-v.patch

Comment 28

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0002-Bug-959503-B2G-RIL-move-netHelper-related-code-up-to.patch

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0003-Bug-960894-1-4-allow-representing-multiple-clients-i.patch

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0004-Bug-960894-2.a-4-move-ril_worker-init-code-out-of-Ra.patch

Comment 31

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0005-Bug-960894-2.b-4-allow-sharing-WorkerMessenger-betwe.patch

Comment 32

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0006-Bug-960894-3.a-4-Move-PENDING_NETWORK_TYPE-into-RIL-.patch

Comment 33

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0007-Bug-960894-3.b-4-don-t-modify-global.patch

Comment 34

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0008-Bug-960894-3.c-4-construct-global-objects-from-const.patch

Comment 35

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0009-Bug-960894-3.d-4-look-up-foreign-objects-through-thi.patch

Comment 36

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0010-Bug-960894-3.e-4-multiple-contexts-in-one-RIL-worker.patch

Comment 37

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3] 0011-Bug-960894-3.f-4-really-merge-all-RIL-worker-instanc.patch

Please help include/test these [v1.3] patches in tarako patch queue.  Thank you!

Comment 38

[Kai-Zhen Li [:seinlin][:kli]]

Comment on attachment 8365818 [details] [diff] [review]
[v1.3] 0011-Bug-960894-3.f-4-really-merge-all-RIL-worker-instanc.patch

Danny, I don't have a device, can you help to check and give a feedack? Thanks!

Comment 39

[Vicamo Yang [:vicamo][:vyang]]

Working branch for m-c: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/960894/merge-ril-workers
and for v1.3: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/960894/v1.3

Comment 40

[Danny Liang [:dliang]]

Comment on attachment 8365818 [details] [diff] [review]
[v1.3] 0011-Bug-960894-3.f-4-really-merge-all-RIL-worker-instanc.patch

Patches have been merged/verified on v1.3 and it can save ~1.73MB. We have uploaded patches to tarako patch queue as 0016-Bug-960894-Utilize-one-single-RIL-worker-in-DSDS.patch

Comment 41

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8364425 [details] [diff] [review]
part 3.e/4: multiple contexts in one RIL worker

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

::: dom/system/gonk/ril_worker.js
@@ +14397,5 @@
> +
> +  executeContextJob: function(clientId, func) {
> +    CURRENT_CLIENT_ID = clientId; // push
> +    try {
> +      func.call(this, this.instances[clientId]);

Following warning message keeps popping up when running xpcshell tests.  It happens when RIL (or a test script) tries to register a new client through |ContextPool.registerClient|.  It does not fail the test, but is a sign for my bad taste. :(

  System JS : WARNING /data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js:14407 - reference to undefined property this.instances[clientId]

Comment 42

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 8364422 [details] [diff] [review]
part 3.d/4: look up foreign objects through 'this.context'

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

Missed 'GsmPDUHelper' in |ComprehensionTlvHelper.writeDateTimeZoneTlv()| and |ComprehensionTlvHelper.writeLanguageTlv()|.

Comment 43

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer : WIP3

Rebase only.

Comment 44

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.b/4: don't modify shared global variables after initialized : v2

Rebase only.

Comment 45

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.c/4: refactor object literals to object ctors : v2

Rebase only.

Comment 46

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.d/4: look up foreign objects through 'this.context' : v2

Rebase and address comment 42.

Comment 47

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.e/4: multiple contexts in one RIL worker : v2

Rebase

Comment 48

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: fix existing test cases for ril worker

Mostly replace |worker.foo| with |worker.ContextPool.instances[0].foo|.

try: https://tbpl.mozilla.org/?tree=Try&rev=65a6f2aede62

Comment 49

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer

This patch adds an additional parameter to postRILMessage, which is meant to be called from native code and executed in worker thread.  That additional parameter identifies which context should the message be delivered to.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8372171 [details] [diff] [review]
part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer

If JS_LookupProperty returns false, calling JS_DefineFunction is not ok.

Also, I assume you're using JS_LookupProperty instead of JS_GetProperty for a reason?  Might be worth documenting.

>+    JS::Value argv[] = {
>+        INT_TO_JSVAL(mClientId),
>+        OBJECT_TO_JSVAL(array)
>+    };

This is a rooting hazard.  Please root the array properly!  We really need to get the static analysis working on b2g.  :(

Comment 51

[Vicamo Yang [:vicamo][:vyang]]

Request for 1.3T? so that we can really land to gecko tarako branch.

Comment 52

[Joe Cheng [:jcheng] (please needinfo)]

remove [tarako] since it's marked 1.3T?

Comment 53

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

Comment on attachment 8363659 [details] [diff] [review]
part 2.a/4: move ril_worker init code out of RadioInterfaceLayer ctor

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

This looks good and deserves r+. I gave f+ because bug 905568, refactored RIL code a little bit, has been landed and I'd like to see the patch rebased again (though I believe no major work for this bug required due to the refactoring).

Comment 54

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

Comment on attachment 8363661 [details] [diff] [review]
part 2.b/4: allow sharing WorkerMessenger between RadioInterface instances

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

Same as part 2.a/4.

Comment 55

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

Comment on attachment 8371360 [details] [diff] [review]
part 3.d/4: look up foreign objects through 'this.context' : v2

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

Thanks! Hopefully I didn't miss something :)

::: dom/system/gonk/ril_worker.js
@@ +10197,5 @@
>    processEventNotify: function(cmdDetails, ctlvs) {
>      let textMsg = {};
>  
> +    let ctlv = this.context.StkProactiveCmdHelper.searchForTag(
> +        COMPREHENSIONTLV_TAG_ALPHA_ID, ctlvs);

nit: idention

@@ +10344,1 @@
>          COMPREHENSIONTLV_TAG_ALPHA_ID, ctlvs);

nit: indention

Comment 56

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

Comment on attachment 8371362 [details] [diff] [review]
part 3.e/4: multiple contexts in one RIL worker : v2

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

::: dom/system/gonk/ril_worker.js
@@ +14385,5 @@
> +  Buf: null,
> +  RIL: null
> +};
> +
> +for (let s of Context.prototype.lazySymbols) {

Let's consider not using for-of due to performance issue. See bug 969231.

@@ +14404,5 @@
> +
> +let ContextPool = {
> +  instances: [],
> +
> +  executeContextJob: function(clientId, func) {

Per our previous discussion, if the reason of having executeContextJob is printing debug messages with clientId, I am in favor of assigning 'clientId' to debug().

Comment 57

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer : v2

Hi Boris,

I use LookupProperty because |postRILMessage| should only be defined by this cpp.  It's either undefined or defined as hook to the native function in the same file.  Is there still other risk or difference using LookupProperty here?

Comment 58

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.a/4: move ril_worker init code out of RadioInterfaceLayer ctor : v2

Rebase only

Comment 59

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.b/4: allow sharing WorkerMessenger between RadioInterface instances : v2

Comment 60

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.f/4: really merge all RIL worker instances into one : v2

This part is also affected by bug 905568.  Rebase only.

Comment 61

[Boris Zbarsky [:bzbarsky]]

> Is there still other risk or difference using LookupProperty here?

No, it should be fine given that.

However, please see my review comment about it returning false.  If JS_LookupProperty returns false, that means an exception was thrown; this is the normal API contract for JSAPI functions.  So you shouldn't be calling JS_DefineFunction after that; it will just fatally assert that there is a pending exception on the context and that you shouldn't be messing with it.

As a side note, JS_TRUE doesn't exist.

> MOZ_ALWAYS_TRUE(argv.resize(2));

What guarantees that this allocation never fails?

Comment 62

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

Comment on attachment 8373267 [details] [diff] [review]
part 2.a/4: move ril_worker init code out of RadioInterfaceLayer ctor : v2

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

Thanks!

Comment 63

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.d.2/4: use 'this.context.clientId'

Comment 64

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.d.3/4: use 'this.context.debug'

Comment 65

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.e/4: multiple contexts in one RIL worker : v3

Comment 66

[Jon Coppeard (:jonco)]

Comment on attachment 8373264 [details] [diff] [review]
part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer : v2

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

::: ipc/ril/Ril.cpp
@@ +189,3 @@
>  
> +    JS::AutoValueVector argv(aCx);
> +    MOZ_ALWAYS_TRUE(argv.resize(2));

Now that JS::AutoValueArray has landed, it would be better to write this instead of the previous two lines:

  JS::AutoValueArray<2> argv(aCx);

Comment 67

[Vicamo Yang [:vicamo][:vyang]]

https://tbpl.mozilla.org/?tree=Try&rev=dcf27ec961f0

Comment 68

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Boris Zbarsky [:bz] from comment #61)
> However, please see my review comment about it returning false.  If
> JS_LookupProperty returns false, that means an exception was thrown; this is
> the normal API contract for JSAPI functions.  So you shouldn't be calling
> JS_DefineFunction after that; it will just fatally assert that there is a
> pending exception on the context and that you shouldn't be messing with it.

Thank you.  I misunderstood the document.  But it seems JS_GetProperty has the same problem?  Don't know what to do if I got a false value here.  A quick grep tells me most scenarios simply return false/NS_ERROR_* and some of the even return NS_OK or true.

> > MOZ_ALWAYS_TRUE(argv.resize(2));
> 
> What guarantees that this allocation never fails?

Just copied from other files.  I thought that's a convention.

(In reply to Jon Coppeard (:jonco) from comment #66)
> Now that JS::AutoValueArray has landed, it would be better to write this
> instead of the previous two lines:
> 
>   JS::AutoValueArray<2> argv(aCx);

Thank you!

Comment 69

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #67)
> https://tbpl.mozilla.org/?tree=Try&rev=dcf27ec961f0

Fails test_ril_worker_cellbroadcast.js:165, "ril_worker.js:6948 - reference to undefined property this.context".

Comment 70

[Boris Zbarsky [:bzbarsky]]

> Don't know what to do if I got a false value here

You either report the exception or return a failure code to someone who will report the exception, or clear the pending exception off the JSContext.  Which one you want depends on the behavior you want.

What does this code end up doing if the JS_DefineProperty fails?

Comment 71

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

Comment on attachment 8374002 [details] [diff] [review]
part 3.d.3/4: use 'this.context.debug'

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

::: dom/system/gonk/ril_worker.js
@@ +4492,5 @@
>      // SEGMENT_NUMBER    | 8
>      // DATAGRAM          | (NUM_FIELDS – 3) * 8
>      let index = 0;
>      if (message.data[index++] !== 0) {
> +     if (DEBUG) this.context.debug("Ignore a WAP Message which is not WDP.");

nit: indention. I know it has existed before. As I see it, I point it out. :)

@@ +4506,5 @@
>        segmentSeq:     message.data[index++] + 1 // It's zero-based in CDMA WAP Push.
>      };
>  
>      if (message.header.segmentSeq > message.header.segmentMaxSeq) {
> +     if (DEBUG) this.context.debug("Wrong WDP segment info.");

ditto.

Comment 72

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

A error occurs when I test the patches locally:

System JS : WARNING /data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js:6948 - reference to undefined property this.context
TEST-UNEXPECTED-FAIL | /data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js | TypeError: this.context is undefined at /data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js:6948 - See following stack:
GsmPDUHelperObject.prototype.readUCS2String@/data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js:6948
GsmPDUHelperObject.prototype.readGsmCbData@/data/local/tests/xpcshell/dom/system/gonk/tests/header_helpers.js -> resource://gre/modules/ril_worker.js:8102

Line#8102: msg.body = this.readUCS2String.call(bufAdapter, length);
Line#6948: if (DEBUG) this.context.debug("Read UCS2 string: " + str);
so, this in #6948 isn't GsmPDUHelperObject, bufAdapter instead.

This is a miss in current patches. And I love tests! :)

Comment 73

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #72)
> A error occurs when I test the patches locally:
> This is a miss in current patches. And I love tests! :)

Yes, I reported in comment 69.  Already fixed in my working branch.

Comment 74

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer : v3

Hi Boris,

The function in concerned is a WorkerTask dispatched to RIL worker thread.  Return value of a WorkerTask::RunTask will then be the return value of nsIRunnable::Run.  The runnable seems to be wrapped by ExternalRunnableWrapper, which checks JS_IsExceptionPending() and Throw() in its own WorkerRun().

Comment 75

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2.b/4: allow sharing WorkerMessenger between RadioInterface instances : v3

Use camel case.

Comment 76

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.b/4: don't modify shared global variables after initialized : v3

Use camel case.

Comment 77

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.c/4: refactor object literals to object ctors : v3

Rebase and use camel case.

Comment 78

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.d.1/4: look up foreign objects through 'this.context' : v3

Rebase.

Comment 79

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.d.2/4: use 'this.context.clientId' : v2

Use camel case.

Comment 80

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.d.3/4: use 'this.context.debug' : v2

Rebase and address comment 69, 71, 72.

Comment 81

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.e/4: multiple contexts in one RIL worker : v4

1) Rebase, 2) use camel case, and 3) ename attribute |ContextPool.instances| to |ContextPool._contexts| because it's a private member.

Comment 82

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: fix existing test cases for ril worker : v2

Rename attribute |ContextPool.instances| to |ContextPool._contexts| because it's a private member.

Comment 83

[Vicamo Yang [:vicamo][:vyang]]

Mwn & xpchesll https://tbpl.mozilla.org/?tree=Try&rev=988a73d42805

Again, for m-c git working branch, please see https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/960894/merge-ril-workers .  Working branch for v1.3t is pretty outdated.  Will only rebase after m-c changes are landed.

Comment 84

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8375557 [details] [diff] [review]
part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer : v3

I think you just want to JS_ReportPendingException in the failure case.

Comment 85

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer : v4

Address feedback comment 84, rebased onto bug 959787.

Comment 86

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8376068 [details] [diff] [review]
part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer : v4

r=me

Comment 87

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

Comment on attachment 8375577 [details] [diff] [review]
part 4/4: fix existing test cases for ril worker : v2

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

I trust the test results! Working with Vicamo's latest WIP.

::: dom/system/gonk/tests/test_ril_worker_icc.js
@@ +1687,1 @@
>                                      [0x0, 0x0C, 0x0, 0x0, 0x0]:

nit: indention

@@ +1687,2 @@
>                                      [0x0, 0x0C, 0x0, 0x0, 0x0]:
>                                      [0x0, 0x00, 0x0, 0x0, 0x0];

ditto.

Comment 88

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #87)
> Comment on attachment 8375577 [details] [diff] [review]
> part 4/4: fix existing test cases for ril worker : v2
> nit: indention

Thank you.  Will revise after Yoshi's review.

Comment 89

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.a/4: Move PENDING_NETWORK_TYPE into RIL and rename to pendingNetworkType : v2

NO CHANGE, generated from hg.

Comment 90

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 3.d.1/4: look up foreign objects through 'this.context' : v4

Rebase onto bug 963054.

Comment 91

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 4/4: fix existing test cases for ril worker : v3

Address review comment 87.

Comment 92

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d01667dc2bc3
https://hg.mozilla.org/integration/b2g-inbound/rev/5ddf528d09df
https://hg.mozilla.org/integration/b2g-inbound/rev/5dbab941f4e7
https://hg.mozilla.org/integration/b2g-inbound/rev/504dda11bdf9
https://hg.mozilla.org/integration/b2g-inbound/rev/c00a2b2adf68
https://hg.mozilla.org/integration/b2g-inbound/rev/5725bfc6d45e
https://hg.mozilla.org/integration/b2g-inbound/rev/9d60d4e854a3
https://hg.mozilla.org/integration/b2g-inbound/rev/2cdf076eb833
https://hg.mozilla.org/integration/b2g-inbound/rev/3d9787c91234
https://hg.mozilla.org/integration/b2g-inbound/rev/9d4a66700c26
https://hg.mozilla.org/integration/b2g-inbound/rev/a9db19be5b33
https://hg.mozilla.org/integration/b2g-inbound/rev/9cf71aad6202

Comment 93

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d01667dc2bc3
https://hg.mozilla.org/mozilla-central/rev/5ddf528d09df
https://hg.mozilla.org/mozilla-central/rev/5dbab941f4e7
https://hg.mozilla.org/mozilla-central/rev/504dda11bdf9
https://hg.mozilla.org/mozilla-central/rev/c00a2b2adf68
https://hg.mozilla.org/mozilla-central/rev/5725bfc6d45e
https://hg.mozilla.org/mozilla-central/rev/9d60d4e854a3
https://hg.mozilla.org/mozilla-central/rev/2cdf076eb833
https://hg.mozilla.org/mozilla-central/rev/3d9787c91234
https://hg.mozilla.org/mozilla-central/rev/9d4a66700c26
https://hg.mozilla.org/mozilla-central/rev/a9db19be5b33
https://hg.mozilla.org/mozilla-central/rev/9cf71aad6202

Comment 94

[[:fabrice] Fabrice Desré]

\o/

Vicamo, do you mind uplifting to 1.3t ?

Comment 95

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Fabrice Desré [:fabrice] from comment #94)
> \o/ Vicamo, do you mind uplifting to 1.3t ?

Already working on it, will update the patches before leaving office.  But I don't have a Tarako at hand now, so I'll see if it's worthy of rebasing yet again to v1.3 branch, with which I can run some test cases on emulator.

Comment 96

[Vicamo Yang [:vicamo][:vyang]]

https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/960894/v1.3
https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/960894/v1.3t

Comment 97

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer

Obsolete attachment 8365808 [details] [diff] [review] and 8365809 because they have already been uplifted to v1.3t.

Hi, Boris,

Most js value rooting changes are not included in v1.3t plan, so this part is a little bit different especially in that RunTask function that rooting became an important issue in the review process.  I think I need your review again.

In v1.3t, there is no AutoValueArray.  Instead, I found similar thing called |AutoArrayRooter|.

Comment 98

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 2.a/4: move ril_worker init code out of RadioInterfaceLayer ctor

Rebase and generate patch with hg only.

Comment 99

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 2.b/4: allow sharing WorkerMessenger between RadioInterface instances

Re-do patch with hg.

Comment 100

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 3.a/4: Move PENDING_NETWORK_TYPE into RIL and rename to pendingNetworkType

Redo with hg

Comment 101

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 3.b/4: don't modify shared global variables after initialized

hg

Comment 102

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 3.c/4: refactor object literals to object ctors

Rebase

Comment 103

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 3.d.1/4: look up foreign objects through 'this.context'

Rebase

Comment 104

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 3.d.2/4: use 'this.context.clientId'

Comment 105

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 3.d.3/4: use 'this.context.debug'

Comment 106

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 3.e/4: multiple contexts in one RIL worker

Rebase

Comment 107

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 3.f/4: really merge all RIL worker instances into one

Redo with hg.

Comment 108

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #95)
> (In reply to Fabrice Desré [:fabrice] from comment #94)
> > \o/ Vicamo, do you mind uplifting to 1.3t ?
> 
> Already working on it, will update the patches before leaving office.  But I
> don't have a Tarako at hand now, so I'll see if it's worthy of rebasing yet
> again to v1.3 branch, with which I can run some test cases on emulator.

Verifying with 1.3 emulator.

Comment 109

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 4/4: fix existing test cases for ril worker

Don't have a Tarako device.  v1.3 changes look good in emulator.

Danny, mind having another feedback?  I've already rebased the patches in https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/960894/v1.3t .  You can simply pull it into your working branch.

Fabrice, need your approval to land in v1.3t branch. :)

Comment 110

[[:fabrice] Fabrice Desré]

Comment on attachment 8377674 [details] [diff] [review]
[v1.3t] part 4/4: fix existing test cases for ril worker

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

Worked just fine on my tarako!

Comment 111

[Danny Liang [:dliang]]

Comment on attachment 8377674 [details] [diff] [review]
[v1.3t] part 4/4: fix existing test cases for ril worker

Kai-Zhen, please help to veirfy the patch on v1.3t, thanks

Comment 112

[Kai-Zhen Li [:seinlin][:kli]]

Comment on attachment 8377674 [details] [diff] [review]
[v1.3t] part 4/4: fix existing test cases for ril worker

It also works fine on my Tarako, and I can see only one ril worker.

Main Process (pid 83)
Explicit Allocations
31.28 MB (100.0%) -- explicit
├──10.09 MB (32.26%) ++ js-non-window
├───4.96 MB (15.85%) ── heap-unclassified
├───4.69 MB (14.98%) ++ window-objects
├───3.33 MB (10.63%) ++ images
├───2.52 MB (08.05%) ++ heap-overhead
├───2.22 MB (07.11%) ++ workers/workers()/worker(resource://gre/modules/ril_worker.js, 0x4352cc00)
├───1.83 MB (05.85%) ++ (15 tiny)
├───0.88 MB (02.82%) ── xpti-working-set
└───0.77 MB (02.45%) ++ storage/sqlite

Comment 113

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #95)
> Already working on it, will update the patches before leaving office.  But I
> don't have a Tarako at hand now, so I'll see if it's worthy of rebasing yet
> again to v1.3 branch, with which I can run some test cases on emulator.

Found two little imperfections and filed as bug 974260.  Already addressed in my v1.3 and v1.3t branch last night.

Comment 114

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8377600 [details] [diff] [review]
[v1.3t] part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer

Er, why is this back to using a Value[] on the stack?

Comment 115

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8377600 [details] [diff] [review]
[v1.3t] part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer

And in general, interdiffs from the last-reviewed thing would be very much appreciated so I don't have to keep guessing what exactly got changed....

Comment 116

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #114)
> Er, why is this back to using a Value[] on the stack?

Because there is no such thing named JS::AutoValueArray in v1.3t branch.  All the calls to JS_CallFunctionName are made by pass an JS::Value array.  You can find this by `grep -nR --exclude-dir=.git -B 10 -A 3 JS_CallFunctionName .`.  JS::AutoValueArray is just not yet invented at the time.

Comment 117

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [m-c => v1.3t] part 1/4 inter-review patch

Comment 118

[Boris Zbarsky [:bzbarsky]]

OK, so are we talking about a branch backport now, or are we still reviewing patches to land on m-c?

Comment 119

[[:fabrice] Fabrice Desré]

(In reply to Boris Zbarsky [:bz] (reviews will be slow; ask someone else) from comment #118)
> OK, so are we talking about a branch backport now, or are we still reviewing
> patches to land on m-c?

This is the branch backport for b2g 1.3. The reviewed code landed on m-c.

Comment 120

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8378361 [details] [diff] [review]
[m-c => v1.3t] part 1/4 inter-review patch

This is a diff of diffs, not an interdiff... :(

Comment 121

[Boris Zbarsky [:bzbarsky]]

> This is the branch backport for b2g 1.3.

OK, that would have been _really_ nice to say in the review request.  Would have saved me reading through 100+ comments _again_ trying to figure out why review was being requested on something I had already reviewed....

Comment 122

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8377600 [details] [diff] [review]
[v1.3t] part 1/4: allow <1> WorkerCrossThreadDispatcher to <N> RilConsumer

r=me for branch

Comment 123

[Boris Zbarsky [:bzbarsky]]

Hmm. I guess comment 97 did say that, and I missed it somehow.  My apologies.  :(

Comment 124

[Vicamo Yang [:vicamo][:vyang]]

Attachment: [v1.3t] part 3.d.1/4: look up foreign objects through 'this.context' : v2

Rebase onto bug 963054

Comment 125

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/pushloghtml?changeset=2c3a2e723892

Comment 126

[Jason Smith [:jsmith]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #125)
> https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/
> pushloghtml?changeset=2c3a2e723892

That landing doesn't classify itself as a 1.4 landing - that's just a 1.3T landing.

Comment 127

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This landed on trunk on 2/17 though (comment 93), which puts it in 1.4, no?

Comment 128

[Jason Smith [:jsmith]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #127)
> This landed on trunk on 2/17 though (comment 93), which puts it in 1.4, no?

Oh right.