Task cancellation in TaskScheduler and tweaking WeakPtr thread-safety checks

[Gabriel Charette]

Hello all,

the TaskScheduler team discussed how we could better support delayed task cancellation ((1)avoid waking up for cancelled tasks and (2) remove cancelled tasks before they expire to free up memory -- cancelled tasks left behind are common in renderers according to alexclare@).

The main problem here compared with MessageLoop is that TaskScheduler manages its delayed tasks on a service thread (delayed tasks are owned by it until their delay expires at which point they are inserted in their destination sequence). Since task cancellation works through WeakPtr and WeakPtr isn't thread-safe (can't be looked up off-sequence), we have a problem.

In practice however, the two things one really shouldn't do with a WeakPtr off-sequence is (1) deref and (2) invalidate it.

Checking its validity is usually wrong off-sequence as it typically means one would use it after (i.e. (1)) but in our case we merely want to check its validity (not use the pointer). This is safe to do without locks so long as |WeakReference::Flag::is_valid_| is atomic (i.e. s/bool/AtomicFlag) and is only toggled one way (which is already the case) -- this would be exposed behind a you-better-know-what-you're-doing method, e.g. WeakPtr::IsValidUnsafe().

This is merely an FYI that this is our plan, no action required unless you want to volunteer to implement this :).

Cheers,

Gab

[Sami Kyostila]

Sounds reasonable (although I'd prefer to avoid the atomics but don't see any easy way to do that here...maybe we could do the scanning on the destination thread instead but that might become complicated.)

I guess we'll still need to destroy the cancelled closure on the destination thread instead of the service thread since some parameters might depend on that?

- Sami

--
You received this message because you are subscribed to the Google Groups "scheduler-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-de...@chromium.org.
To post to this group, send email to schedu...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAJTZ7LK7ddFzZWP_hqS9AsBieFekxStyNtY62MaF5f6a3RWiNg%40mail.gmail.com.

[Gabriel Charette]

Can't avoid the AtomicFlag or it's racy to check it (there's no such thing as a benign data race).

Good point that deletion must still happen on the destination sequence.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAPuLczvYVUW_iNtXb-mZDWGYyGx3jS2TrMj6zO2c5-%3DeYMxD1Q%40mail.gmail.com.

[dan...@chromium.org]

Is this checking some secondary atomic state than what operator bool checks? I am kinda wondering about the scare text instead of just making operator bool explicitly thread-safe.

I was left wondering about the details here, if they are interesting. We're cancelling a task from the posting thread (or some other thread)? Then when we cancel it who is checking the WeakPtr, the cancelling thread? Is the WeakPtr valid until the task is run? What does the cancelling thread do if the WeakPtr is/isn't valid?

[Daniel Cheng]

My understanding is it's neither the posting nor the cancelling thread (sequence?) doing the checking. The cancelling sequence for WeakPtr<T> has to be the sequence that T is destroyed on, right? So this would be an optimization to let the service thread (which could belong to neither of those sequences) to drop cancelled tasks at the front of the queue rather than considering them for scheduling purposes.

Once the task is moved into the destination sequence, the destination sequence will still need to do one final cancellation check (or just call Run and let the standard callback cancellation do its work).

 

This is merely an FYI that this is our plan, no action required unless you want to volunteer to implement this :).

Cheers,

Gab

--

You received this message because you are subscribed to the Google Groups "scheduler-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-de...@chromium.org.
To post to this group, send email to schedu...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAHtyhaS2i6C8%2BNdWy962QjrdnH2xruDZncC%2Bp5QU3R4FTVwsaw%40mail.gmail.com.

[dan...@chromium.org]

On Mon, Feb 6, 2017 at 2:23 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Feb 6, 2017, 11:10 <dan...@chromium.org> wrote:

On Mon, Feb 6, 2017 at 10:56 AM, Gabriel Charette <g...@chromium.org> wrote:

Hello all,

the TaskScheduler team discussed how we could better support delayed task cancellation ((1)avoid waking up for cancelled tasks and (2) remove cancelled tasks before they expire to free up memory -- cancelled tasks left behind are common in renderers according to alexclare@).

The main problem here compared with MessageLoop is that TaskScheduler manages its delayed tasks on a service thread (delayed tasks are owned by it until their delay expires at which point they are inserted in their destination sequence). Since task cancellation works through WeakPtr and WeakPtr isn't thread-safe (can't be looked up off-sequence), we have a problem.

In practice however, the two things one really shouldn't do with a WeakPtr off-sequence is (1) deref and (2) invalidate it.

Checking its validity is usually wrong off-sequence as it typically means one would use it after (i.e. (1)) but in our case we merely want to check its validity (not use the pointer). This is safe to do without locks so long as |WeakReference::Flag::is_valid_| is atomic (i.e. s/bool/AtomicFlag) and is only toggled one way (which is already the case) -- this would be exposed behind a you-better-know-what-you're-doing method, e.g. WeakPtr::IsValidUnsafe().

Is this checking some secondary atomic state than what operator bool checks? I am kinda wondering about the scare text instead of just making operator bool explicitly thread-safe.

I was left wondering about the details here, if they are interesting. We're cancelling a task from the posting thread (or some other thread)? Then when we cancel it who is checking the WeakPtr, the cancelling thread? Is the WeakPtr valid until the task is run? What does the cancelling thread do if the WeakPtr is/isn't valid?

