Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Add support for setting thread affinity based on core speed. #45673

Merged

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Sep 11, 2023

flutter/flutter#134452

This patch parses the speed of all CPU data out of /proc and constructs a table that allows us to request high level CPU affinities: performance, efficiency, and not performance. These affinties are applied where appropriate during Android thread construction.

@jonahwilliams jonahwilliams changed the title bad thread affinity. [Android] Add support for setting thread affinity based on core speed. Sep 16, 2023
// 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) {
Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Member

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

Copy link
Member Author

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!

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ifstream appears to work!

@jonahwilliams jonahwilliams marked this pull request as ready for review September 17, 2023 00:45
@jonahwilliams
Copy link
Member Author

TODO: verify on some non-pixel phones, verify on an emulator

// will abort, as we compile with exceptions disabled.
int speed = 0;
file >> speed;
if (speed > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm reading the docs right, then file.fail() will be true if an int couldn't be read.

https://cplusplus.com/reference/istream/istream/operator%3E%3E/

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, if the open or read fails, then speed will just stay 0 and this check will fail.

std::optional<int32_t> ReadIntFromFile(const std::string& path) {
// size_t data_length = 0u;
std::ifstream file;
file.open(path.c_str());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file.fail() will be true if the open fails.

https://cplusplus.com/reference/fstream/ifstream/open/

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value of cpuinfo_max_freq is spec'd to be in kHz, but I'd switch it to an int64_t anyway.

Might also add a link somewhere to https://www.kernel.org/doc/Documentation/cpu-freq/user-guide.txt


struct CpuIndexAndSpeed {
size_t index;
int32_t speed;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about 64 bits here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// Dont use stoi because if this data isnt a parseable number then it
// will abort, as we compile with exceptions disabled.
int speed = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be int64_t?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe long long.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long long lints that we should use int64_t

Copy link
Member

@zanderso zanderso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm w/ small fixes.

explicit CPUSpeedTracker(std::vector<CpuIndexAndSpeed> data);

/// @brief The class is valid if it has more than one CPU index and a distinct
/// set of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere, weird line breaks on docstrings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

/// @brief Request the given affinity for the current thread.
///
/// Returns true if successfull, or if it was a no-op. This function is
/// only supported on Android devices.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linux too right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically the underlying APIs, but I only added support to Android platform code as testing the linux embedding as well as arbitrary 3rd party linux embedders seems like a bit of a nightmare.

/// The CPUSpeedTracker is initialized once the first time a thread affinity is
/// requested.
std::once_flag cpu_tracker_flag_;
static CPUSpeedTracker* tracker_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, the naming convention for these is gCPUTrackerFlag

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
Copy link
Member

Choose a reason for hiding this comment

The 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.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// 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) {
Copy link
Member

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.

@jonahwilliams
Copy link
Member Author

I updated to use ifstream. fml:: UniqueFD doesn't seem useful as the only way to read data out of them is FileMapping, which is mmap only and non-functional on these files.

fml/cpu_affinity.cc Outdated Show resolved Hide resolved
fml/cpu_affinity.h Outdated Show resolved Hide resolved
Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>
jonahwilliams and others added 2 commits September 18, 2023 17:30
Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>
@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 19, 2023
@auto-submit auto-submit bot merged commit 8ccc8a6 into flutter:main Sep 19, 2023
27 checks passed
@jonahwilliams jonahwilliams deleted the over_complicated_thread_affinity branch September 19, 2023 04:01
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 19, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 19, 2023
…134998)

flutter/engine@e1c784e...589bde9

2023-09-19 skia-flutter-autoroll@skia.org Roll Skia from 4122791099ce to 744807d740c7 (1 revision) (flutter/engine#46019)
2023-09-19 jonahwilliams@google.com [Android] Add support for setting thread affinity based on core speed. (flutter/engine#45673)
2023-09-19 chinmaygarde@google.com [Impeller] Fix STB backend to account for max texture sizes. (flutter/engine#46010)
2023-09-19 matanlurey@users.noreply.github.com [Impeller] Hold the CommandPoolVK at a higher scope. (flutter/engine#46013)
2023-09-19 skia-flutter-autoroll@skia.org Roll Skia from 0c990ab9e097 to 4122791099ce (19 revisions) (flutter/engine#46016)
2023-09-18 kjlubick@users.noreply.github.com Add missing include of SkPath (flutter/engine#45996)
2023-09-18 chinmaygarde@google.com [Impeller] Respect max supported texture size when allocating glyph atlas texture. (flutter/engine#45992)
2023-09-18 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 3_Lh8otTpmVuf-Zwb... to qy5FU4y6sx1FscCpd... (flutter/engine#45998)
2023-09-18 chris@bracken.jp Revert "[Windows] Update vsync on raster thread (#45310)" (flutter/engine#46000)
2023-09-18 matanlurey@users.noreply.github.com Provide a default `--target-variant` for `clang_tidy`. (flutter/engine#45909)
2023-09-18 ychris@google.com Revert "[ios] use python script to generate extension safe frameworks and code sign them" (flutter/engine#46004)
2023-09-18 john@johnmccutchan.com Disable HardwareBuffer backed Platform Views temporarily (flutter/engine#45986)
2023-09-18 john@johnmccutchan.com Tighten up ImageReaderPlatformViewRenderTarget code (flutter/engine#45889)
2023-09-18 ychris@google.com [ios] use python script to generate extension safe frameworks and code sign them (flutter/engine#45781)
2023-09-18 bdero@google.com Bump impeller-cmake to HEAD. (flutter/engine#45953)
2023-09-18 31859944+LongCatIsLooong@users.noreply.github.com [iOS] Remove selectionDidChange call in UndoManager (flutter/engine#45657)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from 3_Lh8otTpmVu to qy5FU4y6sx1F

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
// 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<int64_t> ReadIntFromFile(const std::string& path) {
// size_t data_length = 0u;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code

Mairramer pushed a commit to Mairramer/flutter that referenced this pull request Oct 10, 2023
…lutter#134998)

flutter/engine@e1c784e...589bde9

2023-09-19 skia-flutter-autoroll@skia.org Roll Skia from 4122791099ce to 744807d740c7 (1 revision) (flutter/engine#46019)
2023-09-19 jonahwilliams@google.com [Android] Add support for setting thread affinity based on core speed. (flutter/engine#45673)
2023-09-19 chinmaygarde@google.com [Impeller] Fix STB backend to account for max texture sizes. (flutter/engine#46010)
2023-09-19 matanlurey@users.noreply.github.com [Impeller] Hold the CommandPoolVK at a higher scope. (flutter/engine#46013)
2023-09-19 skia-flutter-autoroll@skia.org Roll Skia from 0c990ab9e097 to 4122791099ce (19 revisions) (flutter/engine#46016)
2023-09-18 kjlubick@users.noreply.github.com Add missing include of SkPath (flutter/engine#45996)
2023-09-18 chinmaygarde@google.com [Impeller] Respect max supported texture size when allocating glyph atlas texture. (flutter/engine#45992)
2023-09-18 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from 3_Lh8otTpmVuf-Zwb... to qy5FU4y6sx1FscCpd... (flutter/engine#45998)
2023-09-18 chris@bracken.jp Revert "[Windows] Update vsync on raster thread (flutter#45310)" (flutter/engine#46000)
2023-09-18 matanlurey@users.noreply.github.com Provide a default `--target-variant` for `clang_tidy`. (flutter/engine#45909)
2023-09-18 ychris@google.com Revert "[ios] use python script to generate extension safe frameworks and code sign them" (flutter/engine#46004)
2023-09-18 john@johnmccutchan.com Disable HardwareBuffer backed Platform Views temporarily (flutter/engine#45986)
2023-09-18 john@johnmccutchan.com Tighten up ImageReaderPlatformViewRenderTarget code (flutter/engine#45889)
2023-09-18 ychris@google.com [ios] use python script to generate extension safe frameworks and code sign them (flutter/engine#45781)
2023-09-18 bdero@google.com Bump impeller-cmake to HEAD. (flutter/engine#45953)
2023-09-18 31859944+LongCatIsLooong@users.noreply.github.com [iOS] Remove selectionDidChange call in UndoManager (flutter/engine#45657)

Also rolling transitive DEPS:
  fuchsia/sdk/core/mac-amd64 from 3_Lh8otTpmVu to qy5FU4y6sx1F

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
harryterkelsen pushed a commit that referenced this pull request Oct 23, 2023
#45673)

flutter/flutter#134452

This patch parses the speed of all CPU data out of /proc and constructs a table that allows us to request high level CPU affinities: performance, efficiency, and not performance. These affinties are applied where appropriate during Android thread construction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants