Closed Bug 854202 Opened 11 years ago Closed 6 years ago

[Per app power metering] - DOM APIs to obtain system metrics and power profile

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: vchang, Assigned: bochen)

References

Details

Attachments

(2 files, 12 obsolete files)

      No description provided.
Blocks: 854200
Attached patch readProfile (obsolete) — Splinter Review
Provide power profile value to gaia.
Attachment #771270 - Flags: feedback?(vchang)
Attached file PowerProfile sample file (obsolete) —
we take this sample file as a sample file.
push this file to /system/etc/power_profile.xml
Attached patch readProfile (obsolete) — Splinter Review
merge conflict
Attachment #771270 - Attachment is obsolete: true
Attachment #771270 - Flags: feedback?(vchang)
Attachment #773040 - Flags: feedback?(vchang)
Attached patch readProfile (obsolete) — Splinter Review
Attachment #773040 - Attachment is obsolete: true
Attachment #773040 - Flags: feedback?(vchang)
Attachment #778393 - Flags: feedback?(vchang)
Comment on attachment 778393 [details] [diff] [review]
readProfile

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

::: dom/power/PowerStatsManager.js
@@ +75,5 @@
> +    this.checkPrivileges();
> +    let request = this.createRequest();
> +    if (DEBUG) {
> +      debug("readProfile: " + item);
> +    }

Nit: I think we don't need if(DEBUG) anymore.

::: dom/power/PowerStatsService.jsm
@@ +301,5 @@
> +    let item = msg.data;
> +    let profilePath = POWER_PROFILE_PATH;
> +    if (DEBUG) {
> +      debug("readProfile: " + item);
> +    }

Nit: Remove if(DEBUG)

