[RESOLVED] Too much for the UI thread to handle

:wave:

I've got a form that acts as a progress report for a long running operation, it has a listview on it (which I might be replacing with a DGV soon) that gets updated with progress updates from a background thread that is doing all the work. The method running on the background thread raises events that the form then handles and adds the string passed in to this event to the listview. The method running on the background thread raises progress update events several times a second and for each of these updates the event handler on the form must invoke a method on the UI thread to actually add the item to the listview.

This works fine on my test PC (which has a quad core CPU and 8 GB of RAM) but others have reported that after a while the form goes to the Not Responding state so I'm assuming all of these updates on the UI thread are being requested too often for it to be able to process them all as well as user input.

So I'm looking for a better solution and have come up with the following:
When the background thread raises these progress updated events, instead of them being added to the listview instantly I add them to a Queue(Of String). Then every second the form would check this queue and remove any items currently in it and add them to the listview - using a Queue(Of T) instead of a List(Of T) should guarantee they come out in the correct order. Both of these operations (adding to the queue and reading/removing from it) would be done under a SyncLock block that would lock on the same object to avoid both threads trying to work with the queue at the same time, so there would still be some contention but I'm hoping it would mean the UI thread only gets busy once a second instead of potentially roughly 30 times a second as it is doing now.

Does that sound like a good plan? Can anyone think of a better way of handling this? :)

Cheers
Chris

Sounds like a plan. I would bound the listview manipulation with

Code:

---

        ListView1.BeginUpdate()

        'add / remove items here

        ListView1.EndUpdate()

---

Yeah that's a good point thanks, as I will be doing more than one update at a time now. As I mentioned in the original post though, I'm thinking of replacing this ListView with a DataGridView (not sure if that has the same methods that you mentioned but I will check) as the Listview has an annoyingly low character limit for items it displays

Well I've implemented this now and all seems to be working fine :) I also changed to a DGV instead of a Listview and unfortunately there isn't a BeginUpdate/EndUpdate method for that but it still seems to be working OK.

I find that the way to do that is to set up a Date object and only update the UI at most once every 400 ms, just in case. Even if you use BeginUpdate() and EndUpdate() I think there could still be problems... That does seem a bit strange, though, because the problem usually occurs on faster computers :)

You don't need a synclock around queue access, as Enqueue and Dequeue are safe to be called at the same time as each other on separate threads.

... Wrong thread?

Quote:

---

Originally Posted by chris128

using a Queue(Of T) instead of a List(Of T) should guarantee they come out in the correct order. Both of these operations (adding to the queue and reading/removing from it) would be done under a SyncLock block that would lock on the same object to avoid both threads trying to work with the queue at the same time

---

No, not wrong thread at all. :)

Quote:

---

Originally Posted by Evil_Giraffe

You don't need a synclock around queue access, as Enqueue and Dequeue are safe to be called at the same time as each other on separate threads.

---

Oh right, I'll have to have a look into that, thanks :)

It seems that this isn't working as well as I thought though, as adding a large number of rows to the DGV every second on a slow computer results in the same issue of it going to Not Responding :( I might look at using the DGV.Rows.AddRange method to add all of the rows at once rather than adding them one by one as I'm assuming this will do something similar to the ListView's BeginUpdate and EndUpdate internally.

Quote:

---

Originally Posted by Evil_Giraffe

You don't need a synclock around queue access, as Enqueue and Dequeue are safe to be called at the same time as each other on separate threads.

---

Hmm I've been looking into this and not sure why you would say that as I can't find any reference on MSDN that confirms this. I did find this thread that suggests if you use Queue.Synchronised then the adding/removing is thread safe but other operations are not http://stackoverflow.com/questions/3...-thread-safety so I think I might just stick with the way I'm currently doing it.

Quote:

---

Originally Posted by chris128

Hmm I've been looking into this and not sure why you would say that as I can't find any reference on MSDN that confirms this. I did find this thread that suggests if you use Queue.Synchronised then the adding/removing is thread safe but other operations are not http://stackoverflow.com/questions/3...-thread-safety so I think I might just stick with the way I'm currently doing it.

---

I use monitor enter /exit, but I agree that the queues must be protected.

Quote:

---

Originally Posted by dbasnett

I use monitor enter /exit, but I agree that the queues must be protected.

---

It's rarely necessary to use a Monitor directly as SyncLock is a simple alias.

Calling Enqueue and Dequeue together can't be a problem. Thread synchronisation issues occur when non-atomic operations interleave and interfere with each other. In the case of a Queue, data is added to one end and removed from the other, so there can be no interference. Dequeuing in two threads at the same time could be a problem, because one thread might start and then be pre-empted by another thread dequeuing the last item. The first thread would then try to complete the operation and fail because the item it was going to dequeue is no longer there.

Quote:

---

Originally Posted by jmcilhinney

It's rarely necessary to use a Monitor directly as SyncLock is a simple alias.

