1339632 - Try to reduce the number of canceled timers that run

Status: NEW

(Core :: XPCOM, defect, P3)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

In some debugging, I noticed that it's pretty common for timers to be canceled after their runnable is dispatched but before it runs. This causes a useless runnable to be dispatched to the event queue.

Many of the cancellations seem to happen here:
http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/ipc/glue/MessagePump.cpp#108

I think we should try to replace as many main thread delayed dispatches with XPCOM DelayedDispatch calls as we can to avoid this.

Comment 1

[Nathan Froyd [:froydnj]]

DelayedDispatch would still cause runnables (potentially several runnables) to be dispatched and processed in the event queue...and we'd still need some way of canceling the work, probably through a flag on the runnable?  I'm not sure that's better than a timer, though it does reduce our dependence on timers, which seems like a good thing.  I'm sure I'm missing something, though...

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

FWIW, my patches in bug 1325254 make timer cancelation a lot faster.

Also note, many many nsIEventTarget classes don't implement DelayedDispatch().

Comment 3

[(no longer active)]

DelayedDispatch actually uses timers internally: <https://dxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.cpp#252>

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I guess I posted this without understanding what's going on here. All this delayed dispatch code is amazingly complex.

I read over bug 1269056, but it never really explains why we're doing what we're doing. The thing I'm mostly interested in is why we dispatch both a normal event and a timer when you call DelayedDispatch, and then have them race. I guess we're trying to ensure that someone posting a delayed runnable won't have to wait too long. But if the timer thread is doing its job, then the timer will dispatch an event to the main thread almost immediately anyway. So why bother? Do you know, Nathan?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Bill McCloskey (:billm) from comment #4)
> I guess I posted this without understanding what's going on here. All this
> delayed dispatch code is amazingly complex.
> 
> I read over bug 1269056, but it never really explains why we're doing what
> we're doing. The thing I'm mostly interested in is why we dispatch both a
> normal event and a timer when you call DelayedDispatch, and then have them
> race. I guess we're trying to ensure that someone posting a delayed runnable
> won't have to wait too long. But if the timer thread is doing its job, then
> the timer will dispatch an event to the main thread almost immediately
> anyway. So why bother? Do you know, Nathan?

My understanding is this: When delayed dispatch was implemented on IPC threads, delayed tasks whose deadline had expired were given priority over all other tasks.  We don't have any similar concept of delayed vs. immediate tasks in XPCOM, so when we moved delayed tasks over to XPCOM-land, we wanted to preserve (insofar as is possible) that behavior.

If we were to implement delayed events with just the timer, it's possible that the timer event might wind up getting buried in the main event queue, and its execution would be delayed far past its deadline.  With the current behavior, if the event queue is backed up, the runnable we stuck into the event queue possibly gets a chance to run much closer to its deadline.  It is entirely possible, of course, that we'd run the runnable too early.  That probably means the even queue isn't backed up, and the timer-based mechanism will work just fine.  Of course the timer might miss the deadline by a significant amount, too, if a flood of work comes in.

It is complicated...and maybe it's not needed, or maybe we should redo the core event queue mechanisms of nsThread to accommodate simple delayed events (we've talked about redoing timers with something like that anyway...).  Does that explanation seem plausible?

Comment 6

[evelynneethelreda]

It is entirely possible, of course, that we'd run the runnable too early. That probably means the even queue isn't backed up, and the timer-based mechanism will work just fine. https://airbrushguides.com/how-to-airbrush-cake/