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.

Sign up for GitHub

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

Jump to bottom

Replace Hashtables with generic ConcurrentDictionaries #218

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Replace Hashtables with generic ConcurrentDictionaries #218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 43 additions & 77 deletions Src/Microsoft.Dynamic/ComInterop/ComTypeDesc.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,19 @@
#if FEATURE_COM

using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Globalization;
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.
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

//Enumerating and writing is mutually exclusive so require locking.
private Hashtable _funcs;
private Hashtable _puts;
private Hashtable _putRefs;
private readonly string _typeName;
private readonly string _documentation;
private ComMethodDesc _getItem;
private ComMethodDesc _setItem;
private Dictionary<string, ComEventDesc> _events;
private static readonly Dictionary<string, ComEventDesc> _EmptyEventsDict = new Dictionary<string, ComEventDesc>();

internal ComTypeDesc(ITypeInfo typeInfo, ComType memberType, ComTypeLibDesc typeLibDesc) : base(memberType) {
Expand Down Expand Up @@ -51,122 +46,101 @@ internal static ComTypeDesc FromITypeInfo(ITypeInfo typeInfo, TYPEATTR typeAttr)

internal static ComTypeDesc CreateEmptyTypeDesc() {
ComTypeDesc typeDesc = new ComTypeDesc(null, ComType.Interface, null);
typeDesc._funcs = new Hashtable();
typeDesc._puts = new Hashtable();
typeDesc._putRefs = new Hashtable();
typeDesc._events = _EmptyEventsDict;
typeDesc.Funcs = new ConcurrentDictionary<string, ComMethodDesc>();
typeDesc.Puts = new ConcurrentDictionary<string, ComMethodDesc>();
typeDesc.PutRefs = new ConcurrentDictionary<string, ComMethodDesc>();
typeDesc.Events = _EmptyEventsDict;

return typeDesc;
}

internal static Dictionary<string, ComEventDesc> EmptyEvents {
get { return _EmptyEventsDict; }
}
internal static Dictionary<string, ComEventDesc> EmptyEvents => _EmptyEventsDict;

internal Hashtable Funcs {
get { return _funcs; }
set { _funcs = value; }
}
internal ConcurrentDictionary<string, ComMethodDesc> Funcs { get; set; }

internal Hashtable Puts {
get { return _puts; }
set { _puts = value; }
}
internal ConcurrentDictionary<string, ComMethodDesc> Puts { get; set; }

internal Hashtable PutRefs {
set { _putRefs = value; }
}
internal ConcurrentDictionary<string, ComMethodDesc> PutRefs { get; set; }

internal Dictionary<string, ComEventDesc> Events {
get { return _events; }
set { _events = value; }
}
internal Dictionary<string, ComEventDesc> Events { get; set; }

internal bool TryGetFunc(string name, out ComMethodDesc method) {
name = name.ToUpper(System.Globalization.CultureInfo.InvariantCulture);
if (_funcs.ContainsKey(name)) {
method = _funcs[name] as ComMethodDesc;
name = name.ToUpper(CultureInfo.InvariantCulture);
if (Funcs.TryGetValue(name, out method)) {
return true;
}
method = null;

return false;
}

internal void AddFunc(string name, ComMethodDesc method) {
name = name.ToUpper(System.Globalization.CultureInfo.InvariantCulture);
lock (_funcs) {
_funcs[name] = method;
}
name = name.ToUpper(CultureInfo.InvariantCulture);
Funcs[name] = method;
}

internal bool TryGetPut(string name, out ComMethodDesc method) {
name = name.ToUpper(System.Globalization.CultureInfo.InvariantCulture);
if (_puts.ContainsKey(name)) {
method = _puts[name] as ComMethodDesc;
name = name.ToUpper(CultureInfo.InvariantCulture);
if (Puts.TryGetValue(name, out method)) {
return true;
}
method = null;

return false;
}

internal void AddPut(string name, ComMethodDesc method) {
name = name.ToUpper(System.Globalization.CultureInfo.InvariantCulture);
lock (_puts) {
_puts[name] = method;
}
name = name.ToUpper(CultureInfo.InvariantCulture);
Puts[name] = method;
}

internal bool TryGetPutRef(string name, out ComMethodDesc method) {
name = name.ToUpper(System.Globalization.CultureInfo.InvariantCulture);
if (_putRefs.ContainsKey(name)) {
method = _putRefs[name] as ComMethodDesc;
name = name.ToUpper(CultureInfo.InvariantCulture);
if (PutRefs.TryGetValue(name, out method)) {
return true;
}
method = null;

return false;
}
internal void AddPutRef(string name, ComMethodDesc method) {
name = name.ToUpper(System.Globalization.CultureInfo.InvariantCulture);
lock (_putRefs) {
_putRefs[name] = method;
}
name = name.ToUpper(CultureInfo.InvariantCulture);
PutRefs[name] = method;

}

internal bool TryGetEvent(string name, out ComEventDesc @event) {
name = name.ToUpper(System.Globalization.CultureInfo.InvariantCulture);
return _events.TryGetValue(name, out @event);
name = name.ToUpper(CultureInfo.InvariantCulture);
return Events.TryGetValue(name, out @event);
}

internal string[] GetMemberNames(bool dataOnly) {
var names = new Dictionary<string, object>();

lock (_funcs) {
foreach (ComMethodDesc func in _funcs.Values) {
lock (Funcs) {
foreach (ComMethodDesc func in Funcs.Values) {
if (!dataOnly || func.IsDataMember) {
names.Add(func.Name, null);
}
}
}

if (!dataOnly) {
lock (_puts) {
foreach (ComMethodDesc func in _puts.Values) {
lock (Puts) {
foreach (ComMethodDesc func in Puts.Values) {
if (!names.ContainsKey(func.Name)) {
names.Add(func.Name, null);
}
}
}

lock (_putRefs) {
foreach (ComMethodDesc func in _putRefs.Values) {
lock (PutRefs) {
foreach (ComMethodDesc func in PutRefs.Values) {
if (!names.ContainsKey(func.Name)) {
names.Add(func.Name, null);
}
}
}

if (_events != null && _events.Count > 0) {
foreach (string name in _events.Keys) {
if (Events != null && Events.Count > 0) {
foreach (string name in Events.Keys) {
if (!names.ContainsKey(name)) {
names.Add(name, null);
}
Expand All @@ -180,30 +154,22 @@ internal static ComTypeDesc FromITypeInfo(ITypeInfo typeInfo, TYPEATTR typeAttr)
}

// this property is public - accessed by an AST
public string TypeName {
get { return _typeName; }
}
public string TypeName => _typeName;

internal string Documentation {
get { return _documentation; }
}
internal string Documentation => _documentation;

// this property is public - accessed by an AST
public ComTypeLibDesc TypeLib { get; }

internal Guid Guid { get; set; }

internal ComMethodDesc GetItem {
get { return _getItem; }
}
internal ComMethodDesc GetItem => _getItem;

internal void EnsureGetItem(ComMethodDesc candidate) {
Interlocked.CompareExchange(ref _getItem, candidate, null);
}

internal ComMethodDesc SetItem {
get { return _setItem; }
}
internal ComMethodDesc SetItem => _setItem;

internal void EnsureSetItem(ComMethodDesc candidate) {
Interlocked.CompareExchange(ref _setItem, candidate, null);
Expand Down
23 changes: 11 additions & 12 deletions Src/Microsoft.Dynamic/ComInterop/IDispatchComObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,18 @@
// See the LICENSE file in the project root for more information.

#if FEATURE_COM
using System.Linq.Expressions;

using System;
using System.Collections;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Diagnostics;
using System.Dynamic;
using System.Globalization;
using System.Linq.Expressions;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Security;
using System.Security.Permissions;

using ComTypes = System.Runtime.InteropServices.ComTypes;
using System.Dynamic;

namespace Microsoft.Scripting.ComInterop {

Expand Down Expand Up @@ -501,9 +500,9 @@ internal IDispatchComObject(IDispatch rcw)

ComMethodDesc getItem = null;
ComMethodDesc setItem = null;
Hashtable funcs = new Hashtable(typeAttr.cFuncs);
Hashtable puts = new Hashtable();
Hashtable putrefs = new Hashtable();
ConcurrentDictionary<string, ComMethodDesc> funcs = new ConcurrentDictionary<string, ComMethodDesc>(Environment.ProcessorCount, typeAttr.cFuncs);
ConcurrentDictionary<string, ComMethodDesc> puts = new ConcurrentDictionary<string, ComMethodDesc>();
ConcurrentDictionary<string, ComMethodDesc> putrefs = new ConcurrentDictionary<string, ComMethodDesc>();

for (int definedFuncIndex = 0; definedFuncIndex < typeAttr.cFuncs; definedFuncIndex++) {
IntPtr funcDescHandleToRelease = IntPtr.Zero;
Expand All @@ -520,7 +519,7 @@ internal IDispatchComObject(IDispatch rcw)
string name = method.Name.ToUpper(CultureInfo.InvariantCulture);

if ((funcDesc.invkind & ComTypes.INVOKEKIND.INVOKE_PROPERTYPUT) != 0) {
puts.Add(name, method);
puts.TryAdd(name, method);

// for the special dispId == 0, we need to store
// the method descriptor for the Do(SetItem) binder.
Expand All @@ -530,7 +529,7 @@ internal IDispatchComObject(IDispatch rcw)
continue;
}
if ((funcDesc.invkind & ComTypes.INVOKEKIND.INVOKE_PROPERTYPUTREF) != 0) {
putrefs.Add(name, method);
putrefs.TryAdd(name, method);
// for the special dispId == 0, we need to store
// the method descriptor for the Do(SetItem) binder.
if (method.DispId == ComDispIds.DISPID_VALUE && setItem == null) {
Expand All @@ -540,11 +539,11 @@ internal IDispatchComObject(IDispatch rcw)
}

if (funcDesc.memid == ComDispIds.DISPID_NEWENUM) {
funcs.Add("GETENUMERATOR", method);
funcs.TryAdd("GETENUMERATOR", method);
continue;
}

funcs.Add(name, method);
funcs.TryAdd(name, method);

// for the special dispId == 0, we need to store the method descriptor
// for the Do(GetItem) binder.
Expand Down