769431 - Correlate performance data with Ivy Bridge MSR_PPX_ENERGY_STATUS for power usage profiles

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

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

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]

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 3

[Joe Olivas]

More sample code

http://software.intel.com/en-us/articles/intel-performance-counter-monitor/

Comment 4

[Benoit Girard (:BenWa)]

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

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

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]

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

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

Here's an interesting analysis that was done on Firefox in the past:
http://softwareprocess.es/static/GreenMining.html

Comment 10

[Joe Olivas]

Attachment: Rough prototype v1

Initial prototype to get energy (in joules).

Comment 11

[Joe Olivas]

Attachment: MSR reader demo version 2

Updated to fix some problems with internal state in the PCM library.

Comment 12

[Joe Olivas]

Attachment: 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)]

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

Licensing is great! Checked the driver source into the repo:
https://github.com/bgirard/Gecko-Profiler-Addon/tree/master/driver

Comment 15

[Joe Olivas]

Attachment: MSR reader - Rebased WIP

Rebasing previous patch. Verified that data is reported from the profiler as before.

Comment 16

[Benoit Girard (:BenWa)]

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

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]

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]

Attachment: 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)]

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]

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

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]

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

Attachment: Delta

Posting a delta to show what I've changed

Comment 25

[Benoit Girard (:BenWa)]

Attachment: WIP v4

Comment 26

[Benoit Girard (:BenWa)]

Attachment: Delta

Comment 27

[Joe Olivas]

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]

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

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

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]

Attachment: 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)]

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]

The plan is for version 2.7 to be released at the end of September.

Comment 34

[Joe Olivas]

2.7 is out.

http://software.intel.com/en-us/articles/intel-power-gadget/

Comment 35

[Benoit Girard (:BenWa)]

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

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

Attachment: rebased

Comment 38

[Joe Olivas]

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

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

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]

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]

Attachment: bug769431-rebased.patch

Update to use the environment variable set by the Power Gadget installer.

Comment 43

[Benoit Girard (:BenWa)]

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]

Attachment: 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)]

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

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

Add-on version 1.12.14 should correctly select the feature

Comment 48

[Benoit Girard (:BenWa)]

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]

Attachment: 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)]

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

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

Attachment: patch - fixed style nits and non win build errors

Comment 53

[Benoit Girard (:BenWa)]

Once this is green we should be ready to land:
https://tbpl.mozilla.org/?tree=Try&rev=73e5d94a710b

Comment 54

[Joe Olivas]

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

Attachment: patch v3

Forgot to ifdef the power feature to win only.

Comment 56

[Benoit Girard (:BenWa)]

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

https://hg.mozilla.org/mozilla-central/rev/b48e11fc1f35