Calling Enqueue and Dequeue together can't be a problem. Thread synchronisation issues occur when non-atomic operations interleave and interfere with each other. In the case of a Queue, data is added to one end and removed from the other, so there can be no interference. Dequeuing in two threads at the same time could be a problem, because one thread might start and then be pre-empted by another thread dequeuing the last item. The first thread would then try to complete the operation and fail because the item it was going to dequeue is no longer there.

---

In the case of this thread you are correct (as usual ;).

In the broader case I feel that it is a bad habit. Today you have one thread adding to the queue and one removing. Tomorrow, for some reason, you have multiple threads removing, and now you have a problem. My experience has been that the problem is tough to track down because of the multi-threading environment (it works, then it doesn't, then it does).

Any time I am mutli-threading and sharing resources I use locking.

JMC - are you sure? The thing is, Enqueue and Dequeue are not atomic operations are they? This is the internal definition of Dequeue:

vb Code:

Dim local As T = Me._array(Me._head)
Me._array(Me._head) = CType(Nothing, T)
Me._head = ((Me._head + 1) Mod Me._array.Length)
Me._size -= 1
Me._version += 1
Return local

and this is Enqueue:

vb Code:

If (Me._size = Me._array.Length) Then
        Dim capacity As Integer = CInt(((Me._array.Length * 200) / 100))
        If (capacity < (Me._array.Length + 4)) Then
            capacity = (Me._array.Length + 4)
        End If
        Me.SetCapacity(capacity)
    End If
    Me._array(Me._tail) = item
    Me._tail = ((Me._tail + 1) Mod Me._array.Length)
    Me._size += 1
    Me._version += 1

Bearing in mind that SetCapacity is what sets the Integer _head, I think in a very specific and rather unlikely set of circumstances this could result in the Enqueue method setting the wrong item to Nothing. I may be wrong, and to be honest I'm very tired right now so I probably am...

If I do continue to use SyncLock, one thing I have thought of that might improve things is if instead of doing something like this as I currently do:

vb Code:

SyncLock SomeObject
   Do Until SomeQueue.Count = 0
      Dim CurrentItem As SomeItem = SomeQueue.Dequeue
      'Add row to DGV here using values from properties of CurrentItem to populate cells
   Loop
End SyncLock

I do this:

vb Code:

Dim TempList As New List(Of SomeItem)
SyncLock SomeObject
   Do Until SomeQueue.Count = 0
      TempList.Add(SomeQueue.Dequeue)
   Loop
End SyncLock
 
For Each CurrentItem As SomeItem In TempList
   'Add row to DGV here using values from properties of CurrentItem to populate cells
Next

As then there will be less time for the background thread to potentially end up sat waiting for the UI thread to finish adding rows to the DGV (which could take a while if there are a large number of updates to add). What do we think? Yay or Nay?

Regardless, using SyncLock won't hurt and it's barely more code. dbasnett's point about preparing for future changes is a relevant one. Better safe than sorry.

Yeah I am still going to use SyncLock, but I'm still having issues with it taking too long to add the rows to the DGV... tried changing it to use DataBinding and this has helped a little but still when there are a large number of updates to add it can take longer than a second for them to all be visible on the DGV and then the next timer interval has been reached so it starts trying to add the next lot of updates...

I guess the only thing I can do is make the timer interval longer but that's not ideal..

Quote:

---

Originally Posted by chris128

Yeah I am still going to use SyncLock, but I'm still having issues with it taking too long to add the rows to the DGV... tried changing it to use DataBinding and this has helped a little but still when there are a large number of updates to add it can take longer than a second for them to all be visible on the DGV and then the next timer interval has been reached so it starts trying to add the next lot of updates...

I guess the only thing I can do is make the timer interval longer but that's not ideal..

---

Are you adding all new information to the DGV, or is some of it the same? Is it like an event log?

Its just a progress log of the operation that is currently running (which can take anywhere from 1 second to 10 minutes depending on the user's environment). The majority of updates will say something like:

Found match: XXXXXX - Gathering additional attributes
followed by
Finished gathering additional attributes
but then any errors encountered will also be reported here as well, along with other informational messages.

EDIT: This is a fairly old screenshot from when it was using the ListView but you get the idea. In this example it has only found one result, where as it could potentially find thousands.
http://cjwdev.files.wordpress.com/2010/10/image8.png

OK so I've decided to go with a different approach as I can't see any way to make this work well in all situations. Rather than having a rolling log window that displays all progress updates, I just have a label that displays the latest update but then also a DGV showing any errors or warnings encountered so that these are not missed (each update has its own type - info, warning, or error). Then I also have the option to save the full log to file and that includes all progress updates (with the time they were generated next to them).

http://www.cjwdev.co.uk/Other/ProgressNew2.png

http://www.cjwdev.co.uk/Other/ProgressNew.png

So that solves all of my problems really - I only have to update a single label each time a progress update event is raised, so the UI does not lock up, but the user still gets to see the current operation along with any warnings/errors and has the option to save the full log to file if they really want to view all of the informational progress updates :)