Skip to content

Use next sibling in SyntaxNode.GetChildPosition() if available #66876

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.

Sign up for GitHub

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

Merged
cston merged 14 commits into from
Mar 8, 2023
Merged

Use next sibling in SyntaxNode.GetChildPosition() if available #66876

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
6d82cde
Use next sibling in SyntaxNode.GetChildPosition() if available
cston Feb 14, 2023
86786a2
Update test
cston Feb 14, 2023
4bea78e
Add test
cston Feb 14, 2023
050f570
Fix build
cston Feb 14, 2023
14c639e
Verify child positions
cston Feb 15, 2023
95880f0
Add verification without cache
cston Feb 15, 2023
8f18f02
Update comments
cston Feb 15, 2023
9e8e9ce
Add comment
cston Feb 16, 2023
a98cb9a
Add tests with trailing separator
cston Feb 16, 2023
bff2282
Remove comment
cston Feb 16, 2023
528a2a8
Add comment
cston Feb 16, 2023
80f0ab8
Use GetCachedSlot()
cston Feb 16, 2023
724aecd
Call GetCachedSlot(index)
cston Feb 17, 2023
c187e3d
Update tests: add comments; increase iterations
cston Mar 8, 2023
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
Prev Previous commit
Next Next commit
Use GetCachedSlot()
  • Loading branch information
@cston
cston committed Feb 16, 2023
commit 80f0ab88cf6303258efc6225e60b67c365f8b93f
4 changes: 2 additions & 2 deletions src/Compilers/Core/Portable/Syntax/SyntaxList.SeparatedWithManyChildren.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ internal override int GetChildPosition(int index)
// 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
&& _children[(valueIndex - 2) >> 1].Value is null
&& (valueIndex >= Green.SlotCount - 2 || _children[(valueIndex + 2) >> 1].Value is { }))
&& GetCachedSlot(valueIndex - 2) is null
&& (valueIndex >= Green.SlotCount - 2 || GetCachedSlot(valueIndex + 2) is { }))
{
return GetChildPositionFromEnd(index);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

GetChildPositionFromEnd

It feels like the correlation between cache checks performed by GetChildPositionFromEnd and cache checks performed here would be more obvious if we were using GetCachedSlot helper rather than working with _children on the low level. Also, could we instead adjust implementation of GetChildPosition to perform the same cache tests for the following child? Then all the logic would be in one place, and, perhaps, we wouldn't need to provide a general GetChildPositionFromEnd helper, since it looks like we really want to handle just 3 specific situations.

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

Copy link
Contributor Author

@cston cston Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like the optimization targets very limited set of scenarios.

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.

Copy link
Contributor Author

@cston cston Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Also, could we instead adjust implementation of GetChildPosition to perform the same cache tests for the following child?

GetChildPosition() doesn't know about separated lists (where the separator is not in the cache) so it would be surprising for that method to skip over some siblings when determining whether using the following sibling is a better choice.

Then all the logic would be in one place, and, perhaps, we wouldn't need to provide a general GetChildPositionFromEnd helper ...

I added GetChildPositionFromEnd() next to GetChildPostion() so the two loops were together. And I think we'd still need the cases that are handled in GetChildPositionFromEnd(), even if the methods were combined. I'll keep the two methods separate so that both implementations are clear.

Copy link
Contributor Author

@cston cston Feb 16, 2023

Choose a reason for hiding this comment

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

It feels like the correlation between cache checks performed by GetChildPositionFromEnd and cache checks performed here would be more obvious if we were using GetCachedSlot helper rather than working with _children on the low level.

Updated.

}
Expand Down