Skip to content
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

Optimize syntactic classification walk #73629

Merged
merged 9 commits into from
May 22, 2024

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented May 22, 2024

Drops us about 0.7% during scrolling from:

image

to

image

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 22, 2024
@CyrusNajmabadi CyrusNajmabadi changed the title WIP: Optimize syntactic classification walk Optimize syntactic classification walk May 22, 2024
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review May 22, 2024 03:52
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner May 22, 2024 03:52
// sorting the results before doing anything with them.
foreach (var child in current.ChildNodesAndTokens())
{
if (child.IsNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

if (child.IsNode)

nit question: I've seen this in a couple places, why not just use the "bool AsNode(out SyntaxNode? node)" method?

Copy link
Member

Choose a reason for hiding this comment

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

If possible, I would prefer it be called IsNode(out SyntaxNode?). I don't think this is possible with a property and method in the same type, but may be possible with it defined as an extension method.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know the AsNode method existed. Will update to that.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

if (child.IsNode)
{
var childNode = child.AsNode()!;
if (childNode.FullSpan.IntersectsWith(_textSpan))
Copy link
Contributor

Choose a reason for hiding this comment

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

if (childNode.FullSpan.IntersectsWith(_textSpan))

if this is a hot path, maybe breaking out of the inner loop is worthwhile:

if (childNode.FullSpan.IntersectsWith(_textSpan))
    stack.Push(childNode);
else if (childNode.Position > _textSpan.End)
    break;

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll def try that

Copy link
Member Author

Choose a reason for hiding this comment

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

done


// It's ok that we're not pushing in reverse. The caller (TotalClassificationTaggerProvider) will be
// sorting the results before doing anything with them.
foreach (var child in current.ChildNodesAndTokens())
Copy link
Contributor

Choose a reason for hiding this comment

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

ChildNodesAndTokens

this is way off topic, but I took another look at the SyntaxList.Enumerator's call to ItemInternal, and I'm not clear why the slotIndex isn't held between calls. By not doing so, the first loop in ItemInternal would need to walk the green slots until it reaches the appropriate slot index.

Again, off topic, so nothing needed in this PR, but it seems odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

def look into that. there are some comments explaining things. but it's possible they're stale, or there is room for improvement here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Note: i'm saying @ToddGrun should look into this :)

Copy link
Contributor

@ToddGrun ToddGrun left a comment

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants