Last Comment Bug 769431 - Correlate performance data with Ivy Bridge MSR_PPX_ENERGY_STATUS for power usage profiles
: Correlate performance data with Ivy Bridge MSR_PPX_ENERGY_STATUS for power us...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla28
Assigned To: Joe Olivas
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-28 13:18 PDT by Benoit Girard (:BenWa)
Modified: 2013-11-07 11:47 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Rough prototype v1 (303.17 KB, patch)
2013-03-06 11:43 PST, Joe Olivas
no flags Details | Diff | Splinter Review
MSR reader demo version 2 (304.42 KB, patch)
2013-03-13 16:48 PDT, Joe Olivas
no flags Details | Diff | Splinter Review
Windows and OS X drivers (72.67 KB, application/octet-stream)
2013-03-13 16:50 PDT, Joe Olivas
no flags Details
MSR reader - Rebased WIP (301.74 KB, patch)
2013-06-20 09:12 PDT, Joe Olivas
no flags Details | Diff | Splinter Review
WIP v3 + Power Planes (CPU + GPU) (310.66 KB, patch)
2013-06-21 11:16 PDT, Joe Olivas
no flags Details | Diff | Splinter Review
Delta (21.38 KB, patch)
2013-06-24 11:41 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
WIP v4 (311.53 KB, patch)
2013-06-24 11:42 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Delta (29.89 KB, patch)
2013-06-24 13:04 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
bug769431-powergadget.patch (18.59 KB, patch)
2013-09-16 15:17 PDT, Joe Olivas
no flags Details | Diff | Splinter Review
rebased (18.73 KB, patch)
2013-10-09 10:02 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
bug769431-rebased.patch (19.02 KB, patch)
2013-10-09 17:01 PDT, Joe Olivas
bgirard: feedback+
Details | Diff | Splinter Review
bug769431-rebased.patch (21.37 KB, patch)
2013-10-16 16:23 PDT, Joe Olivas
bgirard: feedback+
Details | Diff | Splinter Review
bug769431-rebased.patch (22.43 KB, patch)
2013-11-06 16:41 PST, Joe Olivas
bgirard: review+
Details | Diff | Splinter Review
patch - fixed style nits and non win build errors (22.51 KB, patch)
2013-11-06 17:55 PST, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
patch v3 (22.58 KB, text/plain)
2013-11-07 08:21 PST, Benoit Girard (:BenWa)
no flags Details

Description Benoit Girard (:BenWa) 2012-06-28 13:18:56 PDT
Documentation: http://www.intel.com/content/www/us/en/processors/architectures-software-developer-manuals.html

Volume 3B, Chapter 14, Section 7.

A very quick look shows that MSR_PPX_ENERGY_STATUS can provide us with power status reading for each millisecond. If that the case then we should be able to correlate it with the profiler.

Unfortunately we can't simply query this value, we need to use a driver to get access to this value.

Here's an example tool: http://software.intel.com/en-us/articles/intel-power-gadget/.
Comment 1 Benoit Girard (:BenWa) 2012-06-28 16:29:07 PDT
I implemented it. Here's a profile we collect on mac with a dev version of firefox (using the OMTC feature):

http://people.mozilla.com/~bgirard/cleopatra/?report=AMIfv94Xa8PA0IoNF0GvNB_yxtCLl1MdvA1zvO2Oy9p0up5G3OrpQshrKqqcsvXTzAMyUaquyrF6aeGJu-vFhBwtWeT5rilRpOQDZS0vfnhv8I5XsAwNvGqT7LZ8Qx3LdPqcgXuNNdMqsz1oNhoJIS21ZORO2k6A4w

The intensity of the red color in the histogram is the power usage taken at 1ms. I couldn't get the library to give me power information quite at 1Khz so I copy the previous power data if it fails. The unit is Watts*30. Deep red means that we're drawing most power and black means that we're not drawing much power. I can confirm that it works because as soon as we're waiting for further events from the user the power usage drops to a minimum. The height is the call stack height, it's useful for spotting blocking (flat plateau) and with some experience you can recognize the shape of JS execution.

