Improved graph rendering performance by reducing locks in settingscache

GitCommands/Settings/AppSettings.cs

@@ -122,22 +122,19 @@ public static bool RememberAmendCommitState
 
         public static void UsingContainer(RepoDistSettings settingsContainer, Action action)
         {
-            SettingsContainer.LockedAction(() =>
-                {
-                    var oldSC = SettingsContainer;
-                    try
-                    {
-                        SettingsContainer = settingsContainer;
-                        action();
-                    }
-                    finally
-                    {
-                        SettingsContainer = oldSC;
+            var oldSC = SettingsContainer;
+            try
+            {
+                SettingsContainer = settingsContainer;
+                action();
+            }
+            finally
+            {
+                SettingsContainer = oldSC;
 
-                        // refresh settings if needed
-                        SettingsContainer.GetString(string.Empty, null);
-                    }
-                });
+                // refresh settings if needed
+                SettingsContainer.GetString(string.Empty, null);
+            }
         }
 
         [CanBeNull]
@@ -1342,11 +1339,8 @@ public static void SaveSettings()
             SaveEncodings();
             try
             {
-                SettingsContainer.LockedAction(() =>
-                {
-                    SshPath = SshPathLocatorInstance.Find(GitBinDir);
-                    SettingsContainer.Save();
-                });
+                SshPath = SshPathLocatorInstance.Find(GitBinDir);
+                SettingsContainer.Save();
             }
             catch
             {

GitCommands/Settings/ConfigFileSettingsCache.cs

@@ -73,32 +73,23 @@ protected override string GetValueImpl(string key)
         /// <param name="configSection">The configuration section.</param>
         public void AddConfigSection(IConfigSection configSection)
         {
-            LockedAction(() =>
-            {
-                EnsureSettingsAreUpToDate();
-                _configFile.Value.AddConfigSection(configSection);
+            EnsureSettingsAreUpToDate();
+            _configFile.Value.AddConfigSection(configSection);
 
-                // mark as dirty so the updated configuration is persisted
-                SettingsChanged();
-            });
+            // mark as dirty so the updated configuration is persisted
+            SettingsChanged();
         }
 
         public IReadOnlyList<string> GetValues(string key)
         {
-            return LockedAction(() =>
-            {
-                EnsureSettingsAreUpToDate();
-                return _configFile.Value.GetValues(key);
-            });
+            EnsureSettingsAreUpToDate();
+            return _configFile.Value.GetValues(key);
         }
 
         public IReadOnlyList<IConfigSection> GetConfigSections()
         {
-            return LockedAction(() =>
-            {
-                EnsureSettingsAreUpToDate();
-                return _configFile.Value.ConfigSections;
-            });
+            EnsureSettingsAreUpToDate();
+            return _configFile.Value.ConfigSections;
         }
 
         /// <summary>
@@ -108,15 +99,12 @@ public IReadOnlyList<IConfigSection> GetConfigSections()
         /// <param name="performSave">If <see langword="true"/> the configuration changes will be saved immediately.</param>
         public void RemoveConfigSection(string configSectionName, bool performSave = false)
         {
-            LockedAction(() =>
+            EnsureSettingsAreUpToDate();
+            _configFile.Value.RemoveConfigSection(configSectionName);
+            if (performSave)
             {
-                EnsureSettingsAreUpToDate();
-                _configFile.Value.RemoveConfigSection(configSectionName);
-                if (performSave)
-                {
-                    _configFile.Value.Save();
-                }
-            });
+                _configFile.Value.Save();
+            }
         }
     }
 }

GitCommands/Settings/FileSettingsCache.cs

@@ -62,24 +62,21 @@ protected override void Dispose(bool disposing)
         {
             if (disposing)
             {
-                LockedAction(() =>
+                if (_saveTimer != null)
                 {
-                    if (_saveTimer != null)
+                    _saveTimer.Dispose();
+                    _saveTimer = null;
+                    _fileWatcher.Changed -= _fileWatcher_Changed;
+                    _fileWatcher.Renamed -= _fileWatcher_Renamed;
+                    _fileWatcher.Created -= _fileWatcher_Created;
+
+                    if (_autoSave)
                     {
-                        _saveTimer.Dispose();
-                        _saveTimer = null;
-                        _fileWatcher.Changed -= _fileWatcher_Changed;
-                        _fileWatcher.Renamed -= _fileWatcher_Renamed;
-                        _fileWatcher.Created -= _fileWatcher_Created;
-
-                        if (_autoSave)
-                        {
-                            Save();
-                        }
-
-                        _fileWatcher.Dispose();
+                        Save();
                     }
-                });
+
+                    _fileWatcher.Dispose();
+                }
             }
 
             base.Dispose(disposing);

GitCommands/Settings/SettingsCache.cs

