4

I have a main form called ProxyTesterForm, which has a child form ProxyScraperForm. When ProxyScraperForm scrapes a new proxy, ProxyTesterForm handles the event by testing the scraped proxy asynchronously, and after testing adds the proxy to a BindingList which is the datasource of a DataGridView.

Because I am adding to a databound list which was created on the UI thread I am calling BeginInvoke on the DataGridView so the update happens on the appropriate thread.

Without the BeginInvoke call in the method I will post below, I can drag the form around on my screen during processing and it doesn't stutter and is smooth. With the BeginInvoke call, it's doing the opposite.

I have a few ideas on how to fix it, but wanted to hear from smarter people than me here on SO so I solve this properly.

  1. Use a semaphore slim to control the amount of simultaneous updates.

  2. Add asynchronously processed items to a list outside of the scope of the the method I will post below, and iterate over that list in a Timer_Tick event handler, calling BeginInvoke for each item in the list every 1 second, then clearing that list and wash, rinse, repeat until the job is done.

  3. Give up the convenience of data binding and go virtual mode.

  4. Anything else someone might suggest here.

    private void Site_ProxyScraped(object sender, Proxy proxy)
    {
        Task.Run(async () =>
        {
            proxy.IsValid = await proxy.TestValidityAsync(judges[0]);
            proxiesDataGridView.BeginInvoke(new Action(() => { proxies.Add(proxy); }));
        });
    }
    
JohnWick
  • 4,929
  • 9
  • 37
  • 74
  • Have you considered a cancelationtoken to prevent multiple simultaneous requests? – Stefan Apr 29 '18 at 07:30
  • @Stefan No I didn't. Isn't that what a semaphore is for? I've only used cancellationtokens to cancel tasks. – JohnWick Apr 29 '18 at 07:30
  • To be honest I don't think you need `Task.Run` here. Instead make `Site_ProxyScraped` async since I assume this is an event handler. Then await `proxy.TestValidtiyAsync` and BeginInvoke can stay as it is. – FCin Apr 29 '18 at 07:34
  • @FCin I've heard async void method signatures are to be avoided. I'm not sure it's running the task that is causing the performance issue i'm having, but I can try. – JohnWick Apr 29 '18 at 07:35
  • Event handlers are one exception. The only I recall. – Tanveer Badar Apr 29 '18 at 07:38
  • Ok, I removed the Task.Run call and the same issue remains. Commenting out the BeginInvoke call and the UI remains responsive, so I know I must be overwhelming it and need a creative solution. Maybe I just can't use data binding here :\ – JohnWick Apr 29 '18 at 07:40
  • You say `I've heard async void method signatures are to be avoided` and you do this `Task.Run(async` which is exactly what you want to avoid – FCin Apr 29 '18 at 07:40
  • @FCin Yeah but that's a lambda and not a method signature...I'm new to C#, still learning the ropes especially with TPL. – JohnWick Apr 29 '18 at 07:41
  • How many times are you calling `Site_ProxyScraped`? – FCin Apr 29 '18 at 07:42
  • @DavidStampher: `Isn't that what a semaphore is for?` well, not completely. Although it tend to have the same effect. With a semaphore you would often wait. A cancelation token allows you to cancel half the way. For example, if `TestValidityAsync` takes some time; you can choose to not execute the `BeginInvoke` if a new call has arrives. It is a sligtly different approach than the semaphore. – Stefan Apr 29 '18 at 07:45
  • @FCin Maybe 10-20 times per second. It varies with bursts though. – JohnWick Apr 29 '18 at 07:45
  • @DavidStampher: if your data isn't representing video data, 10-20 is a lot, and to much for conventional the UI thread. How long does it take to `TestValidityAsync`? – Stefan Apr 29 '18 at 07:47
  • @Stefan Gotcha thanks for the tip. But I don't really want to cancel the BeginInvoke call ever, because then the proxy won't be added to the list called proxies which means it won't be visible in the UI. – JohnWick Apr 29 '18 at 07:47
  • @Stefan That varies too. On the long end, 30 seconds for a timeout exception to be raised. On the short end, maybe 200ms or so. – JohnWick Apr 29 '18 at 07:48
  • You must store data in a view-model layer: not directly in your UI to overcome this problem. – Stefan Apr 29 '18 at 07:48
  • @Stefan Ok, that is something I haven't done before. Is it pretty simple to implement in my case? – JohnWick Apr 29 '18 at 07:49
  • 1
    Consider sepperation of layers: it will help you to overcome lots of problems here. See this (my own XD ) post for example; https://stackoverflow.com/questions/23648832/viewmodels-in-mvc-mvvm-seperation-of-layers-best-practices/23649191#23649191 – Stefan Apr 29 '18 at 07:49
  • 1
    Key is to store the result of `proxy.IsValid = await proxy.TestValidityAsync(judges[0]);` in a `Dictionary` or a `Collection`. It will be super fast and doens't need to interact with the UI. Than later you can be considered about updating the UI. maybe with a timer with interval of 500ms or something like that. You would update the UI from the `Dictionary` or `Collection` – Stefan Apr 29 '18 at 07:52
  • @Stefan Alright, I will have to look into that. Not 100% sure how to implement what you are suggesting in my specific case but maybe after some googling around I can figure it out. I've heard of the DataView object, maybe that is on the right track. – JohnWick Apr 29 '18 at 07:52
  • @Stefan Yeah, that is exactly what I was thinking about doing. Think i'll give that a try and see if it fixes my problem here. Seems like a smart work around. – JohnWick Apr 29 '18 at 07:53
  • @DavidStampher: it's not a work around: its a design principle ;-)... typically designed to overcome these kind of problems :-) – Stefan Apr 29 '18 at 08:01

2 Answers2

3

In Windows every thread that has UI has a message queue - this queue is used to send UI messages for the windows for this thread, those message include things like mouse moved, mouse up/down, etc.

Somewhere in every UI framework there is a loop that reads a message from the queue, processes it and then wait for the next message.

Some messages are lower priority, for example the mouse move message is generated only when the thread is ready to process it (because the mouse tends to move a lot)

BeginInvoke also uses this mechanism, it send a message telling the loop there's code it needs to run.

What you are doing is flooding the queue with your BeginInvoke message and not letting it handle UI events.

The standard solution is to limit the amount of BeginInvoke calls, for example, collect all the items you need to add and use one BeginInvoke call to add them all.

Or add in batches, if you make just one BeginInvoke call per second for all the objects found in this second you probably not effect the UI responsiveness and the user won't be able to tell the difference.

Nir
  • 29,306
  • 10
  • 67
  • 103
  • Thanks for the excellent explanation. I had a feeling this was the case, and the solution you suggested was pretty much what I was thinking as well, but wanted to confirm with people that know better than I. – JohnWick Apr 29 '18 at 08:20
1

Note: For the actual answer on why this is happening, see @Nir's answer. This is only an explanation to overcome som problems and to give some directions. It's not flawless, but it was in line of the conversation by comments.

Just some quick proto type to add some separation of layers (minimal attempt):

//member field which contains all the actual data
List<Proxy> _proxies = new List<Proxy>();

//this is some trigger: it might be an ellapsed event of a timer or something
private void OnSomeTimerOrOtherTrigger()
{ 
      UIupdate();
}

//just a helper function
private void UIupdate
{
    var local = _proxies.ToList(); //ensure static encapsulation 
    proxiesDataGridView.BeginInvoke(new Action(() => 
    {    
         //someway to add *new ones* to UI
         //perform actions on local copy
    }));
}

private void Site_ProxyScraped(object sender, Proxy proxy)
{
    Task.Run(async () =>
    {
        proxy.IsValid = await proxy.TestValidityAsync(judges[0]);
        //add to list
        _proxies.Add(proxy);
    });
}
Stefan
  • 17,448
  • 11
  • 60
  • 79
  • 1
    I would convert that `Site_ProxyScraped` to async though. No need for `Task.Run` here. – FCin Apr 29 '18 at 08:00
  • @Stefan Thanks, I am going to give this a try shortly. If I go with this solution I will mark this as the answer. – JohnWick Apr 29 '18 at 08:00
  • @FCin I will do that from now on with event handlers that need to await calls. – JohnWick Apr 29 '18 at 08:01
  • @FCin: yes, that's a good suggestion, although I am not what's calling that function and if OP is able to handle where the caller is not async. – Stefan Apr 29 '18 at 08:02
  • @Stefan May I ask what you mean by "ensure static encapsulation" when you create that local copy of _proxies? Also, should I call _proxies.Clear() after adding the items to my datagridviews datasource? – JohnWick Apr 29 '18 at 08:15
  • 1
    @DavidStampher: By `ensure static encapsulation`; it's my slang for: deferred/delayed execution and enclosure. Imagine the case where the UI is updating from the list of `_proxies`, and that list is altered somewhere else, which is likely because the UI is relatively slow. Accessing a list from 2 diffent places at the same time can lead to problems: i.e. accessing elements which might be removed. So: working on a local copy would overcome that problem. That's what's going on there. I added a comment: "perform actions on local copy". – Stefan Apr 29 '18 at 08:48