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

do not invalidate the BackgroundCompiler config on DocumentChanged #4121

Merged
merged 1 commit into from
Dec 17, 2017
Merged

do not invalidate the BackgroundCompiler config on DocumentChanged #4121

merged 1 commit into from
Dec 17, 2017

Conversation

dhwed
Copy link
Contributor

@dhwed dhwed commented Dec 15, 2017

Completion is currently taking 5-10 seconds to show up in dotnet core SDK projects with many references.

After some investigation, it looks like the call to BackgroundCompiler.InvalidateConfiguration is tying up the reactor by creating a new IncrementalBuilder every time an open file is modified. It doesn't appear to me that this invalidation is required, but I may be missing something.

This change fixes the performance issue, and I haven't seen any issues during my testing.

@KevinRansom Did I miss a reason why this invalidation is required on the DocumentChanged event? If I am correct that it isn't necessary, can we also skip this invalidation anywhere else?

@msftclas
Copy link

msftclas commented Dec 15, 2017

CLA assistant check
All CLA requirements met.

@forki
Copy link
Contributor

forki commented Dec 16, 2017

@vasily-kirichenko said he fixed exactly the same bug a year ago. But somehow the rewrite introduced it again

Copy link
Contributor

@cartermp cartermp left a comment

Choose a reason for hiding this comment

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

Looks good code-wise and completion does feel faster for me. Thanks @dhwed!

@cartermp cartermp merged commit 795c7c6 into dotnet:master Dec 17, 2017
@saul
Copy link
Contributor

saul commented Dec 17, 2017

Is it worth @dsyme weighing in here about when invalidation is necessary? We can probably invalidate in fewer cases, too.

@dsyme
Copy link
Contributor

dsyme commented Dec 19, 2017

Yes, that looks wrong

@saul When does DocumentReloaded get triggered?

@KevinRansom Do you know why we are doing anything at all for DocumentChanged? Was there a particular case that motivated this? e.g. why do we need to call UpdateDocumentInfoWithProjectId at all for OnDocumentChanged?

    member private this.OnDocumentChanged(projectId:ProjectId, documentId:DocumentId) = projectInfoManager.UpdateDocumentInfoWithProjectId(projectId, documentId, "OnDocumentChanged", invalidateConfig=false)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants