854204 - [Per app power metering] - Calculate CPU usage per process

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: General, defect)

Comment 1

[Gavin Lai[:glai]]

I studied the implementation of CPU in Android. 
The primary implementation is in processAppUsage() - http://androidxref.com/4.2.2_r1/xref/packages/apps/Settings/src/com/android/settings/fuelgauge/PowerUsageSummary.java#433

userTime and systemTime can be obtained in /proc/<pid>/stats.
cpuSpeedTime can be obtained in /sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state
PowerProfile has the information of average power consumption. 

The general form of equation is
processPower = ratio[step1] * Time * powerCpuNormal[step1] + ... + ratio[stepk] * Time * powerCpuNormal[stepk]

ratio[step] = cpuStepTime[step]/ TotalTime
Time = userTime + systemTime
powerCpuNormal[stepk] is the information in powerProfile

Comment 2

[Gavin Lai[:glai]]

Attachment: CPU power per app

collect CPU user time and CPU system time per process.

Comment 3

[Gavin Lai[:glai]]

Attachment: CPU power per app

support to analyze power consumption of CPU per webapp

Comment 4

[Gavin Lai[:glai]]

Attachment: (wip)PU power per app

Comment 5

[Gavin Lai[:glai]]

Attachment: CPU power per app

Comment 6

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 780214 [details] [diff] [review]
CPU power per app

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

::: dom/power/PowerStatsService.jsm
@@ +125,5 @@
> +        if (powerInfoArr.length < 3) {
> +          debug("The format of power-meter-info is not right.");
> +          break;
> +        }
> +        let index = powerInfoArr[0];

Can we valid the index value here ? Return error if the index is invalid.

@@ +138,5 @@
> +        } else if (index.substring(0, 7) == HTTP_URL) {
> +          this.createAppInfo(index, item, value);
> +        } else if (index.substring(0, 8) == HTTPS_URL) {
> +          this.createAppInfo(index, item, value);
> +        }

Just call this.createAppInfo(index, item, value) because we have validate the index.

