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

jonahwilliams · 2023-09-11T22:10:43Z

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.

…d_thread_affinity

jonahwilliams · 2023-09-17T00:14:55Z

fml/cpu_affinity.cc

+// 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) {


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.

(also lsan and asan helped me fix several bugs here 😆 )

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

I should note that I did mostly copy this from the Dart SDK. Will try std::ifstream though!

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.

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.

ifstream appears to work!

jonahwilliams · 2023-09-17T00:45:33Z

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

zanderso · 2023-09-18T23:16:17Z

fml/cpu_affinity.cc

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


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/

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?

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

zanderso · 2023-09-18T23:17:32Z

fml/cpu_affinity.cc

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


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

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

zanderso

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

zanderso · 2023-09-18T23:26:17Z

fml/cpu_affinity.h

+
+struct CpuIndexAndSpeed {
+  size_t index;
+  int32_t speed;


How about 64 bits here?

zanderso · 2023-09-18T23:26:28Z

fml/cpu_affinity.cc

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


Should this be int64_t?

Or maybe long long.

long long lints that we should use int64_t

zanderso

lgtm w/ small fixes.

chinmaygarde · 2023-09-18T17:20:13Z

fml/cpu_affinity.h

+  explicit CPUSpeedTracker(std::vector<CpuIndexAndSpeed> data);
+
+  /// @brief The class is valid if it has more than one CPU index and a distinct
+  /// set of


Here and elsewhere, weird line breaks on docstrings.

chinmaygarde · 2023-09-18T17:20:38Z

fml/platform/android/cpu_affinity.h

+/// @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.


Linux too right?

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.

chinmaygarde · 2023-09-18T17:21:39Z

fml/platform/android/cpu_affinity.cc

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


IIRC, the naming convention for these is gCPUTrackerFlag

chinmaygarde · 2023-09-18T17:23:53Z

fml/cpu_affinity.h

+/// @brief A class that computes the correct CPU indices for a requested CPU
+/// affinity.
+///
+///        Note: this is visible for testing.


In docstrings, I believe this can be an at-note (sorry for whoever I pinged earlier.)

chinmaygarde · 2023-09-18T17:28:44Z

fml/cpu_affinity.cc

+// 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) {


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 · 2023-09-18T23:54:15Z

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

fml/cpu_affinity.h

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

…core speed. (flutter/engine#45673)

…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

dnfield · 2023-09-19T16:29:21Z

fml/cpu_affinity.cc

+// 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;


Commented out code

…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

#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.

bad thread affinity.

78e513e

github-actions bot added the platform-android label Sep 11, 2023

jonahwilliams added 8 commits September 15, 2023 12:01

Merge branch 'main' of github.com:flutter/engine into over_complicate…

7c9b5c9

…d_thread_affinity

Add failed CPU parsing.

a871abd

++

aa4329d

++

cdf81f5

++

094d8bc

++

13f4353

refactor to FML.

f9a70a7

Unittesting.

2135bce

jonahwilliams changed the title ~~bad thread affinity.~~ [Android] Add support for setting thread affinity based on core speed. Sep 16, 2023

jonahwilliams added 5 commits September 16, 2023 15:42

++

e8b5a7e

++

e9d762d

licenses

2a336e9

++

653e28b

++

d54b976

jonahwilliams commented Sep 17, 2023

View reviewed changes

jonahwilliams marked this pull request as ready for review September 17, 2023 00:45

lol ifstream

44e8afa

jonahwilliams requested a review from zanderso September 18, 2023 23:09

zanderso reviewed Sep 18, 2023

View reviewed changes

zanderso approved these changes Sep 18, 2023

View reviewed changes

chinmaygarde reviewed Sep 18, 2023

View reviewed changes

int64_t

e7dc40b

doc cleanup

6b5e848

zanderso reviewed Sep 19, 2023

View reviewed changes

fml/cpu_affinity.cc Outdated Show resolved Hide resolved

fml/cpu_affinity.h Outdated Show resolved Hide resolved

Update fml/cpu_affinity.h

0168863

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

jonahwilliams and others added 2 commits September 18, 2023 17:30

Update fml/cpu_affinity.cc

9a04811

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>

Merge branch 'main' into over_complicated_thread_affinity

8804f7f

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 19, 2023

auto-submit bot merged commit 8ccc8a6 into flutter:main Sep 19, 2023
27 checks passed

jonahwilliams deleted the over_complicated_thread_affinity branch September 19, 2023 04:01

engine-flutter-autoroll mentioned this pull request Sep 19, 2023

Roll Flutter Engine from e1c784e3f841 to 589bde9a95c9 (16 revisions) flutter/flutter#134998

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 19, 2023

8ccc8a62f [Android] Add support for setting thread affinity based on …

40a1529

…core speed. (flutter/engine#45673)

dnfield reviewed Sep 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jonahwilliams commented Sep 11, 2023 •

edited

jonahwilliams Sep 17, 2023

jonahwilliams Sep 17, 2023

zanderso Sep 18, 2023

jonahwilliams Sep 18, 2023

zanderso Sep 18, 2023

chinmaygarde Sep 18, 2023

jonahwilliams Sep 18, 2023

jonahwilliams commented Sep 17, 2023

zanderso Sep 18, 2023

jonahwilliams Sep 18, 2023

zanderso Sep 18, 2023

zanderso Sep 18, 2023

zanderso left a comment

zanderso Sep 18, 2023

jonahwilliams Sep 18, 2023

zanderso Sep 18, 2023

zanderso Sep 18, 2023

jonahwilliams Sep 18, 2023

zanderso left a comment

chinmaygarde Sep 18, 2023

jonahwilliams Sep 19, 2023

chinmaygarde Sep 18, 2023

jonahwilliams Sep 18, 2023

chinmaygarde Sep 18, 2023

jonahwilliams Sep 19, 2023

chinmaygarde Sep 18, 2023

jonahwilliams Sep 19, 2023

chinmaygarde Sep 18, 2023

jonahwilliams commented Sep 18, 2023

dnfield Sep 19, 2023

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

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

Conversation

jonahwilliams commented Sep 11, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Sep 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanderso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Sep 18, 2023

Choose a reason for hiding this comment

jonahwilliams commented Sep 11, 2023 •

edited