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

Remove cancellation token from tagging context object. #57167

Merged
merged 4 commits into from
Oct 18, 2021

Conversation

CyrusNajmabadi
Copy link
Member

No description provided.

this.ThreadingContext.JoinableTaskFactory.Run(
() => ProduceTagsAsync(context, new DocumentSnapshotSpan(document, spanToTag), _owner._typeMap));
() => ProduceTagsAsync(context, new DocumentSnapshotSpan(document, spanToTag), _owner._typeMap, cancellationToken));
Copy link
Member

@sharwell sharwell Oct 15, 2021

Choose a reason for hiding this comment

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

⚠️ cancellationToken will be captured for this lambda even on code paths where canReuseCache is true. I didn't evaluate how common this is, but if this method normally doesn't use this code path and normally doesn't allocate (or limited allocations) and is called frequently, this could be observable in perf.

Copy link
Member Author

Choose a reason for hiding this comment

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

we capture everything else passed along here. i'm not sure why this woudl matter.

similary, this codepaths runs once per Copy operation. it doesn't seem like any change here would be relevant.

/// <summary>
/// Remove once TypeScript finishes https://github.com/dotnet/roslyn/issues/57180.
/// </summary>
protected virtual Task ProduceTagsAsync(TaggerContext<TTag> context, DocumentSnapshotSpan snapshotSpan, int? caretPosition)
Copy link
Member Author

Choose a reason for hiding this comment

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

once this merged in, i'll make the requisite changes on the TS side @DanielRosenwasser @amcasey @minestarks

note: there is no ABI or functionality change in my PR. just the introduction of a preferred method to use instead. once i move TS over to using it, i'll remove the now-legacy ABI stubs.

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

:shipit:

@CyrusNajmabadi CyrusNajmabadi merged commit 82a74fe into dotnet:main Oct 18, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the taggerCancellation branch October 18, 2021 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants