Merge lp:~felix-velasco/do/single-thread-reload into lp:do

[Félix Velasco]

[Félix Velasco (felix-velasco)]

This is a cleaner fix to the 100% CPU bug.

Whenever a full reload is requested, it gets scheduled in the update thread, instead of starting a new one. If the thread is reloading, it aborts it and begins a full reload.

[Chris Halse Rogers (raof)]

Sorry for taking so long to get around to looking at this.

The idea behind this patch is good, but I don't think that this implementation using Monitor.Pulse is correct. As I read it, Reload () is always going to block until UniverseUpdateLoop () is active - at which point, Pulsing the lock is not very useful, because nothing's waiting for it.

Because you're waiting on universe_monitor while holding the lock on universe_monitor, Update () isn't going to be able to acquire the lock on universe_monitor until UniverseUpdateLoop () has finished waiting (ie: has hit UpdateTimeout) and released the lock.

I think what you should be using is an AutoResetEvent - that doesn't require an extra lock or boolean, and you can wait on it in the same way as you're currently waiting on the lock.

Because it's been so long since you asked for this review, I'll merge this and make the changes if I don't hear from you in a couple of days.

Sorry again for the delay in review!

review: Needs Fixing

[Félix Velasco (felix-velasco)]

First of all, I had almost forget this code, and I had lost any hope of it ever being merged. Thanks for reviewing after so long!

Then again, I think you didn't read it well. Reload () is never blocked by UniverseUpdateLoop (). In order to invoke Wait () or PulseAll (), you _need_ to own the lock, or a SynchronizationLockException is raised. Wait () both releases the lock, and waits until the current thread can reacquire it, so other threads (i.e., Reload's) can acquire the lock and call Pulse ().

The universe_reload_requested boolean is a flag that allows a partial reload to abort, since a full universe reload has been requested, and will launch afterwards. I know it's not a very elegant solution, but it works.

I haven't worked with AutoResetEvent and, from what I've seen in the docs, I haven't find a way that would prevent the use of the flag. Could you please elaborate? I'd love finding a nicer way to do it.

[Chris Halse Rogers (raof)]

Yeah; when I woke up this morning I thought “I need to look into managed threading more - does Wait release the lock or something”? Thanks for increasing my knowledge of C# thread constructs :).

On further thought, an AutoResetEvent wouldn't work, but a ManualResetEvent should. That would entail:
a) Dropping the universe_monitor & turning universe_reload_requested into a ManualResetEvent
b) Everywhere where you check the status of universe_reload_requested, replace it with a .WaitOne with a 0 timeout, so it doesn't block.
c) Everywhere you set universe_reload_requested to true, replace with universe_reload_requested.Set ()
d) Everywhere you set universe_reload_requested to false, replace with universe_reload_requested.Reset ()
e) Instead of waiting on universe_monitor, universe_reload_requested.WaitOne (UpdateTimeout)

I *think* that would work; it seems you've got a better idea of C# threading than I have, though!

[Félix Velasco (felix-velasco)]

I've been looking at the ManualResetEvent documentation, and the biggest problem I see in using it is that it leaves managed code for execution. A WaitOne call, even one with no timeout, is an expensive operation, specially if we compare it to checking a boolean.

Both solutions apparently provide the same functionality, but I see no benefits in using ManualResetEvent. All in all, it's your call :D

[Chris Halse Rogers (raof)]

I'm a fan of the ManualResetEvent. I feel that it's simpler - you've
got just the one object instead of two, and I'm more familiar with it.

I don't think that the leaving managed code for execution part is a
problem[1]. That's really an implementation detail. Similarly, the
performance doesn't concern me - we'll be calling WaitOne about 3 times
in 5 minutes, and from a non-GUI thread. Each WaitOne could take on the
order of *seconds* and we wouldn't have a serious performance
problem :).

If there aren't any other benefits to the locking + Wait + Pulse method,
I'd prefer to go with the ManualResetEvent. That's more familiar to me,
I think it's intent is clearer, and getting threading right is already
difficult enough :).

[1] Incidentally, where in the docs do you get this? I see that
WaitHandles can be used to wrap native objects, but it's not obvious to
me that it necessarily needs to. After all, *all* threading is
eventually going to need to leave managed code because it'll using OS
threads. You're obviously knowledgeable about managed threading, so any
info you'd like to disseminate would be cool :).

[Félix Velasco (felix-velasco)]

Please go ahead with ManualResetEvent.

I read about the managed code leaving here (
http://www.yoda.arachsys.com/csharp/threads/waithandles.shtml), but I've
been unable to find any similar information anywhere else, so I don't think
it can be trust. My fault.

El 7 de abril de 2010 02:12, Chris Halse Rogers <email address hidden> escribió:

> I'm a fan of the ManualResetEvent. I feel that it's simpler - you've
> got just the one object instead of two, and I'm more familiar with it.
>
> I don't think that the leaving managed code for execution part is a
> problem[1]. That's really an implementation detail. Similarly, the
> performance doesn't concern me - we'll be calling WaitOne about 3 times
> in 5 minutes, and from a non-GUI thread. Each WaitOne could take on the
> order of *seconds* and we wouldn't have a serious performance
> problem :).
>
> If there aren't any other benefits to the locking + Wait + Pulse method,
> I'd prefer to go with the ManualResetEvent. That's more familiar to me,
> I think it's intent is clearer, and getting threading right is already
> difficult enough :).
>
> [1] Incidentally, where in the docs do you get this? I see that
> WaitHandles can be used to wrap native objects, but it's not obvious to
> me that it necessarily needs to. After all, *all* threading is
> eventually going to need to leave managed code because it'll using OS
> threads. You're obviously knowledgeable about managed threading, so any
> info you'd like to disseminate would be cool :).
>
> --
>
> https://code.launchpad.net/~felix-velasco/do/single-thread-reload/+merge/15312
> You are the owner of lp:~felix-velasco/do/single-thread-reload.
>

