Grab 4 more bits for use as node flags. Use additional bit to store if nodes contain attributes within them. #72070
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| { | ||
| get => _nodeFlagsAndSlotCount.SlotCount; | ||
| set => _nodeFlagsAndSlotCount.SlotCount = value; | ||
| } |
There was a problem hiding this comment.
bridge members to ensure nothing else needs to be updated. It would be good to rename these in the future to actually be properties.
This comment was marked as outdated.
This comment was marked as outdated.
|
@jaredpar i'm doing some low level compiler spelunking though. is it possible i just broke things? |
I'm pretty certain you broke things 😄. I just happened to see the notification and wanted to save you a few seconds by posting the stack of the failure so you didn't have to. |
|
Ah good. I wanted to make sure you weren't investigating thinking this was a CI issue vs a CY issue. |
| } | ||
|
|
||
| return result; | ||
| return new AttributeSyntax(SyntaxKind.Attribute, name, argumentList, this.context); |
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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.
CyrusNajmabadi
changed the title
| @@ -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; | |||
There was a problem hiding this comment.
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.
|
@dotnet/roslyn-compiler @jaredpar this is ready for review. |
| @@ -244,12 +264,13 @@ internal enum NodeFlags : byte | |||
| ContainsSkippedText = 1 << 3, | |||
| ContainsAnnotations = 1 << 4, | |||
There was a problem hiding this comment.
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.
| @@ -581,6 +586,7 @@ private void WriteGreenFactory(Node nd, bool withSyntaxFactoryContext = false) | |||
| if (nd.Name != "SkippedTokensTriviaSyntax" && | |||
| nd.Name != "DocumentationCommentTriviaSyntax" && | |||
| nd.Name != "IncompleteMemberSyntax" && | |||
| nd.Name != "AttributeSyntax" && | |||
There was a problem hiding this comment.
💭 I wasn't sure why this case was needed. It seems like the flags will always be correct, whether or not this is cached.
There was a problem hiding this comment.
➡️ Answer was given in a different comment #72070 (comment)
There was a problem hiding this comment.
Note: i looked into potentially being able to cache thse nodes, and it quickly became quite complex. The core issue is that the caching logic really depends today on only caching things without flag bits on them (except for IsNotMissing bit). Trying to flow through that it's also ok to have other bits was not easy. And that tracking has to happen at the cached node level itself, as well as when combining the info from the child nodes being combined.
overall, it seems fien that we don't have perfect caching here for attribute nodes.
src/Compilers/Core/Portable/Syntax/GreenNode.NodeFlagsAndSlotCount.cs
Outdated
Show resolved
Hide resolved
| @@ -244,12 +266,13 @@ internal enum NodeFlags : byte | |||
| ContainsSkippedText = 1 << 3, | |||
| ContainsAnnotations = 1 << 4, | |||
| IsNotMissing = 1 << 5, | |||
| ContainsAttributes = 1 << 6, | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
ghost
added this to the 
Fixes #72066
Examples of where we'd like to use this:
value/fieldinto contextual keywords, likeawait).Critically, i did not want to increase the size of a node. So, the approach i took was to take the two current values (SlotCount and NodeFlags) which were each 1byte (so 16bits total), and transformed them into a combined 16bit value, where slotcount was only 4 bits, and NodeFlags 12 bits. As all normal nodes have a value of <=12 (a DeclareStatementSyntax in vb), that's enough space to store directly in the node. This freed up 4 bits for use for more flags.
As part of this PR i demonstrate then using that in teh syntax generator to fast determine which files have attributes in them, and to also fast descend only into the parts of hte trees with attributes.