MessagePort::dispatchMessages() should be smarter about how it schedules events |
|||||||
Issue descriptionMessagePort::dispatchMessages() should be smarter about how it schedules events When we notice message(s) to read from a MessagePort, we PostTask to the main thread and dispatch all messages before yielding. This could be a source of jank on the main thread.
,
Oct 4 2016
+gab so that luckyluke peeps are aware My comment here may be beyond the scope of this specific bug, but since I'm already typing, it seems at least to me like we're missing a set of principles that explain how mojo messages relate to tasks and priority. Right now we've got two great systems -- an ipc system, and a schedulable messageloop system. But, they mostly are designed separately. Seems like we could do well to bring their worlds more closely together, someday? Curious what others think. :)
,
Oct 4 2016
@nduca - I'm not sure I get your comment. The mojo c++ bindings need a base::SingleThreadTaskRunner in order to dispatch events. This appears to fit very nicely with the scheduler. Take a look at the WebSockets code. We just pass the loading TaskRunner to the WebSocket and that is passed along to the Mojo bindings, so every event related to WebSockets in the renderer process is managed by the scheduler. Isn't that pretty close to exactly what we want?
,
Oct 4 2016
Gah, yeah, I got MessagePort::dispatch confused with MessagePipeDispatcher. Class names crossed in my head, sorry for the noise! :)
,
Oct 5 2016
Right, using base::SingleThreadTaskRunner as the unit of schedulability seems to work nicely here. Gab, I assume Lucky Luke can work with that too?
,
Oct 5 2016
MessagePort::dispatchMessages() should be smarter about how it schedules events seems to work nicely here. Gab, I assume Lucky Luke can work with that too? One thing I've wondered is if we should prefer to instead use SequencedTaskRunner. I see no reason why we need to expose a "thread" detail or restrict an endpoint's scheduling to a single thread as long as it's sequenced.
,
Oct 5 2016
Yes, you will soon be able to do:
base::CreateTaskRunnerWithTraits(TaskTraits(), ExecutionMode::<mode>);
Where <mode> is one of:
enum class ExecutionMode {
// Can execute multiple tasks at a time in any order.
PARALLEL,
// Executes one task at a time in posting order. The sequence’s priority is
// equivalent to the highest priority pending task in the sequence.
SEQUENCED,
// Executes one task at a time on a single thread in posting order.
SINGLE_THREADED,
};
and TaskTraits is from https://cs.chromium.org/chromium/src/base/task_scheduler/task_traits.h?rcl=1475652159&l=79 which can specify things like TaskPriority of tasks posted to this TaskRunner, etc. (TaskTraits can also easily be expanded if there is eventually another trait you believe tasks should be differentiated on).
,
Oct 5 2016
Attaching to our tracker bug to remember the potential for collaboration here.
,
Oct 5 2016
#6: Agreed that SequencedTaskRunner is probably the minimum guarantee we need. I think we ended up on SingleThreadTaskRunner mostly because there are a couple of well known magic threads in the renderer process that code wants to run on. I don't see why we couldn't relax this requirement.
,
Oct 5 2016
,
Oct 5 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 5 2017
This was recently slightly improved by https://chromium-review.googlesource.com/c/chromium/src/+/676174. I think we can close this bug and reopen if this function actually shows up in profiles. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by esprehn@chromium.org
, Oct 4 2016Components: -Blink>DOM Blink>Scheduling