CA1041 performance #6143
Assignees
Labels
Bug
The product is not behaving according to its current intended design
help wanted
The issue is up-for-grabs, and can be claimed by commenting
Performance
Indicates a performance problem
Milestone
Comments
6 tasks
mavasani
added
Bug
|
I would be willing to work on this. |
Registering callbacks should not add so much of a performance cost to analyzer execution. @Youssef1313 are you able to share a performance trace or a repro project? |
|
I have seen it in Uno's binlogs. I'm enabling the analyzer in unoplatform/uno#12641 to see if it will still show up. But I'm not sure I'll be able to get a meaningful trace. I could record a trace while building Uno locally, but there are parts of the build that are much more costly than CA1041 so I don't think anything related to CA1041 will show up in the trace (unless there is a way to know the time interval of the trace during which the analyzer is run maybe?) |
|
A quick note: This line will allocate a BindingDiagnosticBag. Since the analyzer calls |
@Youssef1313 Even if the analyzer execution does not lead to invoking that method, the compiler will do so to finish binding. I don't believe that should justify moving away from the analyzer causing force binding of attributes. We would just be moving the performance cost from the analyzer to the rest of the compilation process. Unless we can definitely reduce the overall build time cost, and show that analyzer is causing some unnecessary compute overhead, we may have to just live with this. |
|
@mavasani CPU Stacks view for the analyzer
Are you able to see the trace in https://developercommunity.visualstudio.com/t/Performance-trace-for-CA1041/10395065 ? |
|
I created a "fake" codebase using: for (int i = 0; i < 10000; i++)
{
var x = $$"""
class C{{i}}
{
void M1 () { }
void M2 () { }
void M3 () { }
void M4 () { }
void M5 () { }
string P1 { get; set; }
string P2 { get; set; }
string P3 { get; set; }
string P4 { get; set; }
string P5 { get; set; }
event EventHandler E1;
event EventHandler E2;
event EventHandler E3;
event EventHandler E4;
event EventHandler E5;
}
""";
File.WriteAllText($"C{i}.cs", x);
}The following would be needed in csproj to avoid too much build warnings: <Nullable>disable</Nullable>
<NoWarn>$(NoWarn);CS0067</NoWarn>Then I started PerfView with: Then performed a build in VS. |
|
For the above codebase, CA1041 is the most expensive:
|
|
@Youssef1313 - are you able to see any noticeable build time difference with this analyzer enabled vs disabled? |
|
Yes. Csc task goes up from ~9 seconds to ~14 seconds on my machine. For a very simple analyzer like this, I think this increase is quite a lot. |
|
For the performance trace attached to AB#1838338, this analyzer was only responsible for 700ms of thread time. Almost all of that time was spent in this check: |
|
Moving a "has any attributes" check to before the visibility check will improve things, provided |
|
I took a look at the Uno's binlog again from unoplatform/uno#12641 and the binlog time is much less than what I originally reported. Actually all analyzers now take much less time. In original issue, the binlog shows 3 minutes. The newest binlog I have is only 2 seconds. This is very much surprising, but this improvement could be related to the many performance improvements done in Roslyn recently (I'm guessing - but anyway, the situation appears to be much better now). I'm keeping this open though to move the slow check after |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Seeing the following in a binlog:
Looks like this is a very simple analyzer, but is somewhat slow. The analyzer registers symbol actions for named types, methods, fields, properties, and events.
So I assume this is alone too much work.
The analyzer should use IAttributeOperation when it's added back (but the analyzer will not work for people consuming a compiler without this IOp), or register a syntax action for AttributeSyntax.
Yes we try to avoid syntax analysis in many cases, but I think for this one we might see a good performance gain.
The text was updated successfully, but these errors were encountered: