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

Avoid race condition in map for TupleElementIndex #58500

Merged
merged 7 commits into from
Feb 11, 2022
Merged

Avoid race condition in map for TupleElementIndex #58500

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bb4050c
Remove tuple element index map
jcouv Dec 25, 2021
44387c7
Add test for index of underlying field in long tuple
jcouv Dec 26, 2021
c63cb79
Remove instrumentation
jcouv Dec 26, 2021
27d7a4b
Use CompareExchange
jcouv Jan 14, 2022
e5cac13
Revert "Use CompareExchange"
jcouv Jan 18, 2022
f6dd0d8
Merge remote-tracking branch 'dotnet/main' into tuple-map
jcouv Feb 2, 2022
30a00e9
Address feedback
jcouv Feb 2, 2022
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
31 changes: 26 additions & 5 deletions src/Compilers/CSharp/Portable/Symbols/FieldSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@

#nullable disable

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Runtime.InteropServices;
using Microsoft.CodeAnalysis.CSharp.Emit;
using Microsoft.CodeAnalysis.RuntimeMembers;
using Microsoft.CodeAnalysis.Symbols;
using Roslyn.Utilities;

Expand Down Expand Up @@ -464,15 +466,34 @@ public virtual int TupleElementIndex
get
{
// wrapped tuple fields already have this information and override this property
Debug.Assert(!(this is TupleElementFieldSymbol or TupleErrorFieldSymbol));
Debug.Assert(!(this is TupleElementFieldSymbol or TupleErrorFieldSymbol or Retargeting.RetargetingFieldSymbol));
if (!ContainingType.IsTupleType)
{
return -1;
}

var map = ContainingType.TupleFieldDefinitionsToIndexMap;
if (map is object && map.TryGetValue(this.OriginalDefinition, out int index))
if (!ContainingType.IsDefinition)
{
return index;
return this.OriginalDefinition.TupleElementIndex;
}

return -1;
var tupleElementPosition = NamedTypeSymbol.MatchesCanonicalTupleElementName(Name);
int arity = ContainingType.Arity;
if (tupleElementPosition <= 0 || tupleElementPosition > arity)
{
// ex: no "Item2" in 'ValueTuple<T1>'
return -1;
}
Debug.Assert(tupleElementPosition < NamedTypeSymbol.ValueTupleRestPosition);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please clarify why we can assume this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this assert be moved to immediately following the if (!ContainingType.IsDefinition) { ... } block?


WellKnownMember wellKnownMember = NamedTypeSymbol.GetTupleTypeMember(arity, tupleElementPosition);
MemberDescriptor descriptor = WellKnownMembers.GetDescriptor(wellKnownMember);
Symbol found = CSharpCompilation.GetRuntimeMember(ImmutableArray.Create<Symbol>(this), descriptor, CSharpCompilation.SpecialMembersSignatureComparer.Instance,
accessWithinOpt: null); // force lookup of public members only
Copy link
Member

@cston cston Jan 4, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Can we call comparer.MatchFieldSignature(this, descriptor.Signature) directly instead? #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

GetRuntimeMember checks a few other things (static-ness, accessibility). I'd rather stay close to logic we used in MakeSynthesizedTupleMembers.

Comment on lines +491 to +492
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
Symbol found = CSharpCompilation.GetRuntimeMember(ImmutableArray.Create<Symbol>(this), descriptor, CSharpCompilation.SpecialMembersSignatureComparer.Instance,
accessWithinOpt: null); // force lookup of public members only
Symbol found = CSharpCompilation.GetRuntimeMember(
ImmutableArray.Create<Symbol>(this),
descriptor,
CSharpCompilation.SpecialMembersSignatureComparer.Instance,
accessWithinOpt: null); // force lookup of public members only


return found is not null
? tupleElementPosition - 1
: -1;
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Dec 26, 2021 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

is any of this expensive? would it be worth caching the reuslt of this? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

Considered this question and I lean towards not caching.
When using tuple syntax, we create TupleElementFieldSymbol for default elements (and TupleVirtualElementFieldSymbol for named elements) which stores location and name. So this path is only used when accessing a field directly from ValueTuple type (from source or PE) and that'd be less common.
I've filed an issue for discussion which proposes to remove this wrapping with TupleElementFieldSymbol. If we go down that route then caching would make more sense.

Copy link
Member

Choose a reason for hiding this comment

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

cool, that makes sense to me :)

}
}

Expand Down
14 changes: 0 additions & 14 deletions src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingNamedTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,20 +135,6 @@ public override ImmutableArray<Symbol> GetMembers(string name)
return this.RetargetingTranslator.Retarget(_underlyingType.GetMembers(name));
}

