Create a thermal manager class and wire it in to nanobench behind a flag

Create a thermal manager class and wire it in to nanobench behind a flag

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1671573002

Committed: https://skia.googlesource.com/skia/+/3ebd050be551e9d1364dbd115708e8f86397cd6f

[joshua.litt, 2016-02-04 21:38:47]

Description was changed from

==========
Create a thermal manager class and wire it in to nanobench behind a flag

BUG=skia:
==========

to

==========
Create a thermal manager class and wire it in to nanobench behind a flag

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[joshualitt, 2016-02-04 21:43:44]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, djsollen@google.com, joshualitt@google.com

[joshualitt, 2016-02-04 21:43:45]

ptal, it turns out we can read sysfs even when not root, though I'm not sure if
its the same sysfs table as the root table.

[djsollen, 2016-02-05 04:54:15]

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.cpp
File tools/ThermalManager.cpp (right):

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.cp...
tools/ThermalManager.cpp:71: fDisabled = fBase >= fPoint + fThreshold;  // We
disable any trip point which start off
why do we need to disable these?

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h
File tools/ThermalManager.h (right):

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h#...
tools/ThermalManager.h:14: #define USE_THERMAL_MANAGER
defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
It would be more clear to me if you wrote it something like...

#if defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
#    define THERMAL_MANAGER_SUPPORTED
#endif

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h#...
tools/ThermalManager.h:26: bool coolOffIfNecessary(uint32_t sleepInterval = 10,
uint32_t maxSleeps = 1000);
can you document that the sleep interval is in seconds, ms, ns?  Also I think it
is more clear to have a timeout value instead of maxSleeps, since the latter
requires me to do the math in my head each time to get the timeout value (which
to me is the number I care about).

[joshualitt, 2016-02-05 13:50:20]

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.cpp
File tools/ThermalManager.cpp (right):

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.cp...
tools/ThermalManager.cpp:71: fDisabled = fBase >= fPoint + fThreshold;  // We
disable any trip point which start off
On 2016/02/05 04:54:15, djsollen wrote:
> why do we need to disable these?

I added a comment in the header.  Some trip points seem to be negative values or
0 or start off lower than the base reading from temp.  No amount of sleeping
will work in these cases.

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h
File tools/ThermalManager.h (right):

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h#...
tools/ThermalManager.h:14: #define USE_THERMAL_MANAGER
defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
On 2016/02/05 04:54:15, djsollen wrote:
> It would be more clear to me if you wrote it something like...
> 
> #if defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
> #    define THERMAL_MANAGER_SUPPORTED
> #endif

Acknowledged.

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h#...
tools/ThermalManager.h:14: #define USE_THERMAL_MANAGER
defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
On 2016/02/05 04:54:15, djsollen wrote:
> It would be more clear to me if you wrote it something like...
> 
> #if defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
> #    define THERMAL_MANAGER_SUPPORTED
> #endif

Acknowledged.

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h#...
tools/ThermalManager.h:26: bool coolOffIfNecessary(uint32_t sleepInterval = 10,
uint32_t maxSleeps = 1000);
On 2016/02/05 04:54:15, djsollen wrote:
> can you document that the sleep interval is in seconds, ms, ns?  Also I think
it
> is more clear to have a timeout value instead of maxSleeps, since the latter
> requires me to do the math in my head each time to get the timeout value
(which
> to me is the number I care about).

Acknowledged.

[mtklein, 2016-02-05 13:54:21]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2016-02-05 13:54:22]

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h
File tools/ThermalManager.h (right):

https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h#...
tools/ThermalManager.h:14: #define USE_THERMAL_MANAGER
defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
On 2016/02/05 04:54:15, djsollen wrote:
> It would be more clear to me if you wrote it something like...
> 
> #if defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
> #    define THERMAL_MANAGER_SUPPORTED
> #endif

More than that, Derek's version is legal preprocessor.  The version written here
doesn't necessarily work.  defined and !defined go in #if and #elif only.

[joshualitt, 2016-02-08 20:30:02]

On 2016/02/05 13:54:22, mtklein wrote:
> https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h
> File tools/ThermalManager.h (right):
> 
>
https://codereview.chromium.org/1671573002/diff/20001/tools/ThermalManager.h#...
> tools/ThermalManager.h:14: #define USE_THERMAL_MANAGER
> defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
> On 2016/02/05 04:54:15, djsollen wrote:
> > It would be more clear to me if you wrote it something like...
> > 
> > #if defined(SK_BUILD_FOR_ANDROID) || defined(SK_BUILD_FOR_UNIX)
> > #    define THERMAL_MANAGER_SUPPORTED
> > #endif
> 
> More than that, Derek's version is legal preprocessor.  The version written
here
> doesn't necessarily work.  defined and !defined go in #if and #elif only.

any other feedback? I'd like to land this so we can start experimenting on the
bots to see if it improves reliability.

[djsollen, 2016-02-08 20:59:33]

https://codereview.chromium.org/1671573002/diff/80001/tools/ThermalManager.cpp
File tools/ThermalManager.cpp (right):

https://codereview.chromium.org/1671573002/diff/80001/tools/ThermalManager.cp...
tools/ThermalManager.cpp:70: FILE* tempFile = fopen(path, "r");
I can't think of any, but is there a way to avoid the fopen/fclose for each file
in /sys/class/thermal in between each test?

https://codereview.chromium.org/1671573002/diff/80001/tools/ThermalManager.h
File tools/ThermalManager.h (right):

https://codereview.chromium.org/1671573002/diff/80001/tools/ThermalManager.h#...
tools/ThermalManager.h:26: ThermalManager(int32_t threshold = 1);
don't provide defaults if the code will never use them.

https://codereview.chromium.org/1671573002/diff/80001/tools/ThermalManager.h#...
tools/ThermalManager.h:28: bool coolOffIfNecessary(uint32_t sleepIntervalMs =
10, uint32_t timeoutMs = 1000);
since these never change why not pass them in the constructor?

[joshualitt, 2016-02-09 13:51:54]

https://codereview.chromium.org/1671573002/diff/80001/tools/ThermalManager.cpp
File tools/ThermalManager.cpp (right):

https://codereview.chromium.org/1671573002/diff/80001/tools/ThermalManager.cp...
tools/ThermalManager.cpp:70: FILE* tempFile = fopen(path, "r");
On 2016/02/08 20:59:33, djsollen wrote:
> I can't think of any, but is there a way to avoid the fopen/fclose for each
file
> in /sys/class/thermal in between each test?

There can be many of these.  The nexus six has nearly 20.  I might be able to
keep them open, but given its untimed I'm not sure why it matters.

However, if you have a strong preference, I can probably find a way to keep them
open.

https://codereview.chromium.org/1671573002/diff/80001/tools/ThermalManager.h
File tools/ThermalManager.h (right):

https://codereview.chromium.org/1671573002/diff/80001/tools/ThermalManager.h#...
tools/ThermalManager.h:26: ThermalManager(int32_t threshold = 1);
On 2016/02/08 20:59:33, djsollen wrote:
> don't provide defaults if the code will never use them.

Acknowledged.

https://codereview.chromium.org/1671573002/diff/80001/tools/ThermalManager.h#...
tools/ThermalManager.h:28: bool coolOffIfNecessary(uint32_t sleepIntervalMs =
10, uint32_t timeoutMs = 1000);
On 2016/02/08 20:59:33, djsollen wrote:
> since these never change why not pass them in the constructor?

Acknowledged.

[djsollen, 2016-02-09 13:56:16]

lgtm

[joshualitt, 2016-02-09 13:59:24]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2016-02-09 13:59:25]

The CQ bit was unchecked by joshualitt@google.com

[joshualitt, 2016-02-09 13:59:31]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2016-02-09 13:59:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1671573002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1671573002/100001

[commit-bot: I haz the power, 2016-02-09 14:02:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-09 14:02:01]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[joshualitt, 2016-02-09 14:21:47]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2016-02-09 14:21:48]

The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1671573002/#ps120001
(title: "tweaks")

[commit-bot: I haz the power, 2016-02-09 14:21:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1671573002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1671573002/120001

[commit-bot: I haz the power, 2016-02-09 15:18:11]

Description was changed from

==========
Create a thermal manager class and wire it in to nanobench behind a flag

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Create a thermal manager class and wire it in to nanobench behind a flag

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/3ebd050be551e9d1364dbd115708e8f86397cd6f
==========

[commit-bot: I haz the power, 2016-02-09 15:18:12]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/3ebd050be551e9d1364dbd115708e8f86397cd6f

[scroggo, 2016-02-10 17:01:10]

scroggo@google.com changed reviewers:
+ kjlubick@google.com, scroggo@google.com

[scroggo, 2016-02-10 17:01:11]

https://codereview.chromium.org/1671573002/diff/120001/tools/ThermalManager.cpp
File tools/ThermalManager.cpp (right):

https://codereview.chromium.org/1671573002/diff/120001/tools/ThermalManager.c...
tools/ThermalManager.cpp:61: sleep(fSleepIntervalMs);
This breaks the AOSP mips build. From [1]:

external/skia/bench/../tools/ThermalManager.cpp:61:13: error: use of undeclared
identifier 'sleep'
            sleep(fSleepIntervalMs);

The fix for this should look like crrev.com/1568883002

Derek, this is the third time we've broken AOSP mips by calling sleep. Should we
add a builder (on the CQ) that builds our testing libraries on mips so we can
catch this sooner?

[1]
https://android-build.storage.googleapis.com/builds/git_master-skia-linux-aos...