Skip to content
Browse files

Lockless read from existing dictionary

Lock used for writes only. Any reader goes against existing "current"
cache ref.
Any writer replaces shared dictionary ref under lock.
  • Loading branch information...
1 parent 0b204f4 commit 8c05a25be1e297beb6b39666c05befbd60fc2bc2 @nurih nurih committed
Showing with 21 additions and 17 deletions.
  1. +15 −9 Driver/Core/MongoUrl.cs
  2. +6 −8 DriverUnitTests/Core/MongoUrlTests.cs
View
24 Driver/Core/MongoUrl.cs
@@ -15,13 +15,8 @@
using System;
using System.Collections.Generic;
-using System.Linq;
-using System.Text;
-using System.Text.RegularExpressions;
-using System.Threading;
using MongoDB.Bson;
-using MongoDB.Driver.Internal;
namespace MongoDB.Driver
{
@@ -49,6 +44,7 @@ public class MongoUrl : IEquatable<MongoUrl>
{
// private static fields
private static object __staticLock = new object();
+
private static Dictionary<string, MongoUrl> __cache = new Dictionary<string, MongoUrl>();
// private fields
@@ -283,9 +279,10 @@ public TimeSpan WaitQueueTimeout
public static void ClearCache()
{
// Replace cache with new empty one. GC will remove old cache when not referenced.
- // Exchange is atomic: Any thread accessing the cache will either have the old reference or the new one.
- var newEmptyCache = new Dictionary<string, MongoUrl>();
- Interlocked.Exchange(ref __cache, newEmptyCache);
+ lock(__staticLock)
+ {
+ __cache = new Dictionary<string, MongoUrl>();
+ }
}
/// <summary>
@@ -304,6 +301,8 @@ public static MongoUrl Create(string url)
{
lock (__staticLock)
{
+ // re-grab the current cache ref. We got lock, but another thread may have modified before we entered lock.
+ cacheRef = __cache;
if (!cacheRef.TryGetValue(url, out mongoUrl))
{
mongoUrl = new MongoUrl(url);
@@ -319,7 +318,14 @@ public static MongoUrl Create(string url)
cacheRef[canonicalUrl] = mongoUrl; // cache under canonicalUrl also
}
}
- cacheRef[url] = mongoUrl;
+ // copy old dictionary values to new one. Can't guarantee concurrent write while unlocked reader may be accessing cachRef
+ Dictionary<string, MongoUrl> updatedCache = new Dictionary<string, MongoUrl>(cacheRef.Count+1);
+ updatedCache[url] = mongoUrl;
+ foreach (var entry in cacheRef)
+ {
+ updatedCache[entry.Key] = entry.Value;
+ }
+ __cache = updatedCache;
}
}
}
View
14 DriverUnitTests/Core/MongoUrlTests.cs
@@ -17,7 +17,6 @@
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
-using System.Text;
using NUnit.Framework;
using MongoDB.Bson;
@@ -526,10 +525,9 @@ public void TestCacheSameInstanceAcrossMultipleCreate()
var t2 = MongoUrl.Create("mongodb://host2");
var cache2 = getCacheReference(t2);
-
- Assert.AreSame(cache1, cache2);
- Assert.AreEqual(2, cache1.Count);
- Assert.AreEqual(2, cache2.Count);
+ Assert.AreNotSame(cache1, cache2, "Cache internal dictionary is replaced upon insertion to allow lockless reads");
+ Assert.AreEqual(1, cache1.Count, "Cache dictionary should have 1 item after the first insertion");
+ Assert.AreEqual(2, cache2.Count, "Cache dictionary shoudl have 2 items after the second insertion");
}
[Test]
@@ -537,11 +535,11 @@ public void TestCacheDifferentInstanceOnceClearCache()
{
var target = MongoUrl.Create("mongodb://host1");
var cache1 = getCacheReference(target);
-
+
MongoUrl.ClearCache();
-
+
var cache2 = getCacheReference(target);
- Assert.AreNotSame(cache1, cache2,"The 2 cache references shoudl refer to different object since CacheClear() exchanges the old ref to a new empty dictionary");
+ Assert.AreNotSame(cache1, cache2, "The 2 cache references shoudl refer to different object since CacheClear() exchanges the old ref to a new empty dictionary");
}

5 comments on commit 8c05a25

@craiggwilson

line 298 in the full file might cause this to fail on processors that do no support full memory fences for reads. Since we support these processors, this needs to be a volatile read.

That being said, this is the second pull request around this issue(I think). Are you experiencing some issue in relation to performance around this? Generally, this bit of code will run with an uncontested lock which is extremely fast. Could you elaborate a little on the problem you are attempting to solve?

@nuknuk
Owner

Craig, please feel free to drop/ ignore the pull request, or I can rescind it. The original need has been eliminated, and usage of the lib is not problematic under my current production load.

As discussed https://groups.google.com/forum/?fromgroups=#!topicsearchin/mongodb-csharp/group:mongodb-csharp$20AND$20subject:pull$20AND$20subject:request/mongodb-csharp/3Th4r22F5OQ before, the patch was my attempt to reduce locking around what is mostly a read only operation for most of the life of the client application. The reason multiple requests were submitted was that the initial implementaion I submitted was not protecting enough, and I checked in more revisions.

The optimizer should not remove or the assignment since the ref obtained is used.
Once the ref is obtained, the ref count on the dictionary shoudl be increased, preventing GC until released.
You are more knowledgable on this, but this revision might be OK, since a 'dirty" read via the assignment will only lead the reader to maybe miss the item and take a lock then do a (now syncronized) read then discover the items is already there. The assignment itself is not a multi-op so the reference gotten will be either the old or the new bot not a partial or empty pointer.
Is the risk you are referring to that the reference would obtain null?

@craiggwilson

No, not possible that null could get obtained, but definitely possible that have the reference is written and the other half is not (hence reading an non-sensical reference.

My question is still this: is this causing some form of slowness for you? Like I said, this part of the code would largely run in an uncontested lock which is extremely fast.

@nuknuk
Owner

This is not causing undue slowness in my production code. As I said, i'm no longer pursuing submittal of this patch. If what I worte before sounds like i'm having a performance issue on this right now, it was a typo. I edited it with emphasis - not a problem.


As for the discussion regarding thread safety:

http://msdn.microsoft.com/en-us/library/aa691278%28v=vs.71%29.aspx reference assignments are atomic. No half reference assignment should occur. The assignment is of a reference type.

Lock, even uncontested, may cost one or two orders of magnitude on the op, http://msdn.microsoft.com/en-us/magazine/cc163744.aspx#S4 "ten to hundreds of times longer than an ordinary instruction" stated here, other sources claim a yield is forced, so the time slice is forgone etc.

@craiggwilson

Good point. You are right about the atomic write of references.

Please sign in to comment.
Something went wrong with that request. Please try again.