Reland "Listen for Vsync callback on the UI thread directly" #31494
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
ColdPaleLight
changed the title
ColdPaleLight
changed the title
|
@ColdPaleLight Is #31546 the fix for the revert? |
Yes! I was able to reproduce the failed test locally, and finally located the cause. I have verified it myself |
| bool MessageLoopAndroid::Register(JNIEnv* env) { | ||
| jclass clazz = env->FindClass("android/os/Looper"); | ||
|
|
||
| if (clazz == nullptr) { |
There was a problem hiding this comment.
simplification nit: this could be a FML_CHECK, and the FML_CHECK in the consumption side can be dropped.
| g_looper_class->obj(), "myLooper", "()Landroid/os/Looper;"); | ||
| FML_CHECK(g_looper_my_looper_method_ != nullptr); | ||
|
|
||
| g_looper_quit_method_ = |
There was a problem hiding this comment.
There's no check for g_looper_quit_method_ != nullptr. Is this intended?
There was a problem hiding this comment.
It's missed before, added now
| running_ = false; | ||
| } | ||
| } | ||
| LooperPrepare(); |
There was a problem hiding this comment.
does this means that ALooper_forThread() returns null? or are we calling prepare unnecessarily?
There was a problem hiding this comment.
nvm, this is initializing the state in the JVM, right? could we add comments?
| } | ||
| } | ||
|
|
||
| bool MessageLoopAndroid::Register(JNIEnv* env) { |
There was a problem hiding this comment.
is it possible to add a comment to indicate that this could just use AChoreographer once Flutter drops supports for API level < 24?
There's not concrete plan, but worth mentioning for future maintainers that using the NDK simplifies this quite a bit.
There was a problem hiding this comment.
There is a related issue here: flutter/flutter#62166.
I think in the future, if we need to replace the current implementation with NDK, we need to re-evaluate the possible benefits it brings, such as how much performance can be improved, whether the code is simple and maintainable, etc. So I think it's more reasonable to let it exist as an issue instead of writing it in the comments :-)
ColdPaleLight
added
the
waiting for tree to go green
…lutter#31494)" This reverts commit a67a366. This was causing a StrictMode.detectLeakedClosableObjects warning because a Choreographer instance was obtained on the Dart/UI thread but it was not being disposed on some versions of Android. The engine may need to implement this using NDK APIs instead of JNI (see flutter/flutter#62166)
|
@ColdPaleLight this change got reverted for the reason specified in the revert PR. This is generally a good optimization, so if you'd like to reland it, you could try to use the NDK API for API level >= 24 and add TODO to remove the JNI fallback when we drop support for API level < 24. |
3 participants
revert #29923
reland #29889
I think it can be reland since flutter/flutter#98824 has been closed.
Pre-launch Checklist
writing and running engine tests.
///).