Address some low-hanging fruit to use newer/better .NET features #11448
Conversation
This is based only on code inspection / static analysis / searching around. It is not based on profiling.
|
cc: @MihaZupan, fyi on the SearchValues usage |
There was a problem hiding this comment.
PR Overview
This pull request updates several components and unit tests to leverage newer and more efficient .NET features, simplifying asynchronous operations and collection enumeration while improving code readability. Key changes include:
- Replacing manual string concatenation with string.Create under NET.
- Using WaitAsync and AsTask for asynchronous operations in place of older patterns.
- Switching from manual enumerator conversions (GetEnumerator().ToArray()) to direct ToArray() calls.
- Refactoring thread naming and static field declarations to use string interpolation and readonly modifiers.
- Updating unit tests to iterate over character ranges with a for loop and checking invalid characters using explicit conditions.
Changes
| File | Description |
|---|---|
| src/Build/BackEnd/Components/Communications/NodeProviderOutOfProcBase.cs | Improved string creation and wait logic based on target .NET version. |
| src/Build.UnitTests/Globbing/MSBuildGlob_Tests.cs | Updated iteration over invalid path characters and using explicit arrays for file name chars. |
| src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupIntrinsicTask.cs | Switched from IndexOf to Contains for clarity. |
| src/Build/BackEnd/Components/BuildRequestEngine/BuildRequestEngine.cs | Changed thread identifier usage to Environment.CurrentManagedThreadId. |
| src/Build.UnitTests/BackEnd/CacheAggregator_Tests.cs | Simplified collection enumerations using ToArray(). |
| src/Build/BackEnd/Client/MSBuildClientPacketPump.cs | Updated asynchronous read operations to use AsTask. |
| src/Build.UnitTests/FileUtilitiesRegex_Tests.cs | Removed redundant regex match check. |
| src/Build/BackEnd/Components/FileAccesses/FileAccessManager.cs | Updated string interpolation for file access prefix. |
| src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs | Removed unnecessary ContinueWith calls during asynchronous plugin initialization. |
| src/Build/BackEnd/BuildManager/BuildRequestEngine/BuildRequestEngine.cs | Adjusted logging to use new thread id retrieval. |
| src/Build.UnitTests/BackEnd/ConfigCache_Tests.cs, CacheAggregator.cs, IsolateProjects_Tests.cs, ResultsCache_Tests.cs, ResultCacheBasedBuilds_Tests.cs | Consolidated and simplified collection enumeration calls using ToArray(). |
| src/Build/BackEnd/Components/Communications/NodeProviderInProc.cs, NodeEndpointInProc.cs, TranslatorExtensions.cs, LoggingService.cs | Refactored thread naming, static fields, and lazy initialization to newer .NET constructs. |
Copilot reviewed 191 out of 191 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/Build.UnitTests/Globbing/MSBuildGlob_Tests.cs:74
- Verify that iterating from 0 to 127 covers all invalid path characters as provided by FileUtilities.InvalidPathChars; if the set includes characters outside this range, adjust the loop boundaries accordingly.
for (int i = 0; i < 128; i++)
src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs:118
- Ensure that exceptions thrown during asynchronous plugin initialization remain intentionally unobserved; if not, consider adding proper error handling to capture and log any initialization failures.
_ = GetProjectCachePluginAsync(projectCacheDescriptor, projectGraph, buildRequestConfiguration: null, requestedTargets, cancellationToken);
Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more
| @@ -2434,7 +2434,7 @@ private void WriteNodeUtilizationGraphLine(ILoggingService loggingService, Build | |||
|
|
|||
| bool haveNonIdleNode = false; | |||
| StringBuilder stringBuilder = new StringBuilder(64); | |||
| stringBuilder.AppendFormat("{0}: ", previousEventTime.Ticks); | |||
| stringBuilder.Append(previousEventTime.Ticks).Append(": "); | |||
There was a problem hiding this comment.
This one surprises me a bit, what should I learn from this being preferred?
There was a problem hiding this comment.
Three things:
- AppendFormat needs to parse the format string
- AppendFormat takes object arguments, so Ticks is getting boxed into an object
- On netfx, that boxed object will then have ToString called on it (on core it can do better).
The latter doesn't need to do any parsing, and Append has overloads for all the primitives, so it doesn't need to box. And on core, the formatting of the primitive is then done directly into the buffer.
If this were only targeting core, it could have also been:
stringBuilder.Append($"{previousEventTime.Ticks}: ");which will take advantage of the interpolated string handler support in a dedicated Append overload.
https://devblogs.microsoft.com/dotnet/string-interpolation-in-c-10-and-net-6/
| #if NET | ||
| private static readonly SearchValues<char> s_charsToCleanse = SearchValues.Create( | ||
| #else | ||
| private static readonly char[] s_charsToCleanse = ( |
There was a problem hiding this comment.
oh man putting the dangling paren in this side of the #if is a great readability improvement over what I had been trying
| [GeneratedRegex(XmlDeclarationPattern)] | ||
| private static partial Regex XmlDeclarationRegex { get; } | ||
| #else | ||
| private static Regex XmlDeclarationRegex => s_xmlDeclarationRegex ??= new Regex(XmlDeclarationPattern); |
There was a problem hiding this comment.
Sorry @SimaTian #11228 (comment)
There was a problem hiding this comment.
While I was responsible for introducing Lazy, it's often not the right answer, in particular because you can often get the laziness you need without needing to create the Lazy itself.
In this case, we don't rely on the object identity of the Regex, so we don't need to worry about race conditions to publish an instance; worse case is two concurrent uses will end up using different instances.
My assumption is it'll be relatively rare for there to be such concurrent use on initialization (e.g. two threads in the same process trying to use this property for the first time within the same, say, millisecond), in which case we can avoid paying for the Lazy itself.
| } | ||
| processId ??= | ||
| #if NET | ||
| Environment.ProcessId; |
There was a problem hiding this comment.
#11274 is unifying this with a helper method, can we stick with that?
| return Version.Parse( | ||
| #if NET | ||
| version.AsSpan(1)); | ||
| #else | ||
| version.Substring(1)); | ||
| #endif |
There was a problem hiding this comment.
I sure hope this hyper-specific net35 task isn't used in a modern build 🙃
| internal static bool IsHexDigit(char candidate) | ||
| { | ||
| return char.IsDigit(candidate) || IsHexAlphabetic(candidate); | ||
| return char.IsDigit(candidate) || ((uint)((candidate | 0x20) - 'a') <= 'f' - 'a'); | ||
| // TODO: Is the intent here really to include Unicode digits, or could this be char.IsAsciiHexChar? |
There was a problem hiding this comment.
Looks like all we say in docs is
Operands must evaluate to a decimal or hexadecimal number or a four-part dotted version. Hexadecimal numbers must begin with 0x.
But does any programming language accept 0xA٦? char.IsAsciiHexChar sounds like what we would have meant.
I suppose, being me, I should say we put it behind a changewave. I'll do that.
| @@ -24,8 +26,18 @@ internal static bool IsSimpleStringChar(char candidate) | |||
|
|
|||
| internal static bool IsHexDigit(char candidate) | |||
| { | |||
| return char.IsDigit(candidate) || ((uint)((candidate | 0x20) - 'a') <= 'f' - 'a'); | |||
| // TODO: Is the intent here really to include Unicode digits, or could this be char.IsAsciiHexChar? | |||
| if (ChangeWaves.AreFeaturesEnabled(ChangeWaves.Wave17_14)) | |||
There was a problem hiding this comment.
Is this going to add runtime overhead on every character? I don't want my change to make things worse :) (I'd rather just remove my TODO and have you forget I asked ;-)
This is based only on code inspection / static analysis / searching around. It is not based on profiling. I do not know how much of an impact any of these have; we could choose to cherry-pick from this if there are particular areas that demonstrate gains.