@@ +315,5 @@
> +    let found  = 0;
> +    let result = [];
> +    let error  = null;
> +
> +    for (let i=0;i<target.length;i++) {

Nit: space between "=", "<" and after ";"

@@ +316,5 @@
> +    let result = [];
> +    let error  = null;
> +
> +    for (let i=0;i<target.length;i++) {
> +      if(target[i].getAttribute('name') === item) {

Nit: space after if

@@ +323,5 @@
> +      }
> +    }
> +
> +    target = powerProfile.getElementsByTagName("array");
> +    for (let i=0;i<target.length;i++) {

Nit: space between "=", "<" and after ";"

@@ +326,5 @@
> +    target = powerProfile.getElementsByTagName("array");
> +    for (let i=0;i<target.length;i++) {
> +      if(target[i].getAttribute('name') === item) {
> +        found = 1;
> +        for (let j = 0;j < Math.floor(target[i].childNodes.length/2);j++) {

Nit: space after ";"
Attachment #778393 - Flags: feedback?(vchang)
Attached patch readProfile (obsolete) — Splinter Review
Thanks for your feedback. This is the improved patch.
Attachment #778393 - Attachment is obsolete: true
Attachment #780196 - Flags: feedback?(vchang)
Attached file PowerProfile sample file (obsolete) —
We take this sample file as a sample file.
Push this file to /system/etc/power_profile.xml
Attachment #771275 - Attachment is obsolete: true
Comment on attachment 780196 [details] [diff] [review]
readProfile

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

This patch looks good. Thank you.

::: dom/power/nsIDOMPowerStatsManager.idl
@@ +34,5 @@
>     */
>    nsIDOMDOMRequest clearAllData();
> +
> +  /**
> +   * Read the average power consumption of item in power profile.

What is the item ? Should it be hardware components ?
Attachment #780196 - Flags: feedback?(vchang) → feedback+
Attached patch readPowerProfile (obsolete) — Splinter Review
We found that the power profile is quite sensitive information to vendor. So, we do not create a DOM API.

The values in power profile is average current consumption of each I/O component. These values help PowerStatsService estimate the power consumption.
Attachment #780196 - Attachment is obsolete: true
Attachment #784847 - Flags: review?(vchang)
Attachment #784847 - Flags: review?(jonas)
Assignee: nobody → glai
Comment on attachment 784847 [details] [diff] [review]
readPowerProfile

Hi Blake, I'm not quite familiar with the xml parser. Not sure if you can help to review this patch. This patch tries to parse the xml format file and return the value of specific item. You can find the example file from the attached "PowerProfile sample file". 

Gavin is our intern who is helping us to implement per app power consumption feature.
Attachment #784847 - Flags: review?(vchang)
Attachment #784847 - Flags: review?(mrbkap)
Attachment #784847 - Flags: review?(jonas)
Comment on attachment 784847 [details] [diff] [review]
readPowerProfile

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

Hi Gavin,

Instead of rolling your own "read file and parser XML out of it," I think you should be able to use XMLHttpRequest directly, passing it a file: URI and getting the responseXML after it loads. That should be much simpler.
Attachment #784847 - Flags: review?(mrbkap)
Hi Blake,

Thanks for your advice. I will refine this soon.
Attached patch (wip)readPowerProfile (obsolete) — Splinter Review
Hi Blake,

I'm curious that can XMLHttpRequest open a file in /system/etc/?

I got some problems to use XMLHttpRequest to open the power profile in /system/etc/. The status of XMLHttpRequest is always 0. I use the same code to open http://yahoo.com. The status of XMLHttpRequest is 200. I can't figure out anything wrong in this code....

Any advices will be thankful.
Attachment #784847 - Attachment is obsolete: true
Flags: needinfo?(mrbkap)
Blocks: 854204
Comment on attachment 790736 [details] [diff] [review]
(wip)readPowerProfile

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

Hi Gavin,

I think I see what's going on. See below.

::: dom/power/PowerStatsService.jsm
@@ +325,5 @@
> +    const fileuri = "file:///system/etc/power_profile.xml";
> +    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +                .createInstance(Ci.nsIXMLHttpRequest);
> +    xhr.open("GET", fileuri, true);
> +    xhr.responseType = "text";

I think you want responseType to be "document" instead of "text".

@@ +333,5 @@
> +    xhr.onreadystatechange = function() {
> +      if (xhr.readyState != Ci.nsIXMLHttpRequest.DONE) {
> +        return;
> +      }
> +      if (xhr.status === 200) {

XMLHttpRequest.status doesn't change for file:// URIs. It only makes sense for HTTP requests. You should simply use xhr.responseXML when readyState is DONE.
Flags: needinfo?(mrbkap)
Hi Blake, 

Thanks for your big help. It works well right now. 

I have one more question to ask...I'm trying to modify the readFile funcfion using XMLHttpRequest to read files. Read local files using XMLHttpRequest seems asynchronous. However, lots of codes(bug: 854204) need the results of read file immediately. If I insert callback to solve this problem, I think it is a little bit hard than origin to read these codes...

Is it fine to use XMLHttpRequest in synchronous?
Or, using current readFile function?
Or, other better way to solve this situation?
Flags: needinfo?(mrbkap)
No longer blocks: 854200
Depends on: 854200
(In reply to Gavin Lai[:glai] from comment #15)
> I have one more question to ask...I'm trying to modify the readFile funcfion
> using XMLHttpRequest to read files. Read local files using XMLHttpRequest
> seems asynchronous. However, lots of codes(bug: 854204) need the results of
> read file immediately. If I insert callback to solve this problem, I think
> it is a little bit hard than origin to read these codes...

Are you trying to get these synchronous responses on the main thread? If so, then you should definitely *not* be doing any synchronous I/O. It's worth a couple of extra callbacks in order to avoid locking up the main thread and preventing the user from interacting with the page or browser UI.

If you're off the main thread, then synchronous I/O would be OK.
Flags: needinfo?(mrbkap)
Assignee: glai → gavin09
Attached patch readPowerProfile (obsolete) — Splinter Review
Modify read file related functions to do asynchronous I/O.
Attachment #796434 - Flags: review?(mrbkap)
Attached patch readPowerProfile (obsolete) — Splinter Review
Fit nit and bugs.
Attachment #796434 - Attachment is obsolete: true
Attachment #796434 - Flags: review?(mrbkap)
Attachment #796437 - Flags: review?(mrbkap)
Attached patch readPowerProfile (obsolete) — Splinter Review
Fix nit.
Attachment #796437 - Attachment is obsolete: true
Attachment #796437 - Flags: review?(mrbkap)
Attachment #796484 - Flags: review?(mrbkap)
Attachment #790736 - Attachment is obsolete: true
Attachment #781545 - Attachment is obsolete: true
Comment on attachment 796484 [details] [diff] [review]
readPowerProfile

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

I still have some questions, but this looks pretty good.

::: dom/power/PowerStatsService.jsm
@@ +46,5 @@
>  const HTTPS_URL = "https://";
>  const APP_TYPE = "app";
>  const SYSTEM_TYPE = "system";
>  const BROWSER = "app://browser.gaiamobile.org/manifest.webapp";
> +const POWER_PROFILE_PATH = "/system/etc/power_profile.xml";

This seems to be unused.

@@ +289,5 @@
>      });
>    },
> +
> +  // cpu.speeds.count read the value of counter in different CPU speed step.
> +  readProfile: function readProfile(item, callback) {

This seems to be unused.

@@ +303,5 @@
> +    let result = [];
> +    this.readFile(CPU_COUNT_PATH, function read(text) {
> +      let data = text.split(/\W+/);
> +      for (let i = 0; i < Math.floor(data.length/2); i++) {
> +        result.push(data[2*i+1]);

Please add a comment explaining what the input looks like and why we're only picking the odd elements out of it.

@@ +312,5 @@
> +  },
> +
> +  // readPowerProfile reads the average current consumption in power profile
> +  // file. The power profile is in xml format.
> +  readPowerProfile: function readPowerProfile() {

Do we have to worry about this being called twice (and therefore that powerProfile might not be an empty object)?

@@ +331,5 @@
> +
> +        // parse the xml item
> +        for (let i = 0; i < target.length; i++) {
> +          let item = target[i].getAttribute('name');
> +          powerProfile[item] = target[i].childNodes[0].nodeValue;

I wonder if it wouldn't be more conservative to use target[i].textContent instead of assuming a single child node.

@@ +341,5 @@
> +          let item = target[i].getAttribute('name');
> +          let arr = [];
> +
> +          for (let j = 0; j < Math.floor(target[i].childNodes.length/2); j++) {
> +            arr.push(target[i].childNodes[2*j+1].childNodes[0].nodeValue);

Can you explain why you're only picking every other child node? In general, some comments explaining what the input looks like would help greatly in understand what the code is trying to do.

@@ +357,5 @@
> +    const DONE = 4;
> +    let aSync = true;
> +    let xhr = Cc["@mozilla.org/xmlextras/xmlhttprequest;1"]
> +                .createInstance(Ci.nsIXMLHttpRequest);
> +    xhr.open("GET", uri, aSync);

I hate to do this to you, but for plain text, I think it's much preferable to use OS.File (see <https://developer.mozilla.org/en-US/docs/JavaScript_OS.File/OS.File_for_the_main_thread#Example.3A_Read_the_contents_of_a_file_as_text>) for plain text files. XMLHttpRequest is only better if you're actually reading XML data.
Attachment #796484 - Flags: review?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) [PTO Sep 5-Sep 19] from comment #20)

> I hate to do this to you, but for plain text, I think it's much preferable
> to use OS.File (see

It's fine. I learn a lot! I'll refine soon.
Assignee: gavin09 → bochen
The power_profile.json describes the coefficients for calculating power consumption in JSON format.
This is an example power_profile.json for Unagi.
To run this patch, please:
  1. download the power_profile.json and put it under B2G/device/qcom/unagi/
  2. Modify B2G/device/qcom/unagi/full_unagi.mk and append the following line to PRODUCT_COPY_FILES
       device/qcom/unagi/power_profile.json:system/etc/power_profile.json
Hi Blake, cloud you help review this new patch? Thx.

Update note:
1. Use OS.File instead of XMLHttpRequest.
2. Remove codes of reading CPU counters for clean code.
   (The readCPUCounter() will be moved to Bug 854204 so that we can unblock that bug.)
Attachment #796484 - Attachment is obsolete: true
Attachment #8339800 - Flags: review?(mrbkap)
No longer blocks: 854204
Comment on attachment 8339800 [details] [diff] [review]
Bug-854202-readPowerProfile.patch

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

This looks much better. I only have one comment. r=me with it addressed.

::: dom/power/PowerStatsService.jsm
@@ +86,5 @@
> +          result = JSON.parse(decoder.decode(data));
> +        } catch (ex) {
> +          Cu.reportError("Cannot parse power_profile.json to a json object");
> +        }
> +        debug("readPowerProfile():" + JSON.stringify(result));

I would take these debug statements out of the code before landing this patch. Even though they won't be printed, JSON.stringify will be called which isn't ideal if we're going to throw the result out.
Attachment #8339800 - Flags: review?(mrbkap) → review+
Hi Blake:
  Thanks for your comments. Here is the patch.

Update Notes:
1. remove debug messages in _readPowerProfile()
Attachment #8339800 - Attachment is obsolete: true
Attachment #8342801 - Flags: review?(mrbkap)
Comment on attachment 8342801 [details] [diff] [review]
Bug-854202-readPowerProfile.patch

Looks great! Thanks for sticking with this.
Attachment #8342801 - Flags: review?(mrbkap) → review+
Thanks for you help, Blake!
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: