-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add a general-purpose property bag to Compilation. #5708
Changes from all commits
061f21b
d61d5f5
123ee31
4622f67
1750a66
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,66 @@ | ||
using System; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copyright? |
||
|
||
namespace Microsoft.CodeAnalysis.Collections | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should CompilationContext be in the Collection namespace? Its name doesn't quite sound as a collection. |
||
{ | ||
/// <summary> | ||
/// A mutable typesafe dictionary from a pair of compilation and key (of type <see cref="CompilationContext.Key{T}"/>) | ||
/// to a value (of type T). All keys and values associated with a compilation are reachable for GC purposes as a | ||
/// consequence of this API for only as long as the compilation is reachable. | ||
/// </summary> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding information about thread-safety. |
||
public class CompilationContext | ||
{ | ||
private static CompilationContext _instance = new CompilationContext(); | ||
public static CompilationContext Instance => _instance; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason to have an instance of this type at all? Should the type be static? |
||
private CompilationContext() { } | ||
|
||
public sealed class Key<T> { } | ||
|
||
public bool TryGetValue<T>(Compilation compilation, Key<T> key, out T value) | ||
{ | ||
if (compilation == null) throw new ArgumentNullException(nameof(compilation)); | ||
if (key == null) throw new ArgumentNullException(nameof(key)); | ||
object tempValue; | ||
if (compilation._compilationContext.TryGetValue(key, out tempValue)) | ||
{ | ||
value = (T)tempValue; | ||
return true; | ||
} | ||
else | ||
{ | ||
value = default(T); | ||
return false; | ||
} | ||
} | ||
|
||
public bool TryAdd<T>(Compilation compilation, Key<T> key, T value) | ||
{ | ||
if (compilation == null) throw new ArgumentNullException(nameof(compilation)); | ||
if (key == null) throw new ArgumentNullException(nameof(key)); | ||
return compilation._compilationContext.TryAdd(key, value); | ||
} | ||
|
||
public bool TryRemove<T>(Compilation compilation, Key<T> key, out T value) | ||
{ | ||
if (compilation == null) throw new ArgumentNullException(nameof(compilation)); | ||
if (key == null) throw new ArgumentNullException(nameof(key)); | ||
object tempValue; | ||
if (compilation._compilationContext.TryRemove(key, out tempValue)) | ||
{ | ||
value = (T)tempValue; | ||
return true; | ||
} | ||
else | ||
{ | ||
value = default(T); | ||
return false; | ||
} | ||
} | ||
|
||
public T GetOrAdd<T>(Compilation compilation, Key<T> key, T value) | ||
{ | ||
if (compilation == null) throw new ArgumentNullException(nameof(compilation)); | ||
if (key == null) throw new ArgumentNullException(nameof(key)); | ||
return (T)compilation._compilationContext.GetOrAdd(key, value); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like there should be a way to change a value for a particular key. |
||
} |
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.
My original comment is gone now, but I'll re-iterate: Make this a lazily allocated dictionary. An empty ConcurrentDictionary carries quite a bit of overhead.
The canonical pattern is:
Another way to mitigate the allocation hit for an empty ConcurrentDictionary is to specify a smaller concurrency level (the default is 4 times the processor count) and initial capacity (default is 31).