969231 - consider removing ES6 for-of loops

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: RIL, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

Attachment: no-nice-things.patch

Previously we found that the ES6 for-of loops are very slow on b2g since they require an allocation on each iteration of the loop.  See bug 927079.

Currently RIL uses the ES6 for-of construct in a number of places.  Since we know that for-of can be a performance problem it might be nice to pre-emptively replace these cases with traditional for loops.

The attached patch does this.

I had hoped that the reduced loop allocations might show some improvement on about:memory, but they do not.

So this change would be purely a precaution to remove something that has been shown to be a problem elsewhere.

Comment 1

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

Comment on attachment 8372037 [details] [diff] [review]
no-nice-things.patch

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

Thanks for the patch. Please address the comment below. Also, as bug 905568, which refactors RIL code a bit, is going to be landed very soon, I'd like to see a new patch rebased on that bug. Thank you.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +271,3 @@
>        }
> +
> +      ignoreMessages(RIL_IPC_MOBILECONNECTION_MSG_NAMES);

Should be ignoreMessages(this, RIL_IPC_MOBILECONNECTION_MSG_NAMES), here and below.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: no-nice-things.patch (v2)

Ack. Good catch.

I'll take a look at re-basing to bug 905568 next week while I'm at the tarako meetup.

Thanks!

Comment 3

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

(In reply to Ben Kelly [:bkelly] from comment #2)
> Created attachment 8372253 [details] [diff] [review]
> no-nice-things.patch (v2)
> 
> Ack. Good catch.
> 
> I'll take a look at re-basing to bug 905568 next week while I'm at the
> tarako meetup.
> 
> Thanks!

Out of curiosity: are there known performance concerns on for-in loops?

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Hsin-Yi Tsai  [:hsinyi] from comment #3)
> Out of curiosity: are there known performance concerns on for-in loops?

Not that I'm aware of, other than that I thought for-in was discouraged for iterating arrays.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: no-nice-things.patch (v3)

Rebased patch now that bug 905568 has landed.

Comment 6

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

Comment on attachment 8373883 [details] [diff] [review]
no-nice-things.patch (v3)

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

We have used Iterator in for-of loops, e.g. |for (let [, apnSetting] of Iterator(this.apnSettings.byApn))|. I guess this is also what we like to avoid?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Oh yes, I'm sorry I missed these.  My regex was not flexible enough when I was searching.  I'll refactor these tomorrow.  Thanks!

Comment 8

[Rick Waldron [:rwaldron]]

(In reply to Ben Kelly [:bkelly] from comment #0)
> Created attachment 8372037 [details] [diff] [review]
> no-nice-things.patch
> 
> Previously we found that the ES6 for-of loops are very slow on b2g since
> they require an allocation on each iteration of the loop.  See bug 927079.
> 
> Currently RIL uses the ES6 for-of construct in a number of places.  Since we
> know that for-of can be a performance problem it might be nice to
> pre-emptively replace these cases with traditional for loops.
> 
> The attached patch does this.
> 
> I had hoped that the reduced loop allocations might show some improvement on
> about:memory, but they do not.
> 
> So this change would be purely a precaution to remove something that has
> been shown to be a problem elsewhere.

I don't see any addition bugs filed for improvements to for-of performance.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Rick Waldron [:rwaldron] from comment #8)
> I don't see any addition bugs filed for improvements to for-of performance.

See bug 927548, which was referenced in bug 927079 comment 5.

Comment 10

[Rick Waldron [:rwaldron]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> (In reply to Rick Waldron [:rwaldron] from comment #8)
> > I don't see any addition bugs filed for improvements to for-of performance.
> 
> See bug 927548, which was referenced in bug 927079 comment 5.

Ah, thank you!

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: no-nice-things.patch (v4)

Hsin-Yi, here is an updated patch.  I think I got them all this time!

I tested sending/receiving SMS and phone calls on my hamachi.  I'm not sure how else to test all the code paths here.  Its easy to make typos with a change like this, so any other tests I could run would be helpful.

Comment 12

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

(In reply to Ben Kelly [:bkelly] from comment #11)
> Created attachment 8377786 [details] [diff] [review]
> no-nice-things.patch (v4)
> 
> Hsin-Yi, here is an updated patch.  I think I got them all this time!
> 
> I tested sending/receiving SMS and phone calls on my hamachi.  I'm not sure
> how else to test all the code paths here.  Its easy to make typos with a
> change like this, so any other tests I could run would be helpful.

Thanks for the patch, Ben. But I am sorry that I need to work on some urgent bugs (target on Feb. 28) so I'd probably need to hold the review for a few more days.

To test a gecko/ril patch, we usually rely on automation tests on emulator. So, you could build emulator and then run 'marionette-webapi' tests under dom/icc/tests/marionette, dom/mobileconnecion/tests/marionette and dom/telephony/tests/marionette in addition to 'xpcshell' tests under dom/system/gonk/tests. 

For manual test part, I'd expect you test data connection, mms sending and mms receiving as well.

Comment 13

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

Comment on attachment 8377786 [details] [diff] [review]
no-nice-things.patch (v4)

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

Sorry for being so late. The patch looks good. Thanks for catching this and bringing the message to me.

Before landing, please make sure you've run the automation tests and verified the patch manually. Please see comment 12 to get the test scenario. Sorry that test cases haven't covered enough. :(

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

I haven't had time to test this in the last three months and the patches are probably rotted by now.  Given that this was speculative and not based on a real user facing issue I think its best just to not land the patch.  Its obviously not a priority and doesn't make sense to introduce risk for no visible gain.

Sorry for taking your review time without following through Hsin-Yi!