Battery life is always an important resource for hand-held device. The less energy your device consumes, the longer it can be used without being recharged. Before we can extend the battery life of the mobile device, we should provide better knowledge to understand where the energy is consumed. Android's power consumption model calculates energy consumption based on power profile and system metrics. The power profile is provided by chip vendor and provide average power of the CPU and I/O components. It might have following parameters. screen.on/bluetooth.active/bluetooth.on/screen.full wifi.on/wifi.active/wifi.scan/dsp.audio/dsp.video/radio.active gps.on/battery.capacity/radio.scanning/radio.on/cpu.idle cpu.awake/cpu.active/cpu operated in different frequency and voltage The system metrics should record time usage for CPU and I/O components per process. We can have our power model to calculate per process's power usage. Process(i) power usage = Wifi average power * Wifi usage(P(i)) + Radio average power * Radio usage(P(i)) + CPU average power * CPU usage(P(i)) + .....

In my experience, I would interest in execution time per app, power consumption per app. Further, power consumption of each component per app (e.g., CPU, network, dsp) is also useful information.

Here is a rough architecture of power meter per app. First, calculate the power consumption per app for each component. Then, store the results in PowerStatsDB through PowerStatsService. For apps in Gaia, apps load the power consumption information from PowerStatsDB through PowerStatsService. Any suggestions or words will be appreciated!

After some discussions with Vincent Chang, I propose /dom/power/PowerStatsManager.idl update() will update the power consumption information of the whole components in database. loadAll() will load the whole power consumption information in database. loadApp(in DOMString aManifestUrl) will load the power consumption information of the specific app in database. loadAppComponent(in DOMString aManifestUrl, in DOMString aComponent) will load the power consumption information of the component of the specific app. I’m not sure if the interface is flexible. Any words or suggestions will be appreciated.

upload patch

rename filename for following naming rule

add clearAllData() for clear all stats from DB.

With this we could achieve this: https://bugzilla.mozilla.org/show_bug.cgi?id=874345 :)

update DOM API for architecture v2

This is the prototype of PowerStatsService and PowerStatsDB. PowerStatsService aggregates the power consumption information and provides the power information to PowerStatsManager. PowerStatsDB stores the the power consumption information repeatly to prevent from abnormal shutdown. Also, PowerStatsService restores the power consumption information from PowerStatsDB while the phone is booting up to keep statistics.

Fix PowerStatsDB Error

update info before load PowerStats

Comment on attachment 773038 [details] [diff] [review] Part 1: PowerStatsService & PowerStatsDB Prototype Review of attachment 773038 [details] [diff] [review]: ----------------------------------------------------------------- The overall architecture looks good for me. Good work. Please fix some nits and address feedback comments. ::: dom/power/PowerStatsDB.jsm @@ +7,5 @@ > +this.EXPORTED_SYMBOLS = ['PowerStatsDB']; > + > +const DEBUG = false; > +function debug(s) { dump("-*- PowerStatsDB: " + s + "\n"); } > + You can use debug("xxxx") to prevent duplicate if (DEBUG); let debug; if (DEBUG) { debug = function (s) { dump("-*- PowerStatsDB: " + s + "\n"); }; } else { debug = function (s) {}; } ::: dom/power/PowerStatsManager.js @@ +5,5 @@ > +"use strict"; > + > +const DEBUG = false; > +function debug(s) { dump("-*- PowerStatsManager: " + s + "\n"); } > + You can use debug("xxxx") to prevent duplicate if (DEBUG); let debug; if (DEBUG) { debug = function (s) { dump("-*- PowerStatsDB: " + s + "\n"); }; } else { debug = function (s) {}; } @@ +31,5 @@ > +const nsIClassInfo = Ci.nsIClassInfo; > +const POWERSTATSDATA_CID = Components.ID("{59516e1c-071b-4fc6-88e3-5c832cef94d3}"); > +const nsIDOMMozPowerStatsData = Components.interfaces.nsIDOMMozPowerStatsData; > + > +//contact.js do not do anything in constructor ?? @@ +62,5 @@ > + > +PowerStatsManager.prototype = { > + __proto__: DOMRequestIpcHelper.prototype, > + > + update: function update() { Do we also need to use this.createRequest() here and also check the privilege ? @@ +67,5 @@ > + cpmm.sendAsyncMessage("PowerStats:Update"); > + }, > + > + load: function load(options) { > + //this.checkPrivileges(); Add the privilege check here. @@ +75,5 @@ > + data: options}); > + return request; > + }, > + > + clearAllData: function clearAllData() { Add the privilege check here. @@ +133,5 @@ > + return null; > + } > + > + this.initHelper(aWindow, ["PowerStats:Get:Return", > + "PowerStats:Clear:Return"]); Do we need to have message like PowerStats:Update:Return" ? ::: dom/power/PowerStatsService.jsm @@ +5,5 @@ > +"use strict"; > + > +const DEBUG = false; > +function debug(s) { dump("-*- PowerStatsService: " + s + "\n"); } > + You can use debug("xxxx") to prevent duplicate if (DEBUG); let debug; if (DEBUG) { debug = function (s) { dump("-*- PowerStatsDB: " + s + "\n"); }; } else { debug = function (s) {}; } @@ +72,5 @@ > + debug("receiveMessage " + aMessage.name); > + } > + let mm = aMessage.target; > + let msg = aMessage.json; > + We can add the permission check here. if (!mm.assertPermission("Powerxxx")) { return; } @@ +79,5 @@ > + this.updateAppInfo(); > + break; > + case "PowerStats:Get": > + this.updateAppInfo(); > + this.load(mm,msg); Nit: space after comma. @@ +82,5 @@ > + this.updateAppInfo(); > + this.load(mm,msg); > + break; > + case "PowerStats:Clear": > + this.clearAllData(mm,msg); Nit: space after comma. @@ +111,5 @@ > + case "power-meter-info": > + let powerInfoArr = data.split(" "); > + let index = powerInfoArr[0]; > + let item = powerInfoArr[1]; > + let value = powerInfoArr[2]; Do we need to put some error checking for these values ? What if we receive garbage here. @@ +130,5 @@ > + } > + }, > + > + // nsITimerCallback > + // Timer triggers the backupStats Nit: period at the end of the sentence. @@ +136,5 @@ > + this.backupStats(); > + }, > + > + backupStats: function backupStats(callback) { > + if (DEBUG) { How about s/backupStats/updateStats/ ? BackupStats is a little confusing me. @@ +153,5 @@ > + getDBStats: function getDBStats() { > + this._db.getAll(function onStatsFound(error, result) { > + for (let i = 0; i < result.length; i++) { > + if (result[i].indexType === APP_TYPE) { > + let manifestURL = result[i].index; Check the index is correct before accessing it. if (manifestURL in appPowerInfo) @@ +158,5 @@ > + appPowerInfo[manifestURL] = {pid: -1, > + tmpTime: 0, > + time: result[i].time}; > + } else { > + let system = result[i].index; Same as above here. @@ +168,5 @@ > + }, > + > + load: function load(mm, msg) { > + let options = msg.data; > + Looks like you use length to check the load type. Can we have some comments here ? @@ +257,5 @@ > + }); > + }, > + > + createAppInfo: function createAppInfo(index, item, value) { > + //check if appInfo exists this app info Nit: capitalize the letter, period at the end of the sentence. @@ +269,5 @@ > + debug("appPowerInfo: " + JSON.stringify(appPowerInfo)); > + } > + }, > + > + updateAppInfo: function updateAppInfo() { If this function is not necessary, please remove it. @@ +273,5 @@ > + updateAppInfo: function updateAppInfo() { > + }, > + > + updateSystemInfo: function updateSystemInfo(index, item, timeStamp) { > + // unit of timeStamp is micro second Nit: capitalize the letter, period at the end of the sentence.

update DOM api creates request

Remove update DOM api, because PowerStatsService updates information before load the information.

