views:

85

answers:

2

I've recently tried to implement an asynchronous callback model, so that when developers consume my library, they don't need to worry about the following:

if(control.InvokeRequried) { // invoke } 

That part of the code seems to be working quite nicely, but I discovered something a bit strange today, that I think I've diagnosed but would like to see what others with more experience think, and ways I might be able to resolve it.

The problem I've got, is it seems that my posts are very slow for some reason, and consequently they appear to get a little clogged up, and keep running after I've completed my work. A brief code example:

// called by user
WorkerEventHandler workerDelegate = new WorkerEventHandler(CalcAsync);
workerDelegate.BeginInvoke(p, asyncOp, null, null);

CalcAsync(P p, AsyncOperation asyncOp)
{
   Calculate();
   asyncOp.PostOperationCompleted(new AsyncCompletedEventArgs(null, false, 
      asyncOp.UserSuppliedState);
}

Calculate()
{
   DoStuff()            // updates the progress as it goes
   this.UpdateStatus("Done");   // marks us as done
   this.UpdateProgress(pId, 100);   // increase progress to max
}

When stepping through I get repeated calls to an UpdateProgress being called from the posted events however, so it looks like they've been unable to keep up, and are still being posted. These are handled like so:

// initalized with 
//    onProgressUpdatedDelegate = new SendOrPostCallback(UpdateProgress);    
private SendOrPostCallback onProgressUpdatedDelegate; 
public void UpdateProgress(Guid pId, ProgressEventArgs e)
{
   AsyncOperation asyncOp = GetAsyncOperation(pId);

   if (asyncOp != null)
   {
      asyncOp.Post(this.onProgressUpdatedDelegate, e);
   }
   else
   {
      UpdateProgress(this, e);
   }
}

private void UpdateProgress(object state)
{
   ProgressEventArgs e = state as ProgressEventArgs;
   UpdateProgress(this, e); // does the actual triggering of the EventHandler
}

If it's relevant these events are all writing to a console. But I seem to only see my progress go down on longer operations, which seems to be the same results I see through my WPF test app.

Does it suggest that the Post is just slow? I suppose what I'd really like is every time I call UpdateProgress via the Post I want to purge any existing posts within the queue if this is the case. But I'm not sure this is possible? If it is, is it possible to purge just for these events, as I don't want to then purge my UpdateStatus event accidentally...

Edit

Some of the missing methods if that makes it easier.

public void UpdateProgress(Guid pId, ProgressEventArgs e)
{
   AsyncOperation asyncOp = GetAsyncOperation(pId);
   if (asyncOp != null)
   {
      asyncOp.Post(this.onProgressUpdatedDelegate, e);
   }
   else
   {
      UpdateProgress(this, e);
   }
}

private void UpdateProgress(object sender, ProgressEventArgs e)
{
   ProgressChangedEventHandler handler;
   lock (progressUpdateEventLock)
   {
      handler = progressUpdateEvent;
   }

   if (handler != null)
      handler(sender, e);
}
A: 

Is there some code missing from your example? You have a call to UpdateProgress(this, e), but no corresponding method with that signature. Unless this is a Guid, but I doubt that, as then you'd end up with an infinitely recursive method. You also have a call to UpdateProgress(100), and no corresponding method.

In any event, it would probably be helpful if you used a profiler or, if you don't have one, a Stopwatch, to see where time is being spent in your methods.

It could be that the thread context switches are killing you because you have to do InvokeRequired and then Invoke for every update. If that's the case, it'd probably be helpful to put the Posts into a queue and have a timer that empties the queue periodically. Something like:

Timer queueTimer = new Timer(QueueTimerProc, null, 20, 20);
queueTimer.Start();

void QueueTimerProc(object state)
{
    if (queue.Count > 0)
    {
        if (this.InvokeRequired)
           // Invoke the EmptyQueue method 
        else
           // Call the EmptyQueue method
    }
}

void EmptyQueue()
{
    lock (queue)
    {
        while (queue.Count > 0)
        {
            // dequeue an item and post the update
        }
    }
}
Jim Mischel
I've updated the post to include the missing methods. The UpdateProgress(100) is updated too, to actually pass in the pId value. Currently I'm not doing any 'If InvokeRequired' checks, I believe thats only avaliable on WinForm controls, and this is within a class library. What I was aiming for is that the end user does not need to use invoke and to handle it all in the class library. Rather like using a WebClient to download a file asynchronously, you don't need to Invoke update calls to the UI there, that is what I was aiming for.
Ian
+1  A: 

It isn't that clear what's going on here. Getting multiple UpdateProgress calls on the UI thread for one UpdateProgress call on the worker thread is definitely bad. Your code as posted should not do this.

But, yes, the Post() call is not particularly fast. It takes roughly a millisecond for the message loop on the UI thread to pick up the notification, assuming it is not busy doing something else. This is not one millisecond of hard CPU work, it is a delay induced by the Windows plumbing that handles GetMessage(). The thread context switch is part of it, but just a small part.

This can have notable nasty side-effects. Calling Post() too often, or having the posted delegate doing too much hard work, can seriously affect the UI thread. To the point that it doesn't get around its regular duties anymore. Because of the delay, you can get there quickly; it only takes 1000 posts per second.

Of course, it is never necessary to post that often, the human eye cannot perceive updates at a rate any faster than about 25 per second. Doing it more frequently is just wasted effort. But obtaining this ideal update rate is not particularly easy to achieve, unless the worker thread pays specific attention to elapsed time.

Anyhoo, it is better to leave it up to the client code to tell you how it wants its notifications marshaled. The FileSystemWatcher.SynchronizingObject is a good example of that. If Post() falls apart, that's one way for the client code to fix the problem. Also take a look at the BackgroundWorker class.

Hans Passant
nobugz, thanks for your comment. It may well be that it's the post speed that slow, currently trying to figure out if I can determine for certain if that's the cause. Are you suggesting I let the consumer of the class manage Invokes if neccessary? My aim was to have something like the WebClient.DownloadAsync() method where you don't have to invoke any UI updates from the events triggered.
Ian
I'm merely suggesting to give you client the choice. DownloadAsync() is easy to manage, there's only ever one completion event.
Hans Passant
nobugz, I should have just the single completion event. It's just I have 3 other events to signal the progress/status/solution which I hoped would occur a bit like the WebClient.OnProgressChanged event.Also to be clear, I don't think it's multiple UI calls from a single worker thread call, but lots of workerthread calls that must be too fast.I will consider reverting back and letting the client invoke stuff, better that than strange progress bars!
Ian
Based on the massive speed improvements by doing the dispatching/invoking manually within the client code I'm going to accept your answer nobugz. Bit of a shame after spending so long figuring out the async operation class but ah well. Cheers
Ian
Thanks, but it doesn't make a lot of sense that would make a difference. WPF has only one mechanism to marshal calls, both Dispatcher.BeginInvoke() and AsyncOperation.Post() use it. No idea what might be wrong with your code.
Hans Passant
@nobugz: hmm. I've been looking into the background worker aswell as it is commonly suggested on SO. It seems this works almost exactly the same way as the code I was trying to design. I'm guessing the slowness comes from the Post which calls ThreadPool.QueueUserWorkItem() which is supposed to suffer a performance hit due to security checking. That's the only thing I can think of at the moment. I did think limiting the ThreadPool might help purge old entries, but there's nothing to stop someone changing this back later.
Ian
Post is *not* supposed to call QUWI. It only does that if their is no synchronization provider.
Hans Passant
If you reflect the AsyncOperation.Post() you'll find it calls this.syncContext.Post(d, arg) which simply calls ThreadPool.QueueUserWorkItem(new WaitCallback(d.Invoke), state). Unless I'm missing something?
Ian
Your are looking at the wrong code, WPF installs its own. Should be obvious, QUWI can't call methods on the UI thread.
Hans Passant