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

Cache the hash in compilation options #62289

Merged
merged 9 commits into from
Jul 1, 2022
6 changes: 4 additions & 2 deletions src/Compilers/CSharp/Portable/CSharpCompilationOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ namespace Microsoft.CodeAnalysis.CSharp
/// whether to emit an executable or a library, whether to optimize
/// generated code, and so on.
/// </summary>
#pragma warning disable CS0659 // Type overrides Object.Equals(object o) but does not override Object.GetHashCode().
public sealed class CSharpCompilationOptions : CompilationOptions, IEquatable<CSharpCompilationOptions>
#pragma warning restore CS0659 // Type overrides Object.Equals(object o) but does not override Object.GetHashCode().
{
/// <summary>
/// Allow unsafe regions (i.e. unsafe modifiers on members and unsafe blocks).
Expand Down Expand Up @@ -748,9 +750,9 @@ public override bool Equals(object? obj)
return this.Equals(obj as CSharpCompilationOptions);
}

public override int GetHashCode()
protected override int ComputeHashCode()
{
return Hash.Combine(base.GetHashCodeHelper(),
return Hash.Combine(GetHashCodeHelper(),
Hash.Combine(this.AllowUnsafe,
Hash.Combine(Hash.CombineValues(this.Usings, StringComparer.Ordinal),
Hash.Combine(TopLevelBinderFlags.GetHashCode(), this.NullableContextOptions.GetHashCode()))));
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
*REMOVED*override Microsoft.CodeAnalysis.CSharp.CSharpCompilationOptions.GetHashCode() -> int
Copy link
Member Author

Choose a reason for hiding this comment

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

this is safe. it's jsut a removal of hte override.

Microsoft.CodeAnalysis.CSharp.Conversion.ConstrainedToType.get -> Microsoft.CodeAnalysis.ITypeSymbol?
Microsoft.CodeAnalysis.CSharp.SyntaxKind.InterpolatedMultiLineRawStringStartToken = 9073 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
Microsoft.CodeAnalysis.CSharp.SyntaxKind.InterpolatedRawStringEndToken = 9074 -> Microsoft.CodeAnalysis.CSharp.SyntaxKind
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ public void ConstructorValidation()
}

/// <summary>
/// If this test fails, please update the <see cref="CSharpCompilationOptions.GetHashCode"/>
/// If this test fails, please update the <see cref="CompilationOptions.GetHashCode"/>
/// and <see cref="CSharpCompilationOptions.Equals(CSharpCompilationOptions)"/> methods to
/// make sure they are doing the right thing with your new field and then update the baseline
/// here.
Expand Down
15 changes: 14 additions & 1 deletion src/Compilers/Core/Portable/Compilation/CompilationOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,8 @@ protected set

private readonly Lazy<ImmutableArray<Diagnostic>> _lazyErrors;

private int _hashCode;
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason you're using 0 as the sentinel, not making this int?? Just easier for multithreading purposes?

Copy link
Member Author

Choose a reason for hiding this comment

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

force of habit. it's just a pattern i got very accustomed to for some reason over my years :) happy to change to nullable if you'd like :)

Copy link
Member Author

Choose a reason for hiding this comment

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

it's also likely that i trust ints more (with nullable, not sure what the multithreading concerns may be) :)

Copy link
Member

Choose a reason for hiding this comment

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

It's fine to leave is as, was more for understanding why this approach was taken.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think an int won't tear but an int? might.

If we're concerned about multithreading, is there a need for some kind of memory barrier when we write to _hashCode?

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 believe that there's anything we need to be concerned about with int. Reads and writes are atomic, and since the calculation is stable, the worst that could happen is potentially multiple writes of the same value.

Copy link
Member Author

Choose a reason for hiding this comment

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

yup. i like int because it's just so safe.


// Expects correct arguments.
internal CompilationOptions(
OutputKind outputKind,
Expand Down Expand Up @@ -651,7 +653,18 @@ protected bool EqualsHelper([NotNullWhen(true)] CompilationOptions? other)
return equal;
}

public abstract override int GetHashCode();
public sealed override int GetHashCode()
{
if (_hashCode == 0)
{
var hashCode = ComputeHashCode();
_hashCode = hashCode == 0 ? 1 : hashCode;
}

return _hashCode;
}

protected abstract int ComputeHashCode();

protected int GetHashCodeHelper()
{
Expand Down
44 changes: 44 additions & 0 deletions src/Compilers/Core/Portable/InternalUtilities/Hash.cs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,25 @@ internal static int CombineValues<T>(IEnumerable<T>? values, int maxItemsToHash
return hashCode;
}

internal static int CombineValues<TKey, TValue>(ImmutableDictionary<TKey, TValue> values, int maxItemsToHash = int.MaxValue)
Copy link
Member Author

Choose a reason for hiding this comment

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

these helpers ensure that when we do compute the hashcode we dont' incur allocations going through the CombineValues<T>(IEnumerable<T>) path that existing calls are going through.

Copy link
Member Author

Choose a reason for hiding this comment

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

they are copied from teh IEnumerable version, just tweaked accordingly. we really need shapes :)

