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

Source Generators: pass for intellisense output #52170

Closed
lsoft opened this issue Mar 26, 2021 · 12 comments
Closed

Source Generators: pass for intellisense output #52170

lsoft opened this issue Mar 26, 2021 · 12 comments

Comments

@lsoft
Copy link

lsoft commented Mar 26, 2021

@CyrusNajmabadi mentioned that the recommended running time of the Source Generator should not exceed 5 milliseconds. The requirement is strong, but understandable - since we have to run Source Generators to update intellisense, we have to work quickly to keep the user experience good. OK.

I am developing a Dependency Injector based on Source Generators. I took steps to optimize, but now there is nothing more to optimize, since the profiler shows that 70% -80% of the time the code spends inside Roslyn, requesting symbols. Runtime is far from the desired 5 milliseconds (~100 msec actually). I checked, all the requests are needed, there are no extra ones. Ideally, I would like more speed from the methods of working with symbols, but this issue is about something else.

Here's an example of what my Source Generator creates (this is a simplified and incorrect example, but illustrates what I mean):

public partial class MyDIContainer : IResolution<IA>, IResolution<IB>, IResolution<IC>
{
    public T Get<T>()
    {
        return (this as IResolution<T>).Get();
    }

    //all (except one) lines above are constants actually; they does not need any symbol from Compilation
    //lines below are generated from the actual symbols
    IA IResolution<IA>.Get() => new A();
    IB IResolution<IB>.Get() => new B();
    IC IResolution<IC>.Get() => new C();
}

Proposal

Pass bool passForIntellisense to the Source Generator. On receiving true, Source Generator will generate simplified code that is only suitable for intellisense. On real compilation, or when the user opens the generated code in VS, the Source Generator will receive passForIntellisense = false and generate the complete code.

In my case, this will reduce the running time of the Source Generator to almost zero, since I will be generating the following constant code:

//ALL lines are constants actually; they does not need any symbol from Compilation
public partial class MyDIContainer
{
    public T Get<T>()
    {
        return (this is IResolution<T> r).Get();
    }

    //nothing here
}

For other scenarios, it is possible to generate dummy methods with throw new NotImplementedException ();.

Responsibility for the identity of the code with passForIntellisense = true and passForIntellisense = false will lie with the developer of the Source Generator.

I also invite @YairHalberstadt to comment, he is developing a similar Source Generator (ref #49685) and faced the same performance issue as I did.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 26, 2021
@jasonmalinowski
Copy link
Member

Tagging @chsienki -- this is slight variation of what we're looking at, but similar in idea. We're looking at more of a pipeline based API where you can output multiple trees but we can tag some of them with different reasons. So you'd output your partial with a "needed for semantics" or something, and the other stuff is tagged "needed for runtime" which we'd only call when we actually need it.

@CyrusNajmabadi
Copy link
Member

How would that work in practice @jasonmalinowski ? Say you hovered over a symbol that was source-generated. We'd be able to show info for it because we computed what was "needed for semantics". But then the user says "go to def". What would we do then? Rerun the SG to produce the final code so the user can see what it would actually be? That would seem to violate some workspace invariants. We'd have two version of a file for the same snapshot. Or would any particular project have 3 compilations? The non-SG one, the "needed for semantics" one, and the "needed for display/navigation" one?

@jaredpar jaredpar added the New Feature - Source Generators Source Generators label Mar 29, 2021
@jaredpar jaredpar added this to the 16.10 milestone Mar 29, 2021
@jaredpar jaredpar added Feature Request and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 29, 2021
@jasonmalinowski
Copy link
Member

The non-SG one, the "needed for semantics" one, and the "needed for display/navigation" one?

Potentially yeah, there's more than one. I'd hope that most consumers except say Edit and Continue don't care about that, but I admit I haven't though it through fully. (And I also admit any of the various API shapes that come to mind I don't like either....)

@YairHalberstadt
Copy link
Contributor

More than half the time in my source generator is actually spent in SyntaxNormalizer.NormalizeWhiteSpace.

If performance of formatting generated source could be improved, that would be a much more simple and useful saving for me.

@jasonmalinowski
Copy link
Member

@YairHalberstadt I think we have another open bug tracking that right?

@YairHalberstadt
Copy link
Contributor

Yep. I also have a PR with about 25% speedup at #50778

@CyrusNajmabadi
Copy link
Member

@jaredpar for context on:

More than half the time in my source generator is actually spent in SyntaxNormalizer.NormalizeWhiteSpace.

Can we give some internal resources toward improving things. Our general recommendation for people often mentions just mking a tree and normalizing it to ensure it is parseable and reasonable to look at. It would be good if we could offset the dramatic perf impact it has on SG running time.

Thanks!

@chsienki
Copy link
Contributor

Implemented via #54798

@Neme12
Copy link
Contributor

Neme12 commented Jun 1, 2022

@chsienki That PR didn't address this request. This is actually a request for #57589, not for implementation-only output.

@CyrusNajmabadi
Copy link
Member

@Neme12 SGs should switch to incremental generation so that they only do the minimal work necessary and only regen things when changes actually happen.

@Neme12
Copy link
Contributor

Neme12 commented Jun 1, 2022

@CyrusNajmabadi I don't know why you're arguing with me, I didn't create this issue, I'm just making it clear that the PR didn't address this issue and this is actually a duplicate of #57589.

@CyrusNajmabadi
Copy link
Member

@Neme12 I'm not arguing with you :)

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

No branches or pull requests

8 participants