Closed Bug 945630 Opened 6 years ago Closed 6 years ago

Expose a device-depent preference file for loading device specific preference settings.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed)

RESOLVED FIXED
1.4 S1 (14feb)
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: seinlin, Assigned: vliu)

References

Details

Attachments

(7 files, 3 obsolete files)

It will be better to keep a different set of value for LMK on device get only 128MB memory.
Blocks: 944457
If lowmemorykiller is too aggressive on 128MB device, all content processes could be killed mostly. But if lowmemorykiller start to work too late, the overall performance will be very poor.
Summary: Tune LMK on 128MB device → Tune lowmemorykiller on 128MB device
Presumably this will nee to be done in a device dependent way?

Right now, you've adjusted the settings in a global file that is included for all b2g devices
(In reply to Dave Hylands [:dhylands] from comment #2)
> Presumably this will nee to be done in a device dependent way?
> 
> Right now, you've adjusted the settings in a global file that is included
> for all b2g devices

Yes, if there is a device dependent way to adjust these setting would be better, but I didn't know how to.
The attachment 8341599 [details] [diff] [review] is a set of value where b2g can run on 128MB device for other reference.
KaiZhen, do you mind taking this bug? since you seem to be working on it.
Thanks
blocking-b2g: --- → 1.3?
Whiteboard: [tarako]
Joe, this is specific for Tarako (device with less memory) only. When there is a branch for Tarako, partner can patch from the attachment 8341599 [details] [diff] [review].
triage: 1.3+ to enable customization on lowmemorykiller
Assignee: nobody → kli
blocking-b2g: 1.3? → 1.3+
For lower memory device, I think we could also consider to adjust the LMK parameters with unit of KB. This set of values can also work properly for Tarako.
These parameters could be device dependent as discussed in Comment 2. This will allow a device profile to define them if need; Otherwise the default values will be used.
Attachment #8349448 - Flags: feedback?(dhylands)
Comment on attachment 8349448 [details] [diff] [review]
bug-945630-fix.patch

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

Looks like a reasonable approach to me.

::: b2g/app/b2g.js
@@ +636,5 @@
>  // okay, kernel will still kill processes with larger OomScoreAdjust first even
>  // its OomScoreAdjust don't have a corresponding KillUnderMB.
>  
>  pref("hal.processPriorityManager.gonk.MASTER.OomScoreAdjust", 0);
> +pref("hal.processPriorityManager.gonk.MASTER.KillUnderMB", @MOZ_LMK_MASTER@);

I think that it would be worthwhile to have a comment mentioning that MOZ_LM_MASTER comes from confvars.sh

I certainly wouldn't have know where to go and look to find the actual settings.

::: b2g/confvars.sh
@@ +64,5 @@
>  MOZ_JSDOWNLOADS=1
> +
> +
> +if test "$DEVICE_LMK"; then
> +MOZ_LMK_MASTER=$DEVICE_LMK_MASTER

Sicne the variable that's being replaced is called:
hal.processPriorityManager.gonk.MASTER.KillUnderMB

shouldn't this be named something like MOZ_LMK_MASTER_KILL_UNDER_MB ?

Sure, you're only customizine one variable right now, but if you decided that you need to tune say hal.processPriorityManager.gonk.MASTER.OomScoreAdjust in the future then the names would make sense.
Attachment #8349448 - Flags: feedback?(dhylands) → feedback+
Hi, Dave, There are a lot of parameters in b2g.js which could be adjusted per device. I think export each parameter one by one will increase our effort to maintain. So each device can maintain its own b2g.js if necessary could be also a good choice for us. By default b2g/app/b2g.js will be used.
Attachment #8357004 - Flags: feedback?(dhylands)
I'd rather move device specific prefs in their own file if we really need per-device prefs, but not the full b2g.js
Hi, Fabrice, Do you think if this method is reasonable? When device define a perf file, build it into b2g. Then it will be loaded after b2g.js to override those perfs need per device.
Kai-Zhen, yes that looks reasonable.
Attachment #8357004 - Flags: feedback?(dhylands)
This would be a blocker for tarako, but not a blocker for 1.3.
blocking-b2g: 1.3+ → 1.3?
Attached patch WIP.diff (obsolete) — Splinter Review
(In reply to Kai-Zhen Li from comment #10)
> Created attachment 8357004 [details] [diff] [review]
> use_device_b2g_js.diff
> 
> Hi, Dave, There are a lot of parameters in b2g.js which could be adjusted
> per device. I think export each parameter one by one will increase our
> effort to maintain. So each device can maintain its own b2g.js if necessary
> could be also a good choice for us. By default b2g/app/b2g.js will be used.

b2g.js is packed in omni.ja and it is not recommend putting any device-dependent js file in it. 

(In reply to Fabrice Desré [:fabrice] from comment #11)
> I'd rather move device specific prefs in their own file if we really need
> per-device prefs, but not the full b2g.js

I think it is better putting device specific prefs (Suppose it called dev-pref.js) in their won device folder. After discussed with Kai-Zhen and Michael Wu, here comes out the idea. Currently b2g has user.js in $(TARGET_OUT)/b2g/defaults/ and it is loaded in the run time. So the idea is partner adding dev-pref.js in device folder and copy it into the same place with user.js in the build time. Partner only add EXPORT_DEV_PREF to point out the path in device folder for dev-pref.js and b2g copies the dev-pref.js in gonk-misc/Android.mk. Once the device boots up, the setting in def-pref.js overwrites the original in b2g.js. Partner only add any settings they want in def-pref.js to reach the goal.

May I get any feedback about the idea? Thanks
Attachment #8361475 - Flags: feedback?(fabrice)
Comment on attachment 8361475 [details] [diff] [review]
WIP.diff

>diff --git a/Android.mk b/Android.mk
>index 15c2459..56ee14e 100644
>--- a/Android.mk
>+++ b/Android.mk
>@@ -208,6 +208,11 @@ $(LOCAL_INSTALLED_MODULE): $(LOCAL_BUILT_MODULE) gaia/profile.tar.gz $(APRIORI)
> 
> 	mkdir -p $(TARGET_OUT)/b2g/defaults/pref
> 	cp -r $(GAIA_PATH)/profile/defaults/* $(TARGET_OUT)/b2g/defaults/
>+ifneq (,$(TARGET_OUT)/b2g/defaults/pref/)
I think we can ignore this check, because it will always be true as the dir is just created two line above.

>+ifneq (,$(EXPORT_DEV_PREF))
>+	cp $(EXPORT_DEV_PREF)/*.js $(TARGET_OUT)/b2g/defaults/pref/
>+endif
>+endif
> 
> 	cd $(TARGET_OUT) && tar xvfz $(abspath $<)
>
That works for me. Let's keep the default values in b2g.js and override with a device specific file.
Comment on attachment 8361475 [details] [diff] [review]
WIP.diff

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

::: Android.mk
@@ +208,4 @@
>  
>  	mkdir -p $(TARGET_OUT)/b2g/defaults/pref
>  	cp -r $(GAIA_PATH)/profile/defaults/* $(TARGET_OUT)/b2g/defaults/
> +ifneq (,$(TARGET_OUT)/b2g/defaults/pref/)

Kai-Zhen is right that we don't need this test.
Attachment #8361475 - Flags: feedback?(fabrice) → feedback+
Thanks for your feedback. I will propose a patch to land. :fabrice, can you help me to review the path? Or do you suggest anyone to review. Thanks.
Flags: needinfo?(fabrice)
(In reply to Vincent Liu[:vliu] from comment #19)
> Thanks for your feedback. I will propose a patch to land. :fabrice, can you
> help me to review the path? Or do you suggest anyone to review. Thanks.

Usually mwu reviews this kind of patches.
Flags: needinfo?(fabrice)
Summary: Tune lowmemorykiller on 128MB device → Expose a device-depent preference file for loading device specific preference settings.
Hi, Vincent, switch this bug to you.
Assignee: kli → vliu
Attached file pull request for bug 945630.html (obsolete) —
Hi mwu,

This pull request is for the target mentioned in Comment 15. Can you have a review for me? Thanks.
Attachment #8361475 - Attachment is obsolete: true
Attachment #8361515 - Flags: review?(mwu)
(We'd like to be able to reconfigure the LMK values external to Gecko on a per device basis as well)
Blocks: 942267
blocking-b2g: 1.3? → 1.3+
Mwu,

Please help move the code review ahead.
Flags: needinfo?(mwu)
Attachment 8349448 [details] [diff] looks like a nice way to go as I don't want to have to fork b2g.js as attachment 8361515 [details] implies.  Instead we should just be able to override the default LMK values in b2g.js by passing variables from the primary gonk build down to the gecko build.
Vincent

Please review mvines comment and see if any changes are needed.
Flags: needinfo?(vliu)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #25)
> Attachment 8349448 [details] [diff] looks like a nice way to go as I don't
> want to have to fork b2g.js as attachment 8361515 [details] implies. 
> Instead we should just be able to override the default LMK values in b2g.js
> by passing variables from the primary gonk build down to the gecko build.

The former method looks pretty nice, but I think it is not very good to maintain because pref is exported one by one and each pref need to involve more than one file. The later method is quite simple, just need to maintain a js file with a few pref to be overrided under device directory.
I don't want to maintain a full fork of b2g.js, so if I can just supply an external device.js that overrides any settings I want from the gecko b2g.js then that works too!
(In reply to Michael Vines [:m1] [:evilmachines] from comment #25)
> Attachment 8349448 [details] [diff] looks like a nice way to go as I don't
> want to have to fork b2g.js as attachment 8361515 [details] implies. 
> Instead we should just be able to override the default LMK values in b2g.js
> by passing variables from the primary gonk build down to the gecko build.

I don't think you should fork b2g.js as attachment 8361515 [details] implies. The idea is

1.) Create a file named *.js and put this file into device/<partner_path>/.
2.) Only put any preference settings you want to override like default LMK in this file. 
3.)  Assign the path of this *.js file to $EXPORT_DEV_PREF in device/<partner_path>/*.mk.
4.) In compiler time, this *.js file will copy into /system/b2g/defaults/pref/

When the device boot up, this file will be loaded and then override preference value set in b2g.js.

There is also one concern about your Attachment. If there exist more than one devices have different LMK preference values requirement, it is hard to maintain different variables for different device in confvars.sh. Furthermore, maintain device dependent settings in gecko is not a good thing.
Flags: needinfo?(vliu)
Cool, lets get the gecko changes for this landed ASAP.   I see no patch up for r? for that here yet, where is that at?

The gonk-misc/ change (attachment 8361515 [details]) is ancillary at best, one can just throw this in device/partner_path/device.mk just as easily:

  PRODUCT_COPY_FILES += device/partner_path/device-prefs.js:system/b2g/defaults/pref/device-prefs.js
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #30)
> 
>   PRODUCT_COPY_FILES +=
> device/partner_path/device-prefs.js:system/b2g/defaults/pref/device-prefs.js

/system/b2g/* will be cleaned during gonk build, files copied to this dir by PRODUCT_COPY_FILES will also be clean too. So I think to copy file to /system/b2g need to do as attachment 8361515 [details] does.
(In reply to Kai-Zhen Li from comment #31)
> /system/b2g/* will be cleaned during gonk build, files copied to this dir by
> PRODUCT_COPY_FILES will also be clean too. So I think to copy file to
> /system/b2g need to do as attachment 8361515 [details] does.

Is there an existing Gecko patch for the code that would read $(TARGET_OUT)/b2g/defaults/dev-prefs.js at runtime?
(In reply to Michael Vines [:m1] [:evilmachines] from comment #32)
> (In reply to Kai-Zhen Li from comment #31)
> > /system/b2g/* will be cleaned during gonk build, files copied to this dir by
> > PRODUCT_COPY_FILES will also be clean too. So I think to copy file to
> > /system/b2g need to do as attachment 8361515 [details] does.
> 
> Is there an existing Gecko patch for the code that would read
> $(TARGET_OUT)/b2g/defaults/dev-prefs.js at runtime?

As I know the value of arrayCount in the below link counts the number of *.js files in /system/b2g/defaults/pref/ and then openPrefFile for them at runtime.

http://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/Preferences.cpp#1090
Ah, perfect.  I have what I need then.  Thanks!

Moving this bug back to v1.3? as per comment 14.

FWIW, instead of fixing this bug I think we should be fixing the nasty |rm -rf| at https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L209.  If we just tell Make about the stuff that is getting installed into /system/b2g then it'll do the right thing for free in cases such as this.
No longer blocks: 942267
blocking-b2g: 1.3+ → 1.3?
(In reply to Michael Vines [:m1] [:evilmachines] from comment #34)
> Ah, perfect.  I have what I need then.  Thanks!
> 
> Moving this bug back to v1.3? as per comment 14.
> 
> FWIW, instead of fixing this bug I think we should be fixing the nasty |rm
> -rf| at
> https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L209.  If we
> just tell Make about the stuff that is getting installed into /system/b2g
> then it'll do the right thing for free in cases such as this.

Going to move this to 1.3T? then if it's not a CS blocker, as tarako has been flagged on this bug.
blocking-b2g: 1.3? → 1.3T?
> 
> Going to move this to 1.3T? then if it's not a CS blocker, as tarako has
> been flagged on this bug.

Since other partners also target at using 256 MB in their FFOS 1.3 project, I would suggest to stay in 1.3+
Which partners do you mean?  Folks downstream of us won't need this bug anymore for LMK tuning.  Harmless if it is added to v1.3 though, we won't use it.   If it is added please don't assume that all the gecko pref overrides will come from the same directory in the source tree as the current patch implies.  But like I mentioned in comment 34, it would be better to fix this bug by removing that |rm| that is blocking the existing PRODUCT_COPY_FILES mechanism from serving this need.  We don't need yet another way to copy stuff into /system at build time.
(In reply to Michael Vines [:m1] [:evilmachines] from comment #38)
> Which partners do you mean?  Folks downstream of us won't need this bug
> anymore for LMK tuning.  Harmless if it is added to v1.3 though, we won't
> use it.   If it is added please don't assume that all the gecko pref
> overrides will come from the same directory in the source tree as the
> current patch implies.  But like I mentioned in comment 34, it would be
> better to fix this bug by removing that |rm| that is blocking the existing
> PRODUCT_COPY_FILES mechanism from serving this need.  We don't need yet
> another way to copy stuff into /system at build time.

Sorry i am confused here. I thought the main purpose of this bug is to provide a mechanism for partners such that they can override the default LMK value with their own preferred value. That's why i thought it is better to include this in 1.3 as well. Do i misunderstand anything here?

Thanks
> I thought the main purpose of this bug is to provide a mechanism for partners such that they can override the default LMK value with their own preferred value. 

Yeah it was.  But it took to long so I did it another way ;)
(In reply to Michael Vines [:m1] [:evilmachines] from comment #40)
> > I thought the main purpose of this bug is to provide a mechanism for partners such that they can override the default LMK value with their own preferred value. 
> 
> Yeah it was.  But it took to long so I did it another way ;)

OH I see...anyway as long as your 1.3 CS release provides a mechanism to let partners set their own LMK value, I have no other concern :)
Yes it does.  It's unnecessarily complex due to the gonk-misc |rm| that inhibits the use of the standard PRODUCT_COPY_FILES solution, but it works fine enough for now.
Comment on attachment 8361515 [details]
pull request for bug 945630.html

I think I would prefer a name like EXPORT_DEVICE_PREFS. Also, I think we should do cp -n to ensure gaia prefs aren't accidentally overridden.
Attachment #8361515 - Flags: review?(mwu) → review+
Flags: needinfo?(mwu)
(In reply to Michael Wu [:mwu] from comment #43)
> Comment on attachment 8361515 [details]
> pull request for bug 945630.html
> 
> I think I would prefer a name like EXPORT_DEVICE_PREFS. Also, I think we
> should do cp -n to ensure gaia prefs aren't accidentally overridden.

As for your suggestion, a pull request is attached.
Attachment #8361515 - Attachment is obsolete: true
(In reply to Michael Wu [:mwu] from comment #43)
> Comment on attachment 8361515 [details]
> pull request for bug 945630.html
> 
> I think I would prefer a name like EXPORT_DEVICE_PREFS. Also, I think we
> should do cp -n to ensure gaia prefs aren't accidentally overridden.

As for your suggestion, a pull request is attached.
Attachment #8366438 - Attachment is obsolete: true
Attachment #8366439 - Flags: review?(mwu)
Why is the pull request for the 1.3 branch? It needs to land on master before going on branches, and it'll also need to block to go on branches.
(In reply to Michael Vines [:m1] [:evilmachines] from comment #34)
> FWIW, instead of fixing this bug I think we should be fixing the nasty |rm
> -rf| at
> https://github.com/mozilla-b2g/gonk-misc/blob/master/Android.mk#L209.  If we
> just tell Make about the stuff that is getting installed into /system/b2g
> then it'll do the right thing for free in cases such as this.

I agree, but it's a little tricky. We'd need a list of files defining what goes in /system/b2g, but the exact thing we want doesn't exist. Closest thing might be $(GECKO_SRC)/b2g/installer/package-manifest.in, but even that specifies globs for certain sets of files. There's no complete list of files that define what goes in /system/b2g, and without a list of files we can't generate entries for PRODUCT_COPY_FILES or whatever else. Fortunately this is only impossible in theory - in practice, we scoop up nearly every loose file and stuff them into omni.ja. The only remaining files which we wouldn't have info for are the spellcheck dictionaries. Not sure if we actually use them on B2G..
(In reply to Michael Wu [:mwu] from comment #46)
> Why is the pull request for the 1.3 branch? It needs to land on master
> before going on branches, and it'll also need to block to go on branches.

Sorry for the missing since the bug was fired on the request of tarako and it is currently on v1.3 branch. Here is the pull request for master branch, please have a review. Thanks.
Attachment #8366468 - Flags: review?(mwu)
The list of files untarred into /system/b2g won't change between (minor) incremental builds.  If one just pulled in a drastically different Gecko then maybe there's a little bit of stale stuff in /system/b2g/ until the next full build, but that's no different than anything else in the tree.   So I wonder if that |rm| really buys us anything in practical terms.
Attachment #8366468 - Flags: review?(mwu) → review+
moving this to 1.3T+ for Tarako branch
blocking-b2g: 1.3? → 1.3T+
Comment on attachment 8366439 [details]
pull request for bug 945630 to 1.3 branch.html

No need to ask me to review branch versions.
Attachment #8366439 - Flags: review?(mwu)
Keywords: checkin-needed
Master: 6a350895b8aad70b78500304ae6704488c6e85da
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: 1.3 C2/1.4 S2(17jan) → 1.4 S1 (14feb)
1.3T+ already. remove [tarako] whiteboard
Whiteboard: [tarako]
Hi Ryan,

As Comment 51, A pull request to uplifting to v1.3 branch need to be merged. Please see the below link.

Comment on attachment 8366439 [details]
pull request for bug 945630 to 1.3 branch.html


Besides v1.3, v1.3t branch also need this patch to uplift. Please see the attached pull request in this Comment. Thanks
Flags: needinfo?(ryanvm)
For the most part, I handle the Gecko side only. Also, I hadn't heard that we were expected to be doing the 1.3T uplifts. That's a change from our previous commitments to partner branches.

Also, does 1.3T+ mean 1.3+ automatically too?
Flags: needinfo?(ryanvm) → needinfo?(jhford)
> Also, does 1.3T+ mean 1.3+ automatically too?

My understanding is that it does not.
actually partner should do the merge to 1.3T branch, our TAM will talk to our partner
Flags: needinfo?(jhford)
You need to log in before you can comment on or make changes to this bug.