Because the power gadget sample is not a permissive license I can't post the patches but it's 50 LoC so that's not a big deal.
Comment 2 Joe Olivas 2012-06-29 09:56:27 PDT
Another tool exists that uses this same MSR, but is under a permissive license and has source available.

http://software.intel.com/en-us/articles/power-gov/
Comment 4 Benoit Girard (:BenWa) 2012-06-29 10:17:23 PDT
I blogged about the result here:
http://benoitgirard.wordpress.com/2012/06/29/correlating-power-usage-with-performance-data-using-the-gecko-profiler-and-intel-sandy-bridge/

A few of my coworkers have expressed interest so I'm considering it implementing it properly in the profiling system.
Comment 5 Benoit Girard (:BenWa) 2012-06-29 10:19:20 PDT
(In reply to Joe Olivas from comment #2)
> Another tool exists that uses this same MSR, but is under a permissive
> license and has source available.
> 
> http://software.intel.com/en-us/articles/power-gov/

Excellent! That's a big help.
Comment 6 Benoit Girard (:BenWa) 2013-01-17 08:51:07 PST
I don't have the resources to support this properly since its need a custom (but rather common) CPU, a custom driver for each platform.

If someone is interested in collecting these profiles I can help arrange a configuration for their system to repeat the setup from comment 4.
Comment 7 Joe Olivas 2013-01-17 11:19:45 PST
I should be able to put together the resources to do it. If you can send along the patch and setup details for your experiment, that would be a big help.
Comment 8 Benoit Girard (:BenWa) 2013-01-17 11:43:32 PST
Cool. I'll re-open the bug because there's certainly an interest in this feature. I couldn't find my previous patch but here's a detailed description of what needs to be done.

1) The difficult part is having a driver that can be distributed ideally for our all desktop platform (win, osx, linux) but a subset would be fine.

