Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix concurrency errors with ConcurrentDictionary #394

Merged
merged 5 commits into from Jul 27, 2018
Merged

Fix concurrency errors with ConcurrentDictionary #394

merged 5 commits into from Jul 27, 2018

Conversation

zyzhu
Copy link
Contributor

@zyzhu zyzhu commented Jul 25, 2018

Attempt to fix issue 340 in pull 341. I simply replaced dictionary with concurrent dictionary. Ping @tpetricek for review.
#341

@kblohm
Copy link
Contributor

kblohm commented Jul 25, 2018

Hi,
you might want to have a look at ConcurrentDictionary.GetOrAdd(key, Func) .

@zyzhu
Copy link
Contributor Author

zyzhu commented Jul 25, 2018

@kblohm Thanks for the tip. Code is simplified a lot.
BTW, @dsyme and I initiated some discussions about general data science in f# in the following thread. Any inputs would be more than welcome.
https://github.com/fslaborg/FsLab/issues/137

@kblohm
Copy link
Contributor

kblohm commented Jul 25, 2018

@zyzhu
Since you are using the ConcurrentDictionary.GetOrAdd(TKey, TValue) overload and not ConcurrentDictionary.GetOrAdd(TKey, Func<TKey,TValue>), you are compiling your factory-lambda every single time you call the function, so your caching is not really doing anything now.
This should probably be something along the lines of:

let createTypedVector : _ -> seq<OptionalValue<obj>> -> _ =
    let cache = ConcurrentDictionary<_, _ -> _>()
    fun typ ->
        let valueFactory t =   
            let par = Expression.Parameter(typeof<seq<OptionalValue<obj>>>)
            let body = Expression.Call(createTypedVectorMi.MakeGenericMethod([| t |]), par)
            Expression.Lambda<Func<seq<OptionalValue<obj>>, IVector>>(body, par).Compile().Invoke      
        cache.GetOrAdd(typ, Func<_,_>(valueFactory))

Also, while having a quick look at the module, there is a getCachedCompileProjection aswell that is using a Dictionary to cache.

@zyzhu
Copy link
Contributor Author

zyzhu commented Jul 26, 2018

@kblohm Thanks for pointing it out. I added back trygetvalue so that cache shall work. I also changed getCachedCompileProjection

@kblohm
Copy link
Contributor

kblohm commented Jul 26, 2018

@zyzhu now you are not really solving the concurrency problem. Right now checking for the value and adding the value is not atomic, that is what GetOrAdd is for.

@zyzhu
Copy link
Contributor Author

zyzhu commented Jul 26, 2018

@kblohm Clearly I'm a newbie on concurrent dictionary. I learned about valuefactory today and fixed it along your lines.
A dumb question. Why the cache stays inside createTypedVector function? Isn't the cache re-initiated everytime a thread calls this function?

@zyzhu zyzhu requested a review from tpetricek July 27, 2018 01:23
cache.Add(typ, f.Invoke)
f.Invoke )
let cache = ConcurrentDictionary<_, seq<OptionalValue<obj>> -> _>()
let valueFactory typ =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this to:

   let valueFactory = 
       Func<_,_> (fun typ -> 
         let par = Expression.Parameter(typeof<seq<OptionalValue<obj>>>)
         let body = Expression.Call(createTypedVectorMi.MakeGenericMethod([| typ |]), par)
         Expression.Lambda<Func<seq<OptionalValue<obj>>, _>>(body, par).Compile().Invoke)
   fun typ ->
     cache.GetOrAdd(typ, valueFactory)    

So the Func<_,_> call to create the delegate is only executed once

[| for name, fldTy, proj in getMemberProjections typ ->
name, fldTy, proj.Compile() |]
fun typ ->
cache.GetOrAdd(typ, valueFactory)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar change requested here

Copy link
Member

@dsyme dsyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two changes requested, otherwise great

@zyzhu
Copy link
Contributor Author

zyzhu commented Jul 27, 2018

@dsyme Thanks for review. I've made above changes.

@zyzhu zyzhu merged commit 233fa34 into fslaborg:master Jul 27, 2018
@zyzhu zyzhu deleted the fix_issue_341 branch August 4, 2018 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants