Skip to content

pass in cancellation token to bail out early in HasReferenceToAssembly#36176

Merged
heejaechang merged 1 commit intodotnet:masterfrom
heejaechang:addCancellation2
Jun 7, 2019
Merged

pass in cancellation token to bail out early in HasReferenceToAssembly#36176
heejaechang merged 1 commit intodotnet:masterfrom
heejaechang:addCancellation2

Conversation

@heejaechang
Copy link
Copy Markdown
Contributor

this particular code path can take a long time if metadata reference hasn't created its symbols yet.

this can get really bad if we try to show code lens for something like IDispose.Dipose and it tries to create all metdata references in the solution.

since it is not cancellable, just passing by a Dipose method can cause this to occupy CPU 100% for several seconds and there is no way to make it go away.

now we pass in cancellation token so that we can bail out sooner.

this particular code path can take a long time if metadata reference hasn't created its symbols yet.

this can get really bad if we try to show code lens for something like IDispose.Dipose and it tries to create all metdata references in the solution.

since it is not cancellable, just passing by a Dipose method can cause this to occupy CPU 100% for several seconds and there is no way to make it go away.

now we pass in cancellation token so that we can bail out  sooner.
@heejaechang heejaechang requested a review from a team as a code owner June 5, 2019 07:47
@heejaechang
Copy link
Copy Markdown
Contributor Author

can you take a look @dotnet/roslyn-ide

@heejaechang heejaechang changed the title pass in cancellation token to bail out early pass in cancellation token to bail out early in HasReferenceToAssembly Jun 5, 2019
@heejaechang
Copy link
Copy Markdown
Contributor Author

heejaechang commented Jun 5, 2019

@jinujoseph can we take this to preview2? once @olegtk fixed cancellation issue on code lens OOP, this is next one that showed up.

I don't think it is a regression. we always had this problem, just didn't notice before. but now since we are focusing on issues around this, I guess it is good time to fix this along with other ones we are fixing.

@jinujoseph
Copy link
Copy Markdown
Contributor

@olegtk is your changes already in preview2 ?

@olegtk
Copy link
Copy Markdown
Contributor

olegtk commented Jun 5, 2019

Not yet in Preview2, but can be today if I get it approved.

@jinujoseph
Copy link
Copy Markdown
Contributor

@heejaechang , we will not be able to make the final build for preview2.
Let's try to get this in preview3 ( i am retargetting)

@jinujoseph jinujoseph changed the base branch from release/dev16.2-preview2 to master June 6, 2019 06:31
@jinujoseph jinujoseph modified the milestones: 16.2.P2, 16.2.P3 Jun 6, 2019
@heejaechang heejaechang merged commit bbc78ed into dotnet:master Jun 7, 2019
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.

6 participants