Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Lockless read of MongoUrl cache #118

Closed
wants to merge 3 commits into from

3 participants

nuk nuk Craig Wilson Chip M
nuk nuk
nuknuk commented

MongoUrl.Create() was taking a lock on each call, whether read or write.
Implemented lockcless read to reduce cost of Create when the MongoUrl is already in cache.

chipm added some commits
Chip M chipm MongoUrl cache implementation with less locking
Using check-lock-check, no need to take a lock on each cache read.
7d1a387
Chip M chipm Get local ref to cache instance 0b204f4
nuk nuk

Previous implementation took a lock each time a Create(url) was issued. By using Interlocked.Exchange() lock is not needed on calls that would hit the cache. Only cache-miss will require a lock.

Now using a more coarse lock{} but only on write.

Chip M chipm 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.
8c05a25
Craig Wilson
Collaborator

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?

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?

Collaborator

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.

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.

Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 3 unique commits by 1 author.

Jun 16, 2012
Chip M chipm MongoUrl cache implementation with less locking
Using check-lock-check, no need to take a lock on each cache read.
7d1a387
Chip M chipm Get local ref to cache instance 0b204f4
Jun 23, 2012
Chip M chipm 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.
8c05a25
This page is out of date. Refresh to see the latest.

Showing 2 changed files with 105 additions and 19 deletions. Show diff stats Hide diff stats

  1. +33 17 Driver/Core/MongoUrl.cs
  2. +72 2 DriverUnitTests/Core/MongoUrlTests.cs
