-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
Use next sibling in SyntaxNode.GetChildPosition() if available #66876
Changes from 12 commits
6d82cde
86786a2
4bea78e
050f570
14c639e
95880f0
8f18f02
9e8e9ce
a98cb9a
bff2282
528a2a8
80f0ab8
724aecd
c187e3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,13 +2,11 @@ | |
// 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; | ||
|
||
namespace Microsoft.CodeAnalysis.Syntax | ||
{ | ||
internal partial class SyntaxList | ||
{ | ||
internal class SeparatedWithManyChildren : SyntaxList | ||
internal sealed class SeparatedWithManyChildren : SyntaxList | ||
{ | ||
private readonly ArrayElement<SyntaxNode?>[] _children; | ||
|
||
|
@@ -39,6 +37,25 @@ internal SeparatedWithManyChildren(InternalSyntax.SyntaxList green, SyntaxNode? | |
|
||
return _children[i >> 1].Value; | ||
} | ||
|
||
internal override int GetChildPosition(int index) | ||
{ | ||
// If the previous sibling (ignoring separator) is not cached, but the next sibling | ||
// (ignoring separator) is cached, use the next sibling to determine position. | ||
int valueIndex = (index & 1) != 0 ? index - 1 : index; | ||
// The check for valueIndex >= Green.SlotCount - 2 ignores the last item because the last item | ||
// is a separator and separators are not cached. In those cases, when the index represents | ||
// the last or next to last item, we still want to calculate the position from the end of | ||
// the list rather than the start. | ||
if (valueIndex > 1 | ||
&& GetCachedSlot(valueIndex - 2) is null | ||
&& (valueIndex >= Green.SlotCount - 2 || GetCachedSlot(valueIndex + 2) is { })) | ||
{ | ||
return GetChildPositionFromEnd(index); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like the correlation between cache checks performed by It looks like the optimization targets very limited set of scenarios. For example, when we are far from the end and the next sibling isn't cached, but the one after the next is cached, we won't take advantage of that cached sibling. Would it make sense to generalize the optimization. For example, even if nothing is cached, it still might be faster to calculate from end simply because the item is closer to the end than to the front. Similarly, we could make decision based on the proximity of the first cached item going backwards vs. going forward. Perhaps we could maintain a bitmap of cached items to speed-up the process of finding closest cached items. #Closed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, the optimization specifically targets the scenario where the first iteration through the child list is in reverse, so the previous siblings are not in the cache. That's the one scenario we have currently where there is a perf issue. The optimization here is simply to calculate the position based on the offset from the immediately following sibling when that sibling is in the cache; otherwise, we use the existing approach. We should limit the change here to fixing this one scenario, to avoid perf regressions in other cases. If additional scenarios arise, we can consider further optimizations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I added There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Updated. |
||
} | ||
|
||
return base.GetChildPosition(index); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -632,6 +632,31 @@ internal virtual int GetChildPosition(int index) | |
return this.Position + offset; | ||
} | ||
|
||
// Similar to GetChildPosition() but calculating based on the positions of | ||
// following siblings rather than previous siblings. | ||
internal int GetChildPositionFromEnd(int index) | ||
{ | ||
var green = this.Green; | ||
int offset = green.GetSlot(index)?.FullWidth ?? 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This matches the behavior of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I won't object to adjusting that one too. |
||
int slotCount = green.SlotCount; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider moving the |
||
while (index < slotCount - 1) | ||
{ | ||
index++; | ||
var nextSibling = this.GetCachedSlot(index); | ||
if (nextSibling != null) | ||
{ | ||
return nextSibling.Position - offset; | ||
} | ||
var greenChild = green.GetSlot(index); | ||
if (greenChild != null) | ||
{ | ||
offset += greenChild.FullWidth; | ||
} | ||
} | ||
|
||
return this.EndPosition - offset; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. overall, i have no issue with the approach. but i think we def need tests around ensuring that reverse iterating produces nodes/tokens in the correct location. can you add a bunch of tests that show that if we reverse iterate the positions of things are all correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added verifying results from |
||
|
||
public Location GetLocation() | ||
{ | ||
return this.SyntaxTree.GetLocation(this.Span); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ | |
using Roslyn.Test.Utilities; | ||
using Roslyn.Utilities; | ||
using Xunit; | ||
using SeparatedWithManyChildren = Microsoft.CodeAnalysis.Syntax.SyntaxList.SeparatedWithManyChildren; | ||
|
||
namespace Microsoft.CodeAnalysis.Test.Utilities | ||
{ | ||
|
@@ -315,6 +316,8 @@ void checkTimeout() | |
} | ||
} | ||
} | ||
|
||
tree.VerifyChildNodePositions(); | ||
} | ||
|
||
var explicitNodeMap = new Dictionary<SyntaxNode, IOperation>(); | ||
|
@@ -381,6 +384,72 @@ void checkControlFlowGraph(IOperation root, ISymbol associatedSymbol) | |
} | ||
} | ||
|
||
internal static void VerifyChildNodePositions(this SyntaxTree tree) | ||
{ | ||
var nodes = tree.GetRoot().DescendantNodesAndSelf(); | ||
foreach (var node in nodes) | ||
{ | ||
var childNodesAndTokens = node.ChildNodesAndTokens(); | ||
if (childNodesAndTokens.Node is { } container) | ||
{ | ||
for (int i = 0; i < childNodesAndTokens.Count; i++) | ||
{ | ||
if (container.GetNodeSlot(i) is SeparatedWithManyChildren separatedList) | ||
{ | ||
verifyPositions(separatedList); | ||
} | ||
} | ||
} | ||
} | ||
|
||
static void verifyPositions(SeparatedWithManyChildren separatedList) | ||
{ | ||
var green = (Microsoft.CodeAnalysis.Syntax.InternalSyntax.SyntaxList)separatedList.Green; | ||
|
||
// Calculate positions from start, using existing cache. | ||
int[] positions = getPositionsFromStart(separatedList); | ||
|
||
// Calculate positions from end, using existing cache. | ||
AssertEx.Equal(positions, getPositionsFromEnd(separatedList)); | ||
|
||
// Avoid testing without caches if the number of children is large. | ||
if (separatedList.SlotCount > 100) | ||
{ | ||
return; | ||
} | ||
|
||
// Calculate positions from start, with empty cache. | ||
AssertEx.Equal(positions, getPositionsFromStart(new SeparatedWithManyChildren(green, null, separatedList.Position))); | ||
|
||
// Calculate positions from end, with empty cache. | ||
AssertEx.Equal(positions, getPositionsFromEnd(new SeparatedWithManyChildren(green, null, separatedList.Position))); | ||
} | ||
|
||
// Calculate positions from start, using any existing cache of red nodes on separated list. | ||
static int[] getPositionsFromStart(SeparatedWithManyChildren separatedList) | ||
{ | ||
int n = separatedList.SlotCount; | ||
var positions = new int[n]; | ||
for (int i = 0; i < n; i++) | ||
{ | ||
positions[i] = separatedList.GetChildPosition(i); | ||
} | ||
return positions; | ||
} | ||
|
||
// Calculate positions from end, using any existing cache of red nodes on separated list. | ||
static int[] getPositionsFromEnd(SeparatedWithManyChildren separatedList) | ||
{ | ||
int n = separatedList.SlotCount; | ||
var positions = new int[n]; | ||
for (int i = n - 1; i >= 0; i--) | ||
{ | ||
positions[i] = separatedList.GetChildPositionFromEnd(i); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and just to check. this actually computes, and is unaffected by anything potentially cached with the first for-loop? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, thanks. Actually, neither of the two loops cache any nodes, but both loops get the positions of the nodes using the cache is there at the time the verify method is called. Add comments and also added verification without empty caches for both directions. |
||
return positions; | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// The reference assembly System.Runtime.InteropServices.WindowsRuntime was removed in net5.0. This builds | ||
/// up <see cref="CompilationReference"/> which contains all of the well known types that were used from that | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the failure for these tests without the fix? Timeout? If so, consider adding a comment about that. #Resolved