2

I have this form that spawns a new thread and starts listening and waiting for UDP packets in a loop. What I need is to keep the UI updated with the number of bytes received.

For that, I have setup an event which I'll raise as soon as a packet is received and pass the number of bytes received as an argument. Since I'm not running on the UI thread, I cannot simply update the UI directly. Here's what I'm currently doing:

private void EVENTHANDLER_UpdateTransferProgress(long receivedBytes) {
    if(InvokeRequired) {
        Invoke(new MethodInvoker(() => {
            totalReceivedBytes += receivedBytes;
            Label.Text = totalReceivedBytes.ToString("##,0");
        }));
    }
}

But this is still running on the same thread as the packet reception loop and it will not return to that loop - and wait for another packet - until this EVENTHANDLER_UpdateTransferProgress method returns.

My question is basically about the following line in the method above:

Label.Text = totalReceivedBytes.ToString("##,0");

Updating the UI like this slows down the packet reception. If I take that line off (or comment it), the packet reception will be much faster.

How can I possibly solve this issue? I think more threads is the key, but I'm not sure how to properly implement them in this situation... I'm using Windows Forms with .NET 2.0.

EDIT:

On my previous testing, the above seem to be true and it may actually be to some extent. But after a little more testing I realized the problem was on the whole Invoke(new MethodInvoker(() => { ... })); thing. When I remove that (the UI won't be updated of course) and leave EVENTHANDLER_UpdateTransferProgress but keep raising the event, the packet reception is much faster.

I tested receiving some file which took in average about ~1.5sec without calling Invoke() at all on the event handler. When I did call Invoke() in the event handler, even without updating any control in the UI or doing any operation (in other words, the anonymous method body was empty), it took much longer, around ~5.5sec. You can see it's a big difference.

Is there anyway to improve this?

rfgamaral
  • 16,546
  • 57
  • 163
  • 275
  • What version of .NET are you using? – Sean Thoman Apr 08 '12 at 21:57
  • @SeanThoman For this specific application, .NET 2.0. – rfgamaral Apr 08 '12 at 21:59
  • 1
    See this thread - http://stackoverflow.com/questions/661561/how-to-update-gui-from-another-thread-in-c – Sean Thoman Apr 08 '12 at 22:05
  • @SeanThoman I've implemented the accepted answer and I didn't seem to notice any difference to my original implementation. However, I've done a little more testing and the problem seems to be on the Invoke itself, that's what actually slowing down the whole thing. Sorry about that. I have to update the question... – rfgamaral Apr 08 '12 at 22:37

3 Answers3

3

The problem with your approach is that it updates the UI on every single packet. If you received 1000 packets every second, you would update the UI 1000 times every second! The monitor probably doesn't refresh more than 100 times per second, and nobody is going to be able to read it if it updates more than 10 times per second.

A better way to approach this problem is to put the totalReceivedBytes += receivedBytes; in the thread that handles the I/O and put a timer on the UI thread that executes Label.Text = totalReceivedBytes.ToString("##,0"); only a few times per second at most. When the transfer starts, start the timer; when the transfer stops, stop the timer.

Gabe
  • 84,912
  • 12
  • 139
  • 238
  • Yes, I have eventually reached that same conclusion after posting this question. It will probably speed up a lot... Just to clarify, you are talking about the WinForms timer? Or is there a better alternative? I keep reading that the WinForms timer has all sorts of problems... But I'm not aware of an alternative with some sort of "ticker" event. – rfgamaral Apr 09 '12 at 03:13
  • Yes, it's just the standard `System.Windows.Forms.Timer` that you put on a form. I'm not sure of its problems, but it is exactly what you want for this task. – Gabe Apr 09 '12 at 04:05
  • 1
    +1 This is the correct approach. Invoke blocks the calling thread. BeginInvoke does not but, at a high signalling rate, the GUI thread message queue can fill up with gunge faster than it can be emptied. A 1 second Forms.Timer is a better approach. This is one of those few times when polling is the better option :) – Martin James Apr 09 '12 at 11:48
  • +1: This is absolutely the right approach. `Invoke` and `BeginInvoke` are **way** overused and overrated. – Brian Gideon Apr 09 '12 at 13:25