> @@ +136,5 @@ > > + this.backupStats(); > > + }, > > + > > + backupStats: function backupStats(callback) { > > + if (DEBUG) { > > How about s/backupStats/updateStats/ ? BackupStats is a little confusing me. > The purpose of this function is backup the stats to prevent from losing stats when the phone crash. I'm afraid of the name of updateStats may confuse with updateAppInfo and updateSystemInfo. > @@ +153,5 @@ > > + getDBStats: function getDBStats() { > > + this._db.getAll(function onStatsFound(error, result) { > > + for (let i = 0; i < result.length; i++) { > > + if (result[i].indexType === APP_TYPE) { > > + let manifestURL = result[i].index; > > Check the index is correct before accessing it. > if (manifestURL in appPowerInfo) PowerStatsService calls getDBStats when a phone boots up. appPowerInfo and systemPowerInfo are empty when a phone boots up. I will implement this error checking when calling saveStats. > @@ +168,5 @@ > > + }, > > + > > + load: function load(mm, msg) { > > + let options = msg.data; > > + > > Looks like you use length to check the load type. Can we have some comments > here ? Or the load DOM API changes into two DOM APIs? loadAll and loadComponent(index)?

patch update

Move readProfile DOM API to Bug 854202.

Comment on attachment 778375 [details] [diff] [review] Part 0: PowerStatsManager IDL v9 Review of attachment 778375 [details] [diff] [review]: ----------------------------------------------------------------- Looks good for me.

Comment on attachment 778379 [details] [diff] [review] Part 1: PowerStatsService & PowerStatsDB Prototype Review of attachment 778379 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/power/PowerStatsDB.jsm @@ +46,5 @@ > + upgradeSchema: function upgradeSchema(aTransaction, > + aDb, > + aOldVersion, > + aNewVersion) { > + debug("upgrade schema from: " + aOldVersion + " to " + aNewVersion); Nit: capitalize the letter. Period at the end of the sentence. @@ +57,5 @@ > + objectStore.createIndex("index", "index", { unique: true }); > + objectStore.createIndex("indexType", "indexType", {unique: false}); > + objectStore.createIndex("time" , "time" , { unique: false }); > + > + debug("Created object stores and indexes"); Nit: Period at the end of the sentence. @@ +64,5 @@ > + }, > + > + saveStats: function saveStats(aStat, aResultCb) { > + debug("enter saveStats"); > + Please give the meaningful message here. @@ +68,5 @@ > + > + this.dbNewTxn("readwrite", function(txn, store) { > + debug("New aStat: " + JSON.stringify(aStat)); > + let request = store.index("index").openCursor(aStat.index, > + "next"); Nit: Please align the "next". @@ +73,5 @@ > + request.onsuccess = function onsuccess(event) { > + let cursor = event.target.result; > + if (!cursor) { > + this._saveStats(txn, store, aStat); > + return;a Nit: Please remove "a". @@ +81,5 @@ > + }, > + > + clear: function clear(aResultCb) { > + this.dbNewTxn("readwrite", function(txn, store) { > + debug("clear All Data in DB!"); Nit: capitalize the letter. Please also give a meaningful message here and below. @@ +88,5 @@ > + }, > + > + _saveStats: function _saveStats(txn, store, powerStats) { > + debug("_saveStats: " + JSON.stringify(powerStats)); > + Ditto @@ +92,5 @@ > + > + if (Array.isArray(powerStats)) { > + let len = powerStats.length - 1; > + for (let i = 0; i <= len; i++) { > + store.put(powerStats[i]); Inline len here. "for (let i = 0; i < powerStats.length; i++)" @@ +107,5 @@ > + aTxn.result = []; > + > + aStore.mozGetAll().onsuccess = function setTxnResult(event) { > + aTxn.result = event.target.result; > + debug("load DB: " + JSON.stringify(aTxn.result)) Ditto. ::: dom/power/PowerStatsManager.js @@ +104,5 @@ > + } > + }, > + > + init: function(aWindow) { > + debug("init()"); Nit: please give meaningful message here. @@ +140,5 @@ > + flags: nsIClassInfo.DOM_OBJECT}) > +} > + > +//this.NSGetFactory = XPCOMUtils.generateNSGetFactory([PowerStatsData, > +// PowerStatsManager]); Nit: Please remove unnecessary code. ::: dom/power/PowerStatsService.jsm @@ +124,5 @@ > + this.backupStats(); > + }, > + > + backupStats: function backupStats(callback) { > + debug("backup stats"); Nit: please give meaningful message here. @@ +142,5 @@ > + for (let i = 0; i < result.length; i++) { > + if (result[i].indexType === APP_TYPE) { > + let manifestURL = result[i].index; > + appPowerInfo[manifestURL] = {pid: -1, > + tmpTime: 0, Please check manifestURL is in the array before access it. if (manifestURL in appPowerInfo) @@ +147,5 @@ > + time: result[i].time}; > + } else { > + let system = result[i].index; > + systemPowerInfo[system] = {startTime: 0, > + time: result[i].time}; Ditto. @@ +189,5 @@ > + } > + > + for (let i in systemPowerInfo) { > + result[i] = systemPowerInfo[i]; > + } Is it possible that result will be overwritten here ? @@ +203,5 @@ > + let error = 0; > + let options = msg.data[0]; > + if (DEBUG) { > + debug("result: " + JSON.stringify(appPowerInfo[options])); > + } We don't need this if (DEBUG) here.

:vhang, thanks for your feedback. This is the improved patch.

Comment on attachment 780193 [details] [diff] [review] Part 1: PowerStatsService & PowerStatsDB Prototype Review of attachment 780193 [details] [diff] [review]: ----------------------------------------------------------------- This patch looks good. Please address the comments and ask for review. ::: dom/power/PowerStatsService.jsm @@ +65,5 @@ > + > + gIDBManager.initWindowless(myGlobal); > + this._db = new PowerStatsDB(myGlobal); > + > + this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); Why do you create the timer twice ? @@ +131,5 @@ > + debug("clear DB error"); > + } > + }); > + this.saveStats(appPowerInfo); > + this.saveStats(systemPowerInfo); Nit: These two functions should be called in the callback of this._db_clear().

Hi Jonas, could you please review this power metering framework?

Fix Bugs.

Fix bugs.

Convert PowerStatsManager to WebIDL.

Convert PowerStatsManager to WebIDL. And, fix bugs to record multiple process id (pid) for browser.

Fix nit and bugs.

Comment on attachment 793919 [details] [diff] [review] Part 1: PowerStatsService & PowerStatsDB Prototype Review of attachment 793919 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately I won't have time to review this anytime soon. API-wise I think we should use an API similar to what we are using for network usage (though obviously without the per-interface part). That way we can use similar code to do graphs of power usage and network usage. Is there a reason we couldn't do that? ::: dom/apps/src/PermissionsTable.jsm @@ +181,5 @@ > certified: ALLOW_ACTION > }, > + "powerstats-manage": { > + app: DENY_ACTION, > + privileged: PROMPT_ACTION, Is there a reason to give privileged apps access to this API? I'd rather leave this to just certified apps unless there's strong reason to do otherwise.

Comment on attachment 790599 [details] [diff] [review] Part 0: Add a WebIDL for PowerStatsManager. Review of attachment 790599 [details] [diff] [review]: ----------------------------------------------------------------- Could we align this better with the network usage API? If the two can use the same syntax that would make app-development easier I think.

(In reply to Jonas Sicking (:sicking) (vacation until 9/9. In norway until 9/16) from comment #34) > Comment on attachment 790599 [details] [diff] [review] > Part 0: Add a WebIDL for PowerStatsManager. > > Review of attachment 790599 [details] [diff] [review]: > ----------------------------------------------------------------- > > Could we align this better with the network usage API? If the two can use > the same syntax that would make app-development easier I think. Hi, Jonas: I think keeping current power metering interface is better because of two reasons: 1. The load() function provides more flexibilities on searching metering results. In current design, we allow users to search the power consumption per app, per component, or get all data record in the database. If we align this API with the network usage API, we may increase the work of App. developers since they need to parse the metering results by themselves. 2. The purpose of these two APIs are slightly different. The purpose of network statistics API is to "generate a report of daily network usage," so it stores and expresses data in a per-day basis. On the other hand, the purpose of power metreing API is to "measure the power consumption during a period of time." It allows users define or reset the start time of power metering by calling the clearAllData() function. This gives users more flexibilities on measuring power consumption. For example, we can reset the database and measure the screen power consumption when watching youtube videos. So, could we keep current power metering interace unchanged?

(In reply to bochen from comment #35) > (In reply to Jonas Sicking (:sicking) (vacation until 9/9. In norway until > 9/16) from comment #34) > > Comment on attachment 790599 [details] [diff] [review] > > Part 0: Add a WebIDL for PowerStatsManager. > > > > Review of attachment 790599 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Could we align this better with the network usage API? If the two can use > > the same syntax that would make app-development easier I think. > > Hi, Jonas: > > I think keeping current power metering interface is better because of two > reasons: > > 1. The load() function provides more flexibilities on searching metering > results. In current design, we allow users to search the power consumption > per app, per component, or get all data record in the database. If we align > this API with the network usage API, we may increase the work of App. > developers since they need to parse the metering results by themselves. > > 2. The purpose of these two APIs are slightly different. The purpose of > network statistics API is to "generate a report of daily network usage," so > it stores and expresses data in a per-day basis. On the other hand, the > purpose of power metreing API is to "measure the power consumption during a > period of time." It allows users define or reset the start time of power > metering by calling the clearAllData() function. This gives users more > flexibilities on measuring power consumption. For example, we can reset the > database and measure the screen power consumption when watching youtube > videos. > > So, could we keep current power metering interace unchanged? Hi Jonas: Sorry, maybe I mistake your comments. I found the Network Stats 2.0 API draft on https://developer.mozilla.org/en-US/docs/WebAPI/Network_Stats_2_0_proposal#nsIDOMMozNetworkStatsManager. Did you suggest us to follow the naming of the nsIDOMMozNetworkStatsManager defined in this draft?

Comment on attachment 790599 [details] [diff] [review] Part 0: Add a WebIDL for PowerStatsManager. Review of attachment 790599 [details] [diff] [review]: ----------------------------------------------------------------- remove review request because we want to make some modification

Comment on attachment 793919 [details] [diff] [review] Part 1: PowerStatsService & PowerStatsDB Prototype Review of attachment 793919 [details] [diff] [review]: ----------------------------------------------------------------- remove review request because we want to make some modification on API

What I suggest is something like this: dictionary PowerStatsOptions { DOMString component; DOMString manifestURL; jsval start; // date jsval end; // date }; interface nsIDOMMozPowerStatsManager { nsIDOMDOMRequest getPowerStats(in jsval options); readonly attribute jsval components; // array of DOMStrings. nsIDOMDOMRequest clearAllData(); readonly attribute long sampleRate; readonly attribute long maxStorageSamples; }; This is the exact same API as used for network stats, but with "Network" changed to "Power" and "connectionType" changed to "component". By using the same API it should be easier to share storage backend as well as to write code that displays data from both APIs. Even nicer might be to do something like dictionary ResourceStatsOptions { DOMString component; DOMString manifestURL; jsval start; // date jsval end; // date }; interface nsIDOMMozResourceStatsManager { nsIDOMDOMRequest getResourceStats(in jsval options); readonly attribute jsval components; // array of DOMStrings. nsIDOMDOMRequest clearAllData(); readonly attribute long sampleRate; readonly attribute long maxStorageSamples; }; And have a single API which manages both network and power data. We could allow passing "network.wifi" to get wifi info, "power.sensor" to get power consumption by sensors, "network" to get total network, or "power" to get total power consumption.

Hi Jonas: Thanks for your suggestion. I think we can align all PowerStats APIs with the NetworkStats APIs except two attributes: “sampleRate” and “maxStorageSamples”. Since both attributes are predefined values, it is a little hard to determine a proper sampling rate to meet all needs. Hence, could we just provide an accumulated power statistics since last boot (or the last time calling clearAllData()) and leave the decision of sampling rate and the work of statistics recording to a Gaia application? In addition, we want to propose a little modification on getPowerStats(). Currently, the getPowerStats() API returns the power consumption of components and applications. The power consumption is calculated by the Chrome process. We wanna to move the calculation of power consumption out of Gecko and implement it as an application. The getPowerStats() API only provides raw statistics data (such as usr_time, sys_time, etc.) instead of thecalculation results. Since Firefox OS blocks the access of some system-level information (such as /proc/stats), such a modification would provide more flexibility to third-party developers to build up their own power metering tools based on our APIs. So, could we propose a webidl like this: dictionary PowerStatsOptions { DOMString[] components; DOMString[] manifestURL; } interface PowerStatsManager : EventTarget { DOMRequest getPowerStats(any options); DOMRequest clearAllData(); readonly attribute any components; } The getPowerStats() would return a json object like this: {"app://browser.gaiamobile.org/manifest.webapp":{ "CPU":[ {"usr_time":[{"100MHz":1438200},{"250MHz":234553}]}, {"sys_time":[{"100MHz":882000},{"250MHz":121453}]} ], "LCD":[ {"brightness":[{"level_10":1432}, {"level_30":21862}, {"level_70":68642}]} ] }} (The above json object records how many cycles the application executed under different CPU frequencies and the time the application run in the front-end under different LCD brightness levels.) Based on this, we can create an application to capture raw statistics data periodically (also define the sampling rate) and implement an power model to calculate power consumptions. The application can also store daily power consumption in a database to log the usage of applications. (In reply to Jonas Sicking (:sicking) from comment #39) > What I suggest is something like this: > > > dictionary PowerStatsOptions > { > DOMString component; > DOMString manifestURL; > jsval start; // date > jsval end; // date > }; > > interface nsIDOMMozPowerStatsManager > { > nsIDOMDOMRequest getPowerStats(in jsval options); > readonly attribute jsval components; // array of DOMStrings. > nsIDOMDOMRequest clearAllData(); > readonly attribute long sampleRate; > readonly attribute long maxStorageSamples; > }; > > This is the exact same API as used for network stats, but with "Network" > changed to "Power" and "connectionType" changed to "component". > > By using the same API it should be easier to share storage backend as well > as to write code that displays data from both APIs. > > Even nicer might be to do something like > > dictionary ResourceStatsOptions > { > DOMString component; > DOMString manifestURL; > jsval start; // date > jsval end; // date > }; > > interface nsIDOMMozResourceStatsManager > { > nsIDOMDOMRequest getResourceStats(in jsval options); > readonly attribute jsval components; // array of DOMStrings. > nsIDOMDOMRequest clearAllData(); > readonly attribute long sampleRate; > readonly attribute long maxStorageSamples; > }; > > And have a single API which manages both network and power data. We could > allow passing "network.wifi" to get wifi info, "power.sensor" to get power > consumption by sensors, "network" to get total network, or "power" to get > total power consumption.

I don't understand how we could use a third party app to measure power consumption. It would take far too much system resources if we were to have a separate process running in the background just to measure power statistics. Why can't we use the same sample rate for data usage as we do for power usage? How often do we sample data usage right now? If we're in the background counting cycles then saving those cycles at the same time as we are saving network stats, into the same database, seems like it will very little overhead above work that we're already doing.

Oops, I didn't consider the extra resources consumed by a third-party app. You're right, Jonas. We will send a patch to align with NetworkStats API asap. Thanks for your suggestion.

Hi Jonas: Could you give some suggestions about the implementation of PowerStats API: 1. Sharing same database with NetworkStats I saw some of our members are adding new features to NetworkStats API recently. To avoid blocking their development, could we create a separate database for PowerStats in current development and fire a new bug to discuss how to merge these two databases? 2. Timing of clear database I found that systems like Android would clear its power usage record after reboot or unplugged from USB port (changing from 'charging' state to 'not charging' state). (My Android phone also stops counting the power consumption during charging.) Do we need to implement a similar functionality in PowerStats? 3. The length of SampleRate and the size of maxSamplePeriod In current settings, the sampleRate and the maxStorageSamples of NetworkStats are 1 day and 60 days. Do we need to follow these two settings? BTW, I think the naming of 'sampleRate' may cause a little confusion since the unit of sampling rate is inverse of time but the NetworkStats API returns time in seconds. Could we change the name 'sampleRate' to 'samplePeriod'? 4. The timing of saving power usage to database NetworkStats saves network usage from cache to database at four time points: (1) Query from an App., (2) Timer expired (1 day, same as sampleRate), (3) cached data exceed threshold, and (4) system shutdown. For PowerStats, I would like to save power usage to database at two time points: (1) Query from an App. and (2) Timer expired (same as sampleRate). (System shutdown would trigger a save operation if we decide not to clear database after reboot.) Is that okay? 5. Query options NetworkStats allows query "wifi" or "mobile" network usage individually or return both. For PowerStats, do we need to support the query of the power usage of multiple components, such like “cpu + wifi” or “cpu + screen”? Or, we only accept the query for “the power consumption of individual component” and “the power consumption of all components”?

(In reply to bochen from comment #44) > Hi Jonas: > > Could you give some suggestions about the implementation of PowerStats API: > > 1. Sharing same database with NetworkStats > I saw some of our members are adding new features to NetworkStats API > recently. To avoid blocking their development, could we create a separate > database for PowerStats in current development and fire a new bug to discuss > how to merge these two databases? We should do whatever is simplest code-wise. I would expect that would be to put everything in the same database, but I don't know the code very well so maybe there are simpler ways. But the fact that someone else is also working on changes to the database is not a reason to avoid it. Just talk to them about your plans and needs. > 2. Timing of clear database > I found that systems like Android would clear its power usage record > after reboot or unplugged from USB port (changing from 'charging' state to > 'not charging' state). (My Android phone also stops counting the power > consumption during charging.) Do we need to implement a similar > functionality in PowerStats? I don't think we should. There already is an expiration policy in the current API. IIRC we only keep a limited number of data points. > 3. The length of SampleRate and the size of maxSamplePeriod > In current settings, the sampleRate and the maxStorageSamples of > NetworkStats are 1 day and 60 days. Do we need to follow these two settings? A sample rate of once per day sounds pretty low. Both for network stats and power stats. I guess technically we could have different sample rates for the two, though it does sound surprising to me that the two has different requirements. Is there a reason you want to use a different sample rate? > BTW, I think the naming of 'sampleRate' may cause a little confusion > since the unit of sampling rate is inverse of time but the NetworkStats API > returns time in seconds. Could we change the name 'sampleRate' to > 'samplePeriod'? Sounds good to me. > 4. The timing of saving power usage to database > NetworkStats saves network usage from cache to database at four time > points: (1) Query from an App., (2) Timer expired (1 day, same as > sampleRate), (3) cached data exceed threshold, and (4) system shutdown. > For PowerStats, I would like to save power usage to database at two time > points: (1) Query from an App. and (2) Timer expired (same as sampleRate). > (System shutdown would trigger a save operation if we decide not to clear > database after reboot.) Is that okay? I don't have an opinion on this. I thought the idea was that anytime we write data to disk, we would write all performance data. Seems silly to only write one of the two data types if we're doing the IO anyway. > 5. Query options > NetworkStats allows query "wifi" or "mobile" network usage individually > or return both. For PowerStats, do we need to support the query of the power > usage of multiple components, such like “cpu + wifi” or “cpu + screen”? Or, > we only accept the query for “the power consumption of individual component” > and “the power consumption of all components”? I think storing per component is enough and then allow querying per component as well as total. Whatever app displays the UI can always do multiple queries if needed.

To be clear, if you want to do a completely separate system from the network API, that's fine. It just seemed simpler and more efficient to me to have the two live in the same database. That way any time that we flush usage data to disk, we can store all metrics with a single write, rather than to write to multiple files independently. Additionally it seemed to me that the UX requirements for displaying this data would be the same for network and power stats. And so using the same policies for how fine-grained data to store, and how long to keep it would be the same. And it would also make it easier to create a UX which displays power and disk usage together. However if you think that the UX for power usage and network usage is going to be different, then definitely don't feel constrained to stick with what the network API looks like. Since this is a certified API anyway, it's really easy to change the API in the future if we decide that we want. So don't worry too much about creating a very long-term API. Instead we should optimize for efficient and clean code. But we should take into account both the API implementation as well as the app that uses the API.

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #45) > > 2. Timing of clear database > > I found that systems like Android would clear its power usage record > > after reboot or unplugged from USB port (changing from 'charging' state to > > 'not charging' state). (My Android phone also stops counting the power > > consumption during charging.) Do we need to implement a similar > > functionality in PowerStats? > > I don't think we should. There already is an expiration policy in the > current API. IIRC we only keep a limited number of data points. Here is my 2 cents. The meaning of network metering and power metering for user is different. Because of monthly data usage charges, user cares about the data of network metering. For data of power metering, user only cares about it when the power consumption is too high and user wants to check which app has the higher power consumption. And then user can delete that APP to extend the device usage time. If device is in charging mode, user won't care about this. This is why the data of power consumption should be reset after charging. Or user don't know which app uses most power. So it seems be better to have different timers of clearing database between network metering and power metering.

(In reply to Ken Chang from comment #47) ...snip... > For data of power metering, user only cares about it when the power > consumption is too high and user wants to check which app has the higher > power consumption. And then user can delete that APP to extend the device > usage time. If device is in charging mode, user won't care about this. This > is why the data of power consumption should be reset after charging. Or user > don't know which app uses most power. I don't think that power usage data should be reset after charging. You should be able to keep X days (or N entries) of usage data and since the phone gets charged every day (and every time you plug it into a PC to say transfer images), I don't think we should be clearing the data just becuase we did some charging.

Ultimately this is a UX question of course. So as usual, please do check with the UX team. However I agree with Dave. If I'm running low on batteries every day and one day decide to look into it, I would want to be able to look at historical data. Not just see why I'm low on batteries today. Likewise, if a user runs low on battery, I want that user to be able to go to a friend the next morning and show him/her the phone. The friend should then be able to look at why the user ran out of battery the day before, even if the user charged the phone over-night. We can certainly make it visible in the timeline when you last charged and make it easier to see what consumed battery since that last charge. But historical data also seems useful.

Hi Jonas: Could you give some comments about the webidl design? Thx a lot.

Hi Vincent: Could you give some comments about this patch? Thx.

(In reply to bochen from comment #52) > Created attachment 8336651 [details] [diff] [review] > Part 1: PowerStats API implementation > > Hi Vincent: > Could you give some comments about this patch? Thx. Hi bochen, how about the webidl part ? Is it ok for review right now ? If the answer is yes, I think we should review that part first.

Hi Vincent, the webidl part is also ready for review. Sorry that I didn't mention this. :P (In reply to Vincent Chang[:vchang] from comment #53) > (In reply to bochen from comment #52) > > Created attachment 8336651 [details] [diff] [review] > > Part 1: PowerStats API implementation > > > > Hi Vincent: > > Could you give some comments about this patch? Thx. > > Hi bochen, how about the webidl part ? Is it ok for review right now ? If > the answer is yes, I think we should review that part first.

In current patch (Attachment #828521 [details] [diff]), we store app's manifestURL in the database rather than storing appID because we want to track every power usage record even though some apps were deleted. I asked Paul today about the security concern of storing app's manifestURL and he replied me that it is fine to store manifestURL. But, Paul also mentioned that from a privacy standpoint, users may want to clear all related data when he/she delete one app. So, we concluded that we may need to support deleting the power usage record of a particular app on the API. The following change is what I propose to add on the webidl: -DOMRequest clearStats(); +DOMRequest clearStats(optional DOMString manifestURL); // claer all stats if manifestURL is null Hi Paul: Thanks for your suggestion today. Is there anything I forgot or misunderstood?

You have the requirement correct - you are essentially creating a history of the apps that have been used on the device. This has privacy implications - and a user should have some control over this. I think we need two controls provided to the user here: - a button(or some mechanism) in the UI to 'clear all power history data' - clear the power history for an app that has been removed from the phone. Sounds like you have the first one covered with clearStats() - do we need a seperate bug to create the Gaia change to allow the user to call this though? For the second part, I am not the best person to answer, but I think you want to integrate with the code that is already in WebApps.jsm (Fabrice can comment better than I can here). From what I can figure out though, webapps.jsm has a _clearPrivateData method [1]. I don't know the terminology but other modules are observers of the "webapps-clear-data" subject. For example DOMStorage handles this here [2]. So not sure how you want to do it, but my guess is that you should adhere to this pattern so that your API just automatically cleans up whenever an app is removed. [1] http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#3566 [2] http://mxr.mozilla.org/mozilla-central/source/dom/src/storage/DOMStorageObserver.cpp#251

Yes, Paul is correct, you should add an observer to the "webapps-clear-data" topic and handle the cleanup there.

Actually, listening for app uninstall seems better than webapps-clear-data. If the user goes to clear data for an app, it still seems useful to retain power usage. It shouldn't be that much data and it's always good for the user to be able to see what's sucking up his/her battery.

Hi Paul, Fabrice and Jonas: Thank you for all your comments. As Jonas said, we decided to retain the power statistics because we think that keeping that data can help users trace back the history of power consumption. But, I also agree Paul's and Fabrice's concern that users need to have some control over their privacy -- delete the power usage history. So, I wanna to propose two possible solutions here: =========== Solution 1: Add a new preference (e.g. "dom.mozPowerStats.clearAppData") here to control whether the fxos need to clear the power usage history automatically when an app is deleted, and let users have the ability to enable/disable this setting from Gaia. Here are the proposed modification: 1. Check pref setting whenever receives "webapps-clear-data" notifcation: (In PowerStatsService.jsm) observe: function observe(aSubject, aTopic, aData) { switch (aTopic) case "webapps-clear-data": if (Services.prefs.getBoolPref("dom.mozPowerStats.clearAppData")) { // clear the power usage of the deleted app } else { // do nothing but return } break; } 2. Extend clearStats() method to allow clear power usage of a particular app manually: (In PowerStatsManager.webidl) DOMRequest clearStats(optional DOMString manifestURL); =========== Solution 2: Prompt a dialog [1] to ask user decision whenever an app is deleted. Here are the proposed modification: 1. Prompt a dialog whenever receives "webapps-clear-data" notifcation: (In PowerStatsService.jsm) observe: function observe(aSubject, aTopic, aData) { switch (aTopic) case "webapps-clear-data": let prompt = new Prompt({ window: window, title: "Delete power usage?", buttons: ["Ok", "Cancel"] }).show(function(data) { if(data.button == 0) { // clear the power usage of the deleted app } else { // do nothing but return } }); break; } 2. We still need to modify clearStats() method to allow clear power usage manually: (In PowerStatsManager.webidl) DOMRequest clearStats(optional DOMString manifestURL); =========== Any suggestion about this? [1] https://developer.mozilla.org/en-US/Add-ons/Firefox_for_Android/API/Prompt.jsm

Oops, my fault. I found that I can't use Prompt.jsm directly because it is for add-ons. I'll try to find another way to prompt a dialog.

I can live with clearing that information only when we uninstall the app. I'm a bit afraid of the UX complexity if we offer that as an independent functionality.

I don't there is any need to prompt here, just clear the data for an app on app uninstall. So solution 1 gets my vote too. Jonas commented above that we should be clearing on uninstall rather than on the webapps-clear-data topic, but practically they are the same thing at the moment. If there is a notification that is sent on app uninstall maybe use that instead, but im not fussed either way.

There is such a notification, so please use that. Clearing old power metrics data for privacy reasons seems like a separate issue from this bug, so please file that separately.

Hi Jonas: This is the webidl part of PowerStats API, could you help review this patch? Thanks a lot. Update Note: 1. getStats(): query power stats from indexedDB. This function supports 4 different query methods: (1) query an app's power consumption of a specific component (2) query an app's total power consumption (sum of power consumption of all components) (3) query all apps' power consumption of a specific component (4) query all apps' total power consumption 2. clearStats(): clear power stats. This function supports clear all stats or the stats of a particular app. 3. supportComponents: return the name of components supporting power statistics. (In this patch, it returns null only and I will add component names in the other bugs) 4. samplePeriod: In this patch, we store stats to indexedDB once per hour. 5. maxStorageDays: in this patch, the power stats are kept in indexedDB 7 days. 6. I store power stats once per hour rather than once per day because I wanna to solve the timezone change problem -- if we store data once per day, when users change their location, it is hard to divide the power stats record according to users' current time zone. (Please see the attachment, which explains this problem in graph.) So, in this patch, I propose to use a smaller sample period (1 hour) and when users call getStats(), the FxOS would sum up 24 entries in indexedDB (i.e. records within 24 hrs) to one day according to users' current time zone. (In reply to Borting Chen [:borting] from comment #64) > Created attachment 8344487 [details] [diff] [review] > Part 0: Add a WebIDL for PowerStatsManager

(In reply to Borting Chen [:borting] from comment #67) > Created attachment 8344491 [details] [diff] [review] > Part 0: Add a WebIDL for PowerStatsManager Hi Jonas: This is the webidl part of PowerStats API, could you help review this patch? Thanks a lot. Update Note: 1. getStats(): query power stats from indexedDB. This function supports 4 different query methods: (1) query an app's power consumption of a specific component (2) query an app's total power consumption (sum of power consumption of all components) (3) query all apps' power consumption of a specific component (4) query all apps' total power consumption 2. clearStats(): clear power stats. This function supports clear all stats or the stats of a particular app. 3. supportComponents: return the name of components supporting power statistics. (In this patch, it returns null only and I will add component names in the other bugs) 4. samplePeriod: In this patch, we store stats to indexedDB once per hour. 5. maxStorageDays: in this patch, the power stats are kept in indexedDB 7 days. 6. I store power stats once per hour rather than once per day because I wanna to solve the timezone change problem -- if we store data once per day, when users change their location, it is hard to divide the power stats record according to users' current time zone. (Please see the attachment 8344490 [details], which explains this problem in graph.) So, in this patch, I propose to use a smaller sample period (1 hour) and when users call getStats(), the FxOS would sum up 24 entries in indexedDB (i.e. records within 24 hrs) to one day according to users' current time zone.

Hi Dave: This patch adds/modifies some notifications in ContenParent.cpp to support PowerStats API catch a signal when the content process is created or destroyed. Could you help review this patch? Thanks a lot. Update Note: 1. when sending out "ipc:content-shutdown" notification, I add app's manifestURL and pid in the subject. 2. I also created an "ipc:content-start" notification to notify PowerStats API the creation of content process. This notification also contains app's manifestURL and pid. 3. I send pid to PowerStats API because some apps (like browser) may execute several instances at the same time and I use pid to distinguish them.

Hi Jonas: This patch is the js implementation of PowerStats API, could you help review this patch? Thanks a lot. Update Notes: 1. The API is controlled by a preference "dom.mozPowerStats.enabled" and only certified apps can access this API. 2. we cache the power consumption record of each app in "powerInfo" object first, and then save them to indexedDB, when: (1) the timer expired (2) users call getStats() 3. When an app is createed, we give the powerInfo a new property to cache the power consumption and use app's pid to name that property. In addition, one app's power consumption may be cached by several properties of the powerInfo object, because: (1) one app may run several instance concurrently (such as browser) (2) user may create/destroy an app several times in the past one hour and each time the app would get different pid. 4. When PowerStatsService is initiated, we set up an one-shot timer to fire first event on the hour (i.e. at first XX:00:00) for calibration. After this timer expired, we store stats to DB and set up another perodic timer to fire an event every hour. 5. When users call getStats(), PowerStatsService will save all cahced power stats to indexedDB first and then perform a read from indexedDB. 6. The process steps of saveStats() are listed as following: (1) We merge the record cached in powerInfo object according to app's manifestURL first and a new object, "stats," is created. (2) In the past one hour, if users have called getStats(), there should already have some apps' power stats saved in the indexedDB. So, we update these entries first if there are records stored in the "stats" object. (We also remove these records from the "stats" object after we update them to indexedDB.) (2) Remove those entries which live in the indexedDB more than maxStorageDays (i.e. 7 days). This step is exucted only when the saveStats() is triggered by timer expiration. (3) Save the rest record stored in the "stats" object to indexedDB.

Hi Jonas, Paul abd Fabrice: I've created a following bug Bug 947779 to discuss the privacy issues for PowerStats API. Please leave your suggestions on that bug if you get any new idea on the privacy design. Thanks a lot.

Comment on attachment 8344491 [details] [diff] [review] Part 0: Add a WebIDL for PowerStatsManager Review of attachment 8344491 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/webidl/PowerStats.webidl @@ +5,5 @@ > + > +[JSImplementation="@mozilla.org/powerStatsData;1", Pref="dom.mozPowerStats.enabled"] > +interface MozPowerStatsData > +{ > + readonly attribute unsigned long power; // power consumption What are the units? And are the power stats persistent? So if my phone reboots several times in a day, will the stats be cumulative? or since the last reboot?

(In reply to Dave Hylands [:dhylands] from comment #72) > What are the units? > > And are the power stats persistent? So if my phone reboots several times in > a day, will the stats be cumulative? or since the last reboot? The units should be mW. (In current design, we refer the equations presented in [1] to calculate power consumption of each component) For the 2nd question, the answer is yes. The power stats would be stored in the DB for 7 days unless you call the clearStats() function. [1] http://robertdick.org/publications/zhang10oct.pdf

Comment on attachment 8344493 [details] [diff] [review] Bug-854200-part1-ContentParentNotification.patch Review of attachment 8344493 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +687,5 @@ > tp->SetOwnerElement(aFrameElement); > uint32_t chromeFlags = 0; > > + // get manifestURL > + nsCOMPtr<mozIApplication> BrowserApp = aContext.GetBrowserOwnerApp(); nit: variable name BrowserApp should start with lower case letter. @@ +693,5 @@ > + BrowserApp->GetManifestURL(manifestURL); > + > + // Send manifestURL and pid of browser to PowerStatsService > + props->SetPropertyAsInt32(NS_LITERAL_STRING("pid"), cp->Pid()); > + props->SetPropertyAsAString(NS_LITERAL_STRING("AppManifestURL"), manifestURL); I noticed that ipc:content-shutdown also sends an mChildID. Should this notification also include mChildID for consitency? I also think it might be better to use mChildID rather than PID if all you're looking for is a way to uniquely identify the child. I'm not sure of any security concerns about using pids, but it seems like pid might breakdown in certain circumstances (like when an app runs in-process for some reason). Also, it's conceivable that 2 different apps (launched at 2 different times) could have the same pid (since pids are recycled and are only 16-bits). mChildID will be unique across a given instance of the b2g parent. @@ +1295,5 @@ > + // This part is for per app power metering. > + // browserInfo contains pid to let PowerStatsService know when > + // tab is closing. > + nsAutoString browserInfo; > + browserInfo.AppendLiteral("app://browser.gaiamobile.org/manifest.webapp"); Is gaiamobile.org right? It's what we use, but wouldn't the URL be different on released phones?

Comment on attachment 8344491 [details] [diff] [review] Part 0: Add a WebIDL for PowerStatsManager Review of attachment 8344491 [details] [diff] [review]: ----------------------------------------------------------------- Ehsan, can you take a look at the API here.

Comment on attachment 8344491 [details] [diff] [review] Part 0: Add a WebIDL for PowerStatsManager Review of attachment 8344491 [details] [diff] [review]: ----------------------------------------------------------------- I think that we should reconcile this API with the network stats API, and unify them into a single resource stats API. I strongly suggest looking into unifying the database for both types of information, and also gathering the stats and writing them to the database at the same type as we do for network stats. Unfortunately, bug 887699 changed the network stats API in a way that it now looks very different than what was previously proposed on this bug. The basic reason for that change is to support multiple sim cards. For the purposes of the resource stats API, I think we should first try to abstract away the current network stats API into a resource stats API which only supports network information, and then work on adding support for power consumption information to that API in a separate patch. (Please feel free to file another bug for moving the network stats API over to the resource stats API.) Speaking about the API, I think the basics of what you have here is solid. For the network information, you can use "network.wifi" to designate wifi and "network.<iccid>" for cellular data consumption for <iccid>. I have some other comments in the patch. ::: dom/webidl/PowerStats.webidl @@ +15,5 @@ > +{ > + /** > + * Manifest URL of the application. > + */ > + readonly attribute DOMString manifestURL; This should be nullable, right? ::: dom/webidl/PowerStatsManager.webidl @@ +7,5 @@ > +dictionary PowerStatsOptions > +{ > + /** > + * componet specifies which component's power statistics will be returned. > + * If null, all components' power statistics are returned. Please document the possible values for component. @@ +15,5 @@ > + */ > + DOMString component; > + DOMString manifestURL; > + Date start; > + Date end; Please replace the usages of Date with an integer, see <https://www.w3.org/Bugs/Public/show_bug.cgi?id=22824>. @@ +30,5 @@ > + * If options.manifestURL is null, return power statistics of all apps. > + * > + * If success, the request result will be an array of MozPowerStats object. > + */ > + DOMRequest getStats(optional PowerStatsOptions options); Can you use promises in this API? @@ +40,5 @@ > + > + /** > + * Return the name of components supporting power statistics. > + */ > + readonly attribute any supportComponents; // return an array of DOMString Nit: supportedComponents @@ +50,5 @@ > + > + /** > + * Maximum number of days that the API keeps power statistics in database. > + */ > + readonly attribute unsigned long maxStorageDays; Note that this attribute has been renamed to maxStorageAge and is now returning the age in milliseconds.

Comment on attachment 8344494 [details] [diff] [review] Part 2: PowerStats API implementation Clearing this request for now, as the API and the implementation will change.

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #76) > I think that we should reconcile this API with the network stats API, and > unify them into a single resource stats API. We just hold a internal meeting to discuss about how to integrate these two APIs into a unified one, which is going to be a big change compared to what we currently have. We'll post our proposal on the WebAPI mailing list when we think we're ready.

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #78) > (In reply to :Ehsan Akhgari (needinfo? me!) from comment #76) > > I think that we should reconcile this API with the network stats API, and > > unify them into a single resource stats API. > > We just hold a internal meeting to discuss about how to integrate these two > APIs into a unified one, which is going to be a big change compared to what > we currently have. We'll post our proposal on the WebAPI mailing list when > we think we're ready. Sounds great! Thanks, and sorry for being the nay-sayer here. :-)

Comment on attachment 8344493 [details] [diff] [review] Bug-854200-part1-ContentParentNotification.patch Review of attachment 8344493 [details] [diff] [review]: ----------------------------------------------------------------- I noticed that I forgot to clear the review flag. I can't really give you an r+ since I'm not a DOM peer. You may want to ask bent who might be appropriate for a review.

Please let's wait on further code review here until we agree on the API. Thanks!

I've created a new bug for discussing the new API. Please visit Bug #951976 and leave your comments. Thanks a lot.

Hi Kyle: This patch updates contenParent to send a notification to the b2g process when an app is launched or terminated. This notification contains the manifestURL and pid in order to track the resource consumption of that application (e.g. # of CPU ticks that an app consumed). Could you help review this patch? Thanks. Update Note: 1. Because we want to record the stats of a running app, we send a notification ("ipc:content-loaded") to the b2g process when an app is created. This notification contains the manifestURL and the pid of the launched app. 2. When an app is terminated, the b2g process also needs to obtain the manifestURL and the pid of that app. So, we add manifestURL and pid to "ipc:content-shutdown" notification and observe this notification in the b2g process.

Does that work when there are multiple apps per process? (like we do on tarako, or with the callscreen app on all devices).

Hi gene: This patch creates a statsCache in ResourceStatsService.jsm to store application's stats temporarily. Could tou help review this patch? Thanks. Update Note: 1. Create a statsCache object which stores stats temporarily. In statsCache, each property (key) stores the stats of an App, and the name of the property is composed by "appId,serviceType". 2. Capture notification when an App is launched or terminated. When an app is launched, create a property in statsCache if necessary, and log the pid of the running app in stats[key].runningList. When an app is terminated, delete the pid of the app from stats[key].runningList.

(In reply to Fabrice Desré [:fabrice] from comment #92) > Does that work when there are multiple apps per process? (like we do on > tarako, or with the callscreen app on all devices). No, it can't. Do you have any suggestion to catch the event when an app is loaded in the same process? Thanks.

(In reply to Borting Chen [:borting] from comment #94) > (In reply to Fabrice Desré [:fabrice] from comment #92) > > Does that work when there are multiple apps per process? (like we do on > > tarako, or with the callscreen app on all devices). > > No, it can't. Do you have any suggestion to catch the event when an app is > loaded in the same process? Thanks. Each app get it's own TabParent. Can you use that?

Comment on attachment 8446981 [details] [diff] [review] Bug-854200-part0-ContentParentNotification_v3.patch Review of attachment 8446981 [details] [diff] [review]: ----------------------------------------------------------------- Disable review flag according to Fabrice's suggestion.

Comment on attachment 8446983 [details] [diff] [review] Bug-854200-part1-StatsCache_v1.patch Review of attachment 8446983 [details] [diff] [review]: ----------------------------------------------------------------- Disable review flag according to Fabrice's suggestion.

Hi Fabrice: I am still confused about your suggestion -- using TabParent to get the time that the callscreen app is launched. Here is my understanding about how the callscreen app works: 1. The callscreen app is preloaded in an iframe of the system app. 2. When we get a phone call, the system app will load the iframe instead of calling the CreateBrowserOrApp() function to launch the callscreen app. So, here is my question? 1. If we do not execute the CreateBrowserOrApp() function, how to get the appId from TabParent (or TabContext)? 2. If so, can we count the cost (energy consumption caused by CPU and LCD) of the callscreen app as a part of the cost of the system app? For the Tarako case (multiple apps per process), I think it is hard to distinguish which app is running in a process because the process will execute the tasks of different apps in an interleaved manner. So, maybe we can consider another way to count the CPU power consumption in the Tarako phone?

Hi Borting - sorry I didn't realize you were working on power usage. Indeed that will be hard to know which app in a process is consuming or cpu or not. So that's probably fine to just ignore the multiple apps per process situation for now.

Comment on attachment 8446981 [details] [diff] [review] Bug-854200-part0-ContentParentNotification_v3.patch Review of attachment 8446981 [details] [diff] [review]: ----------------------------------------------------------------- Hi Kyle: This patch updates contenParent to send a notification to the b2g process when an app is launched or terminated. This notification contains the manifestURL and pid in order to track the resource consumption of that application (e.g. # of CPU ticks that an app consumed). Could you help review this patch? Thanks. Update Note: 1. Because we want to record the stats of a running app, we send a notification ("ipc:content-loaded") to the b2g process when an app is launched. This notification contains the manifestURL and the pid of the launched app. 2. When an app is terminated, the b2g process also needs to obtain the manifestURL and the pid of that app. So, we add manifestURL and pid to "ipc:content-shutdown" notification and observe this notification in the b2g process.

Comment on attachment 8446983 [details] [diff] [review] Bug-854200-part1-StatsCache_v1.patch Review of attachment 8446983 [details] [diff] [review]: ----------------------------------------------------------------- Hi gene: This patch creates a statsCache in ResourceStatsService.jsm to store application's stats temporarily. Could you help review this patch? Thanks. Update Note: 1. Create a statsCache object which stores stats temporarily. In statsCache, each property (key) stores the power/network stats of an App, and the name of the property is in the form of "appId,serviceType". 2. Capture notification when an App is launched or terminated. When an app is launched, create a property in statsCache if necessary, and log the pid of the running app in stats[key].runningList. When an app is terminated, delete the pid of the app from stats[key].runningList.

Hi gene: This patch sets up a timer to sync cached stats (stored in statsCache) to DB once per day. Update Note: 1. Create a timer which sync statsCache to DB at 00:00 (local time) everyday. When the system is started/restarted, we set up a timer first to fire an event at 00:00 local time in order to do caliberation. After that, a periodic timer (expired once/day) is set. 2. When the timer is expired, _syncStatsCacheToDB() is called to do synchronization. This work only saves stats that have been updated since last sync (by checking the stats[key].isSync) to DB. 3. If the sync process is triggered by an timer, after the sync, all stats stored in statsCache are reset to 0. If an app has been terminated, this app is removed from statsCache.

Hi gene: This patch retrieves today's stats from DB when we start/restart phone. Here explains why we doing so: -- Before we restart phone, the b2g process syncs stats (st1) to DB. -- After phone is restared, we run some apps, which generates some new stats record (st2). -- The problem happens when the timer is expired and the b2g process tries to sync st2 to DB. This will overrite old stats (st1) saved in DB. So, we retrieves today's stats from DB when system start/restart, and stores them in statsCache. The retriving steps are: -- Calculate the normalized timestamp (Ts) that will be used when performing next sync -- Retrieve stats in DB which timestamp is equal to Ts -- Store the retrieved stats to statsCache.

Hi gene: This patch implements a requestQueue to avoid statsCache be updated simultaneously. To avoid race condition like Bug 964228, we create a requestQueue which helps to process the requests that access statsCache in a sequence. Here expalains how I use Promise to implement requestQueue: 1. A request is composed of a serious of operations. In each request, at least one operation will access the statsCache. For example, calling getStats() is a request which contains two operations: calling _syncStatsCacheToDB(), and calling _db.getStats(). The first operation will access the statsCache. 2. We ceates a requestQueue to save requests to be processed. The sequence in the requestQueue also indicates the processing order of the requests. 3. To process requests in sequence, each request is assigned with a promise. When a new request (newReq) is pushed to the requestQueue, we first find the request previous to the new request (prevReq). Then, we set the first operation of newReq be executed when the promise of prevReq is resolved. This makes sure that one request would trigger the process of next request. (These are implemeneted in _pushRequestToQueue() function). 4. When processing operations of a request, we also use promise to guarantee the operations are executed in sequence. Executing first operation would return a promise, and the following operations are triggered by creating a promise chain, such as: let promise = op1(); promise.then(op2).then(op3).then(...); The last resolve function and reject function assigned to the promise chain would resolve the promise assigned to this request, which triggers the process of next request. (These are implemeneted in _createExecRequest() function). This patch also updates some functions because these functions would access the statsCache: 1. _db.getLatestRecord() would return a promise. This function is also the first request pushed to requestQueue. 2. _syncStatsCacheToDB() would return a promise. Calling _syncStatsCacheToDB() should push a request to the requestQueue. 3. Calling _appLaunched() and _appTerminated() should also push a request to the requestQueue.

Hi gene: This patch modifies the process of getStats(), clearStats() and clearAllStats(). When calling these functions, we first sync cached stats to DB first, then do get/clean operations. Calling these functions also push a request to the requestQueue.

Hi gene: This patch updates xpcshell testcase to test _db.getLatestRecord(). And, here is the try result: https://tbpl.mozilla.org/?tree=Try&rev=265e838865e9

Comment on attachment 8446983 [details] [diff] [review] Bug-854200-part1-StatsCache_v1.patch Review of attachment 8446983 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/resourcestats/ResourceStatsService.jsm @@ +39,5 @@ > + // Create indexedDB. > + this._db = new ResourceStatsDB(); > + > + // Create a statsCache to store raw stats data. > + this.statsCache = Object.create(null); Just do |this.statsCache = {};|. Besides, please add a declaration for this new member and document it. I have to go through the file to know what exactly is in a _statsCache. That's not a good sign. BTW, I suppose this is also a private member, so by the convention of this file, its name has to be prefixed by '_'. this.ResourceStatsService = { /** * <What's a _statsCache exactly> */ _statsCache: null, ... @@ +137,5 @@ > + return result; > + }, > + > + // Add the launched app to statsCache. > + _appLaunched: function(aManifestURL, aPid) { nit: you'd either name it '_onAppLaunched' or '_addToLaunchedApp'. The original name is kind of confusing and doesn't reveal any bit that it does. @@ +150,5 @@ > + this.statsCache[key] = { > + runningList: [], // Record the pids of the running App. > + isSync: true, // Flag indicates whether stats has been synced to DB. > + network: Object.create(null), // network stats. > + power: Object.create(null) // power stats. nit: use object literal for these simple cases. @@ +169,5 @@ > + // Delete aPid from the running list > + let key = this._appIdService2Key(appId, ""); > + let stats = this.statsCache[key]; > + let index = stats.runningList.indexOf[aPid]; > + if (!index) { Don't quite understand what does this mean. Do you mean |if (index != -1)|?

Comment on attachment 8457712 [details] [diff] [review] Bug-854200-part2-SyncStatsToDB_v1.patch Review of attachment 8457712 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/resourcestats/ResourceStatsService.jsm @@ +44,5 @@ > > + // Calculate the timestamp which would be recorded when we sync stats to DB. > + let date = new Date(); > + let offset = date.getTimezoneOffset() * 60 * 1000; > + this.syncTimestamp = this._db._normalizeTime(date.getTime(), offset); As you've seen here, ResourceStatsDB::_normalizeTime has a name prefixed by underscore, which means it's a private member function. So if you really need it, you have to rename it to strip that underscore first. @@ +95,5 @@ > + let currentTime = date.getTime(); > + let nextFireTime = this.syncTimestamp + this._db.sampleRate + offset; > + > + this.timer.initWithCallback(oneShotUpdateEvent, nextFireTime - currentTime, > + Ci.nsITimer.TYPE_ONE_SHOT); So we have: currentTime = date.getTime() this.syncTimestamp = this._db._normalizeTime(date.getTime(), offset) = ((currentTime - offset) / SAMPLE_RATE) * SAMPLE_RATE ~= currentTime - offset; nextFireTime = this.syncTimestamp + SAMPLE_RATE + offset ~= currentTime - offset + SAMPLE_RATE + offset = currentTime + SAMPLE_RATE; nextFireTime - currentTime ~= SAMPLE_RATE; Basically you're initializing a timer with delay equals to SAMPLE_RATE with TYPE_ONE_SHOT policy. And in the timer callback, you initialize a periodically timer event with exactly the same period. So I wonder why you can't simply have: this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); this.timer.initWithCallback(this, SAMPLE_RATE, Ci.nsITimer.TYPE_REPEATING_PRECISE_CAN_SKIP); And when does this timer stop? @@ +234,5 @@ > + // 3. If this function is triggered by a timer, clean the statsCache. > + let self = this; > + this._db.saveNetworkStats(network, this.syncTimestamp, > + function(aError, aResult) { > + self._db.savePowerStats(power, self.syncTimestamp, You may run the two save method in parallel by: let toBeDone = ["network", "power"]; function done(aWhich) { toBeDone.splice(toBeDone.indexOf(aWhich), 1); if (toBeDone.length) { return; } ... } this._db.saveNetworkStats(network, this.syncTimestamp, done.bind(this, "network")); this._db.savePowerStats(power, this.syncTimestamp, done.bind(this, "power")); In this way, it can be easily scaled to as many components as you have. @@ +270,5 @@ > + } > + > + // Sync cached stats to DB. > + this._syncStatsCacheToDB(true); > + }, nit: alignment

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #108) > Review of attachment 8457712 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/resourcestats/ResourceStatsService.jsm > @@ +44,5 @@ > > > > + // Calculate the timestamp which would be recorded when we sync stats to DB. > > + let date = new Date(); > > + let offset = date.getTimezoneOffset() * 60 * 1000; And I wonder why you need any thing about time zones? From MDN [1] we have: The value returned by the getTime method is the number of milliseconds since 1 January 1970 00:00:00 UTC. The comments right above ResourceStatsDB::_normalizeTime() have: // Convert to UTC according to the current timezone and the filter timestamp // to get SAMPLE_RATE precission. That's completely wrong. You don't need any UTC conversion at all. Besides, in the function body we have: let time = Math.floor((aTime - aOffset) / SAMPLE_RATE) * SAMPLE_RATE; No! There can be more or less than 86400 seconds in one day [2]. You cannot round up a day with such simple math. Under some boundary conditions, this may result in either record overwritten or missing data. [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getTime [2]: http://en.wikipedia.org/wiki/Day

Comment on attachment 8457724 [details] [diff] [review] Bug-854200-part3-GetLatestStatsRecord_v1.patch Review of attachment 8457724 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/resourcestats/ResourceStatsDB.jsm @@ +119,5 @@ > + // Create Stores. > + this._upgradeToVer1(aDb); > + if (++nextVersion > aNewVersion) { > + break; > + } So we can image every case we'll have, except that last one, will need such check. Not a good idea. Besides, we had a problem in SMS database before: next schema upgrade dispatched before all operations inside current upgrade function complete. The solution was to pass a callback function as an additional argument to the upgrade functions. This also follows the arguments of all upgrade functions should be better unified. Something like: _upgradeToVersionN: function(aTransaction, aNextStep) { ... aNextStep(); } upgradeSchema: function(aTransaction, aDb, aOldVersion, aNewVersion) { (function step(aNextVersion) { if (aNextVersion > aNewVersion) { // Done. return; } let func = this["_upgradeToVersion" + aNextVersion]; if (!func) { // Give some error here. return; } func.call(this, aTransaction, step.bind(this, aNextVersion + 1)); }).call(this, 1); } @@ +145,5 @@ > + let range = IDBKeyRange.only(aTimestamp); > + let request = aStore.index("timestamp").openCursor(range); > + request.onsuccess = function onsuccess(aEvent) { > + let cursor = aEvent.target.result; > + if (cursor) { nit: please bail-out early. @@ +151,5 @@ > + > + // Ignore sum-up record: > + // (1) the system resource usage (appId = 0 & serviceType = ""), > + // (2) the total resource consumption of a component (component = "") > + if ((!value.appId && !value.serviceType) || !value.component) { Please compare to nsIScriptSecurityManager::NO_APP_ID instead. @@ +163,5 @@ > + aStatsCache[key] = { > + runningList: [], > + isSync: true, > + network: Object.create(null), > + power: Object.create(null) nit: object literal. @@ +168,5 @@ > + }; > + } > + > + // Save record to aStatsCache[key]. > + aStatsCache[key].network[value.component] = { This function named '_getLatestPowerRecord' but the items are actually saved in |aStatsCache[key].network|. Is there something wrong? @@ +169,5 @@ > + } > + > + // Save record to aStatsCache[key]. > + aStatsCache[key].network[value.component] = { > + consumedPower: value.consumedPower The attribute name 'consumedPower' just doesn't appear in any previous patches. That's really confusing. I don't even know whether it's a valid name or not. @@ +185,5 @@ > + let range = IDBKeyRange.only(aTimestamp); > + let request = aStore.index("timestamp").openCursor(range); > + request.onsuccess = function onsuccess(aEvent) { > + let cursor = aEvent.target.result; > + if (cursor) { nit: bail-out early. @@ +200,5 @@ > + // Note that we do not validate aStatsCache[key] again because it has > + // already checked in _getLatestPowerRecord(). > + // Save record to aStatsCache[key]. > + let key = value.appId + "," + value.serviceType; > + aStatsCache[key].power[value.component] = { ditto. A function named '_getLatestNetworkRecord' actually stores records in |aStatsCache[key].power|. @@ +218,5 @@ > + getLatestRecord: function(aTimestamp, aStatsCache) { > + // Create a dummy callback function. > + let resultCb = function(aError, aResult) { > + return; > + }; Let _dbNewTxn accept an optional aTxnCb instead.

Comment on attachment 8457732 [details] [diff] [review] Bug-854200-part4-RequestQueue_v1.patch Review of attachment 8457732 [details] [diff] [review]: ----------------------------------------------------------------- The purpose here is to bind two or more db involved operations into an integral one. However, you don't really have to create your own algorithm especially when you have already imported Promise. A Promise object allows a later call to its |then| method even after it has been resolved or rejected previously. So here is what we should ever do: ======= ResourceStatsDB ======== // 1. Let |getLatestRecord|, |saveNetworkStats| and // |savePowerStats| returns a Promise as you've done. ======= ResourceStatsService ======== // 1. Create an Promise in init() or somewhere appropriate // and keep the last Promise object. this._lastPromise = this._db.getLatestRecord(); // 2. In |_syncStatsCacheToDB| you may have: let promises = []; promises.push(this._db.saveNetworkStats(...) .then(clearNetworkStatCaches)); promises.push(this._db.savePowerStats(...) .then(clearPowerStatCaches)); return Promise.all(promises); // 3. In the timer |notify| function: this._lastPromise = this._lastPromise.then(function() { return this._syncStatsCacheToDB(); }); // 4. In |_onAppLaunched| or |_onAppTerminated|: this._lastPromise = this._lastPromise.then(function() { // Update this._statCaches. }); To prevent misuse and to hide such details, you may also want to create a helper method that accepts only an function: _queueOperation: function(aFunc) { if (typeof aFunc !== 'function') { throw sorry_that_is_not_a_function; } this._lastPromise = this._lastPromise.then(aFunc); },

Comment on attachment 8457739 [details] [diff] [review] Bug-854200-part5-SyncBeforeWrite_v1.patch Review of attachment 8457739 [details] [diff] [review]: ----------------------------------------------------------------- let self = this; this._queueOpertion(function() { return self._syncStatsCacheToDB() .then(self.db.getStats.bind(self.db, ...)); });

Comment on attachment 8457745 [details] [diff] [review] Bug-854200-part6-TestCase_v1.patch Cancel review until we have a more viable solution here.

Comment on attachment 8446981 [details] [diff] [review] Bug-854200-part0-ContentParentNotification_v3.patch Looks like we have to resolve some other issues first.

Comment on attachment 8446983 [details] [diff] [review] Bug-854200-part1-StatsCache_v1.patch Review of attachment 8446983 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/resourcestats/ResourceStatsService.jsm @@ +39,5 @@ > + // Create indexedDB. > + this._db = new ResourceStatsDB(); > + > + // Create a statsCache to store raw stats data. > + this.statsCache = Object.create(null); Agree with Vicamo. Object literal is preferable to Object constructor. @@ +137,5 @@ > + return result; > + }, > + > + // Add the launched app to statsCache. > + _appLaunched: function(aManifestURL, aPid) { I guess this function adds a launched app into the statsCache. Would the suggested name '_addToLaunchedApp' be better as '_addLaunchedAppToStatsCache'? If this name is too long, maybe '_addAppToCache'? @@ +169,5 @@ > + // Delete aPid from the running list > + let key = this._appIdService2Key(appId, ""); > + let stats = this.statsCache[key]; > + let index = stats.runningList.indexOf[aPid]; > + if (!index) { Following the comment above, this function should also be renamed, such as '_onAppTerminated' or '_removeAppFromCache'.

Comment on attachment 8457712 [details] [diff] [review] Bug-854200-part2-SyncStatsToDB_v1.patch Review of attachment 8457712 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/resourcestats/ResourceStatsService.jsm @@ +71,5 @@ > + // Set up a timer for periodic stats update. > + // This timer would be expired at 00:00:00 (local time). > + this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > + > + // In order to do caliberation, we set an one_shot timer fired at 00:00:00 nit: typo: "caliberation" should be "calibration".

Thanks for the comments, the nits are fixed in attachment 8461378 [details] [diff] [review]. (In reply to Vicamo Yang [:vicamo][:vyang] from comment #107) > Comment on attachment 8446983 [details] [diff] [review] > ::: dom/resourcestats/ResourceStatsService.jsm > @@ +39,5 @@ > > + // Create indexedDB. > > + this._db = new ResourceStatsDB(); > > + > > + // Create a statsCache to store raw stats data. > > + this.statsCache = Object.create(null); > > Just do |this.statsCache = {};|. > > Besides, please add a declaration for this new member and document it. I > have to go through the file to know what exactly is in a _statsCache. That's > not a good sign. > > BTW, I suppose this is also a private member, so by the convention of this > file, its name has to be prefixed by '_'. > > this.ResourceStatsService = { > /** > * <What's a _statsCache exactly> > */ > _statsCache: null, > > ... 1. Created _statsCache as a private member as suggested above. 2. Documented the usage of _statsCache. > @@ +137,5 @@ > > + return result; > > + }, > > + > > + // Add the launched app to statsCache. > > + _appLaunched: function(aManifestURL, aPid) { > > nit: you'd either name it '_onAppLaunched' or '_addToLaunchedApp'. The > original name is kind of confusing and doesn't reveal any bit that it does. Per Ethan's suggestion in comment #115, rename: * _appLaunched to _addAppToCache * _appTerminated to _removeAppFromCache > @@ +150,5 @@ > > + this.statsCache[key] = { > > + runningList: [], // Record the pids of the running App. > > + isSync: true, // Flag indicates whether stats has been synced to DB. > > + network: Object.create(null), // network stats. > > + power: Object.create(null) // power stats. > > nit: use object literal for these simple cases. Fixed. > @@ +169,5 @@ > > + // Delete aPid from the running list > > + let key = this._appIdService2Key(appId, ""); > > + let stats = this.statsCache[key]; > > + let index = stats.runningList.indexOf[aPid]; > > + if (!index) { > > Don't quite understand what does this mean. Do you mean |if (index != -1)|? You are right, the |index| should be compared with -1. Fixed.

The fixes are updated in attachment #8461424 [details] [diff] [review]. (In reply to Vicamo Yang [:vicamo][:vyang] from comment #108) > Comment on attachment 8457712 [details] [diff] [review] > Bug-854200-part2-SyncStatsToDB_v1.patch > > Review of attachment 8457712 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/resourcestats/ResourceStatsService.jsm > @@ +44,5 @@ > > > > + // Calculate the timestamp which would be recorded when we sync stats to DB. > > + let date = new Date(); > > + let offset = date.getTimezoneOffset() * 60 * 1000; > > + this.syncTimestamp = this._db._normalizeTime(date.getTime(), offset); > > As you've seen here, ResourceStatsDB::_normalizeTime has a name prefixed by > underscore, which means it's a private member function. So if you really > need it, you have to rename it to strip that underscore first. Fixed. In the new patch, _db._normalizeTime() will never be called. Instead, do normalization calculation in init() function. > @@ +95,5 @@ > > + let currentTime = date.getTime(); > > + let nextFireTime = this.syncTimestamp + this._db.sampleRate + offset; > > + > > + this.timer.initWithCallback(oneShotUpdateEvent, nextFireTime - currentTime, > > + Ci.nsITimer.TYPE_ONE_SHOT); > > So we have: > > currentTime = date.getTime() > this.syncTimestamp = this._db._normalizeTime(date.getTime(), offset) > = ((currentTime - offset) / SAMPLE_RATE) * SAMPLE_RATE > ~= currentTime - offset; > nextFireTime = this.syncTimestamp + SAMPLE_RATE + offset > ~= currentTime - offset + SAMPLE_RATE + offset > = currentTime + SAMPLE_RATE; > nextFireTime - currentTime ~= SAMPLE_RATE; > > Basically you're initializing a timer with delay equals to SAMPLE_RATE with > TYPE_ONE_SHOT policy. And in the timer callback, you initialize a > periodically timer event with exactly the same period. So I wonder why you > can't simply have: > > this.timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); > this.timer.initWithCallback(this, SAMPLE_RATE, > Ci.nsITimer.TYPE_REPEATING_PRECISE_CAN_SKIP); > > And when does this timer stop? After having an internal discussion with Vicamo & Ethan, we will come out a new method to calculate the timestamp which is saved to DB. So, this will be fixed in a future patch. > @@ +234,5 @@ > > + // 3. If this function is triggered by a timer, clean the statsCache. > > + let self = this; > > + this._db.saveNetworkStats(network, this.syncTimestamp, > > + function(aError, aResult) { > > + self._db.savePowerStats(power, self.syncTimestamp, > > You may run the two save method in parallel by: > > let toBeDone = ["network", "power"]; > function done(aWhich) { > toBeDone.splice(toBeDone.indexOf(aWhich), 1); > if (toBeDone.length) { > return; > } > > ... > } > > this._db.saveNetworkStats(network, this.syncTimestamp, > done.bind(this, "network")); > this._db.savePowerStats(power, this.syncTimestamp, > done.bind(this, "power")); > > In this way, it can be easily scaled to as many components as you have. Thanks for the suggestion. The new patch re-implement this function as suggested above. > @@ +270,5 @@ > > + } > > + > > + // Sync cached stats to DB. > > + this._syncStatsCacheToDB(true); > > + }, > > nit: alignment fixed.

triage: feature should not block.

Firefox OS is not being worked on