Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

We do not use OOP for FAR for body-level symbols (like locals/local-functions/etc.). There's no point in sending to OOP as it will do nothing but add overhead. Specifically, a body level symbol already came from source, and it roots the Compilation it came from (through its ISourceAssemblySymbol).

Since we literally only need to examine the symbol's containing method-like-body to look for other references, it's much better to just stay in process. As we have a local, it's highly likely that the caller of this also got that local symbol from a semantic model that they are also holding. So, in most cases there's no additional semantic costs at all, and this just becomes a walk of the existing bound nodes with the same name to find hits, which is the fastest we could hope for without a dedicated compiler API.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner March 15, 2023 21:42
@ghost ghost added the Area-IDE label Mar 15, 2023
@genlu
Copy link
Member

genlu commented Mar 15, 2023

Is this from some perf investigation? Do we have trace that shows the perf difference?

@tmat
Copy link
Member

tmat commented Mar 15, 2023

I understand the potential short term benefit but the long term goal should be to avoid accessing symbols in-proc entirely. Seems like this change goes the opposite direction. If the short term benefit is significant then let's go ahead.

@CyrusNajmabadi
Copy link
Member Author

Is this from some perf investigation? Do we have trace that shows the perf difference?

Yes. This is looking into traces Todd and I are collecting on a file that shows certain lightbulbs running poorly.

@mavasani
Copy link
Contributor

Is this from some perf investigation? Do we have trace that shows the perf difference?

Yes. This is looking into traces Todd and I are collecting on a file that shows certain lightbulbs running poorly.

I have seen the same in traces I have looked at for lightbulb invocation on line with a local declaration.

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Mar 15, 2023

I understand the potential short term benefit but the long term goal should be to avoid accessing symbols in-proc entirely.

RIght. This is orthogonal to that goal though. Ideally here, code actions would be computed and would run in the server entirely. But in that case, none of the find-refs calls would cross boundaries either.

We're currently in this state due to the code-action running inproc, but the FAR running OOP. It's only in such a split world (where symbols are on both sides anyways) where FAR is def not a great choice for us for this set of cases. For things like Global-Symbols, we def would still want to go OOP. But for local symbols, it's always worse :)

Seems like this change goes the opposite direction. If the short term benefit is significant then let's go ahead.

It's neither one way or the other. :) It doesn't hurt us at all in our goal to make us avoid accessing symbols in proc. However, to get there, we need to get all of code-fixes/refactorings OOP. And that's an enormous item that is certainly a ways out. Once we get there, we can remove the idea that these APIs even do any sort of cross-process work :)

@CyrusNajmabadi
Copy link
Member Author

This PR and #67314 make us go from waiting several seconds (usually around 2+) on preview/lightbulb for some particular use cases to being <1s for both.

@CyrusNajmabadi CyrusNajmabadi merged commit 42f4496 into dotnet:main Mar 15, 2023
@ghost ghost added this to the Next milestone Mar 15, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the bodyLevelFAR branch March 15, 2023 23:48
@allisonchou allisonchou modified the milestones: Next, 17.6 P3 Mar 28, 2023
@mavasani mavasani added the Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger. label May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-IDE Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants