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

Use ConcurrentDictionary in CachingScriptMetadataResolver #1627

Merged
merged 5 commits into from
Oct 10, 2019
Merged

Use ConcurrentDictionary in CachingScriptMetadataResolver #1627

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1fadc6e
Use ConcurrentDictionary in CachingScriptMetadataResolver
seesharper Oct 4, 2019
f0a5013
Use ConcurrentDictionary with original implementation
seesharper Oct 5, 2019
4e9033b
Merge branch 'master' into bugfix/cached-metadata-resolver
seesharper Oct 7, 2019
e099eb9
Merge branch 'master' into bugfix/cached-metadata-resolver
filipw Oct 8, 2019
eb359ca
Merge branch 'master' into bugfix/cached-metadata-resolver
david-driscoll Oct 10, 2019
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
32 changes: 5 additions & 27 deletions src/OmniSharp.Script/CachingScriptMetadataResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System.Collections.Generic;
using System.Collections.Concurrent;
using System.Collections.Immutable;
using Microsoft.CodeAnalysis;

Expand All @@ -7,8 +7,8 @@ namespace OmniSharp.Script
public class CachingScriptMetadataResolver : MetadataReferenceResolver
{
private readonly MetadataReferenceResolver _defaultReferenceResolver;
private static Dictionary<string, ImmutableArray<PortableExecutableReference>> DirectReferenceCache = new Dictionary<string, ImmutableArray<PortableExecutableReference>>();
private static Dictionary<string, PortableExecutableReference> MissingReferenceCache = new Dictionary<string, PortableExecutableReference>();
private static ConcurrentDictionary<string, ImmutableArray<PortableExecutableReference>> DirectReferenceCache = new ConcurrentDictionary<string, ImmutableArray<PortableExecutableReference>>();
private static ConcurrentDictionary<string, PortableExecutableReference> MissingReferenceCache = new ConcurrentDictionary<string, PortableExecutableReference>();

public CachingScriptMetadataResolver(MetadataReferenceResolver defaultReferenceResolver)
{
Expand All @@ -29,35 +29,13 @@ public override int GetHashCode()

public override PortableExecutableReference ResolveMissingAssembly(MetadataReference definition, AssemblyIdentity referenceIdentity)
{
if (MissingReferenceCache.ContainsKey(referenceIdentity.Name))
{
return MissingReferenceCache[referenceIdentity.Name];
}

var result = _defaultReferenceResolver.ResolveMissingAssembly(definition, referenceIdentity);
if (result != null)
{
MissingReferenceCache[referenceIdentity.Name] = result;
}

return result;
return MissingReferenceCache.GetOrAdd(referenceIdentity.Name, _ => _defaultReferenceResolver.ResolveMissingAssembly(definition, referenceIdentity));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's the same thing as before. the old code would only cache non-nulls, whereas the new code would cache them.
In the second case it wouldn't cache empty collections too

Copy link
Contributor Author

@seesharper seesharper Oct 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, that is true, But does it matter? If the _defaultReferenceResolver returns null, it would be null for any subsequent requests too? So we save a call into the _defaultReferenceResolver. Unless that could change between request. If it actually is important we can replace this with TryAdd instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is it any other reason for this conditional caching? 😀

}

public override ImmutableArray<PortableExecutableReference> ResolveReference(string reference, string baseFilePath, MetadataReferenceProperties properties)
{
var key = $"{reference}-{baseFilePath}";
if (DirectReferenceCache.ContainsKey(key))
{
return DirectReferenceCache[key];
}

var result = _defaultReferenceResolver.ResolveReference(reference, baseFilePath, properties);
if (result.Length > 0)
{
DirectReferenceCache[key] = result;
}

return result;
return DirectReferenceCache.GetOrAdd(key, _ => _defaultReferenceResolver.ResolveReference(reference, baseFilePath, properties));
}
}
}