Skip to content

Commit

Permalink
Address PR feedback on #74132 (#74193)
Browse files Browse the repository at this point in the history 
* Address PR feedback on #74132

* Spelling
  • Loading branch information
@333fred
333fred committed Jul 18, 2024
1 parent 21262e7 commit fa05fe4
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 68 deletions.
117 changes: 68 additions & 49 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,27 @@ public void SetIsOverloadResolutionPriorityPopulated()
}

/// <summary>
/// Holds infrequently accessed fields. See <seealso cref="_uncommonFields"/> for an explanation.
/// This type is used to hold lazily-initialized fields that many methods will not need. We avoid creating it unless one of the fields is needed;
/// unfortunately, this means that we need to be careful of data races. The general pattern that we use is to check for a flag in <see cref="_packedFlags"/>.
/// If the flag for that field is set, and there was a positive result (ie, there are indeed custom attributes, or there is obsolete data), then it
/// is safe to rely on the data in the field. If the flag for a field is set but the result is empty (ie, there is no obsolete data), then we can be in
/// one of 3 scenarios:
/// <list type="number">
/// <item><see cref="_uncommonFields"/> is itself null. In this case, no race has occurred, and the consuming code can safely handle the lack of
/// <see cref="_uncommonFields"/> however it chooses.</item>
/// <item><see cref="_uncommonFields"/> is not null, and the backing field has been initialized to some empty value, such as
/// <see cref="ImmutableArray{T}.Empty"/>. In this case, again, no race has occurred, and the consuming code can simply trust the empty value.</item>
/// <item><see cref="_uncommonFields"/> is not null, and the backing field is uninitialized, either being <see langword="default" />, or is some
/// kind of sentinel value. In this case, a data race has occurred, and the consuming code must initialize the field to empty to bring it back
/// into scenario 2.</item>
/// </list>
/// </summary>
/// <remarks>
/// The initialization pattern for this type <b>must</b> follow the following pattern to make the safety guarantees above:
/// If the field initialization code determines that the backing field needs to be set to some non-empty value, it <b>must</b> first call <see cref="AccessUncommonFields"/>,
/// set the backing field using an atomic operation, and then set the flag in <see cref="_packedFlags"/>. This ensures that the field is always set before the flag is set.
/// If this order is reversed, the consuming code may see the flag set, but the field not initialized, and incorrectly assume that there is no data.
/// </remarks>
private sealed class UncommonFields
{
public ParameterSymbol _lazyThisParameter;
Expand All @@ -298,64 +317,64 @@ private sealed class UncommonFields
public int _lazyOverloadResolutionPriority;
}

private UncommonFields CreateUncommonFields()
private UncommonFields AccessUncommonFields()
{
var retVal = new UncommonFields();
if (!_packedFlags.IsObsoleteAttributePopulated)
{
retVal._lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized;
}
var retVal = _uncommonFields;
return retVal ?? InterlockedOperations.Initialize(ref _uncommonFields, createUncommonFields());

if (!_packedFlags.IsUnmanagedCallersOnlyAttributePopulated)
UncommonFields createUncommonFields()
{
retVal._lazyUnmanagedCallersOnlyAttributeData = UnmanagedCallersOnlyAttributeData.Uninitialized;
}

//
// Do not set _lazyUseSiteDiagnostic !!!!
//
// "null" Indicates "no errors" or "unknown state",
// and we know which one of the states we have from IsUseSiteDiagnosticPopulated
//
// Setting _lazyUseSiteDiagnostic to a sentinel value here would introduce
// a number of extra states for various permutations of IsUseSiteDiagnosticPopulated, UncommonFields and _lazyUseSiteDiagnostic
// Some of them, in tight races, may lead to returning the sentinel as the diagnostics.
//
var retVal = new UncommonFields();
if (!_packedFlags.IsObsoleteAttributePopulated)
{
retVal._lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized;
}

if (_packedFlags.IsCustomAttributesPopulated)
{
retVal._lazyCustomAttributes = ImmutableArray<CSharpAttributeData>.Empty;
}
if (!_packedFlags.IsUnmanagedCallersOnlyAttributePopulated)
{
retVal._lazyUnmanagedCallersOnlyAttributeData = UnmanagedCallersOnlyAttributeData.Uninitialized;
}

if (_packedFlags.IsConditionalPopulated)
{
retVal._lazyConditionalAttributeSymbols = ImmutableArray<string>.Empty;
}
//
// Do not set _lazyUseSiteDiagnostic !!!!
//
// "null" Indicates "no errors" or "unknown state",
// and we know which one of the states we have from IsUseSiteDiagnosticPopulated
//
// Setting _lazyUseSiteDiagnostic to a sentinel value here would introduce
// a number of extra states for various permutations of IsUseSiteDiagnosticPopulated, UncommonFields and _lazyUseSiteDiagnostic
// Some of them, in tight races, may lead to returning the sentinel as the diagnostics.
//

if (_packedFlags.IsCustomAttributesPopulated)
{
retVal._lazyCustomAttributes = ImmutableArray<CSharpAttributeData>.Empty;
}

if (_packedFlags.IsOverriddenOrHiddenMembersPopulated)
{
retVal._lazyOverriddenOrHiddenMembersResult = OverriddenOrHiddenMembersResult.Empty;
}
if (_packedFlags.IsConditionalPopulated)
{
retVal._lazyConditionalAttributeSymbols = ImmutableArray<string>.Empty;
}

if (_packedFlags.IsMemberNotNullPopulated)
{
retVal._lazyNotNullMembers = ImmutableArray<string>.Empty;
retVal._lazyNotNullMembersWhenTrue = ImmutableArray<string>.Empty;
retVal._lazyNotNullMembersWhenFalse = ImmutableArray<string>.Empty;
}
if (_packedFlags.IsOverriddenOrHiddenMembersPopulated)
{
retVal._lazyOverriddenOrHiddenMembersResult = OverriddenOrHiddenMembersResult.Empty;
}

if (_packedFlags.IsExplicitOverrideIsPopulated)
{
retVal._lazyExplicitClassOverride = null;
}
if (_packedFlags.IsMemberNotNullPopulated)
{
retVal._lazyNotNullMembers = ImmutableArray<string>.Empty;
retVal._lazyNotNullMembersWhenTrue = ImmutableArray<string>.Empty;
retVal._lazyNotNullMembersWhenFalse = ImmutableArray<string>.Empty;
}

return retVal;
}
if (_packedFlags.IsExplicitOverrideIsPopulated)
{
retVal._lazyExplicitClassOverride = null;
}

private UncommonFields AccessUncommonFields()
{
var retVal = _uncommonFields;
return retVal ?? InterlockedOperations.Initialize(ref _uncommonFields, CreateUncommonFields());
return retVal;
}
}

private readonly MethodDefinitionHandle _handle;
Expand Down
60 changes: 41 additions & 19 deletions src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,28 @@ public void SetOverloadResolutionPriorityPopulated()
public readonly bool IsOverloadResolutionPriorityPopulated => (_bits & IsOverloadResolutionPriorityPopulatedBit) != 0;
}

/// <summary>
/// This type is used to hold lazily-initialized fields that many properties will not need. We avoid creating it unless one of the fields is needed;
/// unfortunately, this means that we need to be careful of data races. The general pattern that we use is to check for a flag in <see cref="_flags"/>.
/// If the flag for that field is set, and there was a positive result (ie, there are indeed custom attributes, or there is obsolete data), then it
/// is safe to rely on the data in the field. If the flag for a field is set but the result is empty (ie, there is no obsolete data), then we can be in
/// one of 3 scenarios:
/// <list type="number">
/// <item><see cref="_uncommonFields"/> is itself null. In this case, no race has occurred, and the consuming code can safely handle the lack of
/// <see cref="_uncommonFields"/> however it chooses.</item>
/// <item><see cref="_uncommonFields"/> is not null, and the backing field has been initialized to some empty value, such as
/// <see cref="ImmutableArray{T}.Empty"/>. In this case, again, no race has occurred, and the consuming code can simply trust the empty value.</item>
/// <item><see cref="_uncommonFields"/> is not null, and the backing field is uninitialized, either being <see langword="default" />, or is some
/// kind of sentinel value. In this case, a data race has occurred, and the consuming code must initialize the field to empty to bring it back
/// into scenario 2.</item>
/// </list>
/// </summary>
/// <remarks>
/// The initialization pattern for this type <b>must</b> follow the following pattern to make the safety guarantees above:
/// If the field initialization code determines that the backing field needs to be set to some non-empty value, it <b>must</b> first call <see cref="AccessUncommonFields"/>,
/// set the backing field using an atomic operation, and then set the flag in <see cref="_flags"/>. This ensures that the field is always set before the flag is set.
/// If this order is reversed, the consuming code may see the flag set, but the field not initialized, and incorrectly assume that there is no data.
/// </remarks>
private sealed class UncommonFields
{
public ImmutableArray<CSharpAttributeData> _lazyCustomAttributes;
Expand Down Expand Up @@ -322,31 +344,31 @@ static bool anyUnexpectedRequiredModifiers(ParamInfo<TypeSymbol>[] propertyParam
}
}

private UncommonFields CreateUncommonFields()
private UncommonFields AccessUncommonFields()
{
var retVal = new UncommonFields();
if (!_flags.IsObsoleteAttributePopulated)
{
retVal._lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized;
}
var retVal = _uncommonFields;
return retVal ?? InterlockedOperations.Initialize(ref _uncommonFields, createUncommonFields());

if (!_flags.IsUseSiteDiagnosticPopulated)
UncommonFields createUncommonFields()
{
retVal._lazyCachedUseSiteInfo = CachedUseSiteInfo<AssemblySymbol>.Uninitialized;
}
var retVal = new UncommonFields();
if (!_flags.IsObsoleteAttributePopulated)
{
retVal._lazyObsoleteAttributeData = ObsoleteAttributeData.Uninitialized;
}

if (_flags.IsCustomAttributesPopulated)
{
retVal._lazyCustomAttributes = ImmutableArray<CSharpAttributeData>.Empty;
}
if (!_flags.IsUseSiteDiagnosticPopulated)
{
retVal._lazyCachedUseSiteInfo = CachedUseSiteInfo<AssemblySymbol>.Uninitialized;
}

return retVal;
}
if (_flags.IsCustomAttributesPopulated)
{
retVal._lazyCustomAttributes = ImmutableArray<CSharpAttributeData>.Empty;
}

private UncommonFields AccessUncommonFields()
{
var retVal = _uncommonFields;
return retVal ?? InterlockedOperations.Initialize(ref _uncommonFields, CreateUncommonFields());
return retVal;
}
}

private bool MustCallMethodsDirectlyCore()
Expand Down

0 comments on commit fa05fe4

Please sign in to comment.