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

NRE #505

Closed
vasily-kirichenko opened this issue Jan 2, 2015 · 20 comments
Closed

NRE #505

vasily-kirichenko opened this issue Jan 2, 2015 · 20 comments

Comments

@vasily-kirichenko
Copy link
Contributor

image

@forki
Copy link
Member

forki commented Jan 2, 2015

Mhm could still be everywhere.
We could be brutal and open debugger?
On Jan 2, 2015 4:23 PM, "Vasily Kirichenko" [email protected]
wrote:

[image: image]
https://cloud.githubusercontent.com/assets/873919/5597033/7b5ad624-92ac-11e4-9ae2-105348f0ec76.png


Reply to this email directly or view it on GitHub
#505.

@forki
Copy link
Member

forki commented Jan 2, 2015

Do you want me to pu Debugger.Break into this line: https://github.com/fsprojects/Paket/blob/master/src/Paket/Program.fs#L213 ?

@vasily-kirichenko
Copy link
Contributor Author

Wait a minute, I'll try to reproduce it under debugger. Seem to appear after paket version updated.

@vasily-kirichenko
Copy link
Contributor Author

Cannot catch it :( Yes, add Debugger.Break please.

@vasily-kirichenko
Copy link
Contributor Author

image

@vasily-kirichenko
Copy link
Contributor Author

image

@vasily-kirichenko
Copy link
Contributor Author

Cannot understand what here is null tho :(

@vasily-kirichenko
Copy link
Contributor Author

image

@vasily-kirichenko
Copy link
Contributor Author

Maybe it's caused by concurrent access to pathPenalties dictionary (if it's concurrent)?

@vasily-kirichenko
Copy link
Contributor Author

It seems that 4 threads are invoking this code:

image

@forki
Copy link
Member

forki commented Jan 2, 2015

wow didn't know that Dictionary is not thread-safe.
http://msdn.microsoft.com/en-us/library/dd287191%28v=vs.110%29.aspx seems to be the one I should use, right?

@vasily-kirichenko
Copy link
Contributor Author

:) Absolutely!

@forki
Copy link
Member

forki commented Jan 2, 2015

WTF. Why isn't the thread-safe version the default?
Ok I changed it.

@isaacabraham
Copy link
Contributor

Most of the collections in the BCL aren't thread safe.

@vasily-kirichenko
Copy link
Contributor Author

All concurrent collection are much slower, for example, here http://stackoverflow.com/questions/15252115/concurrentdictionary-performance-at-a-single-thread-misunderstanding ConcurrentDictionary is 3x slower than Dictionary.

@forki
Copy link
Member

forki commented Jan 2, 2015

Yep I understan that - so I think in special case one could switch to UnsafeButFasterDictionary ;-)

Do you think we can close this? Or do you still see this error

@vasily-kirichenko
Copy link
Contributor Author

I don't see it anymore. If it returns, I reopen this issue.

@haf
Copy link
Member

haf commented Jan 2, 2015

Just beware that ConcurrentDictionary has no notion of strict serialisability as:

  • there's no global time
  • you may perform double work when adding to the cache
  • you cannot be guaranteed to read-your-writes between threads

Also note that ConcurrentDictionary is likely faster for your use-case of a cache, since your reads will outnumber your writes.

@forki
Copy link
Member

forki commented Jan 2, 2015

So all is well?

@haf
Copy link
Member

haf commented Jan 2, 2015

Yes ;)

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

No branches or pull requests

4 participants