50 Driver/Core/MongoUrl.cs
@@ -15,12 +15,8 @@
15 15
16 16 using System;
17 17 using System.Collections.Generic;
18   -using System.Linq;
19   -using System.Text;
20   -using System.Text.RegularExpressions;
21 18
22 19 using MongoDB.Bson;
23   -using MongoDB.Driver.Internal;
24 20
25 21 namespace MongoDB.Driver
26 22 {
@@ -48,6 +44,7 @@ public class MongoUrl : IEquatable<MongoUrl>
48 44 {
49 45 // private static fields
50 46 private static object __staticLock = new object();
  47 +
51 48 private static Dictionary<string, MongoUrl> __cache = new Dictionary<string, MongoUrl>();
52 49
53 50 // private fields
@@ -281,7 +278,11 @@ public TimeSpan WaitQueueTimeout
281 278 /// </summary>
282 279 public static void ClearCache()
283 280 {
284   - __cache.Clear();
  281 + // Replace cache with new empty one. GC will remove old cache when not referenced.
  282 + lock(__staticLock)
  283 + {
  284 + __cache = new Dictionary<string, MongoUrl>();
  285 + }
285 286 }
286 287
287 288 /// <summary>
@@ -292,28 +293,43 @@ public static void ClearCache()
292 293 public static MongoUrl Create(string url)
293 294 {
294 295 // cache previously seen urls to avoid repeated parsing
295   - lock (__staticLock)
  296 + MongoUrl mongoUrl;
  297 + // get ref to current dictionary instance.
  298 + var cacheRef = __cache;
  299 +
  300 + if (!cacheRef.TryGetValue(url, out mongoUrl))
296 301 {
297   - MongoUrl mongoUrl;
298   - if (!__cache.TryGetValue(url, out mongoUrl))
  302 + lock (__staticLock)
299 303 {
300   - mongoUrl = new MongoUrl(url);
301   - var canonicalUrl = mongoUrl.ToString();
302   - if (canonicalUrl != url)
  304 + // re-grab the current cache ref. We got lock, but another thread may have modified before we entered lock.
  305 + cacheRef = __cache;
  306 + if (!cacheRef.TryGetValue(url, out mongoUrl))
303 307 {
304   - if (__cache.ContainsKey(canonicalUrl))
  308 + mongoUrl = new MongoUrl(url);
  309 + var canonicalUrl = mongoUrl.ToString();
  310 + if (canonicalUrl != url)
305 311 {
306   - mongoUrl = __cache[canonicalUrl]; // use existing MongoUrl
  312 + if (cacheRef.ContainsKey(canonicalUrl))
  313 + {
  314 + mongoUrl = cacheRef[canonicalUrl]; // use existing MongoUrl
  315 + }
  316 + else
  317 + {
  318 + cacheRef[canonicalUrl] = mongoUrl; // cache under canonicalUrl also
  319 + }
307 320 }
308   - else
  321 + // copy old dictionary values to new one. Can't guarantee concurrent write while unlocked reader may be accessing cachRef
  322 + Dictionary<string, MongoUrl> updatedCache = new Dictionary<string, MongoUrl>(cacheRef.Count+1);
  323 + updatedCache[url] = mongoUrl;
  324 + foreach (var entry in cacheRef)
309 325 {
310   - __cache[canonicalUrl] = mongoUrl; // cache under canonicalUrl also
  326 + updatedCache[entry.Key] = entry.Value;
311 327 }
  328 + __cache = updatedCache;
312 329 }
313   - __cache[url] = mongoUrl;
314 330 }
315   - return mongoUrl;
316 331 }
  332 + return mongoUrl;
317 333 }
318 334
319 335 // public methods
74 DriverUnitTests/Core/MongoUrlTests.cs
@@ -16,7 +16,7 @@
16 16 using System;
17 17 using System.Collections.Generic;
18 18 using System.Linq;
19   -using System.Text;
  19 +using System.Reflection;
20 20 using NUnit.Framework;
21 21
22 22 using MongoDB.Bson;
@@ -478,5 +478,75 @@ public void TestEquals()
478 478 Assert.IsFalse(n != null);
479 479 Assert.IsFalse(null != n);
480 480 }
  481 +
  482 + [Test]
  483 + public void TestCacheEmptyWhenInstanceConstructed()
  484 + {
  485 + var target = new MongoUrl("mongodb://host1");
  486 +
  487 + Dictionary<string, MongoUrl> cache = getCacheReference(target);
  488 +
  489 + Assert.IsNotNull(cache);
  490 + Assert.AreEqual(0, cache.Count);
  491 + }
  492 +
  493 + [Test]
  494 + public void TestCacheHasItemAfterCreate()
  495 + {
  496 + var target = MongoUrl.Create("mongodb://host1");
  497 +
  498 + Dictionary<string, MongoUrl> cache = getCacheReference(target);
  499 +
  500 + Assert.IsNotNull(cache);
  501 + Assert.AreEqual(1, cache.Count);
  502 + }
  503 +
  504 + [Test]
  505 + public void TestCacheEmptyAfterClear()
  506 + {
  507 + var target = MongoUrl.Create("mongodb://host1");
  508 +
  509 + Dictionary<string, MongoUrl> cache = getCacheReference(target);
  510 +
  511 + Assert.AreEqual(1, cache.Count);
  512 + MongoUrl.ClearCache();
  513 +
  514 + cache = getCacheReference(target);
  515 +
  516 + Assert.AreEqual(0, cache.Count);
  517 + }
  518 +
  519 +
  520 + [Test]
  521 + public void TestCacheSameInstanceAcrossMultipleCreate()
  522 + {
  523 + var t1 = MongoUrl.Create("mongodb://host1");
  524 + var cache1 = getCacheReference(t1);
  525 + var t2 = MongoUrl.Create("mongodb://host2");
  526 + var cache2 = getCacheReference(t2);
  527 +
  528 + Assert.AreNotSame(cache1, cache2, "Cache internal dictionary is replaced upon insertion to allow lockless reads");
  529 + Assert.AreEqual(1, cache1.Count, "Cache dictionary should have 1 item after the first insertion");
  530 + Assert.AreEqual(2, cache2.Count, "Cache dictionary shoudl have 2 items after the second insertion");
  531 + }
  532 +
  533 + [Test]
  534 + public void TestCacheDifferentInstanceOnceClearCache()
  535 + {
  536 + var target = MongoUrl.Create("mongodb://host1");
  537 + var cache1 = getCacheReference(target);
  538 +
  539 + MongoUrl.ClearCache();
  540 +
  541 + var cache2 = getCacheReference(target);
  542 + Assert.AreNotSame(cache1, cache2, "The 2 cache references shoudl refer to different object since CacheClear() exchanges the old ref to a new empty dictionary");
  543 + }
  544 +
  545 +
  546 + private Dictionary<string, MongoUrl> getCacheReference(MongoUrl target)
  547 + {
  548 + var fieldInfo = typeof(MongoUrl).GetField("__cache", BindingFlags.Static | BindingFlags.GetField | BindingFlags.NonPublic);
  549 + return fieldInfo.GetValue(target) as Dictionary<string, MongoUrl>;
  550 + }
481 551 }
482   -}
  552 +}

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.