remove facetconfig locks

[eladmarg]

No description provided.

[eladmarg]

failed because of environment problems

Error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.

[NightOwl888]

Thanks for the PR.

However as is, many of the methods are no longer atomic and may cause concurrency issues. Please change the declaration of fieldTypes from:

private readonly IDictionary<string, DimConfig> fieldTypes = new ConcurrentDictionary<string, DimConfig>();

to:

private readonly ConcurrentDictionary<string, DimConfig> fieldTypes = new ConcurrentDictionary<string, DimConfig>();

to expose the methods that make ConcurrentDictionary atomic and see the other comments about the changes to each of the methods.

[NightOwl888]

I got it wrong. Upon closer inspection of the AddOrUpdate documentation:

If you call AddOrUpdate simultaneously on different threads, addValueFactory may be called multiple times, but its key/value pair might not be added to the dictionary for every call.

For modifications and write operations to the dictionary, ConcurrentDictionary<TKey,TValue> uses fine-grained locking to ensure thread safety. (Read operations on the dictionary are performed in a lock-free manner.) However, the addValueFactory and updateValueFactory delegates are called outside the locks to avoid the problems that can arise from executing unknown code under a lock. Therefore, AddOrUpdate is not atomic with regards to all other operations on the ConcurrentDictionary<TKey,TValue> class.

So in other words, this PR is not the equivalent functionality of the locks that exist now, as this method does not make the entire block atomic like the lock does.

I am curious, what issues are you actually seeing with these locks?

[eladmarg]

I am curious, what issues are you actually seeing with these locks?

nothing special, just performance penalty on GetTopChildrens method

by the way, I would keep this in this implementation, its just changing the property configuration, hierarchical / multi dimension etc..

[eladmarg]

any updates with this?

[NightOwl888]

@eladmarg

Given the fact that these locks exist in Lucene and this is not a reasonable locking alternative, we should keep them in place here. I would suggest holding off on this until we investigate #417 further, as it is still up in the air whether we should just use Lazy<T> or build our own wrapper around LazyInitializer. Once we have that solution (or solutions) worked out, we can apply it to FacetConfig as well.

Your last commit, 0f18945 does contain some useful removal of casts except for 1 call to Math.Abs. On this line, this is a bit twiddle that is equivalent to Java's >>> operator (unsigned right shift). The cast was done to make it unsigned, however, there is a faster approach that doesn't require a cast (at least not with int) is to use TripleShift.

I removed more than 1000 casts last night and have passing tests, but I would like to run some benchmarks to see how removing them compares with the casts in place before submitting the PR. Benchmarking the TripleShift(this int number, int bits) method against the two casts shows almost a 20% improvement in performance, and during the tests there seems to be less locking contention.

[eladmarg]

cool, the expensive parts are the casts, if you have this on separate PR, this one can be closed.
waiting to see it in action

[eladmarg]

closed in #423