My understanding is it's neither the posting nor the cancelling thread (sequence?) doing the checking. The cancelling sequence for WeakPtr<T> has to be the sequence that T is destroyed on, right? So this would be an optimization to let the service thread (which could belong to neither of those sequences) to drop cancelled tasks at the front of the queue rather than considering them for scheduling purposes.

I see, so it doesn't prevent waking up for a cancelled task at the front of the queue, but it would for a cancelled task coming up after the current task being run. Thanks, makes sense.

 

Once the task is moved into the destination sequence, the destination sequence will still need to do one final cancellation check (or just call Run and let the standard callback cancellation do its work).

 

This is merely an FYI that this is our plan, no action required unless you want to volunteer to implement this :).

Cheers,

Gab

--

You received this message because you are subscribed to the Google Groups "scheduler-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-dev+unsubscribe@chromium.org.

To post to this group, send email to schedu...@chromium.org.

[Gabriel Charette]

I wanted to keep operator bool's sequence check to catch improper |if (weak_foo) weak_foo->Do();|  but I guess a sequence check in operator->() is sufficient.

Re.  what to do when service thread finds a cancelled task, options:

 1) Post it normally to its sequence (it will no-op per being cancelled).

 2) Post it to a custom queue on its sequence that knows to not even bother with Run().

(1) is simpler unless it's surprising to see stacks that imply the task was Run() before its delay expired (even though such Run() call would only be on cancelled tasks) -- FWIW, I think that's fine.

To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-de...@chromium.org.

To post to this group, send email to schedu...@chromium.org.

[dan...@chromium.org]

On Mon, Feb 6, 2017 at 3:02 PM, Gabriel Charette <g...@chromium.org> wrote:

I wanted to keep operator bool's sequence check to catch improper |if (weak_foo) weak_foo->Do();|  but I guess a sequence check in operator->() is sufficient.

Yep that was my thought also.

Re.  what to do when service thread finds a cancelled task, options:

 1) Post it normally to its sequence (it will no-op per being cancelled).

 2) Post it to a custom queue on its sequence that knows to not even bother with Run().

(1) is simpler unless it's surprising to see stacks that imply the task was Run() before its delay expired (even though such Run() call would only be on cancelled tasks) -- FWIW, I think that's fine.

Cool, both seem reasonable. Trace events along the way can help by being explicit if the task is a cancelled task or not.

 

On Mon, Feb 6, 2017 at 2:37 PM <dan...@chromium.org> wrote:

On Mon, Feb 6, 2017 at 2:23 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Feb 6, 2017, 11:10 <dan...@chromium.org> wrote:

On Mon, Feb 6, 2017 at 10:56 AM, Gabriel Charette <g...@chromium.org> wrote:

Hello all,

the TaskScheduler team discussed how we could better support delayed task cancellation ((1)avoid waking up for cancelled tasks and (2) remove cancelled tasks before they expire to free up memory -- cancelled tasks left behind are common in renderers according to alexclare@).

The main problem here compared with MessageLoop is that TaskScheduler manages its delayed tasks on a service thread (delayed tasks are owned by it until their delay expires at which point they are inserted in their destination sequence). Since task cancellation works through WeakPtr and WeakPtr isn't thread-safe (can't be looked up off-sequence), we have a problem.

In practice however, the two things one really shouldn't do with a WeakPtr off-sequence is (1) deref and (2) invalidate it.

Checking its validity is usually wrong off-sequence as it typically means one would use it after (i.e. (1)) but in our case we merely want to check its validity (not use the pointer). This is safe to do without locks so long as |WeakReference::Flag::is_valid_| is atomic (i.e. s/bool/AtomicFlag) and is only toggled one way (which is already the case) -- this would be exposed behind a you-better-know-what-you're-doing method, e.g. WeakPtr::IsValidUnsafe().

Is this checking some secondary atomic state than what operator bool checks? I am kinda wondering about the scare text instead of just making operator bool explicitly thread-safe.

I was left wondering about the details here, if they are interesting. We're cancelling a task from the posting thread (or some other thread)? Then when we cancel it who is checking the WeakPtr, the cancelling thread? Is the WeakPtr valid until the task is run? What does the cancelling thread do if the WeakPtr is/isn't valid?

My understanding is it's neither the posting nor the cancelling thread (sequence?) doing the checking. The cancelling sequence for WeakPtr<T> has to be the sequence that T is destroyed on, right? So this would be an optimization to let the service thread (which could belong to neither of those sequences) to drop cancelled tasks at the front of the queue rather than considering them for scheduling purposes.

I see, so it doesn't prevent waking up for a cancelled task at the front of the queue, but it would for a cancelled task coming up after the current task being run. Thanks, makes sense.

Once the task is moved into the destination sequence, the destination sequence will still need to do one final cancellation check (or just call Run and let the standard callback cancellation do its work).

 

This is merely an FYI that this is our plan, no action required unless you want to volunteer to implement this :).

Cheers,

Gab

--

You received this message because you are subscribed to the Google Groups "scheduler-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-dev+unsubscribe@chromium.org.

To post to this group, send email to schedu...@chromium.org.