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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

It will be better to keep a different set of value for LMK on device get only 128MB memory.

Comment 1

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

Attachment: adjust_lowmemorykiller.diff

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.

Comment 2

[Dave Hylands [:dhylands]]

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

Comment 3

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

(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.

Comment 4

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

KaiZhen, do you mind taking this bug? since you seem to be working on it.
Thanks

Comment 5

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

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].

Comment 6

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

triage: 1.3+ to enable customization on lowmemorykiller

Comment 7

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

Attachment: adjust_lowmemorykiller_KB.diff

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.

Comment 8

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

Attachment: bug-945630-fix.patch

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.

Comment 9

[Dave Hylands [: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.

Comment 10

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

Attachment: 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.

Comment 11

[[:fabrice] Fabrice Desré]

I'd rather move device specific prefs in their own file if we really need per-device prefs, but not the full b2g.js

Comment 12

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

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.

Comment 13

[[:fabrice] Fabrice Desré]

Kai-Zhen, yes that looks reasonable.

Comment 14

[Jason Smith [:jsmith]]

This would be a blocker for tarako, but not a blocker for 1.3.

Comment 15

[Vincent Liu[:vliu]]

Attachment: WIP.diff

(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

Comment 16

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

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 $<)
>

Comment 17

[[:fabrice] Fabrice Desré]

That works for me. Let's keep the default values in b2g.js and override with a device specific file.

Comment 18

[[:fabrice] Fabrice Desré]

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.

Comment 19

[Vincent Liu[:vliu]]

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.

Comment 20

[[:fabrice] Fabrice Desré]

(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.

Comment 21

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

Hi, Vincent, switch this bug to you.

Comment 22

[Vincent Liu[:vliu]]

Attachment: pull request for bug 945630.html

Hi mwu,

This pull request is for the target mentioned in Comment 15. Can you have a review for me? Thanks.

Comment 23

[Michael Vines [:m1] [:evilmachines]]

(We'd like to be able to reconfigure the LMK values external to Gecko on a per device basis as well)

Comment 24

[Preeti Raghunath(:Preeti)]

Mwu,

Please help move the code review ahead.

Comment 25

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 26

[Preeti Raghunath(:Preeti)]

Vincent

Please review mvines comment and see if any changes are needed.

Comment 27

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

(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.

Comment 28

[Michael Vines [:m1] [:evilmachines]]

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!

Comment 29

[Vincent Liu[: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.

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.

Comment 30

[Michael Vines [:m1] [:evilmachines]]

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

Comment 31

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

(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.

Comment 32

[Michael Vines [:m1] [:evilmachines]]

(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?

Comment 33

[Vincent Liu[:vliu]]

(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

Comment 34

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 35

[Jason Smith [:jsmith]]

(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.

Comment 36

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

> 
> 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+

Comment 37

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Per https://bugzilla.mozilla.org/show_bug.cgi?id=945630#36

Comment 38

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 39

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(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

Comment 40

[Michael Vines [:m1] [:evilmachines]]

> 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 ;)

Comment 41

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(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 :)

Comment 42

[Michael Vines [:m1] [:evilmachines]]

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 43

[Michael Wu [:mwu]]

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.

Comment 44

[Vincent Liu[:vliu]]

Attachment: pull request for bug 945630 to 1.3 branch.html

(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.

Comment 45

[Vincent Liu[:vliu]]

Attachment: pull request for bug 945630 to 1.3 branch.html

(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.

Comment 46

[Michael Wu [: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.

Comment 47

[Michael Wu [:mwu]]

(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..

Comment 48

[Vincent Liu[:vliu]]

Attachment: pull request for bug 945630 to master branch.html

(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.

Comment 49

[Michael Vines [:m1] [:evilmachines]]

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.

Comment 50

[Preeti Raghunath(:Preeti)]

moving this to 1.3T+ for Tarako branch

Comment 51

[Michael Wu [:mwu]]

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

No need to ask me to review branch versions.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Master: 6a350895b8aad70b78500304ae6704488c6e85da

Comment 53

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

1.3T+ already. remove [tarako] whiteboard

Comment 54

[Vincent Liu[:vliu]]

Attachment: pull request for bug 945630 to 1.3t branch.html

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

Comment 55

[Ryan VanderMeulen [: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?

Comment 56

[Michael Vines [:m1] [:evilmachines]]

> Also, does 1.3T+ mean 1.3+ automatically too?

My understanding is that it does not.

Comment 57

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

actually partner should do the merge to 1.3T branch, our TAM will talk to our partner

Comment 58

[Marco Chen [:mchen]]

https://github.com/mozilla-b2g/gonk-misc/commit/850511151d297dc9938a6f5f0a19198e66f8678c