-
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 5 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,21 @@ 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; | ||
if (valueIndex > 1 | ||
&& _children[(valueIndex - 2) >> 1].Value is null | ||
&& (valueIndex >= Green.SlotCount - 2 || _children[(valueIndex + 2) >> 1].Value 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 |
---|---|---|
|
@@ -315,6 +315,8 @@ void checkTimeout() | |
} | ||
} | ||
} | ||
|
||
tree.VerifyChildNodePositions(); | ||
} | ||
|
||
var explicitNodeMap = new Dictionary<SyntaxNode, IOperation>(); | ||
|
@@ -381,6 +383,41 @@ 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 Microsoft.CodeAnalysis.Syntax.SyntaxList.SeparatedWithManyChildren separatedList) | ||
{ | ||
verifyPositions(separatedList); | ||
} | ||
} | ||
} | ||
} | ||
|
||
static void verifyPositions(Microsoft.CodeAnalysis.Syntax.SyntaxList.SeparatedWithManyChildren separatedList) | ||
{ | ||
int n = separatedList.SlotCount; | ||
var positions1 = new int[n]; | ||
for (int i = 0; i < n; i++) | ||
{ | ||
positions1[i] = separatedList.GetChildPosition(i); | ||
} | ||
var positions2 = new int[n]; | ||
for (int i = n - 1; i >= 0; i--) | ||
{ | ||
positions2[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. |
||
AssertEx.Equal(positions1, positions2); | ||
} | ||
} | ||
|
||
/// <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.
Could you elaborate what is the meaning of this condition? #Closed
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.
The preceding conditions have determined the previous sibling is not cached. This condition checks if the index represents the last item or the next to last item. If so, we use
GetChildPositionFromEnd()
to calculate the position from the end of the list rather than the start. If the index is the last item (a separator) or next to last item (followed by a separator which are never cached), there are no following siblings to cache so we can skip the||
condition.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.
I think it would be good capturing this in a comment and explaining why it doesn't matter whether the next item is cached in this case.