Skip to content
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
6b6d090
in progres
CyrusNajmabadi Feb 12, 2024
b2e4644
Flip
CyrusNajmabadi Feb 12, 2024
a7b0f5c
Drop in replacement
CyrusNajmabadi Feb 12, 2024
5c031d0
Merge branch 'main' into flags
CyrusNajmabadi Feb 12, 2024
b90d0fb
Update src/Compilers/Core/Portable/Syntax/GreenNode.cs
CyrusNajmabadi Feb 12, 2024
1b452b3
Merge remote-tracking branch 'upstream/main' into flags
CyrusNajmabadi Feb 13, 2024
ebd279d
fix
CyrusNajmabadi Feb 13, 2024
9e91de5
Add specific list test
CyrusNajmabadi Feb 13, 2024
f8c0aba
Add attribute flag
CyrusNajmabadi Feb 13, 2024
8e54ffb
generators
CyrusNajmabadi Feb 13, 2024
f6087f9
Better
CyrusNajmabadi Feb 13, 2024
2e10fc8
Better
CyrusNajmabadi Feb 13, 2024
37ffbe8
Better
CyrusNajmabadi Feb 13, 2024
4fd426b
fix
CyrusNajmabadi Feb 13, 2024
7658b22
Update vb
CyrusNajmabadi Feb 13, 2024
71d1c60
fix
CyrusNajmabadi Feb 13, 2024
7fb9f8e
extract type
CyrusNajmabadi Feb 13, 2024
ccd891a
Update src/Compilers/Core/Portable/Syntax/GreenNode.cs
CyrusNajmabadi Feb 13, 2024
bef9b62
Move into loop
CyrusNajmabadi Feb 13, 2024
7890a62
Keep both checks
CyrusNajmabadi Feb 13, 2024
47e93e3
Merge branch 'flags' of https://github.com/CyrusNajmabadi/roslyn into…
CyrusNajmabadi Feb 13, 2024
5f23cb5
Simplify
CyrusNajmabadi Feb 13, 2024
8f2bc75
Simplify
CyrusNajmabadi Feb 13, 2024
afe9ad1
docs
CyrusNajmabadi Feb 13, 2024
4ca709a
Setflags
CyrusNajmabadi Feb 13, 2024
9ef94fc
update code
CyrusNajmabadi Feb 13, 2024
fa6c04d
Add special constant
CyrusNajmabadi Feb 15, 2024
abe46cb
Docs
CyrusNajmabadi Feb 15, 2024
e597ad1
Pull out constant
CyrusNajmabadi Feb 15, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -15864,6 +15864,7 @@ internal sealed partial class AttributeSyntax : CSharpSyntaxNode
internal AttributeSyntax(SyntaxKind kind, NameSyntax name, AttributeArgumentListSyntax? argumentList, DiagnosticInfo[]? diagnostics, SyntaxAnnotation[]? annotations)
: base(kind, diagnostics, annotations)
{
this.flags |= NodeFlags.ContainsAttributes;
this.SlotCount = 2;
this.AdjustFlagsAndWidth(name);
this.name = name;
Expand All @@ -15878,6 +15879,7 @@ internal AttributeSyntax(SyntaxKind kind, NameSyntax name, AttributeArgumentList
: base(kind)
{
this.SetFactoryContext(context);
this.flags |= NodeFlags.ContainsAttributes;
this.SlotCount = 2;
this.AdjustFlagsAndWidth(name);
this.name = name;
Expand All @@ -15891,6 +15893,7 @@ internal AttributeSyntax(SyntaxKind kind, NameSyntax name, AttributeArgumentList
internal AttributeSyntax(SyntaxKind kind, NameSyntax name, AttributeArgumentListSyntax? argumentList)
: base(kind)
{
this.flags |= NodeFlags.ContainsAttributes;
this.SlotCount = 2;
this.AdjustFlagsAndWidth(name);
this.name = name;
Expand Down Expand Up @@ -30780,17 +30783,7 @@ public AttributeSyntax Attribute(NameSyntax name, AttributeArgumentListSyntax? a
if (name == null) throw new ArgumentNullException(nameof(name));
#endif

int hash;
var cached = CSharpSyntaxNodeCache.TryGetNode((int)SyntaxKind.Attribute, name, argumentList, this.context, out hash);
if (cached != null) return (AttributeSyntax)cached;

var result = new AttributeSyntax(SyntaxKind.Attribute, name, argumentList, this.context);
if (hash >= 0)
{
SyntaxNodeCache.AddNode(result, hash);
}

return result;
return new AttributeSyntax(SyntaxKind.Attribute, name, argumentList, this.context);
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Feb 13, 2024

Choose a reason for hiding this comment

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

there's a test that cacheable nodes hve no inheritable nodeflags set at all. So if we want to maintain that invariant, attributes are no longer cacheable. The same will apply to vb.

}

public AttributeArgumentListSyntax AttributeArgumentList(SyntaxToken openParenToken, CoreSyntax.SeparatedSyntaxList<AttributeArgumentSyntax> arguments, SyntaxToken closeParenToken)
Expand Down Expand Up @@ -35996,17 +35989,7 @@ public static AttributeSyntax Attribute(NameSyntax name, AttributeArgumentListSy
if (name == null) throw new ArgumentNullException(nameof(name));
#endif

int hash;
var cached = SyntaxNodeCache.TryGetNode((int)SyntaxKind.Attribute, name, argumentList, out hash);
if (cached != null) return (AttributeSyntax)cached;

var result = new AttributeSyntax(SyntaxKind.Attribute, name, argumentList);
if (hash >= 0)
{
SyntaxNodeCache.AddNode(result, hash);
}

return result;
return new AttributeSyntax(SyntaxKind.Attribute, name, argumentList);
}

public static AttributeArgumentListSyntax AttributeArgumentList(SyntaxToken openParenToken, CoreSyntax.SeparatedSyntaxList<AttributeArgumentSyntax> arguments, SyntaxToken closeParenToken)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Threading;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.SourceGeneration;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp
Expand All @@ -21,9 +19,6 @@ private CSharpSyntaxHelper()
public override bool IsCaseSensitive
=> true;

protected override int AttributeListKind
=> (int)SyntaxKind.AttributeList;

public override bool IsValidIdentifier(string name)
=> SyntaxFacts.IsValidIdentifier(name);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Roslyn.Test.Utilities;
using Xunit;
Expand Down Expand Up @@ -17096,4 +17097,35 @@ public void TreatKeywordAsCollectionExprElement()
}
EOF();
}

[Theory, CombinatorialData]
public void CollectionExpressionParsingSlotCounts([CombinatorialRange(1, 20)] int count)
{
// Validate no errors for collections with small and large number of elements. Importantly, we want to test the
// boundary points where the slot count of the collection crosses over the amount that can be directly stored in
// the node, versus the slot count stored in subclass nodes.
var text = $"[{string.Join(", ", Enumerable.Range(1, count).Select(i => $"A{i}"))}]";

UsingExpression(text, TestOptions.Regular);

N(SyntaxKind.CollectionExpression);
N(SyntaxKind.OpenBracketToken);

for (var i = 1; i <= count; i++)
{
N(SyntaxKind.ExpressionElement);
N(SyntaxKind.IdentifierName);
{
N(SyntaxKind.IdentifierToken, $"A{i}");
}

if (i < count)
{
N(SyntaxKind.CommaToken);
}
}

N(SyntaxKind.CloseBracketToken);
EOF();
}
}
15 changes: 0 additions & 15 deletions src/Compilers/Core/Portable/SourceGeneration/ISyntaxHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,12 @@ internal interface ISyntaxHelper
void AddAliases(GreenNode node, ArrayBuilder<(string aliasName, string symbolName)> aliases, bool global);
void AddAliases(CompilationOptions options, ArrayBuilder<(string aliasName, string symbolName)> aliases);

bool ContainsAttributeList(SyntaxNode root);
bool ContainsGlobalAliases(SyntaxNode root);
}

internal abstract class AbstractSyntaxHelper : ISyntaxHelper
{
public abstract bool IsCaseSensitive { get; }
protected abstract int AttributeListKind { get; }

public abstract bool IsValidIdentifier(string name);

Expand All @@ -60,18 +58,5 @@ internal abstract class AbstractSyntaxHelper : ISyntaxHelper
public abstract void AddAliases(CompilationOptions options, ArrayBuilder<(string aliasName, string symbolName)> aliases);

public abstract bool ContainsGlobalAliases(SyntaxNode root);

public bool ContainsAttributeList(SyntaxNode root)
Copy link
Member Author

Choose a reason for hiding this comment

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

yaay, we can no avoid this costly walk. we originally made this a green walk because the red walk was insanely bad. but it's still high on CPU (40% costs in some traces). So this is really nice to avoid even looking at trees without attribuets. and, if a tree has attributes, we still don't need to walk into parts of hte tree that don't. For example, method bodies almost never have attributes in them. But we still had to walk in just in case. Now we almost never would have to.

{
var attributeListKind = this.AttributeListKind;

foreach (var node in root.Green.EnumerateNodes())
{
if (node.RawKind == attributeListKind)
return true;
}

return false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,10 @@ void processMember(SyntaxNode member)
{
cancellationToken.ThrowIfCancellationRequested();

// Don't bother descending into nodes that don't contain attributes.
if (!member.ContainsAttributes)
return;

// nodes can be arbitrarily deep. Use an explicit stack over recursion to prevent a stack-overflow.
var nodeStack = s_nodeStackPool.Allocate();
nodeStack.Push(member);
Expand All @@ -244,6 +248,10 @@ void processMember(SyntaxNode member)
{
var node = nodeStack.Pop();

// Don't bother descending into nodes that don't contain attributes.
if (!node.ContainsAttributes)
continue;

if (syntaxHelper.IsAttributeList(node))
{
foreach (var attribute in syntaxHelper.GetAttributesOfAttributeList(node))
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics;

namespace Microsoft.CodeAnalysis
{
internal abstract partial class GreenNode
{
/// <summary>
/// Combination of <see cref="NodeFlags"/> and <see cref="SlotCount"/> stored in a single 16bit value.
/// </summary>
private struct NodeFlagsAndSlotCount
{
/// <summary>
/// 4 bits for the SlotCount. This allows slot counts of 0-14 to be stored as a direct byte. All 1s
/// indicates that the slot count must be computed.
/// </summary>
private const ushort SlotCountMask = 0b1111000000000000;
private const ushort NodeFlagsMask = 0b0000111111111111;

private const int SlotCountShift = 12;
private const int MaxSlotCount = 15;

/// <summary>
/// 12 bits for the NodeFlags. This allows for up to 12 distinct bits to be stored to designate interesting
/// aspects of a node.
/// </summary>

/// <summary>
/// CCCCFFFFFFFFFFFF for Count bits then Flag bits.
/// </summary>
private ushort _data;

public byte SlotCount
{
readonly get
{
var shifted = _data >> SlotCountShift;
Debug.Assert(shifted <= MaxSlotCount);
var result = (byte)shifted;
return result == MaxSlotCount ? byte.MaxValue : result;
}

set
{
if (value >= MaxSlotCount)
value = MaxSlotCount;
Debug.Assert(value <= MaxSlotCount);

// Clear out everything but the node-flags, and then assign into the slot-count segment.
_data = (ushort)((_data & NodeFlagsMask) | (value << SlotCountShift));
}
}

public NodeFlags NodeFlags
{
readonly get
{
return (NodeFlags)(_data & NodeFlagsMask);
}

set
{
Debug.Assert((ushort)value <= NodeFlagsMask);

// Clear out everything but the slot-count, and then assign into the node-flags segment.
_data = (ushort)((_data & SlotCountMask) | (ushort)value);
}
}
}
}
}
37 changes: 31 additions & 6 deletions src/Compilers/Core/Portable/Syntax/GreenNode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,21 @@ private string GetDebuggerDisplay()
internal const int ListKind = 1;

private readonly ushort _kind;
protected NodeFlags flags;
private byte _slotCount;
private NodeFlagsAndSlotCount _nodeFlagsAndSlotCount;
private int _fullWidth;
Copy link
Member Author

Choose a reason for hiding this comment

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

if we wanted, we could likely grab more bits off of this as well. For example, we could say the max file size was 256MB, giving us 4 more bits here to store data.


protected NodeFlags flags
Copy link
Member Author

Choose a reason for hiding this comment

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

to keep the impact of this pr low, i kept the names of the fields and hid them behind properties. If we do want to change these names we can. Note that _slotCount is a challenge as we have both _slotCount and SlotCount already. I don't mind keeping these named as fields, as they are effectively still fields, jsut more efficiently stored.

{
get => _nodeFlagsAndSlotCount.NodeFlags;
set => _nodeFlagsAndSlotCount.NodeFlags = value;
}

private byte _slotCount
{
get => _nodeFlagsAndSlotCount.SlotCount;
set => _nodeFlagsAndSlotCount.SlotCount = 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.

bridge members to ensure nothing else needs to be updated. It would be good to rename these in the future to actually be properties.


private static readonly ConditionalWeakTable<GreenNode, DiagnosticInfo[]> s_diagnosticsTable =
new ConditionalWeakTable<GreenNode, DiagnosticInfo[]>();

Expand Down Expand Up @@ -234,8 +245,13 @@ public virtual int FindSlotIndexContainingOffset(int offset)
#endregion

#region Flags

/// <summary>
/// Special flags a node can have. Note: while this is typed as being `ushort`, we can only practically use 12
/// of those 16 bits as we use the remaining 4 bits to store the slot count of a node.
/// </summary>
[Flags]
internal enum NodeFlags : byte
internal enum NodeFlags : ushort
{
None = 0,
ContainsDiagnostics = 1 << 0,
Expand All @@ -244,12 +260,13 @@ internal enum NodeFlags : byte
ContainsSkippedText = 1 << 3,
ContainsAnnotations = 1 << 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

A non-inherited form of ContainsAnnotations would be an excellent candidate for another flag, since it would allow us to avoid a large number of costly operations in GetAnnotations for cases where we are searching for annotations in deeply nested nodes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sharwell Exploring that idea in: #72104

IsNotMissing = 1 << 5,
ContainsAttributes = 1 << 6,
Copy link
Member

Choose a reason for hiding this comment

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

I am assuming some mechanism already exists which is propagating this flag from the attribute up through its parents in the syntax tree? And that existing tests of ForAttributeWithMetadataName are helping us verify that this flag is present in the places we expect?

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 am assuming some mechanism already exists which is propagating this flag from the attribute up through its parents in the syntax tree?

Correct. That's the inheritmask below. flags that are in the 'inherit' category are automatically bubbled up nodes when they are created (see AdjustFlagsAndWidth which does that).

And that existing tests of ForAttributeWithMetadataName are helping us verify that this flag is present in the places we expect?

Correct.


FactoryContextIsInAsync = 1 << 6,
FactoryContextIsInQuery = 1 << 7,
FactoryContextIsInAsync = 1 << 7,
FactoryContextIsInQuery = 1 << 8,
FactoryContextIsInIterator = FactoryContextIsInQuery, // VB does not use "InQuery", but uses "InIterator" instead

InheritMask = ContainsDiagnostics | ContainsStructuredTrivia | ContainsDirectives | ContainsSkippedText | ContainsAnnotations | IsNotMissing,
InheritMask = ContainsDiagnostics | ContainsStructuredTrivia | ContainsDirectives | ContainsSkippedText | ContainsAnnotations | ContainsAttributes | IsNotMissing,
}

internal NodeFlags Flags
Expand Down Expand Up @@ -324,6 +341,14 @@ public bool ContainsDirectives
}
}

public bool ContainsAttributes
{
get
{
return (this.flags & NodeFlags.ContainsAttributes) != 0;
}
}

public bool ContainsDiagnostics
{
get
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/Core/Portable/Syntax/SyntaxNode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,8 @@ public bool ContainsDiagnostics
/// </summary>
public bool ContainsDirectives => this.Green.ContainsDirectives;

internal bool ContainsAttributes => this.Green.ContainsAttributes;

/// <summary>
/// Returns true if this node contains any directives (e.g. <c>#if</c>, <c>#nullable</c>, etc.) within it with a matching kind.
/// </summary>
Expand Down
2 changes: 1 addition & 1 deletion src/Compilers/Core/Portable/Syntax/SyntaxTree.cs
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ internal SourceGeneratorSyntaxTreeInfo GetSourceGeneratorInfo(
if (syntaxHelper.ContainsGlobalAliases(root))
result |= SourceGeneratorSyntaxTreeInfo.ContainsGlobalAliases;

if (syntaxHelper.ContainsAttributeList(root))
if (root.ContainsAttributes)
result |= SourceGeneratorSyntaxTreeInfo.ContainsAttributeList;

_sourceGeneratorInfo = result;
Expand Down
Loading