views:

381

answers:

3

I have gotten a bit lazy(it's sometimes good) and started updating WinForms UI by invoking a callback without checking InvokeRequired first.

Are there a performance issues or considerations that I should be aware of?

    private delegate void SetStatusEventHandler(string statusMessage);
    private void SetStatus(string statusMessage)
    {
        Invoke((MethodInvoker) (() =>
        {
            resultLabel.Text = statusMessage;
        }));

        //  - vs -

        if (InvokeRequired)
        {
            SetStatusEventHandler cb = SetStatus;
            Invoke(cb, statusMessage);
        }
        else
        {
            resultLabel.Text = statusMessage;
        }
    }

[EDIT]: Most of times that a method that calls "invoke" will be called at most like say 10~20 times a second with a wide interval inbetween.

[UPDATE] Settled with the following extension method

public static class SmartInvoker
{
    public static void InvokeHandler(this Control control, MethodInvoker del)
    {
        if (control.InvokeRequired)
        {
            control.Invoke(del);
            return;
        }
        del();
    }
}

...

    private void SetStatus(string statusMessage)
    {
        this.InvokeHandler(() => resultLabel.Text = statusMessage);
    }

I guess finding out how to manage extension method classes is another topic I need to dig in. Thank you for your help

+3  A: 

EDIT: See the comments for debate about the whole posting vs immediately dispatching malarky.

Either way, my answer is actually the same: unless this is happening hugely often unnecessarily (i.e. most of the time you're on the UI thread to start with, and it's attached to something like a mouse movement handler) I wouldn't worry. It certainly makes the code simpler. If this is going to be invoked very often, I'd measure and test more :)

Invoke is faster with an EventHandler or MethodInvoker delegate than with others. I don't know if there's any difference between the two - you may want to check.

You can make this even simpler by writing an extension method, e.g.

public static void InvokeHandler(this Control control, MethodInvoker handler)
{
     control.Invoke(handler);
}

Then you can make your code:

private void SetStatus(string statusMessage)
{
    this.InvokeHandler(delegate
    {
        resultLabel.Text = statusMessage;
    });
}

or

private void SetStatus(string statusMessage)
{
    InvokeHandler(() => resultLabel.Text = statusMessage);
}

That way you don't need to specify the delegate type.

Jon Skeet
I didn't think it did... I was under the impression that it simply invokes it *without* involving the event queue
Marc Gravell
@Marc, I just took a quick peek at the reflector code and it looks like invoke will always go through the event queue. Didn't do a deep investigation though.
JaredPar
No, it goes through the same Control.MarshaledInvoke code path. It places it on the queue, but it executes the queue immediately.
Dustin Campbell
@Dustin, not immediately though. It does a post message and then a wait. My understanding is this will cause a pump to occur meaning that other calls can happen inbetween the call to Inovke() and the execution of the acutal delegate.
JaredPar
Thanks - I retracted my post, then. But the result is the same: it executes now...
Marc Gravell
@Dustin, It doesn't go through the windows message queue when it detects same thread access. It does appear though that it's possible for other delegates to execute before it because it executes the entire queue, not just the current delegate.
JaredPar
@Skeet: Method in which I use "Invoke" are sometimes called repeatedly or sometimes very seldom. I was not sure if I could just "invoke" in both cases without performance issues.
Sung Meister
Right. It doesn't go through the windows message queue. I didn't say that. I was referring to "threadCallbackList" that is executed in InvokeMasheledCallbacks(). It doesn't post a message to call that method later. Instead, it calls InvokeMarshaledCallbacks() immediately.
Dustin Campbell
@Dustin, I was reading the code wrong. I still think it's possible for another delegate to execute ahead of this one though if it's posted from the background thread at the right time. Might investigate further later today.
JaredPar
@Sung: It depends on quite how often you mean by "repeatedly". I'd test it for your specific app if I were you. Unless you're really thrashing the thing, I'd go with the simplest code.
Jon Skeet
@Skeet: I guess by repeatedly, it meant to say like, "around 10~20 calls per second" I don't think this can be much of a problem
Sung Meister
Doesn't sound too heavy, no :)
Jon Skeet
+2  A: 

Why not just add an extension method so you don't have to think about it anymore?

public static object SmartInvoke(this Control control, MethodInvoker del) {
  if ( control.InvokeRequired ) {
    control.Invoke(del);
    return;
  }
  del();
}

Now your code becomes

private void SetStatus(string statusMessage) {
  this.SmartInvoke(() => resultLabel.Text = statusMessage);
}
JaredPar
You will need a this before SmartInvoke if you are within the control itself (which I believe is likely). this.SmartInvoke(...);
Samuel
@Samuel, "this" is un-necessary in this case.
JaredPar
I am not sure if I should consider using an extension method. I would like to investigate whether using an extension method is even a good idea or not first. But this sounds like a pretty lazy way to deal with my problem though. Thank you
Sung Meister
@JaredPar, really? I can't seem to get it to work without this. And the return type should be void, you can't implicitly cast void del() to object.
Samuel
@Sung, why would you consider this lazy? Writing the same code everywhere in the application is redundant and will waste time vs. saving it.
JaredPar
@Samuel, you're correct. I didn't realize "this" was a forced value for an extension method in C#. Just verified with a simple test. Post updated.
JaredPar
I also had to use "this."
Sung Meister
By the way, SmartInvoke should not return "object" but "void"
Sung Meister
@JaredPar: "why would you consider this lazy?" I meant it in a good way ;)
Sung Meister
Well, I wish there was an option to mark this as a "supported answer" or some sort. Thanks, JaredPar.
Sung Meister
A: 

I believe it just prevents an unnecessary post if you're already on the same thread. So if that would be the most common scenario (being on the correct thread), it might cause a small performance hit by not checking, but I dont believe its actually required.

Brandon