-
Notifications
You must be signed in to change notification settings - Fork 87
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
Replace Hashtables with generic ConcurrentDictionaries #218
base: main
Are you sure you want to change the base?
Conversation
using System.Runtime.InteropServices.ComTypes; | ||
using System.Threading; | ||
|
||
namespace Microsoft.Scripting.ComInterop { | ||
|
||
public class ComTypeDesc : ComTypeLibMemberDesc { | ||
private string _typeName; | ||
private string _documentation; | ||
//Hashtable is threadsafe for multiple readers single writer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not looked at usage. But I assume this comment is here because it's intended to be used in multithreaded scenarios? Dictionary doesn't provide the same thread safety guarantees as Hashtable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used ConcurrentDictionaries instead. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ConcurrentDictionary seems fine from a thread safety point of view. Hopefully it isn't a perf hit. Just need to find time to look at how these classes are used and will merge it in assuming there are no issues.
d848e2a
to
758fcdb
Compare
This should probably be closed 🙂 |
I intended to do some performance measurements first to reach the right decision, merge or not. |
No description provided.