Correlate performance data with Ivy Bridge MSR_PPX_ENERGY_STATUS for power usage profiles

RESOLVED FIXED in mozilla28

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: BenWa, Assigned: Joe Olivas)

Tracking

unspecified
mozilla28
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 14 obsolete attachments)

22.58 KB, text/plain
Details
(Reporter)

Description

5 years ago
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/.
(Reporter)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
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/
(Assignee)

Comment 3

5 years ago
More sample code

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

Comment 4

5 years ago
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.
(Reporter)

Comment 5

5 years ago
(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.
(Reporter)

Comment 6

4 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 7

4 years ago
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.
(Reporter)

Comment 8

4 years ago
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Reporter)

Comment 9

4 years ago
Here's an interesting analysis that was done on Firefox in the past:
http://softwareprocess.es/static/GreenMining.html
(Assignee)

Comment 10

4 years ago
Created attachment 721830 [details] [diff] [review]
Rough prototype v1

Initial prototype to get energy (in joules).
(Assignee)

Comment 11

4 years ago
Created attachment 724679 [details] [diff] [review]
MSR reader demo version 2

Updated to fix some problems with internal state in the PCM library.
Attachment #721830 - Attachment is obsolete: true
(Assignee)

Comment 12

4 years ago
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.
(Reporter)

Comment 13

4 years ago
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.
(Reporter)

Comment 14

4 years ago
Licensing is great! Checked the driver source into the repo:
https://github.com/bgirard/Gecko-Profiler-Addon/tree/master/driver
(Assignee)

Comment 15

4 years ago
Created attachment 765416 [details] [diff] [review]
MSR reader - Rebased WIP

Rebasing previous patch. Verified that data is reported from the profiler as before.
Attachment #724679 - Attachment is obsolete: true
(Reporter)

Comment 16

4 years ago
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.
(Reporter)

Comment 17

4 years ago
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"
(Assignee)

Comment 18

4 years ago
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.
(Assignee)

Comment 19

4 years ago
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.
Attachment #765416 - Attachment is obsolete: true
(Reporter)

Comment 20

4 years ago
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.
(Assignee)

Comment 21

4 years ago
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.
(Reporter)

Comment 22

4 years ago
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?
(Assignee)

Comment 23

4 years ago
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.
(Reporter)

Comment 24

4 years ago
Created attachment 766801 [details] [diff] [review]
Delta

Posting a delta to show what I've changed
(Reporter)

Comment 25

4 years ago
Created attachment 766802 [details] [diff] [review]
WIP v4
(Reporter)

Comment 26

4 years ago
Created attachment 766847 [details] [diff] [review]
Delta
Attachment #766801 - Attachment is obsolete: true
(Assignee)

Comment 27

4 years ago
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 :)
(Assignee)

Comment 28

4 years ago
Here's an example data set with the new values in Watts:

http://people.mozilla.com/~bgirard/cleopatra/#report=282b6f5dd760bae07fb008bdd3ce1ee7b36a1f69
(Reporter)

Comment 29

4 years ago
(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.
(Reporter)

Comment 30

4 years ago
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
(Assignee)

Comment 31

4 years ago
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.
Attachment #724681 - Attachment is obsolete: true
Attachment #765999 - Attachment is obsolete: true
(Reporter)

Comment 32

4 years ago
Ohh wow, that's much better! Any ETA or pre-released version of the Power Gadget tools we can use to test this patch?
(Assignee)

Comment 33

4 years ago
The plan is for version 2.7 to be released at the end of September.
(Assignee)

Comment 34

4 years ago
2.7 is out.

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

Comment 35

4 years ago
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.
(Reporter)

Comment 36

4 years ago
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.
Flags: needinfo?(joseph.k.olivas)
(Reporter)

Comment 37

4 years ago
Created attachment 814965 [details] [diff] [review]
rebased
Attachment #766802 - Attachment is obsolete: true
Attachment #766847 - Attachment is obsolete: true
(Assignee)

Comment 38

4 years ago
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?
Flags: needinfo?(joseph.k.olivas)
(Reporter)

Comment 39

4 years ago
(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.
(Reporter)

Comment 40

4 years ago
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.
(Assignee)

Comment 41

4 years ago
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.
(Assignee)

Comment 42

4 years ago
Created attachment 815172 [details] [diff] [review]
bug769431-rebased.patch

Update to use the environment variable set by the Power Gadget installer.
Attachment #805613 - Attachment is obsolete: true
(Reporter)

Comment 43

4 years ago
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.
Attachment #815172 - Flags: feedback+
(Assignee)

Comment 44

4 years ago
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 :(
Attachment #815172 - Attachment is obsolete: true
Attachment #818130 - Flags: feedback?(bgirard)
(Reporter)

Comment 45

4 years ago
You have permissions but something is broken against the addon SDK trunk. I can add the feature to the addon myself.
(Reporter)

Comment 46

4 years ago
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.
Attachment #818130 - Flags: feedback?(bgirard) → feedback+
(Reporter)

Comment 47

4 years ago
Add-on version 1.12.14 should correctly select the feature
(Reporter)

Comment 48

4 years ago
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."
(Assignee)

Comment 49

4 years ago
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.
Attachment #818130 - Attachment is obsolete: true
Attachment #828350 - Flags: review?(bgirard)
(Reporter)

Comment 50

4 years ago
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.
Attachment #828350 - Flags: review?(bgirard) → review+
(Reporter)

Comment 51

4 years ago
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.
(Reporter)

Comment 52

4 years ago
Created attachment 828388 [details] [diff] [review]
patch - fixed style nits and non win build errors
Attachment #814965 - Attachment is obsolete: true
Attachment #828350 - Attachment is obsolete: true
(Reporter)

Comment 53

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

Comment 54

4 years ago
(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.
(Reporter)

Comment 55

4 years ago
Created attachment 828679 [details]
patch v3

Forgot to ifdef the power feature to win only.
Attachment #828388 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Assignee: nobody → joseph.k.olivas
(Reporter)

Comment 56

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/b48e11fc1f35
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.