where TKey : notnull
{
if (values == null)
return 0;

var hashCode = 0;
var count = 0;
foreach (var value in values)
{
if (count++ >= maxItemsToHash)
break;

hashCode = Hash.Combine(value.GetHashCode(), hashCode);
}

return hashCode;
}

internal static int CombineValues<T>(T[]? values, int maxItemsToHash = int.MaxValue)
{
if (values == null)
Expand Down Expand Up @@ -143,6 +162,31 @@ internal static int CombineValues(IEnumerable<string?>? values, StringComparer s
return hashCode;
}

internal static int CombineValues(ImmutableArray<string> values, StringComparer stringComparer, int maxItemsToHash = int.MaxValue)
{
if (values == null)
{
return 0;
}

var hashCode = 0;
var count = 0;
foreach (var value in values)
{
if (count++ >= maxItemsToHash)
{
break;
}

if (value != null)
{
hashCode = Hash.Combine(stringComparer.GetHashCode(value), hashCode);
}
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
}

return hashCode;
}

/// <summary>
/// The offset bias value used in the FNV-1a algorithm
/// See http://en.wikipedia.org/wiki/Fowler%E2%80%93Noll%E2%80%93Vo_hash_function
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/Core/Portable/PublicAPI.Unshipped.txt
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ Microsoft.CodeAnalysis.ISymbol.Accept<TArgument, TResult>(Microsoft.CodeAnalysis
Microsoft.CodeAnalysis.SymbolVisitor<TArgument, TResult>
Microsoft.CodeAnalysis.SymbolVisitor<TArgument, TResult>.SymbolVisitor() -> void
Microsoft.CodeAnalysis.SyntaxValueProvider.ForAttributeWithMetadataName<T>(string! fullyQualifiedMetadataName, System.Func<Microsoft.CodeAnalysis.SyntaxNode!, System.Threading.CancellationToken, bool>! predicate, System.Func<Microsoft.CodeAnalysis.GeneratorAttributeSyntaxContext, System.Threading.CancellationToken, T>! transform) -> Microsoft.CodeAnalysis.IncrementalValuesProvider<T>
override sealed Microsoft.CodeAnalysis.CompilationOptions.GetHashCode() -> int
override sealed Microsoft.CodeAnalysis.Diagnostic.Equals(object? obj) -> bool
*REMOVED*static Microsoft.CodeAnalysis.SyntaxNodeExtensions.ReplaceSyntax<TRoot>(this TRoot! root, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxNode!>! nodes, System.Func<Microsoft.CodeAnalysis.SyntaxNode!, Microsoft.CodeAnalysis.SyntaxNode!, Microsoft.CodeAnalysis.SyntaxNode!>! computeReplacementNode, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxToken>! tokens, System.Func<Microsoft.CodeAnalysis.SyntaxToken, Microsoft.CodeAnalysis.SyntaxToken, Microsoft.CodeAnalysis.SyntaxToken>! computeReplacementToken, System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.SyntaxTrivia>! trivia, System.Func<Microsoft.CodeAnalysis.SyntaxTrivia, Microsoft.CodeAnalysis.SyntaxTrivia, Microsoft.CodeAnalysis.SyntaxTrivia>! computeReplacementTrivia) -> TRoot!
static Microsoft.CodeAnalysis.Emit.EditAndContinueMethodDebugInformation.Create(System.Collections.Immutable.ImmutableArray<byte> compressedSlotMap, System.Collections.Immutable.ImmutableArray<byte> compressedLambdaMap, System.Collections.Immutable.ImmutableArray<byte> compressedStateMachineStateMap) -> Microsoft.CodeAnalysis.Emit.EditAndContinueMethodDebugInformation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1117,8 +1117,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
''' Creates a hashcode for this instance.
''' </summary>
''' <returns>A hashcode representing this instance.</returns>
Public Overrides Function GetHashCode() As Integer
Return Hash.Combine(MyBase.GetHashCodeHelper(),
Protected Overrides Function ComputeHashCode() As Integer
Return Hash.Combine(GetHashCodeHelper(),
Hash.Combine(Hash.CombineValues(Me.GlobalImports),
Hash.Combine(If(Me.RootNamespace IsNot Nothing, StringComparer.Ordinal.GetHashCode(Me.RootNamespace), 0),
Hash.Combine(Me.OptionStrict,
Expand Down