@@ -1,12 +1,13 @@
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using JetBrains.Annotations;
 
 namespace GitCommands
 {
     public abstract class SettingsCache : IDisposable
     {
-        private readonly Dictionary<string, object> _byNameMap = new Dictionary<string, object>();
+        private readonly ConcurrentDictionary<string, object> _byNameMap = new ConcurrentDictionary<string, object>();
 
         public void Dispose()
         {
@@ -18,23 +19,6 @@ protected virtual void Dispose(bool disposing)
         {
         }
 
-        public void LockedAction(Action action)
-        {
-            LockedAction<object>(() =>
-            {
-                action();
-                return default;
-            });
-        }
-
-        protected T LockedAction<T>(Func<T> action)
-        {
-            lock (_byNameMap)
-            {
-                return action();
-            }
-        }
-
         protected abstract void SaveImpl();
         protected abstract void LoadImpl();
         protected abstract void SetValueImpl(string key, string value);
@@ -45,48 +29,39 @@ protected T LockedAction<T>(Func<T> action)
 
         private void Clear()
         {
-            LockedAction(() =>
-            {
-                ClearImpl();
-                _byNameMap.Clear();
-            });
+            ClearImpl();
+            _byNameMap.Clear();
         }
 
         public void Save()
         {
-            LockedAction(SaveImpl);
+            SaveImpl();
         }
 
         private void Load()
         {
-            LockedAction(() =>
-                {
-                    Clear();
-                    LoadImpl();
-                });
+            Clear();
+            LoadImpl();
         }
 
         public void Import(IEnumerable<(string name, string value)> keyValuePairs)
         {
-            LockedAction(() =>
+            foreach (var (key, value) in keyValuePairs)
+            {
+                if (value != null)
                 {
-                    foreach (var (key, value) in keyValuePairs)
-                    {
-                        if (value != null)
-                        {
-                            SetValueImpl(key, value);
-                        }
-                    }
-
-                    Save();
-                });
+                    SetValueImpl(key, value);
+                }
+            }
+
+            Save();
         }
 
         protected void EnsureSettingsAreUpToDate()
         {
             if (NeedRefresh())
             {
-                LockedAction(Load);
+                Load();
             }
         }
 
@@ -96,29 +71,23 @@ protected virtual void SettingsChanged()
 
         private void SetValue(string name, string value)
         {
-            LockedAction(() =>
-            {
-                // will refresh EncodedNameMap if needed
-                string inMemValue = GetValue(name);
+            // will refresh EncodedNameMap if needed
+            string inMemValue = GetValue(name);
 
-                if (string.Equals(inMemValue, value))
-                {
-                    return;
-                }
+            if (string.Equals(inMemValue, value))
+            {
+                return;
+            }
 
-                SetValueImpl(name, value);
+            SetValueImpl(name, value);
 
-                SettingsChanged();
-            });
+            SettingsChanged();
         }
 
         private string GetValue(string name)
         {
-            return LockedAction(() =>
-            {
-                EnsureSettingsAreUpToDate();
-                return GetValueImpl(name);
-            });
+            EnsureSettingsAreUpToDate();
+            return GetValueImpl(name);
         }
 
         public bool HasValue(string name)
@@ -132,11 +101,8 @@ public bool HasADifferentValue<T>(string name, T value, Func<T, string> encode)
                 ? encode(value)
                 : null;
 
-            return LockedAction(() =>
-            {
-                string inMemValue = GetValue(name);
-                return inMemValue != null && !string.Equals(inMemValue, s);
-            });
+            string inMemValue = GetValue(name);
+            return inMemValue != null && !string.Equals(inMemValue, s);
         }
 
         public void SetValue<T>(string name, T value, Func<T, string> encode)
@@ -145,12 +111,9 @@ public void SetValue<T>(string name, T value, Func<T, string> encode)
                 ? encode(value)
                 : null;
 
-            LockedAction(() =>
-            {
-                SetValue(name, s);
+            SetValue(name, s);
 
-                _byNameMap[name] = s == null ? (object)null : value;
-            });
+            _byNameMap.AddOrUpdate(name, value, (key, oldValue) => value);
         }
 
         public bool TryGetValue<T>([NotNull] string name, T defaultValue, [NotNull] Func<string, T> decode, out T value)
@@ -160,40 +123,36 @@ public bool TryGetValue<T>([NotNull] string name, T defaultValue, [NotNull] Func
                 throw new ArgumentNullException(nameof(decode), $"The decode parameter for setting {name} is null.");
             }
 
-            T val = defaultValue;
+            value = defaultValue;
 
-            bool result = LockedAction(() =>
-            {
-                EnsureSettingsAreUpToDate();
+            EnsureSettingsAreUpToDate();
 
-                if (_byNameMap.TryGetValue(name, out object o))
+            if (_byNameMap.TryGetValue(name, out object o))
+            {
+                switch (o)
                 {
-                    switch (o)
-                    {
-                        case null:
-                            return false;
-                        case T t:
-                            val = t;
-                            return true;
-                        default:
-                            throw new Exception("Incompatible class for settings: " + name + ". Expected: " + typeof(T).FullName + ", found: " + o.GetType().FullName);
-                    }
+                    case null:
+                        return false;
+                    case T t:
+                        value = t;
+                        return true;
+                    default:
+                        throw new Exception("Incompatible class for settings: " + name + ". Expected: " + typeof(T).FullName + ", found: " + o.GetType().FullName);
                 }
+            }
 
-                string s = GetValue(name);
+            string s = GetValue(name);
 
-                if (s == null)
-                {
-                    val = defaultValue;
-                    return false;
-                }
+            if (s == null)
+            {
+                value = defaultValue;
+                return false;
+            }
 
-                val = decode(s);
-                _byNameMap[name] = val;
-                return true;
-            });
-            value = val;
-            return result;
+            T decodedValue = decode(s);
+            value = decodedValue;
+            _byNameMap.AddOrUpdate(name, decodedValue, (key, oldValue) => decodedValue);
+            return true;
         }
     }
 }