From 75004a93d90960026f3a116697f57ecb1b983939 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Tue, 14 May 2024 15:01:22 -0700 Subject: [PATCH 1/5] Remove unnecessary node walk --- .../Core/Formatting/Engine/TokenData.cs | 39 +++---------------- 1 file changed, 5 insertions(+), 34 deletions(-) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs index e50090dda0738..31b0114466792 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs @@ -4,6 +4,7 @@ using System; using System.Collections.Generic; +using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.Extensions; using Roslyn.Utilities; @@ -63,50 +64,20 @@ public int CompareTo(TokenData other) Contract.ThrowIfFalse(this.TokenStream == other.TokenStream); if (this.IndexInStream >= 0 && other.IndexInStream >= 0) - { return this.IndexInStream - other.IndexInStream; - } + + if (this.Token == other.Token) + return 0; var start = this.Token.SpanStart - other.Token.SpanStart; if (start != 0) - { return start; - } var end = this.Token.Span.End - other.Token.Span.End; if (end != 0) - { return end; - } - - // this is expansive check. but there is no other way to check. - var commonRoot = this.Token.GetCommonRoot(other.Token); - RoslynDebug.Assert(commonRoot != null); - - var tokens = commonRoot.DescendantTokens(); - - var index1 = Index(tokens, this.Token); - var index2 = Index(tokens, other.Token); - Contract.ThrowIfFalse(index1 >= 0 && index2 >= 0); - - return index1 - index2; - } - - private static int Index(IEnumerable tokens, SyntaxToken token) - { - var index = 0; - - foreach (var current in tokens) - { - if (current == token) - { - return index; - } - - index++; - } - return -1; + return 0; } public static bool operator <(TokenData left, TokenData right) From ae3ecd29c8de62c35be30884b3452ab3bfa9653b Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Tue, 14 May 2024 15:17:38 -0700 Subject: [PATCH 2/5] Avoid enumeration allocation in formatter --- .../Core/Formatting/Engine/TokenData.cs | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs index 31b0114466792..546e8a4f2ff42 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs @@ -77,7 +77,34 @@ public int CompareTo(TokenData other) if (end != 0) return end; - return 0; + // this is expansive check. but there is no other way to check. + var commonRoot = this.Token.GetCommonRoot(other.Token); + RoslynDebug.Assert(commonRoot != null); + + using var _ = ArrayBuilder.GetInstance(out var stack); + stack.Push(commonRoot); + + // Do a DFS walk to see which of the two tokens comes first. + while (stack.TryPop(out var current)) + { + if (current.IsNode) + { + // Push children in reverse order to ensure a DFS walk. + foreach (var child in current.AsNode()!.ChildNodesAndTokens().Reverse()) + stack.Push(child); + } + else if (current.IsToken) + { + var currentToken = current.AsToken(); + + if (currentToken == this.Token) + return -1; + else if (currentToken == other.Token) + return 1; + } + } + + throw ExceptionUtilities.Unreachable(); } public static bool operator <(TokenData left, TokenData right) From ab0e8fc0b878744cbdec7cfa36046f773b01c2e5 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Tue, 14 May 2024 15:19:21 -0700 Subject: [PATCH 3/5] Docs --- .../Compiler/Core/Formatting/Engine/TokenData.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs index 546e8a4f2ff42..f90001f59a56a 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs @@ -77,14 +77,15 @@ public int CompareTo(TokenData other) if (end != 0) return end; - // this is expansive check. but there is no other way to check. + // We have two different tokens, which are at the same location. This can happen with things like empty/missing + // tokens. In order to give a strict ordering, we need to walk up the tree to find the first common ancestor + // and see which token we hit first in that ancestor. var commonRoot = this.Token.GetCommonRoot(other.Token); RoslynDebug.Assert(commonRoot != null); using var _ = ArrayBuilder.GetInstance(out var stack); stack.Push(commonRoot); - // Do a DFS walk to see which of the two tokens comes first. while (stack.TryPop(out var current)) { if (current.IsNode) From 2934ad33073386fd95040022ade07529ec8f4911 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Tue, 14 May 2024 16:58:35 -0700 Subject: [PATCH 4/5] Clean up --- .../Core/Extensions/SyntaxNodeExtensions.cs | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/SyntaxNodeExtensions.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/SyntaxNodeExtensions.cs index 782139a28ab85..67fe1c29206da 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/SyntaxNodeExtensions.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Extensions/SyntaxNodeExtensions.cs @@ -178,12 +178,18 @@ public static SyntaxNode GetCommonRoot(this SyntaxNode node1, SyntaxNode node2) { Contract.ThrowIfTrue(node1.RawKind == 0 || node2.RawKind == 0); - // find common starting node from two nodes. - // as long as two nodes belong to same tree, there must be at least one common root (Ex, compilation unit) - var ancestors = node1.GetAncestorsOrThis(); - var set = new HashSet(node2.GetAncestorsOrThis()); + // find common starting node from two nodes. as long as two nodes belong to same tree, there must be at least + // one common root (Ex, compilation unit) + using var _ = PooledHashSet.GetInstance(out var set); + set.AddRange(node2.GetAncestorsOrThis()); - return ancestors.First(set.Contains); + foreach (var ancestor in node1.AncestorsAndSelf()) + { + if (set.Contains(ancestor)) + return ancestor; + } + + throw ExceptionUtilities.Unreachable(); } public static int Width(this SyntaxNode node) From ec7ab6c7021f84232c85a846b4880a43a0842970 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Tue, 14 May 2024 17:07:21 -0700 Subject: [PATCH 5/5] Apply algorithm --- .../Core/Formatting/Engine/TokenData.cs | 37 +++++++++---------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs index f90001f59a56a..1bc2f1c870430 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Formatting/Engine/TokenData.cs @@ -81,31 +81,30 @@ public int CompareTo(TokenData other) // tokens. In order to give a strict ordering, we need to walk up the tree to find the first common ancestor // and see which token we hit first in that ancestor. var commonRoot = this.Token.GetCommonRoot(other.Token); - RoslynDebug.Assert(commonRoot != null); + Contract.ThrowIfNull(commonRoot); - using var _ = ArrayBuilder.GetInstance(out var stack); - stack.Push(commonRoot); + // Now, figure out the ancestor of each token parented by the common root. + var thisTokenAncestor = GetAncestorUnderRoot(this.Token, commonRoot); + var otherTokenAncestor = GetAncestorUnderRoot(other.Token, commonRoot); - while (stack.TryPop(out var current)) + foreach (var child in commonRoot.ChildNodesAndTokens()) { - if (current.IsNode) - { - // Push children in reverse order to ensure a DFS walk. - foreach (var child in current.AsNode()!.ChildNodesAndTokens().Reverse()) - stack.Push(child); - } - else if (current.IsToken) - { - var currentToken = current.AsToken(); - - if (currentToken == this.Token) - return -1; - else if (currentToken == other.Token) - return 1; - } + if (child == thisTokenAncestor) + return -1; + else if (child == otherTokenAncestor) + return 1; } throw ExceptionUtilities.Unreachable(); + + static SyntaxNodeOrToken GetAncestorUnderRoot(SyntaxNodeOrToken start, SyntaxNode root) + { + var current = start; + while (current.Parent != root) + current = current.Parent; + + return current; + } } public static bool operator <(TokenData left, TokenData right)