Improved graph rendering performance by reducing locks in settingscache

[spdr870]

Fixes random lockups when rendering the revision graph. Profiling showed a lot of (waiting) time on the locks in the settingscache. During cell rendering, the setting 'ShowRevisionGridGraphColumn' is called extreme often. In some occasions this caused the application to freeze and not recover (waited few minutes). This also improves the scrolling speed a bit, when revision graph is visible.

Changes proposed in this pull request:

• All the locks in the settingscache are removed
• The dictionary used for caching is replaced with a concurrent dictionary

What did I do to test the code and ensure quality:

• Manual testing
• Profiling

Has been tested on (remove any that don't apply):

• GIT 2.18
• Windows 10

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@bf628b7). Click here to learn what that means.
The diff coverage is 81.81%.

@@            Coverage Diff            @@
##             master    #5540   +/-   ##
=========================================
  Coverage          ?   35.22%           
=========================================
  Files             ?      607           
  Lines             ?    46294           
  Branches          ?     6312           
=========================================
  Hits              ?    16307           
  Misses            ?    29236           
  Partials          ?      751

[jbialobr]

Replacing locks with concurrent dictionary does not guarantee cache objects data consistency. It is like updating a database without transactions. My proposal to fix this is to cache the needed settings locally in places where they are fetched intensively or under another lock. Sth along the lines:

        private bool _showRevisionGridGraphColumn;

        public RevisionDataGridView()
        {
            AppSettings.OnChanged += (s, e) =>
            {
                _showRevisionGridGraphColumn = AppSettings.ShowRevisionGridGraphColumn;
            };
...

[spdr870]

@jbialobr The change you propose is very difficult to do and maintain. The settings are read all over the place. For example in functions like GetJunctionColor().

How about this; keep the concurrentdictionary. It makes it safe to read when other threads are writing. Only do the locks around the Set, Safe and Readfrom file. No locks around GetValue. This ensures the cache data is consistent. If you receive an old value from GetValue, I consider this a timing issue, not a concurrency issue. The GetValue is like ReadUncommitted...:)

[jbialobr]

Sounds appealing, but I am still uneasy about it. There is a mechanism that refreshes the in-memory settings if the corresponding settings file is changed. Now before each GetValue GE ensures that we are in sync with the file. We would have to give up that and move the refreshing into a backgroud thread. Then when reading a settings group, like build server configuration, could be divided by the refreshing in the background into two parts of possibly inconsistent data. I know it is possible with the current implementation but currently it can occur only when someone else has changed the in-file settings between the particular reads of the group. After introducing the background refreshing it is enough that the in-file settings has changed before the group is read (no matter when).

[spdr870]

Wouldn't it be enough to just make the loading locked? The mechanism to make sure that the settings are up-to-date is in this method:
protected void EnsureSettingsAreUpToDate() { if (NeedRefresh()) { LockedAction(Load); } }

The code inside the NeedRefresh is still locked. So even when the GetValue doesn't use locks, when a refresh is required, all GetValues are locked because the settings are loaded.

The GetValue method without locks would be:
private string GetValue(string name) { EnsureSettingsAreUpToDate(); return GetValueImpl(name); }

[jbialobr]

That would be enough, but this way we are still exposed to locks (and related to them slowdowns and deadlocks) while calling the GetValue.

[RussKie]

What if (a big one):

• AppSettings class wasn't a static, but an instance singleton,
• Whenever we detected an underlying config file changes - the loading mechanism loads into a new instance of AppSettings, and
• Performs an atomic swap of the settings instances

No locks needed.

[jbialobr]

What if (a big one):

Depending on how you are going to load the new instance (in background or in the caller thread) you will fall into one of the issues I described above.

[jbialobr]

AppSettings class wasn't a static, but an instance singleton,

In fact it is already the fact because all the AppSettings getters and setters are redirected to a singleton instance of SettingsContainer. It would be enough to recreate the underlying SettingsCache only.

[spdr870]

That would be enough, but this way we are still exposed to locks (and related to them slowdowns and deadlocks) while calling the GetValue.

But only if the settings require reloading. Since the settings hardly ever needs reloading from disk, I do not see an issue.

[spdr870]

After (1 day) testing I got an exception that 2 threads where both reading the same file. So I partly reverted the change. The set/(re)load/Save actions now use a lock. The GetValue is not locked. ConcurrentDictionary is still used. Performance is not affected. Do you consider the change safe after this change?

[jbialobr]

Do you consider the change safe after this change?

I consider this a good direction. Changes proposed here are not safe and require some more work. GetValue calls GetValueImpl which is not thread-safe. It is free to the extending class to decide how it is implemented. See GitExtSettingsCache for instance. Reads and writes may now occur concurrently producing rubbish reads.

[spdr870]

Ok, didn't know about GitExtSettingsCache. Indeed, GetValueImpl is not safe because it uses a non safe dictionary.

What if I change it again, put all the locks back. But instead of normal locks, I use readerwriterslimlock? This way at least everything is save. The overhead of the readerwriterslimlock is not that big. Locking at least is minimized then.

[jbialobr]

If you leave the _byNameMap as ConcurentDictionary then you don't need to put back the lock into TryGetValue. Putting back the lock into GetValue and replacing the locks with readerwriterslimlock is ok.

[spdr870]

I attempted to implement the readerwriterslimlock. Since the level of recursion required, I am not sure this is a good solution. Then I looked at the code again and did some more debugging. I now understand your remark about GetValue and TryGetValue. At first, I didn't saw what the difference is. But now I do!

TryGetValue will look in the cache first, and then calls GetValue if it is not found in the cache. The GetValue doesn't try the cache first. Didn't got that from the functionnames. So, I did what you suggested earlier. I put all the locks back, except for the one in TryGetValue. The dictionary is still concurrent. Also, I added some comments to the method TryGetValue.

Since the tryget is always used in the AppSettings class, the performance gain is still the same as the initial PR. Only more safe. Using the readerwriterslimlock is not needed, since locks are now hardly used.

[spdr870]

Thanks for your help!

Rebased and pushed to master.