unoplatform / uno

Open-source platform for building cross-platform native Mobile, Web, Desktop and Embedded apps quickly. Create rich, C#/XAML, single-codebase apps from any IDE. Hot Reload included! 90m+ NuGet Downloads!!
https://platform.uno
Apache License 2.0
8.94k stars 724 forks source link

[Performance] Consider avoiding symbol visitors in source generators #9791

Closed Youssef1313 closed 2 years ago

Youssef1313 commented 2 years ago

https://github.com/unoplatform/uno/blob/88d7a4024ccdcaa07a6b8cd056ba55ef4e92865e/src/SourceGenerators/Uno.UI.SourceGenerators/NativeCtor/NativeCtorsGenerator.cs#L33-L34

https://github.com/unoplatform/uno/blob/88d7a4024ccdcaa07a6b8cd056ba55ef4e92865e/src/SourceGenerators/Uno.UI.SourceGenerators/DependencyObject/DependencyObjectGenerator.cs#L32-L33

Also doing the same in #6937

Using RegisterForSyntaxNotifications might be more performant. It's not available in Uno.SourceGeneration, unfortunately.

We could:

From Roslyn Discord:

image

image

jeromelaban commented 2 years ago

Let's see what the performance improvements are with some of the latest changes I'm making with https://github.com/unoplatform/uno/pull/9784, and we may be able to remove the use of Uno.SourceGeneration altogether.

Youssef1313 commented 2 years ago

Let's see what the performance improvements are with some of the latest changes I'm making with https://github.com/unoplatform/uno/pull/9784, and we may be able to remove the use of Uno.SourceGeneration altogether.

Nice!! It's going to be very great if we could!

Youssef1313 commented 2 years ago

@jeromelaban Now that #9784 is merged, was it possible to always use Roslyn generation? Is this issue can be worked on now or should I wait for something?

jeromelaban commented 2 years ago

I still don't know if we'll be able to do this. We'll have let https://github.com/unoplatform/uno/pull/9808 be tested a bit more until we can do something about it.

Youssef1313 commented 2 years ago

This doesn't seem to improve performance (https://github.com/unoplatform/uno/pull/9820) for some unknown reason. Let's close