From a1ee6380f312308adbeeff5eb0ded55ecc84b9bf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 5 Jul 2025 12:33:02 +0000 Subject: [PATCH 1/8] Implement parallel processing in ReadIssues with thread-safe collections and performance logging Co-authored-by: pascalberger <2190718+pascalberger@users.noreply.github.com> --- .../FakeFailingIssueProvider.cs | 20 +++++ src/Cake.Issues.Tests/IssueReaderTests.cs | 90 ++++++++++++++++++- src/Cake.Issues.Tests/IssuesFixture.cs | 4 +- src/Cake.Issues/IssuesReader.cs | 34 +++++-- 4 files changed, 138 insertions(+), 10 deletions(-) create mode 100644 src/Cake.Issues.Testing/FakeFailingIssueProvider.cs diff --git a/src/Cake.Issues.Testing/FakeFailingIssueProvider.cs b/src/Cake.Issues.Testing/FakeFailingIssueProvider.cs new file mode 100644 index 000000000..1e33d9302 --- /dev/null +++ b/src/Cake.Issues.Testing/FakeFailingIssueProvider.cs @@ -0,0 +1,20 @@ +namespace Cake.Issues.Testing; + +using System.Collections.Generic; +using Cake.Core.Diagnostics; + +/// +/// Implementation of a that fails during initialization for use in test cases. +/// +/// The Cake log instance. +public class FakeFailingIssueProvider(ICakeLog log) : BaseIssueProvider(log) +{ + /// + public override string ProviderName => "Fake Failing Issue Provider"; + + /// + public override bool Initialize(IReadIssuesSettings settings) => false; + + /// + protected override IEnumerable InternalReadIssues() => []; +} \ No newline at end of file diff --git a/src/Cake.Issues.Tests/IssueReaderTests.cs b/src/Cake.Issues.Tests/IssueReaderTests.cs index 83a6018b0..17efb274f 100644 --- a/src/Cake.Issues.Tests/IssueReaderTests.cs +++ b/src/Cake.Issues.Tests/IssueReaderTests.cs @@ -94,7 +94,7 @@ public void Should_Initialize_Issue_Provider() _ = fixture.ReadIssues(); // Then - fixture.IssueProviders.ShouldAllBe(x => x.Settings == fixture.Settings); + fixture.IssueProviders.OfType().ShouldAllBe(x => x.Settings == fixture.Settings); } [Fact] @@ -142,7 +142,7 @@ public void Should_Initialize_All_Issue_Provider() _ = fixture.ReadIssues(); // Then - fixture.IssueProviders.ShouldAllBe(x => x.Settings == fixture.Settings); + fixture.IssueProviders.OfType().ShouldAllBe(x => x.Settings == fixture.Settings); } [Fact] @@ -349,5 +349,91 @@ public void Should_Set_FileLink_Property() issue2.FileLink.ToString() .ShouldBe($"{repoUrl}/blob/{branch}/{filePath2.Replace(@"\", "/")}#L{line2}"); } + + [Fact] + public void Should_Read_Issues_From_Multiple_Providers_Concurrently() + { + // Given + const int providerCount = 10; + const int issuesPerProvider = 100; + var fixture = new IssuesFixture(); + fixture.IssueProviders.Clear(); + + // Create multiple providers with different issues + for (var i = 0; i < providerCount; i++) + { + var providerIssues = new List(); + for (var j = 0; j < issuesPerProvider; j++) + { + var issue = IssueBuilder + .NewIssue($"Issue{i}-{j}", $"ProviderType{i}", $"ProviderName{i}") + .InFile($@"src\Provider{i}\File{j}.cs", j + 1) + .OfRule($"Rule{j}") + .WithPriority(IssuePriority.Warning) + .Create(); + providerIssues.Add(issue); + } + fixture.IssueProviders.Add(new FakeIssueProvider(fixture.Log, providerIssues)); + } + + // When + var stopwatch = System.Diagnostics.Stopwatch.StartNew(); + var issues = fixture.ReadIssues().ToList(); + stopwatch.Stop(); + + // Then + issues.Count.ShouldBe(providerCount * issuesPerProvider); + + // Verify all providers contributed issues + for (var i = 0; i < providerCount; i++) + { + var providerIssues = issues.Where(x => x.ProviderType == $"ProviderType{i}").ToList(); + providerIssues.Count.ShouldBe(issuesPerProvider); + } + + // Verify all Run properties are set + issues.ShouldAllBe(x => x.Run == fixture.Settings.Run); + + // Log timing for performance verification + System.Console.WriteLine($"Reading {issues.Count} issues from {providerCount} providers took {stopwatch.ElapsedMilliseconds}ms"); + } + + [Fact] + public void Should_Handle_Provider_Initialization_Failures_Concurrently() + { + // Given + var fixture = new IssuesFixture(); + fixture.IssueProviders.Clear(); + + // Add mix of successful and failing providers + fixture.IssueProviders.Add(new FakeIssueProvider(fixture.Log, [ + IssueBuilder.NewIssue("Success1", "ProviderType1", "ProviderName1") + .InFile(@"src\File1.cs", 1) + .OfRule("Rule1") + .WithPriority(IssuePriority.Warning) + .Create() + ])); + + // Create a failing provider by passing null settings later + var failingProvider = new FakeFailingIssueProvider(fixture.Log); + fixture.IssueProviders.Add(failingProvider); + + fixture.IssueProviders.Add(new FakeIssueProvider(fixture.Log, [ + IssueBuilder.NewIssue("Success2", "ProviderType2", "ProviderName2") + .InFile(@"src\File2.cs", 2) + .OfRule("Rule2") + .WithPriority(IssuePriority.Warning) + .Create() + ])); + + // When + var issues = fixture.ReadIssues().ToList(); + + // Then + // Should get issues from successful providers only + issues.Count.ShouldBe(2); + issues.ShouldContain(x => x.MessageText == "Success1"); + issues.ShouldContain(x => x.MessageText == "Success2"); + } } } \ No newline at end of file diff --git a/src/Cake.Issues.Tests/IssuesFixture.cs b/src/Cake.Issues.Tests/IssuesFixture.cs index 53dae27f4..f71ef2413 100644 --- a/src/Cake.Issues.Tests/IssuesFixture.cs +++ b/src/Cake.Issues.Tests/IssuesFixture.cs @@ -7,7 +7,7 @@ public class IssuesFixture public IssuesFixture() { this.Log = new FakeLog { Verbosity = Verbosity.Normal }; - this.IssueProviders = [new(this.Log)]; + this.IssueProviders = [new FakeIssueProvider(this.Log)]; this.Settings = new ReadIssuesSettings( new Core.IO.DirectoryPath(@"c:\Source\Cake.Issues")); @@ -15,7 +15,7 @@ public IssuesFixture() public FakeLog Log { get; init; } - public IList IssueProviders { get; init; } + public IList IssueProviders { get; init; } public ReadIssuesSettings Settings { get; init; } diff --git a/src/Cake.Issues/IssuesReader.cs b/src/Cake.Issues/IssuesReader.cs index 714c2f09a..00b5b5f47 100644 --- a/src/Cake.Issues/IssuesReader.cs +++ b/src/Cake.Issues/IssuesReader.cs @@ -1,7 +1,10 @@ namespace Cake.Issues; +using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; +using System.Threading.Tasks; using Cake.Core.Diagnostics; /// @@ -41,12 +44,17 @@ public IssuesReader( /// List of issues. public IEnumerable ReadIssues() { - // Initialize issue providers and read issues. - var issues = new List(); - foreach (var issueProvider in this.issueProviders) + var stopwatch = Stopwatch.StartNew(); + + // Use thread-safe collection for collecting results + var allIssues = new ConcurrentBag(); + + // Process providers in parallel + _ = Parallel.ForEach(this.issueProviders, issueProvider => { var providerName = issueProvider.GetType().Name; this.log.Verbose("Initialize issue provider {0}...", providerName); + if (issueProvider.Initialize(this.settings)) { this.log.Verbose("Reading issues from {0}...", providerName); @@ -57,6 +65,7 @@ public IEnumerable ReadIssues() currentIssues.Count, providerName); + // Post-process issues - this is thread-safe as each provider gets its own issues currentIssues.ForEach(x => { x.Run = this.settings.Run; @@ -67,14 +76,27 @@ public IEnumerable ReadIssues() } }); - issues.AddRange(currentIssues); + // Add to thread-safe collection + foreach (var issue in currentIssues) + { + allIssues.Add(issue); + } } else { this.log.Warning("Error initializing issue provider {0}.", providerName); } - } + }); + + stopwatch.Stop(); + + var issuesList = allIssues.ToList(); + this.log.Verbose( + "Reading {0} issues from {1} providers took {2} ms", + issuesList.Count, + this.issueProviders.Count, + stopwatch.ElapsedMilliseconds); - return issues; + return issuesList; } } From ecc5c44496d18e14fae81b68719a757c077780b3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 5 Jul 2025 12:36:12 +0000 Subject: [PATCH 2/8] Add comprehensive performance testing and validation for parallel processing Co-authored-by: pascalberger <2190718+pascalberger@users.noreply.github.com> --- .../FakeSlowIssueProvider.cs | 40 ++++++++++++++++ src/Cake.Issues.Tests/IssueReaderTests.cs | 46 +++++++++++++++++++ 2 files changed, 86 insertions(+) create mode 100644 src/Cake.Issues.Testing/FakeSlowIssueProvider.cs diff --git a/src/Cake.Issues.Testing/FakeSlowIssueProvider.cs b/src/Cake.Issues.Testing/FakeSlowIssueProvider.cs new file mode 100644 index 000000000..8d6f2692e --- /dev/null +++ b/src/Cake.Issues.Testing/FakeSlowIssueProvider.cs @@ -0,0 +1,40 @@ +namespace Cake.Issues.Testing; + +using System.Collections.Generic; +using System.Threading; +using Cake.Core.Diagnostics; + +/// +/// Implementation of a that simulates slow processing for performance testing. +/// +public class FakeSlowIssueProvider : BaseIssueProvider +{ + private readonly List issues = []; + private readonly int delayMs; + + /// + /// Initializes a new instance of the class. + /// + /// The Cake log instance. + /// Issues which should be returned by the issue provider. + /// Simulated processing delay in milliseconds. + public FakeSlowIssueProvider(ICakeLog log, IEnumerable issues, int delayMs) + : base(log) + { + issues.NotNull(); + + this.issues.AddRange(issues); + this.delayMs = delayMs; + } + + /// + public override string ProviderName => "Fake Slow Issue Provider"; + + /// + protected override IEnumerable InternalReadIssues() + { + // Simulate slow processing + Thread.Sleep(this.delayMs); + return this.issues; + } +} \ No newline at end of file diff --git a/src/Cake.Issues.Tests/IssueReaderTests.cs b/src/Cake.Issues.Tests/IssueReaderTests.cs index 17efb274f..44d501f73 100644 --- a/src/Cake.Issues.Tests/IssueReaderTests.cs +++ b/src/Cake.Issues.Tests/IssueReaderTests.cs @@ -435,5 +435,51 @@ public void Should_Handle_Provider_Initialization_Failures_Concurrently() issues.ShouldContain(x => x.MessageText == "Success1"); issues.ShouldContain(x => x.MessageText == "Success2"); } + + [Fact] + public void Should_Demonstrate_Parallel_Processing_Benefits_With_Simulated_Delays() + { + // Given - Create providers that simulate processing delays + const int providerCount = 5; + const int delayPerProviderMs = 50; // Simulate 50ms delay per provider + var fixture = new IssuesFixture(); + fixture.IssueProviders.Clear(); + + for (var i = 0; i < providerCount; i++) + { + var issue = IssueBuilder + .NewIssue($"SlowIssue{i}", $"SlowProviderType{i}", $"SlowProviderName{i}") + .InFile($@"src\SlowFile{i}.cs", i + 1) + .OfRule($"SlowRule{i}") + .WithPriority(IssuePriority.Warning) + .Create(); + fixture.IssueProviders.Add(new FakeSlowIssueProvider(fixture.Log, [issue], delayPerProviderMs)); + } + + // When + var stopwatch = System.Diagnostics.Stopwatch.StartNew(); + var issues = fixture.ReadIssues().ToList(); + stopwatch.Stop(); + + // Then + issues.Count.ShouldBe(providerCount); + + // With parallel processing, total time should be significantly less than + // sum of all delays (providerCount * delayPerProviderMs) + var expectedSequentialTime = providerCount * delayPerProviderMs; + var actualTime = stopwatch.ElapsedMilliseconds; + + // Allow for some overhead but expect significant improvement + var maxExpectedParallelTime = expectedSequentialTime * 0.4; // Should be much faster than 40% of sequential time + + System.Console.WriteLine($"Sequential time would be ~{expectedSequentialTime}ms, parallel time was {actualTime}ms"); + + // This assertion may be flaky in CI environments, so we'll use a generous threshold + actualTime.ShouldBeLessThan(expectedSequentialTime); + + // Verify all issues have correct properties set + // The Run property should be set to the same value from settings (even if null) + issues.ShouldAllBe(x => x.Run == fixture.Settings.Run); + } } } \ No newline at end of file From 64d7d1ea82bd252132f137c9c1d91080bf52773b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 5 Jul 2025 15:28:53 +0000 Subject: [PATCH 3/8] Fix code formatting Co-authored-by: pascalberger <2190718+pascalberger@users.noreply.github.com> --- src/Cake.Issues.Tests/IssueReaderTests.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/Cake.Issues.Tests/IssueReaderTests.cs b/src/Cake.Issues.Tests/IssueReaderTests.cs index 44d501f73..8b02ba2c3 100644 --- a/src/Cake.Issues.Tests/IssueReaderTests.cs +++ b/src/Cake.Issues.Tests/IssueReaderTests.cs @@ -417,7 +417,6 @@ public void Should_Handle_Provider_Initialization_Failures_Concurrently() // Create a failing provider by passing null settings later var failingProvider = new FakeFailingIssueProvider(fixture.Log); fixture.IssueProviders.Add(failingProvider); - fixture.IssueProviders.Add(new FakeIssueProvider(fixture.Log, [ IssueBuilder.NewIssue("Success2", "ProviderType2", "ProviderName2") .InFile(@"src\File2.cs", 2) @@ -463,20 +462,19 @@ public void Should_Demonstrate_Parallel_Processing_Benefits_With_Simulated_Delay // Then issues.Count.ShouldBe(providerCount); - + // With parallel processing, total time should be significantly less than // sum of all delays (providerCount * delayPerProviderMs) var expectedSequentialTime = providerCount * delayPerProviderMs; var actualTime = stopwatch.ElapsedMilliseconds; - + // Allow for some overhead but expect significant improvement var maxExpectedParallelTime = expectedSequentialTime * 0.4; // Should be much faster than 40% of sequential time - + System.Console.WriteLine($"Sequential time would be ~{expectedSequentialTime}ms, parallel time was {actualTime}ms"); - + // This assertion may be flaky in CI environments, so we'll use a generous threshold actualTime.ShouldBeLessThan(expectedSequentialTime); - // Verify all issues have correct properties set // The Run property should be set to the same value from settings (even if null) issues.ShouldAllBe(x => x.Run == fixture.Settings.Run); From 2200c52249bd840fb2c0a3f9bb3beab2d2680a4c Mon Sep 17 00:00:00 2001 From: Pascal Berger Date: Sun, 6 Jul 2025 00:06:33 +0200 Subject: [PATCH 4/8] Maintain order of issues --- src/Cake.Issues/IssuesReader.cs | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/Cake.Issues/IssuesReader.cs b/src/Cake.Issues/IssuesReader.cs index 00b5b5f47..e5b2094d8 100644 --- a/src/Cake.Issues/IssuesReader.cs +++ b/src/Cake.Issues/IssuesReader.cs @@ -1,6 +1,5 @@ namespace Cake.Issues; -using System.Collections.Concurrent; using System.Collections.Generic; using System.Diagnostics; using System.Linq; @@ -46,12 +45,12 @@ public IEnumerable ReadIssues() { var stopwatch = Stopwatch.StartNew(); - // Use thread-safe collection for collecting results - var allIssues = new ConcurrentBag(); + var results = new List[this.issueProviders.Count]; // Process providers in parallel - _ = Parallel.ForEach(this.issueProviders, issueProvider => + _ = Parallel.For(0, this.issueProviders.Count, i => { + var issueProvider = this.issueProviders[i]; var providerName = issueProvider.GetType().Name; this.log.Verbose("Initialize issue provider {0}...", providerName); @@ -76,21 +75,18 @@ public IEnumerable ReadIssues() } }); - // Add to thread-safe collection - foreach (var issue in currentIssues) - { - allIssues.Add(issue); - } + results[i] = currentIssues; } else { this.log.Warning("Error initializing issue provider {0}.", providerName); + results[i] = []; } }); stopwatch.Stop(); - var issuesList = allIssues.ToList(); + var issuesList = results.SelectMany(r => r).ToList(); this.log.Verbose( "Reading {0} issues from {1} providers took {2} ms", issuesList.Count, From 089a2bbcfb6c61445e27b5c4a5a3e0f27aac56c1 Mon Sep 17 00:00:00 2001 From: Pascal Berger Date: Sun, 6 Jul 2025 00:30:28 +0200 Subject: [PATCH 5/8] Improve test --- src/Cake.Issues.Tests/IssueReaderTests.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/Cake.Issues.Tests/IssueReaderTests.cs b/src/Cake.Issues.Tests/IssueReaderTests.cs index 8b02ba2c3..5855fa909 100644 --- a/src/Cake.Issues.Tests/IssueReaderTests.cs +++ b/src/Cake.Issues.Tests/IssueReaderTests.cs @@ -468,16 +468,13 @@ public void Should_Demonstrate_Parallel_Processing_Benefits_With_Simulated_Delay var expectedSequentialTime = providerCount * delayPerProviderMs; var actualTime = stopwatch.ElapsedMilliseconds; - // Allow for some overhead but expect significant improvement - var maxExpectedParallelTime = expectedSequentialTime * 0.4; // Should be much faster than 40% of sequential time - - System.Console.WriteLine($"Sequential time would be ~{expectedSequentialTime}ms, parallel time was {actualTime}ms"); + Console.WriteLine($"Sequential time would be ~{expectedSequentialTime}ms, parallel time was {actualTime}ms"); + actualTime.ShouldBeLessThan(expectedSequentialTime, "Parallel reading of issue providers is slower than sequential processing would be"); // This assertion may be flaky in CI environments, so we'll use a generous threshold - actualTime.ShouldBeLessThan(expectedSequentialTime); - // Verify all issues have correct properties set - // The Run property should be set to the same value from settings (even if null) - issues.ShouldAllBe(x => x.Run == fixture.Settings.Run); + // Should be much faster than 40% of sequential time + var maxExpectedParallelTime = expectedSequentialTime * 0.4; + Convert.ToDouble(actualTime).ShouldBeLessThan(maxExpectedParallelTime, "Parallel reading of issue providers is more than 40% of time it took for sequential processing"); } } } \ No newline at end of file From 06e96033f2416f05aa063ae62ea8c451894655b3 Mon Sep 17 00:00:00 2001 From: Pascal Berger Date: Sun, 6 Jul 2025 01:36:10 +0200 Subject: [PATCH 6/8] Cleanup --- src/Cake.Issues/IssuesReader.cs | 68 +++++++++++++++++---------------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/src/Cake.Issues/IssuesReader.cs b/src/Cake.Issues/IssuesReader.cs index e5b2094d8..e71a2dfd5 100644 --- a/src/Cake.Issues/IssuesReader.cs +++ b/src/Cake.Issues/IssuesReader.cs @@ -50,38 +50,7 @@ public IEnumerable ReadIssues() // Process providers in parallel _ = Parallel.For(0, this.issueProviders.Count, i => { - var issueProvider = this.issueProviders[i]; - var providerName = issueProvider.GetType().Name; - this.log.Verbose("Initialize issue provider {0}...", providerName); - - if (issueProvider.Initialize(this.settings)) - { - this.log.Verbose("Reading issues from {0}...", providerName); - var currentIssues = issueProvider.ReadIssues().ToList(); - - this.log.Verbose( - "Found {0} issues using issue provider {1}...", - currentIssues.Count, - providerName); - - // Post-process issues - this is thread-safe as each provider gets its own issues - currentIssues.ForEach(x => - { - x.Run = this.settings.Run; - - if (this.settings.FileLinkSettings != null) - { - x.FileLink = this.settings.FileLinkSettings.GetFileLink(x); - } - }); - - results[i] = currentIssues; - } - else - { - this.log.Warning("Error initializing issue provider {0}.", providerName); - results[i] = []; - } + results[i] = this.ReadIssuesFromProvider(this.issueProviders[i]); }); stopwatch.Stop(); @@ -95,4 +64,39 @@ public IEnumerable ReadIssues() return issuesList; } + + private List ReadIssuesFromProvider(IIssueProvider issueProvider) + { + var providerName = issueProvider.GetType().Name; + this.log.Verbose("Initialize issue provider {0}...", providerName); + + if (issueProvider.Initialize(this.settings)) + { + this.log.Verbose("Reading issues from {0}...", providerName); + var currentIssues = issueProvider.ReadIssues().ToList(); + + this.log.Verbose( + "Found {0} issues using issue provider {1}...", + currentIssues.Count, + providerName); + + // Post-process issues - this is thread-safe as each provider gets its own issues + currentIssues.ForEach(x => + { + x.Run = this.settings.Run; + + if (this.settings.FileLinkSettings != null) + { + x.FileLink = this.settings.FileLinkSettings.GetFileLink(x); + } + }); + return currentIssues; + } + else + { + this.log.Warning("Error initializing issue provider {0}.", providerName); + return []; + } + + } } From e704f2d4cacc2a94ceb848864097bb995d31aa27 Mon Sep 17 00:00:00 2001 From: Pascal Berger Date: Sun, 6 Jul 2025 01:36:21 +0200 Subject: [PATCH 7/8] Don't test for improvement --- src/Cake.Issues.Tests/IssueReaderTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Cake.Issues.Tests/IssueReaderTests.cs b/src/Cake.Issues.Tests/IssueReaderTests.cs index 5855fa909..055391ddf 100644 --- a/src/Cake.Issues.Tests/IssueReaderTests.cs +++ b/src/Cake.Issues.Tests/IssueReaderTests.cs @@ -473,8 +473,8 @@ public void Should_Demonstrate_Parallel_Processing_Benefits_With_Simulated_Delay // This assertion may be flaky in CI environments, so we'll use a generous threshold // Should be much faster than 40% of sequential time - var maxExpectedParallelTime = expectedSequentialTime * 0.4; - Convert.ToDouble(actualTime).ShouldBeLessThan(maxExpectedParallelTime, "Parallel reading of issue providers is more than 40% of time it took for sequential processing"); + //var maxExpectedParallelTime = expectedSequentialTime * 0.4; + //Convert.ToDouble(actualTime).ShouldBeLessThan(maxExpectedParallelTime, "Parallel reading of issue providers is more than 40% of time it took for sequential processing"); } } } \ No newline at end of file From 013d068edf1065fd01994f6ef2b531b14c4b1676 Mon Sep 17 00:00:00 2001 From: Pascal Berger Date: Sat, 12 Jul 2025 20:35:15 +0200 Subject: [PATCH 8/8] Fix linting issue --- src/Cake.Issues/IssuesReader.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Cake.Issues/IssuesReader.cs b/src/Cake.Issues/IssuesReader.cs index e71a2dfd5..8b72d509e 100644 --- a/src/Cake.Issues/IssuesReader.cs +++ b/src/Cake.Issues/IssuesReader.cs @@ -97,6 +97,5 @@ private List ReadIssuesFromProvider(IIssueProvider issueProvider) this.log.Warning("Error initializing issue provider {0}.", providerName); return []; } - } }