> 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?
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.
On Friday, August 10, 2012 5:16:07 AM UTC-5, Daniel Harman wrote:
> 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
> On 10 Aug 2012, at 01:12, nuk nuk <shlu...@gmail.com <javascript:>> > wrote:
> > 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?
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
On 10 Aug 2012, at 12:10, craiggwilson <craiggwil...@gmail.com> wrote:
> 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.
> On Friday, August 10, 2012 5:16:07 AM UTC-5, Daniel Harman wrote:
> 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
> On 10 Aug 2012, at 01:12, nuk nuk <shlu...@gmail.com> wrote:
> > 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?
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.
On Friday, August 10, 2012 4:10:07 AM UTC-7, craiggwilson wrote:
> 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.
> On Friday, August 10, 2012 5:16:07 AM UTC-5, Daniel Harman wrote:
>> 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
>> On 10 Aug 2012, at 01:12, nuk nuk <shlu...@gmail.com> wrote:
>> > 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?
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 :).
On Friday, August 10, 2012 6:17:55 PM UTC-5, nuk nuk wrote:
> 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.
> On Friday, August 10, 2012 4:10:07 AM UTC-7, craiggwilson wrote:
>> 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.
>> On Friday, August 10, 2012 5:16:07 AM UTC-5, Daniel Harman wrote:
>>> 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
>>> On 10 Aug 2012, at 01:12, nuk nuk <shlu...@gmail.com> wrote:
>>> > 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?
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:
On Friday, August 10, 2012 6:24:27 PM UTC-7, craiggwilson wrote:
> 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 :).
> On Friday, August 10, 2012 6:17:55 PM UTC-5, nuk nuk wrote:
>> 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.
>> On Friday, August 10, 2012 4:10:07 AM UTC-7, craiggwilson wrote:
>>> 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.
>>> On Friday, August 10, 2012 5:16:07 AM UTC-5, Daniel Harman wrote:
>>>> 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
>>>> On 10 Aug 2012, at 01:12, nuk nuk <shlu...@gmail.com> wrote:
>>>> > 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?
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:
> 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 :).
> On Friday, August 10, 2012 6:17:55 PM UTC-5, nuk nuk wrote:
>> 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.
>> On Friday, August 10, 2012 4:10:07 AM UTC-7, craiggwilson wrote:
>>> 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.
>>> On Friday, August 10, 2012 5:16:07 AM UTC-5, Daniel Harman wrote:
>>>> 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
>>>> On 10 Aug 2012, at 01:12, nuk nuk <shlu...@gmail.com> wrote:
>>>> > 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?
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;
On Monday, August 13, 2012 8:58:17 AM UTC-7, mongotime wrote:
> 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:
> On Friday, August 10, 2012 6:24:27 PM UTC-7, craiggwilson wrote:
>> 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 :).
>> On Friday, August 10, 2012 6:17:55 PM UTC-5, nuk nuk wrote:
>>> 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.
>>> On Friday, August 10, 2012 4:10:07 AM UTC-7, craiggwilson wrote:
>>>> 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.
>>>> On Friday, August 10, 2012 5:16:07 AM UTC-5, Daniel Harman wrote:
>>>>> 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
>>>>> On 10 Aug 2012, at 01:12, nuk nuk <shlu...@gmail.com> wrote:
>>>>> > 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?
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/2...
Now I just use the framework stuff, or disassemble the libraries that Stephen Toub has put into tpl-df or rx!
Dan
On 13 Aug 2012, at 21:28, nuk nuk <shlum...@gmail.com> wrote:
> 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:
> 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 :).
> On Friday, August 10, 2012 6:17:55 PM UTC-5, nuk nuk wrote:
> 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.
> On Friday, August 10, 2012 4:10:07 AM UTC-7, craiggwilson wrote:
> 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.
> On Friday, August 10, 2012 5:16:07 AM UTC-5, Daniel Harman wrote:
> 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
> On 10 Aug 2012, at 01:12, nuk nuk <shlu...@gmail.com> wrote:
> > 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?
