Repository navigation
Conversation
The adpf-pure-dual branch has had master merged in, so comparing it against the pinned base commit showed hundreds of unrelated commits. Compare against master instead. Also link dev/benchmarks as a tree.
|
|
||
| ### Interaction with the Android Embedder API migration | ||
|
|
||
| Today, Flutter already places Android-specific OS scheduler hints in `flutter/fml/` behind a platform-agnostic interface that is a no-op on other OSes (see [`fml/cpu_affinity.h`](https://github.com/flutter/flutter/blob/4c15b121486e83f0faecee2edb4e86bd41055e19/engine/src/flutter/fml/cpu_affinity.h) and [`fml/platform/android/cpu_affinity.cc`](https://github.com/flutter/flutter/blob/4c15b121486e83f0faecee2edb4e86bd41055e19/engine/src/flutter/fml/platform/android/cpu_affinity.cc)). When `AndroidShellHolder` migrates to the C Embedder API ([flutter/rfc#24](https://github.com/flutter/rfc/pull/24)), we can follow that same pattern so **no `embedder.h` API changes are required**: |
There was a problem hiding this comment.
This seems reasonable to me. We can always add the embedder api later if we really want to push these platform specifics out of the engine. But since they are there now, I'd just keep the same pattern. WDYT @loic-sharma
| 4. Forwards both hint managers through `PlatformViewAndroid::CreateVSyncWaiter` into `VsyncWaiterAndroid` so [`VsyncWaiterAndroid::AwaitVSync`](https://github.com/flutter/flutter/blob/4c15b121486e83f0faecee2edb4e86bd41055e19/engine/src/flutter/shell/platform/android/vsync_waiter_android.cc#L29-L61) can call **[`APerformanceHint_notifyWorkloadReset`](https://developer.android.com/ndk/reference/group/a-performance-hint#aperformancehint_notifyworkloadreset) on Android 16+ (API 36+) when a frame is requested more than 100 ms after the session's last notification** (capped at 3 resets between actual work reports), telling the PowerHAL that the workload history from before the idle period is stale (mirroring Android HWUI's [`HintSessionWrapper::sendLoadResetHint`](https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/libs/hwui/renderthread/HintSessionWrapper.cpp)). | ||
| 5. **Closes both [`APerformanceHintSession`](https://developer.android.com/ndk/reference/group/a-performance-hint#aperformancehintsession) instances** (via [`APerformanceHint_closeSession`](https://developer.android.com/ndk/reference/group/a-performance-hint#aperformancehint_closesession)) and frees their [`AWorkDuration`](https://developer.android.com/ndk/reference/group/a-performance-hint#aworkduration) structs (via [`AWorkDuration_release`](https://developer.android.com/ndk/reference/group/a-performance-hint#aworkduration_release)) in [`AndroidShellHolder::~AndroidShellHolder`](https://github.com/flutter/flutter/blob/4c15b121486e83f0faecee2edb4e86bd41055e19/engine/src/flutter/shell/platform/android/android_shell_holder.cc#L209-L212) before `thread_host_.reset()` joins and destroys `ThreadHost`'s OS threads, ensuring the sessions are closed while their registered thread IDs are still alive. | ||
|
|
||
| ### Note on merged threads for HC |
There was a problem hiding this comment.
Since this has come up a few times, I just want to go on record saying we should remove this. I may put up a few PRs to just remove this since we don't want it codified in the Embedder API (for #24)
gaaclarke
left a comment
There was a problem hiding this comment.
I mentioned in the issue that my 2 concerns are:
- Opportunity cost and skepticism this is higher priority
- Concerns about maintenance on a feature that only affects 10% of devices
If you want to convince me otherwise I think the things you could add to your doc are:
- What problem are you trying to solve? I think if the problem is "I want to make 10% of the fastest devices 10% faster" I don't think that's a very compelling proposition.
- Adding a section on how we will test and maintain this would be helpful. For example, do we have devicelab devices that can use this and run benchmarks on it?
I think my take right now is that this is something we do 5 years from now when the landscape is different and instead focus on raising the floor for our worst devices.
I think I strongly disagree that making 10% of all android devices 20-40% faster, and in particular having that effect kick in when the devices would jank (miss frame deadlines) is not impactful. I'd be surprised if we have many examples of work that achieved a similar outcome/effort ratio.
Sure I can add this. I want to stress that the feature is "take metrics we already measure, plumb them to android, and let android handle it". I don't agree that maintenance will be high here. |
Just to be clear, I wasn't asking that question to be answered in the comments. I was asking for it to be put in the doc, "what problem are you attempting to solve?" =) |
I think those numbers are pretty compelling, like 10s of millions of phones/users. To put that in perspective, in the past we have pushed back on dropping older versions of SDKs when usage was even ~2%, even when there's some maintenance burden trade off. This particular rfc is proposing something that seems to have very little maintenance burden for a large performance improvement on a large number (10% seems large! certainly large in absolute numbers) of devices.
I find this argument compelling as well. Flutter apps could add a small amount of plumbing and the scheduling will behave more like vanilla Android apps, in exchange for some plumbing and very little downside. |
A proposal to adopt Android's
APerformanceHintManagerNDK API (Android 15+ / API 35+) into the Android engine to report per-frame UI and raster thread work durations and post-idle workload resets (Android 16+ / API 36+) to the OS scheduler.gmackall/flutter@adpf-pure-dual, which is logically bucket-able into the following:gmackall/flutter@adpf-pr1-frame-built-callback—[Engine] Add Settings::frame_built_callback for UI frame build timingsgmackall/flutter@adpf-pr2-hint-manager—[Android] Add AndroidPerformanceHintManager wrapper for ADPF API 35+gmackall/flutter@adpf-pr3-dual-session-wiring—[Android] Wire dual-session ADPF UI and Raster work duration reportinggmackall/flutter@adpf-pr4-workload-reset—[Android] Send ADPF notifyWorkloadReset hint on vsync after >100ms idleTracking issue: flutter/flutter#195992 (related: flutter/flutter#114835)
Pre-launch Checklist
design doc.dart run bin/assign_rfc_number.dartlocally when instructed).