Bug 74535 – [chromium] Add postCancellable[Delayed]Task to CCThread

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Tien-Ren Chen]

[chromium] Add postCancellable[Delayed]Task to CCThread

[Tien-Ren Chen]

Created attachment 119284 [details]
Patch

[Tien-Ren Chen]

    The postCancellable[Delayed]Task functions will return CCTaskHandle for the
    newly appended tasks. Where CCTaskHandle is non-copyable but transferable
    weak pointer to the task, such that the task will be automatically cancelled
    upon destruction of the handle.
    
    This allows objects which has shorter lifespan than the thread to safely post
    tasks, so the task would be "scoped" in the sense that the task will be
    cancelled once the owner got destroyed.
    
    HOW TO USE:
    
    class SomeClass {
    private:
        CCTaskHandle m_task;
    };
    
    SomeClass::scheduleSomething() {
        m_task = CCProxy::implThread()->postCancellableDelayedTask(
            createCCThreadTask(this, &SomeClass::method, arg),
            s_timeout);
    }

[Nat Duca]

Comment on attachment 119284 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=119284&action=review

Lets file a new webkit bug now for unit tests for this. You can write those afterward since you've been really nice in doing this upstream in the first place.

> Source/WebCore/WebCore.gypi:3638
> +            'platform/graphics/chromium/cc/CCTaskHandle.cpp',

You can put this the code for these in CCThread.h/cpp  if you want. I'm neutral either way.