@@ +280,5 @@
>        });
>      }
>    },
>  
> +  createAppInfo: function createAppInfo(index, item, value) {

The item should be "pid" only right ? 
Move "if (item !== "pid") return;" in the beginning of the function.

@@ +282,5 @@
>    },
>  
> +  createAppInfo: function createAppInfo(index, item, value) {
> +    // Check if appInfo exists this app info.
> +    if (appPowerInfo[index] == null) {

if (index in appPowerInfo)

@@ +287,5 @@
> +      debug("Add new app power info into appPowerInfo.");
> +      if (item == "pid") {
> +        appPowerInfo[index] = {pid: value[0],
> +                               tmpTime: 0,
> +                               time: 0};

What is the purpose of tmpTime and time ?

@@ +302,5 @@
> +  readProcPidStat: function readProcPidStat(manifestURL, pid) {
> +    let error = 0;
> +    let result = "";
> +
> +    if (pid === -1) {

Please use a constant for "-1", do we need to return an error here ? Please add comments here to explain why don't you return immediately.

@@ +311,5 @@
> +    debug("Path of procPath: " + procPath + ".");
> +
> +    let data = "";
> +    data = this.readFile(procPath);
> +    let statArr = data.split(/(\(.*?\)|[^",\s]+)(?= \w+\s*)/gi);

What is the staeArr ? Can you add a comment to explain it ?

@@ +313,5 @@
> +    let data = "";
> +    data = this.readFile(procPath);
> +    let statArr = data.split(/(\(.*?\)|[^",\s]+)(?= \w+\s*)/gi);
> +    let time = 0;
> +    //time = user time + system time

Nit: space after //, also add period at the end of sentence.

@@ +314,5 @@
> +    data = this.readFile(procPath);
> +    let statArr = data.split(/(\(.*?\)|[^",\s]+)(?= \w+\s*)/gi);
> +    let time = 0;
> +    //time = user time + system time
> +    time = parseInt(statArr[USERTIME_INDEX], DECIMAL) + 

Nit: trail white space.

@@ +323,5 @@
> +  readLastProc: function readLastProc(manifestURL) {
> +    debug("The last time of reading pid: " + appPowerInfo[manifestURL].pid + ".");
> +    let time = this.readProcPidStat(manifestURL, appPowerInfo[manifestURL].pid);
> +
> +    appPowerInfo[manifestURL].time += time;

Is this correct ?

@@ +325,5 @@
> +    let time = this.readProcPidStat(manifestURL, appPowerInfo[manifestURL].pid);
> +
> +    appPowerInfo[manifestURL].time += time;
> +    appPowerInfo[manifestURL].tmpTime = 0;
> +    appPowerInfo[manifestURL].pid = -1;

Please use a constant for "-1".

@@ +330,5 @@
> +  },
> +
> +  updateAppInfo: function updateAppInfo() {
> +    for (let i in appPowerInfo) {
> +      if (appPowerInfo[i].pid !== -1) {

Please use a constant for "-1".

@@ +331,5 @@
> +
> +  updateAppInfo: function updateAppInfo() {
> +    for (let i in appPowerInfo) {
> +      if (appPowerInfo[i].pid !== -1) {
> +        appPowerInfo[i].tmpTime = this.readProcPidStat(i, appPowerInfo[i].pid);

Why do you set the return value to tmpTime rather than time ?

Comment 7

[Gavin Lai[:glai]]

(In reply to Vincent Chang[:vchang] from comment #6)
> Comment on attachment 780214 [details] [diff] [review]
> CPU power per app
> 
> Review of attachment 780214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/power/PowerStatsService.jsm
> @@ +125,5 @@
> > +        if (powerInfoArr.length < 3) {
> > +          debug("The format of power-meter-info is not right.");
> > +          break;
> > +        }
> > +        let index = powerInfoArr[0];
> 
> Can we valid the index value here ? Return error if the index is invalid.

If a process is launched, appPowerInfo does not have the index yet. So, here only check if the index is APP_URL, HTTP_URL or HTTPS_URL. Or, is there any other thing need to validate? 

> @@ +138,5 @@
> > +        } else if (index.substring(0, 7) == HTTP_URL) {
> > +          this.createAppInfo(index, item, value);
> > +        } else if (index.substring(0, 8) == HTTPS_URL) {
> > +          this.createAppInfo(index, item, value);
> > +        }
> 
> Just call this.createAppInfo(index, item, value) because we have validate
> the index.

There will have updateSystemInfo(index, item, value) after system component parts are done. 

> @@ +280,5 @@
> >        });
> >      }
> >    },
> >  
> > +  createAppInfo: function createAppInfo(index, item, value) {
> 
> The item should be "pid" only right ? 
> Move "if (item !== "pid") return;" in the beginning of the function.
> 
> @@ +282,5 @@
> >    },
> >  
> > +  createAppInfo: function createAppInfo(index, item, value) {
> > +    // Check if appInfo exists this app info.
> > +    if (appPowerInfo[index] == null) {
> 
> if (index in appPowerInfo)
> 

I think the function name: createAppInfo may be confusing. I change the function name to collectAppInfo. collectAppInfo function will not only collect pid related information. collectAppInfo is responsible for collecting application related information. I will refine this.

> @@ +287,5 @@
> > +      debug("Add new app power info into appPowerInfo.");
> > +      if (item == "pid") {
> > +        appPowerInfo[index] = {pid: value[0],
> > +                               tmpTime: 0,
> > +                               time: 0};
> 
> What is the purpose of tmpTime and time ?
> 

tmpTime is CPU power consumption of a runnig process. The user time and system time of a running process in /proc/<pid>/stats are increasing. So, tmpTime is temporary. PowerStatsService add tmpTime into time when the process is terminated.

I will write comments here.

> @@ +302,5 @@
> > +  readProcPidStat: function readProcPidStat(manifestURL, pid) {
> > +    let error = 0;
> > +    let result = "";
> > +
> > +    if (pid === -1) {
> 
> Please use a constant for "-1", do we need to return an error here ? Please
> add comments here to explain why don't you return immediately.
> 

I will refine this. Thanks.

> @@ +311,5 @@
> > +    debug("Path of procPath: " + procPath + ".");
> > +
> > +    let data = "";
> > +    data = this.readFile(procPath);
> > +    let statArr = data.split(/(\(.*?\)|[^",\s]+)(?= \w+\s*)/gi);
> 
> What is the staeArr ? Can you add a comment to explain it ?
> 

I will write comments here. Thanks.

> @@ +323,5 @@
> > +  readLastProc: function readLastProc(manifestURL) {
> > +    debug("The last time of reading pid: " + appPowerInfo[manifestURL].pid + ".");
> > +    let time = this.readProcPidStat(manifestURL, appPowerInfo[manifestURL].pid);
> > +
> > +    appPowerInfo[manifestURL].time += time;
> 
> Is this correct ?
> 

I think it is correct here. Is there any problems?
Thanks.

> @@ +331,5 @@
> > +
> > +  updateAppInfo: function updateAppInfo() {
> > +    for (let i in appPowerInfo) {
> > +      if (appPowerInfo[i].pid !== -1) {
> > +        appPowerInfo[i].tmpTime = this.readProcPidStat(i, appPowerInfo[i].pid);
> 
> Why do you set the return value to tmpTime rather than time ?

Update applications information only need to update running processes.


Thanks for the advice.
I will refine this as soon as possible.

Comment 8

[Gavin Lai[:glai]]

Attachment: CPU power per app

Before this patch, PowerStatsService collects time and saves into appPowerInfo. Originally, we plan to calculate power consumption in gaia. Now, PowerStatsService calculates power consumption and saves into appPowerInfo. cpuPower function is responsible for calculating CPU power consumption per application.

Comment 9

[Gavin Lai[:glai]]

Attachment: CPU power per app

Fix bugs. When gecko terminates brower, mAppManifestURL is empty. PowerStatsService does not receive browser's manifestURL.

Comment 10

[Gavin Lai[:glai]]

Attachment: Part 0: Collect application information from ContentParent.

Fix bugs. When closing tabs in browser, ContentParent does not notify PowerStatsService.

Comment 11

[Gavin Lai[:glai]]

Attachment: Part 1: Estimate per app CPU power consumption.

Fix bugs. Browser is a special case. Each tab has its own process id (pid), and these tabs share the same manifestURL of browser. So, PowerStatsService use an array to record the pid of tabs.

Comment 12

[Gavin Lai[:glai]]

Attachment: Part 1: Estimate per app CPU power consumption.

Fix nit.

Comment 13

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 791205 [details] [diff] [review]
Part 1: Estimate per app CPU power consumption.

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

Drop the feedback for now, since you have to modify readProfile() to async call.

Comment 14

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 791076 [details] [diff] [review]
Part 0: Collect application information from ContentParent.

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

Basically, I am fine with this patch. But I feel like I am not capable to feedback this patch. Redirect to Justin.

Comment 15

[Justin Lebar (not reading bugmail)]

ContentParent already notifies the observer service when it's created, in ContentParent::Init().

I think the appropriate thing to do would be to add the pid and manifest URL to that notification, and to the shutdown notification.

See ContentParent::ActorDestroy for an example of how to add metadata to an observer notification  by way of nsHashPropertyBag.

Comment 16

[Vincent Chang[:vchang][changyihsin]]

(In reply to Justin Lebar [:jlebar] from comment #15)
> ContentParent already notifies the observer service when it's created, in
> ContentParent::Init().
> 
> I think the appropriate thing to do would be to add the pid and manifest URL
> to that notification, and to the shutdown notification.
> 
> See ContentParent::ActorDestroy for an example of how to add metadata to an
> observer notification  by way of nsHashPropertyBag.

Thanks for your help, we'll check it.

Comment 17

[Gavin Lai]

Attachment: Part 1: Estimate per app CPU power consumption.

Modify read file related functions to async call.

Comment 18

[Gavin Lai]

Attachment: (wip)Part 0: Collect application information from ContentParent.

Hi Justin,                                                                      
                                                                                
Thanks for your help!                                                           
                                                                                
I add metadata to ipc:content-shutdown notification by way of nsHashPropertyBag.       
And, it works great.

But, I have some problems to get mAppManifestURL by observing ipc:content-created notification. The mAppManifestURL in ContentParent::Init() is {{template}}.
It seems that the content of the application have not loaded into the process yet.
                                                                                
Is it ok to create a notification to get the correct mAppManifestURL and pid?   
What do you think?

Comment 19

[Justin Lebar (not reading bugmail)]

> It seems that the content of the application have not loaded into the process yet.

Ah, indeed.

We should count power used by the {{template}} process towards "system", or whatever we're calling that.

It sounds like you should add a notification which is fired when we change that name, and then update the power stuff accordingly.

Note that my last day at Mozilla is tomorrow, so you'll need to find someone else to help out with this.  bent and khuey are probably the people who know this code the best.

Comment 20

[Justin Lebar (not reading bugmail)]

Comment on attachment 797152 [details] [diff] [review]
(wip)Part 0: Collect application information from ContentParent.

This is the right idea, although there are still kinks to work out (the previous comment, and also the hardcoding of the browser app's URI as two examples).

Comment 21

[Gavin Lai]

(In reply to Justin Lebar [:jlebar] from comment #19)
> > It seems that the content of the application have not loaded into the process yet.
> 
> Ah, indeed.
> 
> We should count power used by the {{template}} process towards "system", or
> whatever we're calling that.
> 
> It sounds like you should add a notification which is fired when we change
> that name, and then update the power stuff accordingly.
> 
> Note that my last day at Mozilla is tomorrow, so you'll need to find someone
> else to help out with this.  bent and khuey are probably the people who know
> this code the best.

Thanks for your help.

Comment 22

[Gavin Lai]

Attachment: (wip)Part 1: Estimate per app CPU power consumption.

Get data by way of nsHashPropertyBag.

Comment 23

[Borting Chen [:borting]]

Hi Kyle:

I got a question: is that allowed to perform a file read in contentParent?

Here is the reason:

In order to calculate CPU power consumption, I want to get the execution time of an app (content process) by reading the status information from "/proc/[pid]/stat".

In my previous design, I let the b2g process observe the "ipc:content-shutdown" notification. When an app is terminated, the b2g process gets the notification (with the pid of that app) and reads the status information from "/proc/[pid]/stat". But, there is a chance that the b2g process fails to read the status information because the target process may be destroyed before the b2g process performs the read operation.

I think an alternative solution is to read the status information earlier and send the status information with the "ipc:content-shutdown" notifiction. So, is that possible to add some codes in ContentParent::ActorDestroy() to read the status information? (and send the status information to the b2g process with "ipc:content-shutdown" notification?)

Thanks.

Comment 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't understand your question.  ContentParent only exists in the b2g process.  By the time ContentParent::ActorDestroy is called, the other process may be gone already.  So I don't see how adding code in ContentParent::ActorDestroy to read from /proc/pid/stat helps solve your problem.

Comment 25

[BMO Automation]

Firefox OS is not being worked on