1

Yes, there is a way to improve this.

The first is to use BeginInvoke instead of Invoke which will not wait for the invoke to return. You should also consider using another form in your method

private void EVENTHANDLER_UpdateTransferProgress(long receivedBytes) {
    if(InvokeRequired) {
        BeginInvoke(new Action<long>(EVENTHANDLER_UpdateTransferProgress),
                    receivedBytes));
        return;
    }
    totalReceivedBytes += receivedBytes;
    Label.Text = totalReceivedBytes.ToString("##,0");
}

So if you call this method from a method that does not require invoking, the update on the GUI is still performed.


Another option that you can do is break of a thread in your download thread. Something in the likes of

public event EventHandler<MonitorEventArgs> ReportProgress;

public void startSendingUpdates(MonitorEventArgs args) {
  EventHandler<MonitorEventArgs> handler = ReportProgress;
  if (handler == null) {
      return;
  }
  ThreadPool.QueueUserWorkItem(delegate {
      while (!args.Complete) {
          handler(this, args);
          Thread.Sleep(800);
      }
  });
}

public void download() {
    MonitorEventArgs args = new MonitorEventArgs();
    startSendingUpdates(args);
    while (downloading) {
        int read = downloadData(bytes);
        args.BytesTransferred += read;
    }
    args.Complete = true;
}

public class MonitorEventArgs : EventArgs {
    public bool Complete { get; set; }
    public long BytesTransferred { get; set; }
}

The overhead of this is kind of small compared to the benefits. Your download thread is not affected by the updates to the GUI (at least not compared to waiting on the GUI to update). The downside is you are occupying a thread in the threadpool, but hey, that's what they're there for! And, the thread shuts down when it's done, since you set the complete flag. You don't need to lock when setting that either, since an extra run in the worker thread is unimportant in the context.

Patrick
  • 17,669
  • 6
  • 70
  • 85
  • Will try `BeginInvoke` instead of `Invoke`, it may help a bit. But I believe the real problem will be fixed with @Gabe's answer above. The event handler will never be called from a method that doesn't require invoke. Not on this program at least. But thanks for the tip, it might prove useful in the future. – rfgamaral Apr 09 '12 at 03:15
  • @RicardoAmaral: The concept is the same. In my version the send thread pushes updates each 800 milliseconds, in Gabe's version the GUI has a timer that updates a certain amount of times per second. So they do the same thing... – Patrick Apr 09 '12 at 10:42
  • @RicardoAmaral: I'm not sure if you noticed, but `ThreadPool.QueueUserWorkItem` actually starts another thread, so your download method continues to run on its own... So basically it's a timer, with a repeat of 800 milliseconds, which ends when the `args.Complete` is `true` – Patrick Apr 09 '12 at 10:56
  • Maybe, but Gabe's version is much simpler to implement and requires far less code :) – rfgamaral Apr 09 '12 at 20:41
0

Have you tried using BeginInvoke instead of Invoke? BeginInvoke() is an asychronous call.

private void EVENTHANDLER_UpdateTransferProgress(long receivedBytes) {
    if(InvokeRequired) {
        BeginInvoke(new MethodInvoker(() => {
            totalReceivedBytes += receivedBytes;
            Label.Text = totalReceivedBytes.ToString("##,0");
        }));
    }
}
Steve Wong
  • 2,038
  • 16
  • 23
  • As @Patrick already suggested (he was a couple minutes faster lol), I will try that and see if it helps but like I said I commented in his answer, I believe the correct answer is Gabe's answer. Will need to perform some testing though. – rfgamaral Apr 09 '12 at 03:18