Lockless read of MongoUrl cache #118

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants

nuknuk commented Jun 23, 2012

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.

nurih added some commits Jun 16, 2012

@nurih nurih MongoUrl cache implementation with less locking
Using check-lock-check, no need to take a lock on each cache read.
7d1a387
@nurih nurih Get local ref to cache instance 0b204f4
Owner

nuknuk commented on 7d1a387 Jun 16, 2012

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.

@nurih nurih 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

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?

Owner

nuknuk replied Sep 19, 2012

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?

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.

Owner

nuknuk replied Sep 19, 2012

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.

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