Related to #8673
Context
When analyzing traces from VS Orchard evaluation I have noticed few places which are easy to optimize by using ImmutableArray instead of ImmutableList.
ImmutableArray is almost always better in patters loop {mutate by builder}; ToImmutable since it is struct, allocates less, and have faster enumerator.
Changes Made
There are many places where using ImmutableArray instead of ImmutableList would render better performance. I however limit changes only on places which sticks out from memory traces as non-negligable.
Testing
Local. PR gate.
Notes
Overall gain will be small, in percent's of overall allocations, but given relatively simple and safe changes, I think it is good tradeoff.
Fixes#8570 part 1
Context
UseTrustedSettings parameter for XslTransformation task is ignored when using dotnet build since the resolver is not setting.
Changes Made
Use an XslCompiledTransform.Transform overload that takes an XmlResolver parameter.
Add the info log message when the UseTrustedSettings is opted-in on the Task
Testing
Enable the earlier disabled test XslDocumentFunctionWorks() on net7.0
Context
When ProjectOptions.DirectoryCacheFactory was introduced only Project support was implemented. ProjectInstance ignores the field and while currently not a required functionality, it is a minor API gap.
Changes Made
Made ProjectInstance use DirectoryCacheFactory if passed in ProjectOptions.
Removed Project.GetDirectoryCacheForEvaluation and instead plumbed the factory all the way down to Evaluator.
Tweaked the unit test project to use Condition on Import rather than on ItemGroup to not expose differences between Project and ProjectInstance in the treatment of conditional items.
Testing
Existing updated unit test.
NuGet.Build.Tasks
From Version 6.7.0-preview.2.33 -> To Version 6.7.0-preview.2.41
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
* Remove MockTerminal in favor of internal fields on Terminal itself.
* Reference Verify library for acceptance testing.
* Convert existing tests to Verify.
Fixes#8673
Context
High memory allocations in CopyOnWritePropertyDictionary ImmutableDictionary was reported by partners.
Changes Made
Use CopyOnWritePropertyDictionary.ImportProperties in obvious places as oppose to CopyOnWritePropertyDictionary.Set in loop.
Testing
Unit tests. Local. Perf measure.
Notes
Fixes#1596
Changes Made
SetConsoleUI now calls into a helper which sets the encoding to support non-en languages and checks if an environment variable exists to change the language to.
Testing
Setting DOTNET_CLI_UI_LANGUAGE=ja now changes msbuild correctly:
image
Doing a complicated build (aka building MSBuild) to use multiple threads shows other threads seem to use the same UI culture:
image
See that chcp remains the same after execution:
image
(Was set to 65001 temporarily but back to the original page before execution.)
Notes
Much of this code is a port of this code: dotnet/sdk#29755
There are some details about the code here.
[!] In addition, it will introduce a breaking change for msbuild just like the SDK.
The break is documented here for the sdk: dotnet/docs#34250
Context
If DOTNET_SYSTEM_GLOBALIZATION_INVARIANT is enabled and console codepage non-ANSI MSBuild will crash on an attempt to create CultureInfo:
Unhandled exception. System.Globalization.CultureNotFoundException: Only the invariant culture is supported in globalization-invariant mode. See https://aka.ms/GlobalizationInvariantMode for more information. (Parameter 'name')
en-US is an invalid culture identifier.
Changes Made
Keep the current thread culture the same if CurrentUICulture is Invariant.
Now that #8688 is in, we no longer get value from the SDK pre-cache. The cost of reading the cache is >50 ms per project, which typically makes it the most expensive part of RAR.
Trace from MSBuild command line build:
Fixes AB#1811627 (@davkean will likely file a GH issue)
Context
ProjectCollection uses a ReaderWriterLockSlim but it never takes it just for reading with EnterReadLock. Instead it uses EnterUpgradeableReadLock, which effectively provides mutual exclusion when reading data and results in unnecessary contention. The reason cited in comments revolves around reentrancy but calling EnterReadLock while already holding the write lock is perfectly legal with LockRecursionPolicy.SupportsRecursion so there's no need to mutually exclude readers.
Changes Made
Made ProjectCollection use plain read lock instead of the upgradeable one.
Simplified the IDisposable holder structs by removing the unneeded interlocked operation.
Testing
Existing unit tests.
Notes
This is a safer version of #8728, which attempts to optimize concurrency in this class even further at the expense of readability. In particular, many readers could be converted to volatile reads to eliminate the possibility of contention with writers, some of which may be long-running. Since the reader-writer contention should not happen in VS scenarios, that PR was closed due to unfavorable risk/benefit and left just for future reference.
NuGet.Build.Tasks
From Version 6.7.0-preview.1.20 -> To Version 6.7.0-preview.2.33
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Fixes#8684Fixes#8273
Context
After #8275, we delete any destination file as part of the Copy task if we determine that we really should copy onto it. Unfortunately, if we try to copy a file onto itself, we delete it before we can copy onto itself, which just means it's gone. Fortunately, we have a check earlier that ensures that we skip any copy operation from a location to the same location. Unfortunately, it's a direct string comparison that doesn't evaluate to full paths first, so it misses slightly more complicated examples.
Changes Made
Take into account full paths
Testing
Unit tests + manual test that it doesn't delete the file anymore
Notes
This implementation tries to remove now-unnecessary full path derivations downstream, hence some added complexity, but it still means extra computation on the happy path if we end up creating a hard/symbolic link. An alternate direction eliminating any full path derivations on the happy path would save about 4% of Copy's execution time, per a quick perf test. (With how many samples I used, "no change" is within a standard deviation.)
Fixes#8634
Context
For SDK/workload references, the SDK is currently already passing relevant metadata such as AssemblyVersion and PublicKeyToken, so there is no need for RAR to open these files and parse their .NET metadata tables to get this information. Also, barring corrupted installs, these files are guaranteed to exist on disk so there is no need for RAR to do any I/O on them (file-exists, get-time-stamp, ..)
Changes Made
Tweaked RAR to trust the item metadata passed by the SDK. The feature is gated on the presence of the FrameworkReferenceName metadatum which is not documented and is used only by the relevant SDK tasks, to my best knowledge.
SDK does not specify the Culture component of assembly names so it's assumed to be neutral. This has passed all relevant validation so far. If non-neutral assemblies are ever used as references, we'll need to define a metadatum for SDK to set.
Testing
Existing and new unit tests.
Experimental insertion with an assert that assembly names calculated from item metadata are equivalent to those extracted from assembly files (verifies that the Culture=neutral assumption is correct).
Checked all assemblies specified in all FrameworkList.xml files shipped in the SDK (+ workloads) and verified that all of them are culture neutral.
Perf results
RAR micro-benchmark where the task is invoked with parameters corresponding to building a simple AspNetCore app:
13.13 ms -> 3.27 ms per invocation without StateFile cache.
15.03 ms -> 5.13 ms per invocation with StateFile cache.
RAR task duration as reported with /clp:PerformanceSummary when building a simple AspNetCore app with MSBuild server enabled:
20 ms -> 10 ms.
Notes
The change is behind a 17.8 changewave.
No changes have been made to per-project caching (via the StateFile parameter) to reduce scope. Changes to the per-project cache file will be done in another PR.
System.Security.Principal.Windows is inbox since net6.0
System.Net.Http is inbox since netcoreapp2.0
System.Reflection.Metadata is inbox since netcoreapp2.0
System.Threading.Tasks.Dataflow is inbox since netcoreapp2.0
Leave System.Net.Http package references which aren't needed as they underlying assembly is inbox on both .NETFramework and .NETCoreApp, to avoid component governance alerts about downloading (but not using) an old version.
By avoiding the dependencies, we minimize the dependency graph and with that the attack surface.
cc @MichaelSimons (removes netstandard1.x dependencies)
Context
The error and warning symbols may be rendered with different width on some terminals, resulting in misaligned output.
Changes Made
To make sure that the message text is always aligned we
Print the symbol.
Move back to the start of the line.
Move forward to the desired column.
Print the message text.
Testing
Windows terminal:
image
Windows cmd:
image
Fedora terminal:
image
Notes
I've also tried saving & restoring cursor position (VT100 functions 7 and 8) but that didn't fully work on Windows. The red X was still off.
Fixes#8704
Context
Errors during restore was not reported when using /tl (live logger) leaving user without actionable information.
Changes Made
When restore success without error or warn - same as before
When restore success with warnings - report success with warnins and list warnings bellow that report
When restore fails - report errors and warnings THEN report Restore failed in 1.5s and do not report Build summary
Update dependencies from https://github.com/dotnet/arcade build 20230421.7
Microsoft.DotNet.Arcade.Sdk , Microsoft.DotNet.XUnitExtensions
From Version 6.0.0-beta.23211.7 -> To Version 6.0.0-beta.23221.7
This file is no longer required by arcade. Per JanKrivanek:
Based on dotnet/arcade#1561 and following changes it seems it was replaced with PushToAzureDevOpsArtifacts
Moreover - it's not defined/called/imported anywhere in current Arcade code.
Context
Minor code cleanup in Microsoft.Common.tasks.
Changes Made
Removed duplicate UsingTask for the ResolveSDKReference task. The redundant ResolveSDKReference doesn't seem to create harm but is not useful.
Alphabetized the UsingTask elements. Retained the separate grouping of Roslyn tasks. Retained the blank lines around tasks that have different Runtimes and/or Conditions (i.e. GenerateResource, RegisterAssembly, and UnregisterAssembly). Ordering the UsingTask elements is intended to aid inspection and maintenance of tasks.
Testing
Tested on Windows 11 and macOS 12. Tested by running unit tests and by having this change in several development branches where msbuild has been run on project files.
Notes
This change is included in the implementation for #8613, which is PR #8614.
* Update dependencies from https://github.com/nuget/nuget.client build 6.7.0.19
NuGet.Build.Tasks
From Version 6.7.0-preview.1.16 -> To Version 6.7.0-preview.1.19
* Update dependencies from https://github.com/nuget/nuget.client build 6.7.0.20
NuGet.Build.Tasks
From Version 6.7.0-preview.1.16 -> To Version 6.7.0-preview.1.20
---------
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Fixes#8671
Context
New layout as agreed offline
specs (content remains same, we'll move here rar-core-scenarios from documentation/design folder)
|-----proposed (the secrets metadata and packages sourcing PRs would get routed here. In future - all in-progress work specs)
|-----archive (the rar-as-service.md from documentation/design will get moved here. In future - all not dismissed but not planned feature spec goes here)
Moving facilitated via git mv to preserve history and make diffs conscise
Jan Krivanek
dotnet bot
Roman Konecny
Jenny Bai
Ladi Prosek
dotnet-maestro[bot]
Rainer Sigwald
Noah Gilson
Mikhail Filippov
Forgind
Ryan Rozario
Jonathan Dodds