-
Notifications
You must be signed in to change notification settings - Fork 6k
[Android] Add support for setting thread affinity based on core speed. #45673
Changes from 14 commits
78e513e
7c9b5c9
a871abd
aa4329d
cdf81f5
094d8bc
13f4353
f9a70a7
2135bce
e8b5a7e
e9d762d
2a336e9
653e28b
d54b976
44e8afa
e7dc40b
6b5e848
0168863
9a04811
8804f7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,119 @@ | ||
// Copyright 2013 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
#include "flutter/fml/cpu_affinity.h" | ||
|
||
#include <optional> | ||
#include <sstream> | ||
#include <string> | ||
|
||
namespace fml { | ||
|
||
CPUSpeedTracker::CPUSpeedTracker(std::vector<CpuIndexAndSpeed> data) | ||
: cpu_speeds_(std::move(data)) { | ||
std::optional<int32_t> max_speed = std::nullopt; | ||
std::optional<int32_t> min_speed = std::nullopt; | ||
for (const auto& data : cpu_speeds_) { | ||
if (!max_speed.has_value() || data.speed > max_speed.value()) { | ||
max_speed = data.speed; | ||
} | ||
if (!min_speed.has_value() || data.speed < min_speed.value()) { | ||
min_speed = data.speed; | ||
} | ||
} | ||
if (!max_speed.has_value() || !min_speed.has_value() || | ||
min_speed.value() == max_speed.value()) { | ||
return; | ||
} | ||
|
||
for (const auto& data : cpu_speeds_) { | ||
if (data.speed == max_speed.value()) { | ||
performance_.push_back(data.index); | ||
} else { | ||
not_performance_.push_back(data.index); | ||
} | ||
if (data.speed == min_speed.value()) { | ||
efficiency_.push_back(data.index); | ||
} | ||
} | ||
|
||
valid_ = true; | ||
} | ||
|
||
bool CPUSpeedTracker::IsValid() const { | ||
return valid_; | ||
} | ||
|
||
const std::vector<size_t>& CPUSpeedTracker::GetIndices( | ||
CpuAffinity affinity) const { | ||
switch (affinity) { | ||
case CpuAffinity::kPerformance: | ||
return performance_; | ||
case CpuAffinity::kEfficiency: | ||
return efficiency_; | ||
case CpuAffinity::kNotPerformance: | ||
return not_performance_; | ||
} | ||
} | ||
|
||
// Get the size of the cpuinfo file by reading it until the end. This is | ||
// required because files under /proc do not always return a valid size | ||
// when using fseek(0, SEEK_END) + ftell(). Nor can they be mmap()-ed. | ||
std::optional<int32_t> ReadIntFromFile(const std::string& path) { | ||
jonahwilliams marked this conversation as resolved.
Show resolved
Hide resolved
|
||
size_t data_length = 0u; | ||
FILE* fp = fopen(path.c_str(), "r"); | ||
if (fp == nullptr) { | ||
return std::nullopt; | ||
} | ||
for (;;) { | ||
char buffer[256]; | ||
size_t n = fread(buffer, 1, sizeof(buffer), fp); | ||
if (n == 0) { | ||
break; | ||
} | ||
data_length += n; | ||
} | ||
fclose(fp); | ||
|
||
if (data_length <= 0) { | ||
return std::nullopt; | ||
} | ||
|
||
// Read the contents of the cpuinfo file. | ||
char* data = reinterpret_cast<char*>(malloc(data_length + 1)); | ||
fp = fopen(path.c_str(), "r"); | ||
if (fp == nullptr) { | ||
free(data); | ||
return std::nullopt; | ||
} | ||
for (uintptr_t offset = 0; offset < data_length;) { | ||
size_t n = fread(data + offset, 1, data_length - offset, fp); | ||
if (n == 0) { | ||
break; | ||
} | ||
offset += n; | ||
} | ||
fclose(fp); | ||
|
||
if (data == nullptr) { | ||
free(data); | ||
return std::nullopt; | ||
} | ||
// Ensure zeroed end of buffer before reading. | ||
data[data_length] = 0; | ||
|
||
// Dont use stoi because if this data isnt a parseable number then it | ||
// will abort, as we compile with exceptions disabled. | ||
int speed = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be int64_t? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or maybe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. long long lints that we should use int64_t |
||
std::istringstream input(data); | ||
input >> speed; | ||
free(data); | ||
|
||
if (speed > 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm reading the docs right, then https://cplusplus.com/reference/istream/istream/operator%3E%3E/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, the tests I added, which include tests for missing files and non-numbers still pass without checking - I assume because in these cases we don't read anything out of the stream? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yeah, if the open or read fails, then |
||
return speed; | ||
} | ||
return std::nullopt; | ||
} | ||
|
||
} // namespace fml |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
// Copyright 2013 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
#pragma once | ||
|
||
#include <optional> | ||
#include <string> | ||
#include <vector> | ||
|
||
namespace fml { | ||
|
||
/// The CPU Affinity provides a hint to the operating system on which cores a | ||
/// particular thread should be scheduled on. The operating system may or may | ||
/// not honor these requests. | ||
enum class CpuAffinity { | ||
/// @brief Request CPU affinity for the performance cores. | ||
/// | ||
/// Generally speaking, only the UI and Raster thread should | ||
/// use this option. | ||
kPerformance, | ||
|
||
/// @brief Request CPU affinity for the efficiency cores. | ||
kEfficiency, | ||
|
||
/// @brief Request affinity for all non-performance cores. | ||
kNotPerformance, | ||
}; | ||
|
||
struct CpuIndexAndSpeed { | ||
size_t index; | ||
int32_t speed; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about 64 bits here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
}; | ||
|
||
/// @brief A class that computes the correct CPU indices for a requested CPU | ||
/// affinity. | ||
/// | ||
/// Note: this is visible for testing. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In docstrings, I believe this can be an at-note (sorry for whoever I pinged earlier.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
class CPUSpeedTracker { | ||
public: | ||
explicit CPUSpeedTracker(std::vector<CpuIndexAndSpeed> data); | ||
|
||
/// @brief The class is valid if it has more than one CPU index and a distinct | ||
/// set of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and elsewhere, weird line breaks on docstrings. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
/// efficiency or performance CPUs. If all CPUs are the same speed this | ||
/// returns false, and all requests to set affinity are ignored. | ||
bool IsValid() const; | ||
|
||
/// @brief Return the set of CPU indices for the requested CPU affinity. | ||
/// | ||
/// If the tracker is valid, this will always return a non-empty set. | ||
const std::vector<size_t>& GetIndices(CpuAffinity affinity) const; | ||
|
||
private: | ||
bool valid_ = false; | ||
std::vector<CpuIndexAndSpeed> cpu_speeds_; | ||
std::vector<size_t> efficiency_; | ||
std::vector<size_t> performance_; | ||
std::vector<size_t> not_performance_; | ||
}; | ||
|
||
/// Visible for testing. | ||
std::optional<int32_t> ReadIntFromFile(const std::string& path); | ||
jonahwilliams marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
} // namespace fml |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
// Copyright 2013 The Flutter Authors. All rights reserved. | ||
// Use of this source code is governed by a BSD-style license that can be | ||
// found in the LICENSE file. | ||
|
||
#include "cpu_affinity.h" | ||
|
||
#include "fml/file.h" | ||
#include "fml/mapping.h" | ||
#include "gtest/gtest.h" | ||
#include "logging.h" | ||
|
||
namespace fml { | ||
namespace testing { | ||
|
||
TEST(CpuAffinity, NormalSlowMedFastCores) { | ||
auto speeds = {CpuIndexAndSpeed{.index = 0, .speed = 1}, | ||
CpuIndexAndSpeed{.index = 1, .speed = 2}, | ||
CpuIndexAndSpeed{.index = 2, .speed = 3}}; | ||
auto tracker = CPUSpeedTracker(speeds); | ||
|
||
ASSERT_TRUE(tracker.IsValid()); | ||
ASSERT_EQ(tracker.GetIndices(CpuAffinity::kEfficiency)[0], 0u); | ||
ASSERT_EQ(tracker.GetIndices(CpuAffinity::kPerformance)[0], 2u); | ||
ASSERT_EQ(tracker.GetIndices(CpuAffinity::kNotPerformance).size(), 2u); | ||
ASSERT_EQ(tracker.GetIndices(CpuAffinity::kNotPerformance)[0], 0u); | ||
ASSERT_EQ(tracker.GetIndices(CpuAffinity::kNotPerformance)[1], 1u); | ||
} | ||
|
||
TEST(CpuAffinity, NoCpuData) { | ||
auto tracker = CPUSpeedTracker({}); | ||
|
||
ASSERT_FALSE(tracker.IsValid()); | ||
} | ||
|
||
TEST(CpuAffinity, AllSameSpeed) { | ||
auto speeds = {CpuIndexAndSpeed{.index = 0, .speed = 1}, | ||
CpuIndexAndSpeed{.index = 1, .speed = 1}, | ||
CpuIndexAndSpeed{.index = 2, .speed = 1}}; | ||
auto tracker = CPUSpeedTracker(speeds); | ||
|
||
ASSERT_FALSE(tracker.IsValid()); | ||
} | ||
|
||
TEST(CpuAffinity, SingleCore) { | ||
auto speeds = {CpuIndexAndSpeed{.index = 0, .speed = 1}}; | ||
auto tracker = CPUSpeedTracker(speeds); | ||
|
||
ASSERT_FALSE(tracker.IsValid()); | ||
} | ||
|
||
TEST(CpuAffinity, FileParsing) { | ||
fml::ScopedTemporaryDirectory base_dir; | ||
ASSERT_TRUE(base_dir.fd().is_valid()); | ||
|
||
// Generate a fake CPU speed file | ||
fml::DataMapping test_data(std::string("12345")); | ||
ASSERT_TRUE(fml::WriteAtomically(base_dir.fd(), "test_file", test_data)); | ||
|
||
auto file = fml::OpenFileReadOnly(base_dir.fd(), "test_file"); | ||
ASSERT_TRUE(file.is_valid()); | ||
|
||
// Open file and parse speed. | ||
auto result = ReadIntFromFile(base_dir.path() + "/test_file"); | ||
ASSERT_TRUE(result.has_value()); | ||
ASSERT_EQ(result.value_or(0), 12345); | ||
} | ||
|
||
TEST(CpuAffinity, FileParsingWithNonNumber) { | ||
fml::ScopedTemporaryDirectory base_dir; | ||
ASSERT_TRUE(base_dir.fd().is_valid()); | ||
|
||
// Generate a fake CPU speed file | ||
fml::DataMapping test_data(std::string("whoa this isnt a number")); | ||
ASSERT_TRUE(fml::WriteAtomically(base_dir.fd(), "test_file", test_data)); | ||
|
||
auto file = fml::OpenFileReadOnly(base_dir.fd(), "test_file"); | ||
ASSERT_TRUE(file.is_valid()); | ||
|
||
// Open file and parse speed. | ||
auto result = ReadIntFromFile(base_dir.path() + "/test_file"); | ||
ASSERT_FALSE(result.has_value()); | ||
} | ||
|
||
TEST(CpuAffinity, MissingFileParsing) { | ||
auto result = ReadIntFromFile("/does_not_exist"); | ||
ASSERT_FALSE(result.has_value()); | ||
} | ||
|
||
} // namespace testing | ||
} // namespace fml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is scary looking but we're running lsan and asan over the tests. Unfortunately cant use the fml file APIs due to the note above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also lsan and asan helped me fix several bugs here 😆 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can
std::ifstream
read from/proc
and/sys
? If it can, I suspect that would simplify this code.If it can't, I'll do a close review of this code =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should note that I did mostly copy this from the Dart SDK. Will try std::ifstream though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah,
std::
was/is banned from the Dart VM, but for new code in the Engine, I believe it is not banned =) cc @chinmaygarde in case he has different advice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd highly recommend fml::UniqueFD, std::ifstream, std::vector (for data), etc.. IIRC, here and elsewhere, you need to loop through freads as well because the size read can be less than the requested size. May well not be a concern here but this doesn't seem like sufficient error handling. And if we we were to refactor this in the future, I'd be wary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ifstream appears to work!