From 441d602a6d74a950bf906f0655c087969b24c0a1 Mon Sep 17 00:00:00 2001 From: Pavlos Touboulidis Date: Thu, 15 May 2014 02:23:03 +0300 Subject: [PATCH 1/2] Fix thread synchronization problem with Sheet.dirty --- OpenRA.Game/Graphics/Sheet.cs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/OpenRA.Game/Graphics/Sheet.cs b/OpenRA.Game/Graphics/Sheet.cs index 4e8cef072c..47e7eb2ff6 100644 --- a/OpenRA.Game/Graphics/Sheet.cs +++ b/OpenRA.Game/Graphics/Sheet.cs @@ -21,6 +21,7 @@ namespace OpenRA.Graphics ITexture texture; bool dirty; byte[] data; + readonly object dirtyLock = new object(); public readonly Size Size; public byte[] Data { get { return data ?? texture.GetData(); } } @@ -68,6 +69,8 @@ namespace OpenRA.Graphics public ITexture Texture { + // This is only called from the main thread but 'dirty' + // is set from other threads too via CommitData(). get { if (texture == null) @@ -78,8 +81,11 @@ namespace OpenRA.Graphics if (dirty) { - texture.SetData(data, Size.Width, Size.Height); - dirty = false; + lock (dirtyLock) + { + texture.SetData(data, Size.Width, Size.Height); + dirty = false; + } } return texture; @@ -140,7 +146,10 @@ namespace OpenRA.Graphics if (data == null) throw new InvalidOperationException("Texture-wrappers are read-only"); - dirty = true; + lock (dirtyLock) + { + dirty = true; + } } } } From 7d352fdad1b9b46b4590d7974b6a9d9cc2e9a819 Mon Sep 17 00:00:00 2001 From: Pavlos Touboulidis Date: Thu, 15 May 2014 02:24:48 +0300 Subject: [PATCH 2/2] MapCache performance improvements --- OpenRA.Game/Graphics/Sheet.cs | 4 +-- OpenRA.Game/Map/MapCache.cs | 65 ++++++++++++++++++++--------------- 2 files changed, 39 insertions(+), 30 deletions(-) diff --git a/OpenRA.Game/Graphics/Sheet.cs b/OpenRA.Game/Graphics/Sheet.cs index 47e7eb2ff6..3c8eb4f078 100644 --- a/OpenRA.Game/Graphics/Sheet.cs +++ b/OpenRA.Game/Graphics/Sheet.cs @@ -79,9 +79,9 @@ namespace OpenRA.Graphics dirty = true; } - if (dirty) + lock (dirtyLock) { - lock (dirtyLock) + if (dirty) { texture.SetData(data, Size.Width, Size.Height); dirty = false; diff --git a/OpenRA.Game/Map/MapCache.cs b/OpenRA.Game/Map/MapCache.cs index 0a7ea1d6cd..0b29ce0659 100644 --- a/OpenRA.Game/Map/MapCache.cs +++ b/OpenRA.Game/Map/MapCache.cs @@ -125,42 +125,50 @@ namespace OpenRA void LoadAsyncInternal() { - for (;;) + Log.Write("debug", "MapCache.LoadAsyncInternal started"); + + // Milliseconds to wait on one loop when nothing to do + var emptyDelay = 50; + // Keep the thread alive for at least 5 seconds after the last minimap generation + var maxKeepAlive = 5000 / emptyDelay; + var keepAlive = maxKeepAlive; + + while (keepAlive-- > 0) { - MapPreview p; + List todo; lock (syncRoot) { - if (generateMinimap.Count == 0) - break; - - p = generateMinimap.Peek(); - - // Preview already exists - if (p.Minimap != null) - { - generateMinimap.Dequeue(); - continue; - } + todo = generateMinimap.Where(p => p.Minimap == null).ToList(); + generateMinimap.Clear(); } + if (todo.Count == 0) + { + Thread.Sleep(emptyDelay); + continue; + } + else + keepAlive = maxKeepAlive; // Render the minimap into the shared sheet - // Note: this is not generally thread-safe, but it works here because: - // (a) This worker is the only thread writing to this sheet - // (b) The main thread is the only thread reading this sheet - // (c) The sheet is marked dirty after the write is completed, - // which causes the main thread to copy this to the texture during - // the next render cycle. - // (d) Any partially written bytes from the next minimap is in an - // unallocated area, and will be committed in the next cycle. - var bitmap = p.CustomPreview ?? Minimap.RenderMapPreview(modData.DefaultRules.TileSets[p.Map.Tileset], p.Map, modData.DefaultRules, true); - p.Minimap = sheetBuilder.Add(bitmap); + foreach (var p in todo) + { + // The rendering is thread safe because it only reads from the passed instances and writes to a new bitmap + var bitmap = p.CustomPreview ?? Minimap.RenderMapPreview(modData.DefaultRules.TileSets[p.Map.Tileset], p.Map, modData.DefaultRules, true); + // Note: this is not generally thread-safe, but it works here because: + // (a) This worker is the only thread writing to this sheet + // (b) The main thread is the only thread reading this sheet + // (c) The sheet is marked dirty after the write is completed, + // which causes the main thread to copy this to the texture during + // the next render cycle. + // (d) Any partially written bytes from the next minimap is in an + // unallocated area, and will be committed in the next cycle. + p.Minimap = sheetBuilder.Add(bitmap); - lock (syncRoot) - generateMinimap.Dequeue(); - - // Yuck... But this helps the UI Jank when opening the map selector significantly. - Thread.Sleep(50); + // Yuck... But this helps the UI Jank when opening the map selector significantly. + Thread.Sleep(Environment.ProcessorCount == 1 ? 25 : 5); + }; } + Log.Write("debug", "MapCache.LoadAsyncInternal ended"); } public void CacheMinimap(MapPreview preview) @@ -171,6 +179,7 @@ namespace OpenRA if (previewLoaderThread == null || !previewLoaderThread.IsAlive) { previewLoaderThread = new Thread(LoadAsyncInternal); + previewLoaderThread.IsBackground = true; previewLoaderThread.Start(); } }