2) If the code to interface with that driver is small we can just land that directly in tools/profiler/*.

3) For testing you can just override the 'r' value with the instantaneous power consumption reading here:
http://mxr.mozilla.org/mozilla-central/source/tools/profiler/TableTicker.cpp#962
This will colorize the graph according to the power usage rather then the input lag. This is all that's needed for testing.

4) Introduce a 'power' feature by modifying mozilla_sampler_get_features(), store the state of the power feature in by setting it from TableTicker::TableTicker. Then back in step 3 rather then replacing 'r' create a new 'p' tag and only generate the data if power is selected.

5) Modify TableTicker::BuildJSObject to add a 'power' field to the JSObject when saving the profile.

I can trivially do all the changes to the add-on and UI if you post the above backend patch.
Comment 9 Benoit Girard (:BenWa) 2013-01-24 12:00:39 PST
Here's an interesting analysis that was done on Firefox in the past:
http://softwareprocess.es/static/GreenMining.html
Comment 10 Joe Olivas 2013-03-06 11:43:36 PST
Created attachment 721830 [details] [diff] [review]
Rough prototype v1

Initial prototype to get energy (in joules).
Comment 11 Joe Olivas 2013-03-13 16:48:41 PDT
Created attachment 724679 [details] [diff] [review]
MSR reader demo version 2

Updated to fix some problems with internal state in the PCM library.
Comment 12 Joe Olivas 2013-03-13 16:50:28 PDT
Created attachment 724681 [details]
Windows and OS X drivers

Adding driver sources for Windows and OS X. Linux uses the 'msr' module, so sources are unnecessary.
Comment 13 Benoit Girard (:BenWa) 2013-03-15 21:26:46 PDT
We've landed major patches to the profiler directory. I can rebase the above patches. I recommend just developing against the same revision and not rebasing. Everything we do here will still apply well but will require manual rebasing.
Comment 14 Benoit Girard (:BenWa) 2013-06-17 11:50:10 PDT
Licensing is great! Checked the driver source into the repo:
https://github.com/bgirard/Gecko-Profiler-Addon/tree/master/driver
Comment 15 Joe Olivas 2013-06-20 09:12:20 PDT
Created attachment 765416 [details] [diff] [review]
MSR reader - Rebased WIP

Rebasing previous patch. Verified that data is reported from the profiler as before.
Comment 16 Benoit Girard (:BenWa) 2013-06-20 09:15:18 PDT
Excellent! Thanks Joe. I'll modify the add-on, host some pre-built binaries next week. We should be able to ship this with Nightly in early July if all goes well. Thanks for your effort.
Comment 17 Benoit Girard (:BenWa) 2013-06-21 08:50:41 PDT
I can't find IntelPMU.h in either the patch or the driver provided.

 1:07.72 TableTicker.h:9:10: fatal error: 'IntelPMU.h' file not found
 1:07.72 #include "IntelPMU.h"
Comment 18 Joe Olivas 2013-06-21 09:03:58 PDT
D'oh! Forgot to 'hg add' those to get them into the patch. Sorry about that.

I'm in the midst of another update to expose the two power planes (CPU and GPU). I'll update this in a bit with these changes and those files added.
Comment 19 Joe Olivas 2013-06-21 11:16:42 PDT
Created attachment 765999 [details] [diff] [review]
WIP v3 + Power Planes (CPU + GPU)

Added the missing files from the last one (d'oh). Also included the gathering of the two power planes, which essentially are specific to CPU and GPU, rather than the entire package, as was collected originally. These just need to be called using the new APIs in the IntelPMU class.

All three energy stats are fairly useful, but no support for the power planes has been added into the profile properties yet.
Comment 20 Benoit Girard (:BenWa) 2013-06-21 12:41:41 PDT
One more missing file:
tools/profiler/x86/pcm/src/msr.h:30:10: fatal error: 
      'MSRAccessor.h' file not found

Looks like the file is in the driver.
Comment 21 Joe Olivas 2013-06-21 13:28:58 PDT
Ah, yes, it looks like it's a different build on OS X vs. Windows.

I don't yet have an OS X system setup on which to test; I'll get it setup and get this build resolved there.
Comment 22 Benoit Girard (:BenWa) 2013-06-21 14:11:10 PDT
I can help with the mac build. Should I hg add the header from the driver? Where do you intent this file to come from and live?
Comment 23 Joe Olivas 2013-06-21 14:31:40 PDT
Sure, that'd be great! My OS X system is a bit outdated :)

I was just hoping to keep the driver code and user code separated. From what I can tell, the Mac version will require MSRAccessor.cpp/h, DriverInterface.c/h, and UserKernelShared.h.

The original sources were a bit scattered, so we can move them where it makes sense. Perhaps just a 'mac' folder in tools/profiler/x86/pcm/src makes it easy, but if you have a better idea, please feel free to follow it.
Comment 24 Benoit Girard (:BenWa) 2013-06-24 11:41:29 PDT
Created attachment 766801 [details] [diff] [review]
Delta

Posting a delta to show what I've changed
Comment 25 Benoit Girard (:BenWa) 2013-06-24 11:42:43 PDT
Created attachment 766802 [details] [diff] [review]
WIP v4
Comment 26 Benoit Girard (:BenWa) 2013-06-24 13:04:14 PDT
Created attachment 766847 [details] [diff] [review]
Delta
Comment 27 Joe Olivas 2013-07-02 10:35:10 PDT
I'm reworking to use the PowerGadget library, and it will instead be reporting directly in Watts, using rdtscs to convert joules -> watts internally. This should be more accurate than using the OS timers, since those wakeups are less reliable.

It looks like Cleopatra will need to be updated, since it is still expecting the power data to be reported in joules, and my system is reporting in the range of ~1700 W :)
Comment 28 Joe Olivas 2013-07-03 10:36:12 PDT
Here's an example data set with the new values in Watts:

http://people.mozilla.com/~bgirard/cleopatra/#report=282b6f5dd760bae07fb008bdd3ce1ee7b36a1f69
Comment 29 Benoit Girard (:BenWa) 2013-07-03 11:00:09 PDT
(In reply to Joe Olivas from comment #27)
> It looks like Cleopatra will need to be updated, since it is still expecting
> the power data to be reported in joules, and my system is reporting in the
> range of ~1700 W :)

Sounds good. I'll update using this profile as a sample. I'll also add a toggle to colorize based on power.
Comment 30 Benoit Girard (:BenWa) 2013-07-05 08:37:52 PDT
Alright I've deployed an update. Be sure to clear your browser cache:
- Check 'Show Power' and optional change the peak setting (toggle show power to update).
- Work and Peak values are updated to follow new measurements
- Mouse over the graph to get a reading of power usage at that instant in time.

https://github.com/bgirard/cleopatra/commit/a89d6710a8ef206b212be25efd1974d8468b054e
Comment 31 Joe Olivas 2013-09-16 15:17:54 PDT
Created attachment 805613 [details] [diff] [review]
bug769431-powergadget.patch

This version ditches the old precarious driver code and instead relies on Intel Power Gadget. It also is meant for use with the coming release of Power Gadget (2.7), though I don't see any reason it would fail horribly with a current version.
Comment 32 Benoit Girard (:BenWa) 2013-09-16 16:05:26 PDT
Ohh wow, that's much better! Any ETA or pre-released version of the Power Gadget tools we can use to test this patch?
Comment 33 Joe Olivas 2013-09-17 08:41:54 PDT
The plan is for version 2.7 to be released at the end of September.
Comment 34 Joe Olivas 2013-10-04 09:42:34 PDT
2.7 is out.

http://software.intel.com/en-us/articles/intel-power-gadget/
Comment 35 Benoit Girard (:BenWa) 2013-10-08 06:52:15 PDT
Alright I'm going to try and look at this but I'm away from the office nearly the entire month. It might have to wait until November.
Comment 36 Benoit Girard (:BenWa) 2013-10-09 09:59:05 PDT
EnergyLib32 wasn't placed in my dll search path, once moved there it's loaded correctly.

I get a deadlock on startup at during a sample where GetTotalPackagePowerInWatts calls malloc. We can't allocate memory from the sample context because the main thread can be holding that lock when we suspend it for sampling.
Comment 37 Benoit Girard (:BenWa) 2013-10-09 10:02:41 PDT
Created attachment 814965 [details] [diff] [review]
rebased
Comment 38 Joe Olivas 2013-10-09 11:06:33 PDT
Thanks for testing.

I'll test again on the released package; perhaps there was a mistake somewhere. I made sure that they were aware of the malloc issue, and I let it run overnight with no issue on what I thought was the release build just to make sure we didn't deadlock.

As for the search path issue, it should get added to the Path variable on Windows through the installer, and visible when you launch via explorer.exe. Is this how you ran it?
Comment 39 Benoit Girard (:BenWa) 2013-10-09 12:44:33 PDT
(In reply to Joe Olivas from comment #38)
> As for the search path issue, it should get added to the Path variable on
> Windows through the installer, and visible when you launch via explorer.exe.
> Is this how you ran it?

It's possible I needed to restart Visual Studio or perhaps reboot my machine. I added it there manually and it worked.
Comment 40 Benoit Girard (:BenWa) 2013-10-09 12:45:13 PDT
FYI the malloc hang is on startup before the window can come up. I tried a few times and was never able to get far enough for the window to appear.
Comment 41 Joe Olivas 2013-10-09 12:59:10 PDT
Ok, it looks like the Path variable is not used, instead it is creating its own environment variable, so I'll have to patch that in. It was my mistake and assumed nothing had changed from an older build; it should be an easy fix.

The malloc issue may have slipped through. I am working on getting it fixed.
Comment 42 Joe Olivas 2013-10-09 17:01:00 PDT
Created attachment 815172 [details] [diff] [review]
bug769431-rebased.patch

Update to use the environment variable set by the Power Gadget installer.
Comment 43 Benoit Girard (:BenWa) 2013-10-10 08:00:16 PDT
Comment on attachment 815172 [details] [diff] [review]
bug769431-rebased.patch

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

Looks great! I'd be nice to have a check in place for the old buggy library since it will deadlock the system but we can probably skip that. Otherwise just a few minor edits and we can land then. I imagine the next update of PowerGadget will include the updated library?

::: tools/profiler/IntelPowerGadget.cpp
@@ +1,4 @@
> +/*
> + * Copyright 2013, Intel Corporation
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");

I'll need to double check the rules for other license.

@@ +55,5 @@
> +        ipg_library.AppendLiteral("/");
> +        ipg_library.AppendLiteral(PG_LIBRARY_NAME);
> +    }
> +
> +    libpowergadget = PR_LoadLibrary(ipg_library.get());

I think this line should go above in the if block. I'd imagine nsCString is constructed as "" and I would rather avoid calling PR_LoadLibrary("").

@@ +166,5 @@
> +IntelPowerGadget::GetCPUFrequency(int node) 
> +{ 
> +    int frequency = 0;
> +    if(GetIAFrequency) {
> +        int ok = GetIAFrequency(node, &frequency);    

It would be nice to strip all this trailing whitespace but not a big deal.

::: tools/profiler/TableTicker.h
@@ +50,5 @@
>      , mPrimaryThreadProfile(nullptr)
>      , mSaveRequested(false)
>      , mUnwinderThread(false)
>      , mFilterCount(aFilterCount)
> +    , mIntelPowerGadget()

We need to turn on power profiling into an options. The average performance profiling will not be interested in power data.
Comment 44 Joe Olivas 2013-10-16 16:23:45 PDT
Created attachment 818130 [details] [diff] [review]
bug769431-rebased.patch

Fixes the above feedback as much as I could; I tried to make the power profiler run only as an option, but it looks like the Add-On needs some updates, and I don't have permissions (as far as I can tell) to get the required packages to build the Add-On. I'm guessing I missed updating something.

The fixed version on PowerGadget is in the same place, with the same version number, so the version check wouldn't do any good :(
Comment 45 Benoit Girard (:BenWa) 2013-11-05 10:28:42 PST
You have permissions but something is broken against the addon SDK trunk. I can add the feature to the addon myself.
Comment 46 Benoit Girard (:BenWa) 2013-11-05 10:37:06 PST
Comment on attachment 818130 [details] [diff] [review]
bug769431-rebased.patch

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

Almost there! I'll add the power feature now to the addon.

::: tools/profiler/TableTicker.h
@@ +65,5 @@
>      mPrivacyMode = hasFeature(aFeatures, aFeatureCount, "privacy");
>      mAddMainThreadIO = hasFeature(aFeatures, aFeatureCount, "mainthreadio");
>  
> +    if(mProfilePower) {
> +      mIntelPowerGadget = new IntelPowerGadget();

needs matching delete.
The initialization logic should be moved outside the constructor and into an init method that can fail if the library isn't found. In that case the power gadget can be delete and the power profiling feature can be turned off immediately rather then failing for each sample.
Comment 47 Benoit Girard (:BenWa) 2013-11-05 10:42:48 PST
Add-on version 1.12.14 should correctly select the feature
Comment 48 Benoit Girard (:BenWa) 2013-11-05 10:51:29 PST
I think the license block in about:license#apache doesn't need any modification to land this:
"This license applies to various files in the Mozilla codebase."
Comment 49 Joe Olivas 2013-11-06 16:41:09 PST
Created attachment 828350 [details] [diff] [review]
bug769431-rebased.patch

Added Init() and disabling on fail, destructor and very minor cleanup. Looks to work with the new add-on that has the Power option.
Comment 50 Benoit Girard (:BenWa) 2013-11-06 17:45:24 PST
Comment on attachment 828350 [details] [diff] [review]
bug769431-rebased.patch

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

Looks great. I can wrap this up for you. Thank you so much!

::: tools/profiler/IntelPowerGadget.cpp
@@ +63,5 @@
> +        libpowergadget = PR_LoadLibrary(ipg_library.get());
> +    }
> +
> +    if(libpowergadget) {
> +        NS_WARNING("Found the IPG library!");

All these warnings are logging and should be removed or ifdef out by default.

::: tools/profiler/TableTicker.h
@@ +111,5 @@
>            info->SetProfile(nullptr);
>          }
>        }
>      }
> +    if(mIntelPowerGadget) {

FYI: null check for free/delete isn't required.
Comment 51 Benoit Girard (:BenWa) 2013-11-06 17:51:57 PST
Comment on attachment 828350 [details] [diff] [review]
bug769431-rebased.patch

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

Looks great. I can wrap this up for you. Thank you so much!

::: tools/profiler/IntelPowerGadget.cpp
@@ +63,5 @@
> +        libpowergadget = PR_LoadLibrary(ipg_library.get());
> +    }
> +
> +    if(libpowergadget) {
> +        NS_WARNING("Found the IPG library!");

All these warnings are logging and should be removed or ifdef out by default.

::: tools/profiler/TableTicker.cpp
@@ +593,5 @@
>      TimeDuration delta = sample->timestamp - sStartTime;
>      currThreadProfile.addTag(ProfileEntry('t', delta.ToMilliseconds()));
>    }
>  
> +  // Change to pg sample

What do you mean by pg sample?

::: tools/profiler/TableTicker.h
@@ +111,5 @@
>            info->SetProfile(nullptr);
>          }
>        }
>      }
> +    if(mIntelPowerGadget) {

FYI: null check for free/delete isn't required.
Comment 52 Benoit Girard (:BenWa) 2013-11-06 17:55:34 PST
Created attachment 828388 [details] [diff] [review]
patch - fixed style nits and non win build errors
Comment 53 Benoit Girard (:BenWa) 2013-11-06 17:57:16 PST
Once this is green we should be ready to land:
https://tbpl.mozilla.org/?tree=Try&rev=73e5d94a710b
Comment 54 Joe Olivas 2013-11-06 18:04:57 PST
(In reply to Benoit Girard (:BenWa) from comment #51)

> Looks great. I can wrap this up for you. Thank you so much!

Your support has been much appreciated!

> ::: tools/profiler/TableTicker.cpp
> @@ +593,5 @@
> >      TimeDuration delta = sample->timestamp - sStartTime;
> >      currThreadProfile.addTag(ProfileEntry('t', delta.ToMilliseconds()));
> >    }
> >  
> > +  // Change to pg sample
> 
> What do you mean by pg sample?

This was a leftover TODO to check if we had a power sample (pg = PowerGadget), rather than any sample. Should have been removed after fixing.
Comment 55 Benoit Girard (:BenWa) 2013-11-07 08:21:06 PST
Created attachment 828679 [details]
patch v3

Forgot to ifdef the power feature to win only.
Comment 56 Benoit Girard (:BenWa) 2013-11-07 08:22:26 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/b48e11fc1f35

Thanks again! This should be either in the tomorrow's nightly build or the day after.
Comment 57 Ryan VanderMeulen [:RyanVM] 2013-11-07 11:47:44 PST
https://hg.mozilla.org/mozilla-central/rev/b48e11fc1f35

Note You need to log in before you can comment on or make changes to this bug.