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

RESOLVED FIXED in 2.2 S8 (20mar)

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

unspecified
2.2 S8 (20mar)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(btseng)
(Assignee)

Comment 1

4 years ago
Posted patch patch v1Splinter Review
Attachment #8568640 - Flags: feedback?(btseng)
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.
blocking-b2g: 2.2? → 2.2+
(Assignee)

Comment 4

4 years ago
Try is green :) Now waiting that Bevis is back from PTO !
Assignee: nobody → felash
Status: NEW → ASSIGNED
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.
Flags: needinfo?(btseng)
Comment on attachment 8568640 [details] [diff] [review]
patch v1

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

Please see my answer in comment 5.
Attachment #8568640 - Flags: feedback?(btseng) → feedback-
(Assignee)

Comment 7

4 years ago
(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.
Flags: needinfo?(btseng)
(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...
Flags: needinfo?(btseng)
(Assignee)

Comment 9

4 years ago
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
Flags: needinfo?(oteo)
(Assignee)

Comment 11

4 years ago
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...
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?
Flags: needinfo?(oteo) → needinfo?(felash)
(Assignee)

Comment 13

4 years ago
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.
Flags: needinfo?(felash) → needinfo?(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.
Flags: needinfo?(oteo)
Flags: needinfo?(beatriz.rodriguezgomez)
Flags: needinfo?(amac.bug)
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)
Flags: needinfo?(beatriz.rodriguezgomez)
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.
Flags: needinfo?(amac.bug)
(Assignee)

Comment 18

4 years ago
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).
Attachment #8573972 - Flags: review?(azasypkin)
(Assignee)

Comment 19

4 years ago
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
(Assignee)

Comment 20

4 years ago
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 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!
Attachment #8573972 - Attachment is obsolete: true
Attachment #8573972 - Flags: review?(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!
Attachment #8575276 - Flags: review?(schung)
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?
Attachment #8575276 - Flags: review?(schung) → review+
(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.
Flags: needinfo?(schung)
(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.
Flags: needinfo?(schung)
(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!
Threeherder is green, landed.

Master: https://github.com/mozilla-b2g/gaia/commit/2360b7abf7a6468abeabccaedc33594e465850fc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Resolution: --- → FIXED
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
Attachment #8575276 - Flags: approval-gaia-v2.2?
Attachment #8575276 - Flags: approval-gaia-v2.2? → approval-gaia-v2.2+
You need to log in before you can comment on or make changes to this bug.