1136170 - [Messages] Adjust the read ahead number to be the same or higher than what the max number of threads on Flame (~150ms)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Julien Wajsberg [:julienw]]

Currently, the dom.sms.maxReadAheadEntries pref is set to "7" from the settings.js file [1].

[1] http://hg.mozilla.org/mozilla-central/file/b74938ef94bc/b2g/chrome/content/settings.js#l547

From what Oleg and I measured, setting it to 9 (which is a little more than the max number of threads on a Flame) seems to boost the measurement a lot (at least 150ms better !).

The reason is that the measurement actually stops at 9 threads as "first panel is displayed" measurement. This is not always like in real life as it depends on the number of groups we have, but this is still happening in real life.

Therefore we should change the pref from 7 to 9.

Moreover I frankly don't understand why we have a settings for this. I think having a pref with the default value to 9 in b2g/app/b2g.js is just good enough.

NI Bevis: what do you think? I can try to provide you a patch if necessary.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Comment 2

[Johan Lorenzo [:jlorenzo]]

Triage: We would like this patch in 2.2 as performance improvement is a part of 2.2, and the current patch looks simple enough and can give a a decent performance increase.

Comment 3

[Julien Wajsberg [:julienw]]

Try is https://treeherder.mozilla.org/#/jobs?repo=try&revision=08a2056588ec

Comment 4

[Julien Wajsberg [:julienw]]

Try is green :) Now waiting that Bevis is back from PTO !

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Hi Julien,

As mentioned in bug 1125751 comment 12, because this configuration is more gaia specific, we would like to 
1. have the setting of "ril.sms.maxReadAheadEntries" for gaia mapped to the preference of "dom.sms.maxReadAheadEntries" referred internally in gecko.
2. The default value of this setting (ril.sms.maxReadAheadEntries) '7' is applied if it is not provided by gaia.

Then, this provides the flexibility for gaia to set the 'ril.sms.maxReadAheadEntries' either in built-time or in run-time according to the device resolution instead.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8568640 [details] [diff] [review]
patch v1

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

Please see my answer in comment 5.

Comment 7

[Julien Wajsberg [:julienw]]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #5)
> Hi Julien,
> 
> As mentioned in bug 1125751 comment 12, because this configuration is more
> gaia specific, we would like to 
> 1. have the setting of "ril.sms.maxReadAheadEntries" for gaia mapped to the
> preference of "dom.sms.maxReadAheadEntries" referred internally in gecko.
> 2. The default value of this setting (ril.sms.maxReadAheadEntries) '7' is
> applied if it is not provided by gaia.
> 
> Then, this provides the flexibility for gaia to set the
> 'ril.sms.maxReadAheadEntries' either in built-time or in run-time according
> to the device resolution instead.

Hey Bevis,

I disagree with you: this preference is not gaia-specific, it's device-specific. So there is no need to change it at run-time, only at built-time. Therefore there is no need to have the complex machinery with the settings, we only need a pref, that can also be set at build-time by partners.

Moreover, I don't actually foresee the need for anyone to change this value once we have the correct one.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #5)
> > Hi Julien,
> > 
> > As mentioned in bug 1125751 comment 12, because this configuration is more
> > gaia specific, we would like to 
> > 1. have the setting of "ril.sms.maxReadAheadEntries" for gaia mapped to the
> > preference of "dom.sms.maxReadAheadEntries" referred internally in gecko.
> > 2. The default value of this setting (ril.sms.maxReadAheadEntries) '7' is
> > applied if it is not provided by gaia.
> > 
> > Then, this provides the flexibility for gaia to set the
> > 'ril.sms.maxReadAheadEntries' either in built-time or in run-time according
> > to the device resolution instead.
> 
> Hey Bevis,
> 
> I disagree with you: this preference is not gaia-specific, it's
> device-specific. So there is no need to change it at run-time, only at
> built-time. Therefore there is no need to have the complex machinery with
> the settings, we only need a pref, that can also be set at build-time by
> partners.
m....
IMO, this is expected to be changed according to the resolution of the device and the number of items to be displayed in the thread_list_ui & thread_ui by the UI design.
The customization mechanism I know so far is to have the default value of the settings provided from gaia.
That's the reason why I suggested this to be modified from gaia's perspective without further modification in gecko.
If there is already a built mechanism to override the default value in b2g/app/b2g.js according to the target device and this is only required to be set in built-time, then I agree with you that we don't need this complex way to map the setting to the pref.

> Moreover, I don't actually foresee the need for anyone to change this value
> once we have the correct one.
Are you trying to say that the number of items to be displayed will always be the same in different  device resolution and there will be no UI design change for this in the future?
This sounds weird to me...

Comment 9

[Julien Wajsberg [:julienw]]

Maybe I'm wrong, I'll ask someone from TEF. Maria, maybe you know what preferences or settings partners can set, and how they set it? How do they even know what they can customize ? Thanks !



I found [1] where it looks like that partners can define the prefs.

[1] https://github.com/mozilla-b2g/gaia/blob/a798c9b590dea73795f3526532477df86c885b4d/build/preferences.js#L9-L12

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] julienw:1136170-adjust-readahead-value > mozilla-b2g:master

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 8571320 [details] [review]
[gaia] julienw:1136170-adjust-readahead-value > mozilla-b2g:master

Here is what could be a gaia patch. But I don't know how to update the value during an upgrade...

Comment 12

[Maria Angeles Oteo (:oteo)]

Hi Julien,
just to clarify, you are not asking for operaton variant customization, aren't you? 
As you want to modify the default value of a parameter, independently of the operator or country the device will be delivered. I mean, you want to apply it to all of them, is it correct?