> Source/WebCore/platform/graphics/chromium/cc/CCTaskHandle.h:40
> +class CCTaskHandle {

Put a comment above the class decl explain what this is used for, how it is used, etc.

> Source/WebCore/platform/graphics/chromium/cc/CCTaskHandle.h:46
> +    void clear();

Cancel? Since we're calling it postCancellable

> Source/WebCore/platform/graphics/chromium/cc/CCTaskHandle.h:69
> +    // We don't hold the ownership of the TaskWrapper. This is a weak pointer which will be reset to null once the TaskWrapper got destructed by the task queue.

The second setence is most of the value. No need to point out that we don't hold ownership.

> Source/WebCore/platform/graphics/chromium/cc/CCThread.h:57
>  

//comments to explain usage?

[James Robinson]

I'd like to understand a bit more about the use cases for this. Would CCScopedThreadProxy work for this? That allows you to scope a set of tasks to a single proxy object and provides a guarantee that no tasks scoped to the proxy run after the proxy is destroyed.

Another approach to things like this that Chromium takes is that tasks are associated with their target and take a reference to it.

[Nat Duca]

(In reply to comment #4)
> I'd like to understand a bit more about the use cases for this. Would CCScopedThreadProxy work for this? That allows you to scope a set of tasks to a single proxy object and provides a guarantee that no tasks scoped to the proxy run after the proxy is destroyed.
> 
> Another approach to things like this that Chromium takes is that tasks are associated with their target and take a reference to it.

I think this makes sense, as we discussed on IM last night. :)

This is for things like DelayBasedTimeSource, which you can see is using hand-coded cancelability. Animation stuff needs this too, which is why trchen is hitting this. This also wraps the notion of canceling a task, which has nice properties too.

The scopedthreadproxy is ~= Messageloopproxy in my head.

While technically, you can with some mental acrobatics say that one is the other, I think from an "intents" point of view, there are two different use-cases. Having a class designed specifically for the "cancellable work" is really convenient.

We might need to make cancel() threadsafe though.

[Nat Duca]

From a chat:
- ScopedThreadProxy: used for "i have a big system that is posting lots of tasks to a single object, and that object might die." Using cancellable tasks would mean like 15 different member vars on the target method for every pending task.

- cancellables: used for "I have a single task that I need to post with arguments, AND I might need to cancel it, and I might die before it runs." Using scoped threadproxy is possible but overkill for specific cancelation.

[Tien-Ren Chen]

Basically my opinion is the same as Nat's. There is some situation that we don't really want to create a CCScopedThreadProxy for each of the task target. One example is CCDelayBasedTimeSource, and another example is the fling driver in WebCompositorInputHandlerImpl (the latter case can actually make use of the former). Currently we solve the "destroyed before task fired" problem by playing with the reference counter, which I feel isn't really safe to do.

The CCTaskHandle provides a general, reusable way to cancel an individual task. Not only it can be used to avoid object lifespan problem, but also allows rescheduling of tasks (which will be handy for delayed animation). And it is relatively cheap (no reference counter is needed).

The cons of CCTaskHandle is that it requires a handle for each of the posted task, but I would say for many cases it is sufficient.

[Tien-Ren Chen]

Created attachment 119319 [details]
Patch

[James Robinson]

CCScopedThreadProxy is definitely overkill if you're posting a task to run on the same thread. CCDelayBasedTimeSource is an interesting use case and I'll definitely think about it.

I don't think that fling animation (or animations in general) are a good use case for this mechanism, though - they should work by requesting a new frame and then receiving callbacks from the scheduler or animation controller. Then cancellation happens by simply removing the target from the list of things the scheduler / animation controller / whatever considers on each tick. For example WebCompositorInputHandleImpl (which implements CCInputHandler) receives a willDraw() callback which is the appropriate place to update fling animation states, if we had fling animations driven from WebCompositorInputHandleImpl.

Chromium's base::CancelableCallback and WebKit's WebCore::Timer take two other approaches to having cancelable delayed tasks. Have you looked at those and are there specific reasons you went with this handle approach instead of one of those?

[Tien-Ren Chen]

(In reply to comment #9)
> Chromium's base::CancelableCallback and WebKit's WebCore::Timer take two other approaches to having cancelable delayed tasks. Have you looked at those and are there specific reasons you went with this handle approach instead of one of those?

WebCore::Timer actually looks promising. Will it work with both threaded and non-threaded mode? One reason I didn't use WebCore::Timer is because I want to work on the same abstraction level as CCThread. It feels weird for some tasks are posted through CCThread while some others through the WebCore.

[James Robinson]

We don't want to use WebCore::Timer directly in our code - it's a dependency we don't want and the scheduling it uses is not good for our use. My suggestion was that you look at the API it uses and see if there are any interesting lessons.  The way it works is something like this:

class A {
  void scheduleSomethingForLater()
  void timerFired(Timer<A>*);
  Timer<A> m_timer;
};

A::A()
  : m_timer(this, &A::timerFired) { }

A::scheduleSomethingForLater()
{
  m_timer.startOneShot(delay); // or startRepeating(), etc
}

~Timer cancels the task.

[Nat Duca]

Yes, and this timer implementation looks almost exactly like a cancellable task. Without having to implement the timer callback interface, rather instead piggybacking on our method binding system.

(In reply to comment #11)
> We don't want to use WebCore::Timer directly in our code - it's a dependency we don't want and the scheduling it uses is not good for our use. My suggestion was that you look at the API it uses and see if there are any interesting lessons.  The way it works is something like this:
> 
> class A {
>   void scheduleSomethingForLater()
>   void timerFired(Timer<A>*);
>   Timer<A> m_timer;
> };
> 
> A::A()
>   : m_timer(this, &A::timerFired) { }
> 
> A::scheduleSomethingForLater()
> {
>   m_timer.startOneShot(delay); // or startRepeating(), etc
> }
> 
> ~Timer cancels the task.

[Nat Duca]

James: next step? Are you saying you only will accept a CCTimer class? I'd like to get trchen unblocked...

As I pointed out, my concern with a Timer and TimerClient/TimerTarget formulation is:
1. We lose the convenience that CCTask* gives you of arbitrary method binding
2. We can't do cross-thread cancellation

#1 is valuable when you have a single class that has a few different things going on.

#2 would help me implement those redundant commit messages we send to the main thread. I could just cancel any pending commit task when I create one.

[James Robinson]

I believe the next step is I get time to sit down and review this carefully and do so. That hasn't happened yet, but I expect it will soon. I suggested Timer as something to look at, not necessarily a model we definitely want.

I do think that whatever solution we adopt should probably look a whole lot like some existing solution either in WebKit, chromium, or elsewhere. This is a fairly common thing in task-based systems and I doubt that our needs are unique enough or that we are clever enough to need and be capable of producing something completely novel.

[Tien-Ren Chen]

(In reply to comment #13)
> James: next step? Are you saying you only will accept a CCTimer class? I'd like to get trchen unblocked...
> 
> As I pointed out, my concern with a Timer and TimerClient/TimerTarget formulation is:
> 1. We lose the convenience that CCTask* gives you of arbitrary method binding
> 2. We can't do cross-thread cancellation
> 
> #1 is valuable when you have a single class that has a few different things going on.
> 
> #2 would help me implement those redundant commit messages we send to the main thread. I could just cancel any pending commit task when I create one.

Currently my implementation doesn't solve #2 either, but I can make a thread-safe version if people need it.

What's really tricky about the thread-safe version is that there is one more state we need to consider when we cancel -- in same-thread situation a task is either done or undone, in cross-thread situation it could be doing in progress.

And there is also the problem that the state can change between calls. For example we can't write something like this: if (task.isPending()) {task.cancel(); doSomethingAssumeUndone();}

I agree with James that we can survey existing implementation to find out what is the best. But it seems that either of of them provides a solution to above problem...

[James Robinson]

I'm not aware of any way to implement an isPending() API for a cross-thread task.  I think chromium just punts on it.

[Nat Duca]

(In reply to comment #16)
> I'm not aware of any way to implement an isPending() API for a cross-thread task.

This is typically done by having a bool cancel() that returns true if canceled.

[Tien-Ren Chen]

(In reply to comment #16)
> I'm not aware of any way to implement an isPending() API for a cross-thread task.  I think chromium just punts on it.

Yea because it doesn't make much sense to only return isPending().

We need something that either

1. provides lock semantics, for example:
taskQueue.pause();
if (task.isPending())
    task.cancel();

2. or atomic compare and exchange , for example:
if (task.cancelIfIsPending())
    doThingsAssumeUndone;

[James Robinson]

Comment on attachment 119319 [details]
Patch

We have CCTimer and CCThread::postDelayedTask() which I believe cover this functionality.