Pull request for c# driver - reduce locking patch

[nuk nuk]

Hi,

I submitted a pull request a while back - never got any action or traction. https://github.com/mongodb/mongo-csharp-driver/pull/118
Is there some other procedure to submit patches? Do i need to contact someone directly?


Thanks!

[Daniel Harman]

No skin in this game, but wouldn't concurrent dictionary be more appropriate, or is the driver held back to support those on .net 1.0? ;)

Sent from my iPad

[craiggwilson]

We are constrained to .NET 3.5 at the moment, so we cannot use the concurrent collections from .NET 4.0.

NukNuk, that is the proper procedure.  We simply missed it.  My apologies.  Regarding your pull request, are you creating so many MongoServer's that this is important.  It adds a bit of complexity that seems unnecessary to me.  In addition, this only satisfies conditions a strong memory models.  To account for a weak memory model (like Itanium processors), you'll need to add volatile to the __cache member or create a memory barrier before reading it.

[Daniel Harman]

Just looked in detail at delta and agree about the problem. The memory barrier then going to harm perf anyway.

Every time I try to write lockless I realise I'm not clever enough! Of course that's why there are only a handful of lockless data structures and you can still get a phd and prize for coming up with a new one!

Dan

Sent from my iPad

[nuk nuk]

A Thread.MemoryBarrier() might do the trick on the first cacheRef = __cache;

The issue is that every reader imposes a yield. Under mine and most users scenarios, the __cache will be rarely written to, yet a lock is imposed on every reader.

Regarding volatile - we may still be OK. Under current code, the thread must yield because lock() {} is called. Under the proposed code, a yield is not mandated when the current cache has the item.


Assuming non-volatile __cache declaration, there are 2 dry-cache outcomes:
1 - that the __cache does not have the url
2 - that the "real" __cache (new one) has it, but the reader sees the old __cache (dry)
In either event, the lock(){} then steps in, and imposes the isolation and full sync with main memory. the __cach is then re-gotten , but never modified. It is copied over and then the main __cache reference is atomically swapped.
The assignment of the __cache object is always done with an effectively read-only value. __cache is not modified because the reference to it is grabbed once. If while a thread is reading __cache another is in the lock, the __cache may be swapped mid code, but cacheRef variable has a point in time reference to __cache. If cache is then modified by another thread, the reader still has the old one, and at most would be compelled to call for a lock and try creating a new MongoUrl() but that would not happen due to the second check.

[craiggwilson]

It's not the Creating of the servers that has the stale __cache read problem, it's the reading elsewhere in the code.  Regardless, I'd like to renew my question of how often are you calling this method?  Since this class is safe to throw into your IoC container (or whatever you are doing), then why are you calling this method so often?  (There may be a valid reason, I'd just like to hear it :).

[mongotime]

Since we are on the subject on optimizations, I had done some hotspot analysis of driver back in 1.3 and had found that the next best thing to optimize was the use of HashTables instead of Dictionary + Lock in certain places. Created a 50% boost in some theoretical cpu-bound tests. See below if interested:

https://groups.google.com/d/msg/mongodb-csharp/Ff81lTXg-gU/axixImQGlWMJ

[nuk nuk]

Short answer - no. Out code calls MongoServer Create() once, then holds it in a static.

Long answer - no. But wanted to code up create server each call so that hot config changes can be made, and so went on to read the driver code. In doing so, encountered the lock around reading the MongoUrl cache, and that inspired me to attempt an improvement.

At this point, looks like this optimization is of low marginal value. So I'm not pressing for its adoption further.

Thank you Craig for considering it and for the valuable insights!

On Friday, August 10, 2012 6:24:27 PM UTC-7, craiggwilson wrote:

[nuk nuk]

Great stuff! Thanks!
One thing this whole subject brings up is the Itanium and upcoming ARM architecture the C# driver might run on.
As Craig points out, weak memory model needs to be considered.
So my take-home is to review patterns such as below to see if they need a volatile (__staticObjectRef)
if (__staticObjRef == null)
{
  newOne = SomeFactory.create();
  __staticObjRef = newOne;
}
return __staticObjRef;

[Daniel Harman]

You could work around that in a lot of ways which would avoid a server recreate on every call and require a lot less ingenuity than trying to write perfectly thread safe lock free containers! I think it was this article that cured me: http://www.drdobbs.com/cpp/lock-free-code-a-false-sense-of-security/210600279

Now I just use the framework stuff, or disassemble the libraries that Stephen Toub has put into tpl-df or rx!

Dan