Made DartMessenger use the injected executor service for executing message handlers #29458
Conversation
|
@dnfield I read through the code and I think this is what we want. Do I need to make a separate injector field for this you think? We might have to add the TaskQueueFactory to the injector but this should solve all of our current cases. Let me know if you want to see me pull this out so we can run extra tests on it. Let me know what you think. cc @xster |
shell/platform/android/io/flutter/embedding/engine/dart/DartMessenger.java
Show resolved
Hide resolved
gaaclarke
force-pushed
the
task-queue-injector
branch
from
0572d81 to
3cfbb23
Compare
gaaclarke
force-pushed
the
task-queue-injector
branch
from
3cfbb23 to
31f35c3
Compare
|
Okay, added a test and cleaned up the docstrings. Ready for review. |
| try { | ||
| @Nullable Runnable runnable = queue.poll(); | ||
| if (runnable != null) { | ||
| runnable.run(); |
There was a problem hiding this comment.
Why not just flush the whole queue from here? What's the advantage of resubmitting to the executor for each task?
There was a problem hiding this comment.
We could. This is more fair to allow other tasks to execute so one TaskQueue can't dominate a whole thread.
There was a problem hiding this comment.
Ok. I think that makes sense. This may be something to revisit as we look at more traces/benchmarks - particularly in cases on devices with few cores.
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
LGTM. I think we may want to revisit which executorService we use, and whether flutterinjector needs more than one, but for now this is good.
…uting message handlers. (flutter/engine#29458)
3 participants
fixes flutter/flutter#92858
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.