Remove TargetOutputItemsInstanceEnumeratorProxy
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 6h
- Merged PRs (30d)
- 139
Description
Similar to https://github.com/dotnet/msbuild/issues/6176 and the fix https://github.com/dotnet/msbuild/pull/7117 there is another place where we have an enumerator proxy to wrap items returned by targets.
We should investigate removing the enumerator proxy if it's not needed.
Here's the declaration:
https://github.com/dotnet/msbuild/blob/a59d7a533c9154e8aa99b823625e7eff199ddf1a/src/Build/BackEnd/Components/Logging/TargetLoggingContext.cs#L116-L168
Here's where `BinaryLogger` has a fast path to "unwrap" it:
https://github.com/dotnet/msbuild/blob/a59d7a533c9154e8aa99b823625e7eff199ddf1a/src/Build/Logging/BinaryLogger/BuildEventArgsWriter.cs#L744-L753
Also see related:
https://github.com/dotnet/msbuild/issues/6704
Right now the collections that store the properties and items during the build are a weird combination of mutable, copy on write and immutable. Since items aren't truly immutable, we have the bug above where we capture the live data structure for logging instead of capturing an immutable snapshot. If the core data structures are truly immutable, we can give an immutable snapshot to loggers without any copying whatsoever, this will also avoid allocations.
The reason it is a problem is when we get a reference to the live collection, logging is asynchronous. The new `BuildEventArgs` object is added to a queue, and on another thread asynchronously it's being either sent to the main node or placed into the other queue for the loggers. In the main node, when a serialized `BuildEventArgs` comes in, it is deserialized and placed into a queue to dispatch to loggers. The problem arises when we capture the items live collection to log during task execution:
https://source.dot.net/#Microsoft.Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/ItemGroupLoggingHelper.cs,5eb3bbf5a68d102b,references
By the time the `BuildEventArgs` is ready to be serialized and processed, the underlying items may have changed already (as the task may have mutated the items), so we don't log the exact values that were captured when the `TaskParameterEventArgs` was created. Had that items collection been a truly immutable snapshot, this wouldn't have happened. Also in that case there's no need to wrap or copy on write since the loggers wouldn't be able to change the items anyway.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.