Comment 13

[Julien Wajsberg [:julienw]]

Maria, yes, that's it !

I'd like to know if this is usual that constructors apply specific pref and/or settings for a specific device, and how they know which values they can customize.

Comment 14

[Maria Angeles Oteo (:oteo)]

OEMs use to attend and apply our requests in case we need to customize a setting or preference.

The problem here is that it's a specific setting that, besides, depends on the screen size or device resolution. So if we do not provide an easy way to determine the value so, magically, the can know what to enter (via a formula, depending on the resolution or whatever) I suspect that it will be difficult that the OEM test it by himself for each type of device and set it to the corresponding value.

Anyway, setting ni to Antonio and Beatriz, in case they want to correct me or add more info.

Comment 15

[Beatriz Rodríguez [:brg]]

No more details to add, this kind of setting is not covered by TEF requirements in certification.

IMHO, it could be great to have a list of preferences/settings that OEM can adapt according to their HW needs in case the parameters is not intelligent enough to set it properly in a magic way (regardless of the value more suitable for Flame or other Mozilla reference device)

Comment 16

[Antonio Manuel Amaya Calvo (:amac)]

Not much to add here. Without a way (hopefully easy) to determine and/or test the best value here, I don't believe most OEMs will check/change this on their own.

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] julienw:1133170-set-readahead > mozilla-b2g:master

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 8573972 [details] [review]
[gaia] julienw:1133170-set-readahead > mozilla-b2g:master

Hey Oleg,

what do you think ?

My numbers are here:
http://jsbin.com/qakiya/6/edit

(first one: without the patch; second one: with the patch after a first launch to set the value).

Comment 19

[Julien Wajsberg [:julienw]]

For posterity:

Without patch:
median=1954
mean=1960.7
std=119.19549767783

With patch (after a first run):
median=1682.5
mean=1800.6
std=349.92100695887

Comment 20

[Julien Wajsberg [:julienw]]

And if I remove the first run in both (the first run is always a lot higher because it happens after a reboot):

Without patch:
median=1946
mean=1930.8888888889
std=77.366731293955

With patch (after a first run):
median=1664
mean=1691.7777777778
std=67.265849020468

Comment 21

[GitHub Autolander Bot]

Attachment: [gaia] azasypkin:pr-28692-julien-read-ahead-nits > mozilla-b2g:master

Comment 22

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8573972 [details] [review]
[gaia] julienw:1133170-set-readahead > mozilla-b2g:master

Hey Julien, everything looks good to me, except for one accidental leftover (9 was hardcoded in settings.js) and unit test nit. 

Since you're on PTO this week, I'll go ahead and fix these minor things and ask Steve for proof-review.

Thanks!

Comment 23

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8575276 [details] [review]
[gaia] azasypkin:pr-28692-julien-read-ahead-nits > mozilla-b2g:master

Steve, this PR is based on Julien's work + tiny separate commit to fix my review comments since Julien is on PTO :)

Could you please take a final look and make sure that everything is ok?

Thanks!

Comment 24

[Steve Chung [:steveck]]

Comment on attachment 8575276 [details] [review]
[gaia] azasypkin:pr-28692-julien-read-ahead-nits > mozilla-b2g:master

The fixing looks good, just one question: The list number count on flame is 8 for the full panel hight, and I think current shipped devices's resolution sould not have more than 8 in a panel(I don't have a peak right now so I'm not sure if it still need 9 for full panel), maybe we can reduce the count to 8 to improve a liitle bit further?

Comment 25

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #24)
> Comment on attachment 8575276 [details] [review]
> [gaia] azasypkin:pr-28692-julien-read-ahead-nits > mozilla-b2g:master
> 
> The fixing looks good, just one question: The list number count on flame is
> 8 for the full panel hight, and I think current shipped devices's resolution
> sould not have more than 8 in a panel(I don't have a peak right now so I'm
> not sure if it still need 9 for full panel), maybe we can reduce the count
> to 8 to improve a liitle bit further?

Thanks for review! Honestly I've never seen Peak, so can't say anything about it :) But I believe Fx0 should be very similar in terms of display size and resolution, do you have one to test?

If Fx0 fits only 8 then I'm totally ok with changing it to 8 as well.

Comment 26

[Steve Chung [:steveck]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #25) 
> Thanks for review! Honestly I've never seen Peak, so can't say anything
> about it :) But I believe Fx0 should be very similar in terms of display
> size and resolution, do you have one to test?
> 
> If Fx0 fits only 8 then I'm totally ok with changing it to 8 as well.

Luckyly we have some Fx0 for demo in Taipei(And thanks for the reminder!) But the bad news is it did need 9 threads for full panel :/ So let's leave it as it is.

Comment 27

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #26)
> Luckyly we have some Fx0 for demo in Taipei(And thanks for the reminder!)
> But the bad news is it did need 9 threads for full panel :/ So let's leave
> it as it is.

Okay, good to know, thanks for verifying!

Comment 28

[Oleg Zasypkin [:azasypkin]]

Threeherder is green, landed.

Master: https://github.com/mozilla-b2g/gaia/commit/2360b7abf7a6468abeabccaedc33594e465850fc

Comment 29

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8575276 [details] [review]
[gaia] azasypkin:pr-28692-julien-read-ahead-nits > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): n/a
[User impact] if declined: first panel of threads will be displayed later + automatic performance measurements will be less accurate
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: n/a

Comment 30

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/6a750c912b326e3bc387df6c5f49ff1d1aff2746