Use hashing for targets list comparison and enable scheduler tests (#11870)

YuliiaKovalova · web-flow · commit 585b8739176c · 2025-05-29T18:18:14.000+02:00
diff --git a/src/Build.UnitTests/BackEnd/Scheduler_Tests.cs b/src/Build.UnitTests/BackEnd/Scheduler_Tests.cs
@@ -101,7 +101,7 @@ public void Dispose()
         /// <summary>
         /// Verify that when a single request is submitted, we get a request assigned back out.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestSimpleRequest()
         {
             CreateConfiguration(1, "foo.proj");
@@ -117,7 +117,7 @@ public void TestSimpleRequest()
         /// <summary>
         /// Verify that when we submit a request and we already have results, we get the results back.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestSimpleRequestWithCachedResultsSuccess()
         {
             CreateConfiguration(1, "foo.proj");
@@ -141,7 +141,7 @@ public void TestSimpleRequestWithCachedResultsSuccess()
         /// <summary>
         /// Verify that when we submit a request with failing results, we get the results back.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestSimpleRequestWithCachedResultsFail()
         {
             CreateConfiguration(1, "foo.proj");
@@ -165,7 +165,7 @@ public void TestSimpleRequestWithCachedResultsFail()
         /// <summary>
         /// Verify that when we submit a child request with results cached, we get those results back.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestChildRequest()
         {
             CreateConfiguration(1, "foo.proj");
@@ -195,7 +195,7 @@ public void TestChildRequest()
         /// <summary>
         /// Verify that when multiple requests are submitted, the first one in is the first one out.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestMultipleRequests()
         {
             CreateConfiguration(1, "foo.proj");
@@ -213,7 +213,7 @@ public void TestMultipleRequests()
         /// <summary>
         /// Verify that when multiple requests are submitted with results cached, we get the results back.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestMultipleRequestsWithSomeResults()
         {
             CreateConfiguration(1, "foo.proj");
@@ -235,7 +235,7 @@ public void TestMultipleRequestsWithSomeResults()
         /// <summary>
         /// Verify that when multiple requests are submitted with results cached, we get the results back.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestMultipleRequestsWithAllResults()
         {
             CreateConfiguration(1, "foo.proj");
@@ -266,7 +266,7 @@ public void TestMultipleRequestsWithAllResults()
         /// Verify that if the affinity of one of the requests is out-of-proc, we create an out-of-proc node (but only one)
         /// even if the max node count = 1.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestOutOfProcNodeCreatedWhenAffinityIsOutOfProc()
         {
             CreateConfiguration(1, "foo.proj");
@@ -288,7 +288,7 @@ public void TestOutOfProcNodeCreatedWhenAffinityIsOutOfProc()
         /// Verify that if the affinity of our requests is out-of-proc, that many out-of-proc nodes will
         /// be made (assuming it does not exceed MaxNodeCount)
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestOutOfProcNodesCreatedWhenAffinityIsOutOfProc()
         {
             _host.BuildParameters.MaxNodeCount = 4;
@@ -313,7 +313,7 @@ public void TestOutOfProcNodesCreatedWhenAffinityIsOutOfProc()
         /// we still won't create any new nodes if they're all for the same configuration --
         /// they'd end up all being assigned to the same node.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestNoNewNodesCreatedForMultipleRequestsWithSameConfiguration()
         {
             _host.BuildParameters.MaxNodeCount = 3;
@@ -336,7 +336,7 @@ public void TestNoNewNodesCreatedForMultipleRequestsWithSameConfiguration()
         /// Verify that if the affinity of our requests is "any", we will not create more than
         /// MaxNodeCount nodes (1 IP node + MaxNodeCount - 1 OOP nodes)
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestMaxNodeCountNotExceededWithRequestsOfAffinityAny()
         {
             _host.BuildParameters.MaxNodeCount = 3;
@@ -366,7 +366,7 @@ public void TestMaxNodeCountNotExceededWithRequestsOfAffinityAny()
         /// node will service an Any request instead of an inproc request, leaving only one non-inproc request for the second round
         /// of node creation.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void VerifyRequestOrderingDoesNotAffectNodeCreationCountWithInProcAndAnyRequests()
         {
             // Since we're creating our own BuildManager, we need to make sure that the default
@@ -414,7 +414,7 @@ public void VerifyRequestOrderingDoesNotAffectNodeCreationCountWithInProcAndAnyR
         /// Verify that if the affinity of our requests is out-of-proc, we will create as many as
         /// MaxNodeCount out-of-proc nodes
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestMaxNodeCountOOPNodesCreatedForOOPAffinitizedRequests()
         {
             _host.BuildParameters.MaxNodeCount = 3;
@@ -444,7 +444,7 @@ public void TestMaxNodeCountOOPNodesCreatedForOOPAffinitizedRequests()
         /// is less than MaxNodeCount, that we only create MaxNodeCount - 1 OOP nodes (for a total of MaxNodeCount
         /// nodes, when the inproc node is included)
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestMaxNodeCountNodesNotExceededWithSomeOOPRequests1()
         {
             _host.BuildParameters.MaxNodeCount = 3;
@@ -474,7 +474,7 @@ public void TestMaxNodeCountNodesNotExceededWithSomeOOPRequests1()
         /// is less than MaxNodeCount, that we only create MaxNodeCount - 1 OOP nodes (for a total of MaxNodeCount
         /// nodes, when the inproc node is included)
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestMaxNodeCountNodesNotExceededWithSomeOOPRequests2()
         {
             _host.BuildParameters.MaxNodeCount = 3;
@@ -511,7 +511,7 @@ public void SchedulerShouldHonorDisableInprocNode()
         /// Make sure that traversal projects are marked with an affinity of "InProc", which means that
         /// even if multiple are available, we should still only have the single inproc node.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestTraversalAffinityIsInProc()
         {
             _host.BuildParameters.MaxNodeCount = 3;
@@ -560,7 +560,7 @@ public void TestProxyAffinityIsInProc()
         /// With something approximating the BuildManager's build loop, make sure that we don't end up
         /// trying to create more nodes than we can actually support.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void VerifyNoOverCreationOfNodesWithBuildLoop()
         {
             // Since we're creating our own BuildManager, we need to make sure that the default
@@ -615,7 +615,7 @@ public void BuildResultNotPlacedInCurrentCacheIfConfigExistsInOverrideCache()
         /// <summary>
         /// Verify that if we get two requests but one of them is a failure, we only get the failure result back.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestTwoRequestsWithFirstFailure()
         {
             CreateConfiguration(1, "foo.proj");
@@ -634,7 +634,7 @@ public void TestTwoRequestsWithFirstFailure()
         /// <summary>
         /// Verify that if we get two requests but one of them is a failure, we only get the failure result back.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestTwoRequestsWithSecondFailure()
         {
             CreateConfiguration(1, "foo.proj");
@@ -653,7 +653,7 @@ public void TestTwoRequestsWithSecondFailure()
         /// <summary>
         /// Verify that if we get three requests but one of them is a failure, we only get the failure result back.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestThreeRequestsWithOneFailure()
         {
             CreateConfiguration(1, "foo.proj");
@@ -673,7 +673,7 @@ public void TestThreeRequestsWithOneFailure()
         /// <summary>
         /// Verify that providing a result to the only outstanding request results in build complete.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestResult()
         {
             CreateConfiguration(1, "foo.proj");
@@ -697,7 +697,7 @@ public void TestResult()
         /// <summary>
         /// Tests that the detailed summary setting causes the summary to be produced.
         /// </summary>
-        [Fact(Skip = "https://github.com/dotnet/msbuild/issues/515")]
+        [Fact]
         public void TestDetailedSummary()
         {
             string contents = ObjectModelHelpers.CleanupFileContents(@"
diff --git a/src/Build/BackEnd/Components/Scheduler/Scheduler.cs b/src/Build/BackEnd/Components/Scheduler/Scheduler.cs
@@ -2239,49 +2239,55 @@ private bool RequestOrAnyItIsBlockedByCanBeServiced(SchedulableRequest request)
         }
 
         /// <summary>
-        /// Determines if we have a matching request somewhere, and if so, assigns the same request ID.  Otherwise
-        /// assigns a new request id.
+        /// Determines if we have a matching request somewhere, and if so, assigns the same request ID.
+        /// Otherwise assigns a new request id.
         /// </summary>
-        /// <remarks>
-        /// UNDONE: (Performance) This algorithm should be modified so we don't have to iterate over all of the
-        /// requests to find a matching one.  A HashSet with proper equality semantics and a good hash code for the BuildRequest
-        /// would speed this considerably, especially for large numbers of projects in a build.
-        /// </remarks>
         /// <param name="request">The request whose ID should be assigned</param>
         private void AssignGlobalRequestId(BuildRequest request)
         {
-            bool assignNewId = false;
-            if (request.GlobalRequestId == BuildRequest.InvalidGlobalRequestId && _schedulingData.GetRequestsAssignedToConfigurationCount(request.ConfigurationId) > 0)
+            // Quick exit if already assigned or if there are no requests for this configuration
+            if (request.GlobalRequestId != BuildRequest.InvalidGlobalRequestId
+                || _schedulingData.GetRequestsAssignedToConfigurationCount(request.ConfigurationId) == 0)
+            {
+                request.GlobalRequestId = _nextGlobalRequestId++;
+                return;
+            }
+
+            HashSet<string> requestTargetsSet = new(request.Targets, StringComparer.OrdinalIgnoreCase);
+
+            // Look for matching requests in the configuration
+            foreach (SchedulableRequest existingRequest in _schedulingData.GetRequestsAssignedToConfiguration(request.ConfigurationId))
             {
-                foreach (SchedulableRequest existingRequest in _schedulingData.GetRequestsAssignedToConfiguration(request.ConfigurationId))
+                if (TargetsMatch(requestTargetsSet, existingRequest.BuildRequest.Targets))
                 {
-                    if (existingRequest.BuildRequest.Targets.Count == request.Targets.Count)
-                    {
-                        List<string> leftTargets = new List<string>(existingRequest.BuildRequest.Targets);
-                        List<string> rightTargets = new List<string>(request.Targets);
+                    request.GlobalRequestId = existingRequest.BuildRequest.GlobalRequestId;
+                    return;
+                }
+            }
 
-                        leftTargets.Sort(StringComparer.OrdinalIgnoreCase);
-                        rightTargets.Sort(StringComparer.OrdinalIgnoreCase);
-                        for (int i = 0; i < leftTargets.Count; i++)
-                        {
-                            if (!leftTargets[i].Equals(rightTargets[i], StringComparison.OrdinalIgnoreCase))
-                            {
-                                assignNewId = true;
-                                break;
-                            }
-                        }
+            // No matching request found, assign a new ID
+            request.GlobalRequestId = _nextGlobalRequestId++;
+        }
 
-                        if (!assignNewId)
-                        {
-                            request.GlobalRequestId = existingRequest.BuildRequest.GlobalRequestId;
-                            return;
-                        }
-                    }
+        /// <summary>
+        /// Determines if two target collections contain the same targets, ignoring order and case.
+        /// </summary>
+        private bool TargetsMatch(HashSet<string> firstTargetsSet, List<string> secondTargetsList)
+        {
+            if (firstTargetsSet.Count != secondTargetsList.Count)
+            {
+                return false;
+            }
+
+            foreach (string target in secondTargetsList)
+            {
+                if (!firstTargetsSet.Contains(target))
+                {
+                    return false;
                 }
             }
 
-            request.GlobalRequestId = _nextGlobalRequestId;
-            _nextGlobalRequestId++;
+            return true;
         }
 
         /// <summary>