public override void InitializeTupleFieldDefinitionsToIndexMap()
{
Debug.Assert(this.IsTupleType);
Debug.Assert(this.IsDefinition); // we only store a map for definitions

var retargetedMap = new SmallDictionary<FieldSymbol, int>(ReferenceEqualityComparer.Instance);
foreach ((FieldSymbol field, int index) in _underlyingType.TupleFieldDefinitionsToIndexMap)
{
retargetedMap.Add(this.RetargetingTranslator.Retarget(field), index);
}

this.TupleData!.SetFieldDefinitionsToIndexMap(retargetedMap);
}

internal override IEnumerable<FieldSymbol> GetFieldsToEmit()
{
foreach (FieldSymbol f in _underlyingType.GetFieldsToEmit())
Expand Down
96 changes: 15 additions & 81 deletions src/Compilers/CSharp/Portable/Symbols/Tuples/TupleTypeSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,7 +447,7 @@ internal static int IsTupleElementNameReserved(string name)
return 0;
}

return matchesCanonicalElementName(name);
return MatchesCanonicalTupleElementName(name);

static bool isElementNameForbidden(string name)
{
Expand All @@ -465,25 +465,25 @@ static bool isElementNameForbidden(string name)
return false;
}
}
}

// Returns 3 for "Item3".
// Returns -1 otherwise.
static int matchesCanonicalElementName(string name)
// Returns 3 for "Item3".
// Returns -1 otherwise.
internal static int MatchesCanonicalTupleElementName(string name)
{
if (name.StartsWith("Item", StringComparison.Ordinal))
{
if (name.StartsWith("Item", StringComparison.Ordinal))
string tail = name.Substring(4);
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Dec 26, 2021 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

Suggested change
string tail = name.Substring(4);
string tail = name.Substring("Item".Length);

(will be the exact same jitted code, but this helps make it clearer what's going on (imo)). #Resolved

Copy link
Member

@cston cston Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

will be the exact same jitted code

It looks like get_Length() is called (see sharplab.io).

if (int.TryParse(tail, out int number))
{
string tail = name.Substring(4);
if (int.TryParse(tail, out int number))
if (number > 0 && string.Equals(name, TupleMemberName(number), StringComparison.Ordinal))
{
if (number > 0 && string.Equals(name, TupleMemberName(number), StringComparison.Ordinal))
{
return number;
}
return number;
}
}

return -1;
}

return -1;
}

/// <summary>
Expand Down Expand Up @@ -567,36 +567,6 @@ public sealed override ImmutableArray<TypeWithAnnotations> TupleElementTypesWith
public sealed override ImmutableArray<FieldSymbol> TupleElements
=> IsTupleType ? TupleData!.TupleElements(this) : default;

/// <summary>
/// For tuple fields that aren't TupleElementFieldSymbol or TupleErrorFieldSymbol, we cache their tuple element index.
/// This supports <see cref="FieldSymbol.TupleElementIndex"/>.
/// For those fields, we map from their definition to an index.
/// </summary>
public SmallDictionary<FieldSymbol, int>? TupleFieldDefinitionsToIndexMap
{
get
{
if (!IsTupleType)
{
return null;
}

if (!IsDefinition)
{
return this.OriginalDefinition.TupleFieldDefinitionsToIndexMap;
}

return TupleData!.GetFieldDefinitionsToIndexMap(this);
}
}

public virtual void InitializeTupleFieldDefinitionsToIndexMap()
{
Debug.Assert(this.IsTupleType);
Debug.Assert(this.IsDefinition); // we only store a map for definitions
_ = this.GetMembers();
}

public TMember? GetTupleMemberSymbolForUnderlyingMember<TMember>(TMember? underlyingMemberOpt) where TMember : Symbol
{
return IsTupleType ? TupleData!.GetTupleMemberSymbolForUnderlyingMember(underlyingMemberOpt) : null;
Expand All @@ -611,10 +581,6 @@ protected ArrayBuilder<Symbol> MakeSynthesizedTupleMembers(ImmutableArray<Symbol
var elementsMatchedByFields = ArrayBuilder<bool>.GetInstance(elementTypes.Length, fillWithValue: false);
var members = ArrayBuilder<Symbol>.GetInstance(currentMembers.Length);

// For tuple fields that aren't TupleElementFieldSymbol or TupleErrorFieldSymbol, we cache/map their tuple element index
// corresponding to their definition. We only need to do that for the definition of ValueTuple types.
var fieldDefinitionsToIndexMap = IsDefinition ? new SmallDictionary<FieldSymbol, int>(ReferenceEqualityComparer.Instance) : null;

NamedTypeSymbol currentValueTuple = this;
int currentNestingLevel = 0;

Expand Down Expand Up @@ -692,7 +658,6 @@ protected ArrayBuilder<Symbol> MakeSynthesizedTupleMembers(ImmutableArray<Symbol
if (IsDefinition)
{
defaultTupleField = field;
fieldDefinitionsToIndexMap!.Add(field, tupleFieldIndex);
}
else
{
Expand Down Expand Up @@ -819,10 +784,6 @@ protected ArrayBuilder<Symbol> MakeSynthesizedTupleMembers(ImmutableArray<Symbol
}

elementsMatchedByFields.Free();
if (fieldDefinitionsToIndexMap is object)
{
this.TupleData!.SetFieldDefinitionsToIndexMap(fieldDefinitionsToIndexMap);
}
return members;

// Returns the nested type at a certain depth.
Expand All @@ -847,11 +808,11 @@ static void collectTargetTupleFields(int arity, ImmutableArray<Symbol> members,
for (int i = 0; i < fieldsPerType; i++)
{
WellKnownMember wellKnownTupleField = GetTupleTypeMember(arity, i + 1);
fieldsForElements.Add((FieldSymbol?)GetWellKnownMemberInType(members, wellKnownTupleField));
fieldsForElements.Add((FieldSymbol?)getWellKnownMemberInType(members, wellKnownTupleField));
}
}

static Symbol? GetWellKnownMemberInType(ImmutableArray<Symbol> members, WellKnownMember relativeMember)
static Symbol? getWellKnownMemberInType(ImmutableArray<Symbol> members, WellKnownMember relativeMember)
{
Debug.Assert(relativeMember >= WellKnownMember.System_ValueTuple_T1__Item1 && relativeMember <= WellKnownMember.System_ValueTuple_TRest__ctor);

Expand Down Expand Up @@ -954,13 +915,6 @@ internal sealed class TupleExtraData

private ImmutableArray<FieldSymbol> _lazyDefaultElementFields;

/// <summary>
/// For tuple fields that aren't TupleElementFieldSymbol or TupleErrorFieldSymbol, we cache their tuple element index.
/// This supports <see cref="FieldSymbol.TupleElementIndex"/>.
/// For those fields, we map from their definition to an index.
/// </summary>
private SmallDictionary<FieldSymbol, int>? _lazyFieldDefinitionsToIndexMap;
Copy link
Contributor

@AlekseyTs AlekseyTs Jan 4, 2022 •
edited by jcouv
Loading

Choose a reason for hiding this comment

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

_lazyFieldDefinitionsToIndexMap

Are we certain that nothing else in this class is prone to a similar race? Would it be simpler and more robust to ensure that members and tuple data are in sync by using an alternative approach? For example, by ensuring that they are stored together at once. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reviewed the other parts of TupleExtraData and didn't spot a similar race condition.
That said, there is a race condition left somewhere (issue #53943) which seems different to the one fixed here. I haven't identified the root cause yet.

For the suggestion to have MakeSynthesizedTupleMembers return the map and have the caller store it, we'd discussed this with Chuck among a few options before the holiday break (but we leaned towards removing the map). I'm open to the approach if Chuck is okay with it.
If we do go that route, do you have an initialization pattern for the caller? Feels like we'd need to spin-wait the threads that are waiting for the winning thread to save the map.


private SmallDictionary<Symbol, Symbol>? _lazyUnderlyingDefinitionToMemberMap;

/// <summary>
Expand Down Expand Up @@ -1093,26 +1047,6 @@ ImmutableArray<FieldSymbol> collectTupleElementFields(NamedTypeSymbol tuple)
}
}

internal SmallDictionary<FieldSymbol, int> GetFieldDefinitionsToIndexMap(NamedTypeSymbol tuple)
{
Debug.Assert(tuple.IsTupleType);
Debug.Assert(tuple.IsDefinition); // we only store a map for definitions
if (_lazyFieldDefinitionsToIndexMap is null)
{
tuple.InitializeTupleFieldDefinitionsToIndexMap();
}

Debug.Assert(_lazyFieldDefinitionsToIndexMap is object);
return _lazyFieldDefinitionsToIndexMap;
}

internal void SetFieldDefinitionsToIndexMap(SmallDictionary<FieldSymbol, int> map)
{
Debug.Assert(map.Keys.All(k => k.IsDefinition));
Debug.Assert(map.Values.All(v => v >= 0));
Interlocked.CompareExchange(ref _lazyFieldDefinitionsToIndexMap, map, null);
}

internal SmallDictionary<Symbol, Symbol> UnderlyingDefinitionToMemberMap
{
get
Expand Down
65 changes: 35 additions & 30 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28562,41 +28562,33 @@ static void verify(FieldSymbol field)
[Fact]
public void TupleWithElementNamedWithDefaultName()
{
// Instrumenting test as part of investigating flakiness: https://github.com/dotnet/roslyn/issues/52658
for (int i = 0; i < 1000; i++)
{
string source = @"
string source = @"
class C
{
(int Item1, int Item2) M() => throw null;
}
";
var comp = CreateCompilation(source);
var m = (MethodSymbol)comp.GetMember("C.M");
var tuple = m.ReturnType;
Assert.Equal("(System.Int32 Item1, System.Int32 Item2)", tuple.ToTestDisplayString());
Assert.IsType<ConstructedNamedTypeSymbol>(tuple);

var item1 = tuple.GetMember<TupleElementFieldSymbol>("Item1");
// Instrumenting test as part of investigating flakiness: https://github.com/dotnet/roslyn/issues/52658
RoslynDebug.AssertOrFailFast(0 == item1.TupleElementIndex);

var item1Underlying = item1.TupleUnderlyingField;
Assert.IsType<SubstitutedFieldSymbol>(item1Underlying);
Assert.Equal("System.Int32 (System.Int32 Item1, System.Int32 Item2).Item1", item1Underlying.ToTestDisplayString());
// Instrumenting test as part of investigating flakiness: https://github.com/dotnet/roslyn/issues/52658
RoslynDebug.AssertOrFailFast(0 == item1Underlying.TupleElementIndex);
Assert.Same(item1Underlying, item1Underlying.TupleUnderlyingField);

var item2 = tuple.GetMember<TupleElementFieldSymbol>("Item2");
// Instrumenting test as part of investigating flakiness: https://github.com/dotnet/roslyn/issues/52658
RoslynDebug.AssertOrFailFast(1 == item2.TupleElementIndex);
var item2Underlying = item2.TupleUnderlyingField;
Assert.IsType<SubstitutedFieldSymbol>(item2Underlying);
// Instrumenting test as part of investigating flakiness: https://github.com/dotnet/roslyn/issues/52658
RoslynDebug.AssertOrFailFast(1 == item2Underlying.TupleElementIndex);
Assert.Same(item2Underlying, item2Underlying.TupleUnderlyingField);
}
var comp = CreateCompilation(source);
var m = (MethodSymbol)comp.GetMember("C.M");
var tuple = m.ReturnType;
Assert.Equal("(System.Int32 Item1, System.Int32 Item2)", tuple.ToTestDisplayString());
Assert.IsType<ConstructedNamedTypeSymbol>(tuple);

var item1 = tuple.GetMember<TupleElementFieldSymbol>("Item1");
Assert.Equal(0, item1.TupleElementIndex);

var item1Underlying = item1.TupleUnderlyingField;
Assert.IsType<SubstitutedFieldSymbol>(item1Underlying);
Assert.Equal("System.Int32 (System.Int32 Item1, System.Int32 Item2).Item1", item1Underlying.ToTestDisplayString());
Assert.Equal(0, item1Underlying.TupleElementIndex);
Assert.Same(item1Underlying, item1Underlying.TupleUnderlyingField);

var item2 = tuple.GetMember<TupleElementFieldSymbol>("Item2");
Assert.Equal(1, item2.TupleElementIndex);
var item2Underlying = item2.TupleUnderlyingField;
Assert.IsType<SubstitutedFieldSymbol>(item2Underlying);
Assert.Equal(1, item2Underlying.TupleElementIndex);
Assert.Same(item2Underlying, item2Underlying.TupleUnderlyingField);
}

[Fact]
Expand All @@ -28615,6 +28607,19 @@ class C
tuple.ToTestDisplayString());
Assert.IsType<ConstructedNamedTypeSymbol>(tuple);

var item1 = tuple.GetMember<TupleElementFieldSymbol>("Item1");
Assert.Equal("System.Int32 (System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32).Item1",
item1.ToTestDisplayString());
Assert.Equal(0, item1.TupleElementIndex);

var item1Underlying = item1.TupleUnderlyingField;
Assert.Equal("System.Int32 (System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32).Item1",
item1Underlying.ToTestDisplayString());
Assert.IsType<SubstitutedFieldSymbol>(item1Underlying);
Assert.Equal(-1, item1Underlying.TupleElementIndex); // should be zero, tracked by https://github.com/dotnet/roslyn/issues/58499

Assert.Same(tuple, item1Underlying.ContainingType);

var item8 = tuple.GetMember<TupleElementFieldSymbol>("Item8");
Assert.Equal("System.Int32 (System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32, System.Int32).Item8",
item8.ToTestDisplayString());
Expand Down