=== modified file 'Do.Platform.Linux/src/Do.Universe/ApplicationItem.cs'
--- Do.Platform.Linux/src/Do.Universe/ApplicationItem.cs	2009-11-19 20:52:16 +0000
+++ Do.Platform.Linux/src/Do.Universe/ApplicationItem.cs	2009-11-27 10:50:26 +0000
@@ -50,25 +50,22 @@
 
 			if (path == null) throw new ArgumentNullException ("path");
 
-			lock (Instances)
-			{
-				if (Instances.ContainsKey (key)) {
-						appItem = Instances [key];
-				} else {
-					DesktopItem item = null;
-					try {
-						item = DesktopItem.NewFromFile (path, 0);
-						appItem = new ApplicationItem (item);
-					} catch (Exception e) {
-						appItem = null;
-						try { item.Dispose (); } catch { }
-						Log.Error ("Could not load desktop item: {0}", e.Message);
-						Log.Debug (e.StackTrace);
-					}
-
-					if (appItem != null)
-						Instances [key] = appItem;
+			if (Instances.ContainsKey (key)) {
+					appItem = Instances [key];
+			} else {
+				DesktopItem item = null;
+				try {
+					item = DesktopItem.NewFromFile (path, 0);
+					appItem = new ApplicationItem (item);
+				} catch (Exception e) {
+					appItem = null;
+					try { item.Dispose (); } catch { }
+					Log.Error ("Could not load desktop item: {0}", e.Message);
+					Log.Debug (e.StackTrace);
 				}
+
+				if (appItem != null)
+					Instances [key] = appItem;
 			}
 			return appItem;
 		}

=== modified file 'Do.Platform.Linux/src/Do.Universe/CategoryItem.cs'
--- Do.Platform.Linux/src/Do.Universe/CategoryItem.cs	2009-11-19 20:52:16 +0000
+++ Do.Platform.Linux/src/Do.Universe/CategoryItem.cs	2009-11-27 10:50:26 +0000
@@ -48,12 +48,9 @@
 		public static CategoryItem GetCategoryItem (string category)
 		{
 			string lowCat = category.ToLower ();
-			lock (Instances)
-			{
-				if (!Instances.ContainsKey (lowCat)) {
-					CategoryItem item = new CategoryItem (category);
-					Instances [lowCat] = item;
-				}
+			if (!Instances.ContainsKey (lowCat)) {
+				CategoryItem item = new CategoryItem (category);
+				Instances [lowCat] = item;
 			}
 			return Instances [lowCat];
 		}

=== modified file 'Do/src/Do.Core/UniverseManager.cs'
--- Do/src/Do.Core/UniverseManager.cs	2009-06-24 22:42:51 +0000
+++ Do/src/Do.Core/UniverseManager.cs	2009-11-27 10:50:26 +0000
@@ -40,6 +40,8 @@
 		UniverseCollection universe;
 		EventHandler initialized;
 		object universe_lock;
+		object universe_monitor;
+		volatile bool universe_reload_requested;
 		
 		const float epsilon = 0.00001f;
 		
@@ -80,6 +82,8 @@
 		{
 			universe = new UniverseCollection ();
 			universe_lock = new object ();
+			universe_monitor = new object ();
+			universe_reload_requested = false;
 
 			update_thread = new Thread (new ThreadStart (UniverseUpdateLoop));
 			update_thread.IsBackground = true;
@@ -165,17 +169,33 @@
 			DateTime startUpdate = DateTime.UtcNow;
 
 			while (true) {
-				Thread.Sleep (UpdateTimeout);
+				lock (universe_monitor) {
+					if (!universe_reload_requested) {
+						Monitor.Wait (universe_monitor, UpdateTimeout);
+					}
+				}
+
 				if (Do.Controller.IsSummoned) continue;
 				startUpdate = DateTime.UtcNow;
-				
+
+				if (universe_reload_requested) {
+					universe_reload_requested = false;
+					ReloadUniverse ();
+					continue;
+				}
+
 				if (rand.Next (10) == 0) {
 					ReloadActions (universe);
+					if (universe_reload_requested)
+						continue;
 				}
-				
+
 				foreach (ItemSource source in PluginManager.ItemSources) {
 					ReloadSource (source, universe);
-					
+
+					if (universe_reload_requested)
+						break;
+
 					if (UpdateRunTime < DateTime.UtcNow - startUpdate) {
 						Thread.Sleep (UpdateTimeout);
 						// sleeping for a bit
@@ -308,7 +328,11 @@
 		/// </summary>
 		public void Reload ()
 		{
-			Services.Application.RunOnThread (ReloadUniverse);
+			universe_reload_requested = true;
+			Log<UniverseManager>.Info ("Requesting universe reload");
+			lock (universe_monitor) {
+				Monitor.PulseAll (universe_monitor);
+			}
 		}
 	}
 }