Skip to content

Conversation

@p-26
Copy link

@p-26 p-26 commented Apr 7, 2022

I implemented some performance improvements because the analyzer takes ~35s on my machine in our solution:
image

@hermanussen
Copy link
Owner

I'll look into these changes as soon as I have some time. But looks pretty good so far. Thanks!

Do the tests pass? And do you have any data on the impact of the changes in terms of performance?

@p-26
Copy link
Author

p-26 commented Apr 7, 2022

Thanks for your timely reply.
The tests pass, yes.
As of performance: I don't see any impacts unfortunately. 😕
image

@p-26
Copy link
Author

p-26 commented Apr 7, 2022

I wanted to profile the Analyzer and wrote a test for it:

[Fact]
        public async Task TestPerformanceProfiling()
        {
            var source =
                File.ReadAllText(
                    @"ErpActionLink.Artifact.cs");
            var rules = File.ReadAllText(@".refrules");
            
            await ReferenceCopAnalyzerVerifier.VerifyReferenceCopAnalysis(source, rules,
                Array.Empty<DiagnosticResult>());
        }

It crashes with a StackOverflow:

The active test run was aborted. Reason: Test host process crashed : Stack overflow.

Here are the file contents:
TestFiles.zip

@p-26
Copy link
Author

p-26 commented Apr 8, 2022

I fixed the test.

By removing RegexOptions.Compiled in the analyzer I was able to improve the performance by ~40%:
image

The profiling result of the original code looks like this:
image

Can you have a look at it? Thanks in advance!

@p-26
Copy link
Author

p-26 commented Apr 12, 2022 via email

@hermanussen
Copy link
Owner

Hi Nicolas,

Please be patient. I just haven't gotten to it yet. But I will eventually, I promise!

@p-26
Copy link
Author

p-26 commented Apr 26, 2022

Hi Nicolas,

Please be patient. I just haven't gotten to it yet. But I will eventually, I promise!

Hi Robin,

Do you know, roughly, when you'll have the opportunity to review my pull request?

Thanks,
Nicolas

@hermanussen
Copy link
Owner

Well, I took a look this weekend. I changed the unit test you added to include timings and tested with the current as well as your new code. These are my results (run in Release configuration):

Current code:
Slowest 5139ms, Fastest 381ms, Average 718ms. Out of 25 runs.

After applying your improvements:
Slowest 5443ms, Fastest 366ms, Average 699ms. Out of 25 runs.

I wouldn't call that a significant performance increase. On the whole it seems rather slow anyway. Once I have some more time, I'll see if I can make some structural performance improvements.

By the way; if you look at the huge difference between the slowest and fastest runs, it seems like the test is not fully isolated and Roslyn does some optimizations/caching. I think that needs to be addressed as well for testing.

@p-26 p-26 marked this pull request as draft April 3, 2025 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants