Skip to content

Avoid computing unused values in lsif#54831

Merged
CyrusNajmabadi merged 3 commits intodotnet:mainfrom
CyrusNajmabadi:lsifPerf
Jul 16, 2021
Merged

Avoid computing unused values in lsif#54831
CyrusNajmabadi merged 3 commits intodotnet:mainfrom
CyrusNajmabadi:lsifPerf

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Jul 14, 2021

This speeds up production of lsif information on my machine for projects from 86 seconds to 29 seconds (down to 33% of the time).

@CyrusNajmabadi CyrusNajmabadi requested a review from mavasani July 14, 2021 22:43
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner July 14, 2021 22:43
@ghost ghost added the Area-IDE label Jul 14, 2021
Copy link
Copy Markdown
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

I know in the hover handler we call build qi without navigation bits.
https://sourceroslyn.io/#Microsoft.CodeAnalysis.LanguageServer.Protocol/Handler/Hover/HoverHandler.cs,71

This eventually ends up here (where streamingPreseneter is null) - https://sourceroslyn.io/#Microsoft.CodeAnalysis.EditorFeatures/Implementation/IntelliSense/Helpers.cs,139 and doesn't include navigation actions.

Should that code be using this option instead, or is this doing something different?

@CyrusNajmabadi CyrusNajmabadi requested a review from dibarbet July 15, 2021 08:00
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@mavasani @dibarbet can you ptal?

Copy link
Copy Markdown
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

Looks OK to me, I'll just echo what we discussed offline here - it would be nicer if lsif could directly use documents and this could become a document option (and then unify the lsp impl to just use that option). But that ship has probably already sailed.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

it would be nicer if lsif could directly use documents and this could become a document option (and then unify the lsp impl to just use that option). But that ship has probably already sailed.

I actually agree. Paging @mavasani @jasonmalinowski to get their thoughts on why we didn't go that route. I get that we might not have a "real" workspace. But it feels not crazy that we could synthesize a reasonable one out of the compilation info we've been given.

@CyrusNajmabadi CyrusNajmabadi merged commit 589ad4f into dotnet:main Jul 16, 2021
@ghost ghost added this to the Next milestone Jul 16, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the lsifPerf branch July 16, 2021 03:21
@allisonchou allisonchou modified the milestones: Next, 17.0.P3 Jul 27, 2021
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