-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Reapply "Fix TaskParameterTaskItem serialization perf" #12135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reapply "Fix TaskParameterTaskItem serialization perf" #12135
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR reapplies a previous fix to improve serialization performance of TaskParameterTaskItem
across AppDomain boundaries, replacing an AppDomain default check with a transparent proxy check and introducing ITranslatable
support.
- Swapped
AppDomain.CurrentDomain.IsDefaultAppDomain()
withRemotingServices.IsTransparentProxy()
in metadata import - Converted
TaskParameterTaskItem
to implementITranslatable
and updated translation methods - Adjusted project references to move
IConstrainedEqualityComparer
andMSBuildNameIgnoreCaseComparer
into the Framework folder
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/Utilities/Microsoft.Build.Utilities.csproj | Removed duplicated shared comparer references |
src/Tasks/Microsoft.Build.Tasks.csproj | Removed shared comparer references |
src/Shared/UnitTests/TaskParameter_Tests.cs | Added AppDomain-crossing unit tests |
src/Shared/TaskParameter.cs | Refactored translation logic and introduced TaskParameterTaskItem as ITranslatable |
src/MSBuildTaskHost/MSBuildTaskHost.csproj | Added framework comparers to TaskHost project |
src/MSBuild/MSBuild.csproj | Removed shared comparer references |
src/Framework/MSBuildNameIgnoreCaseComparer.cs | Updated comparer to use NativeMethods and EscapeHatches |
src/Framework/IConstrainedEqualityComparer.cs | Conditional variance for TASKHOST build |
src/Build/Microsoft.Build.csproj | Removed shared comparer references |
Comments suppressed due to low confidence (1)
src/Shared/TaskParameter.cs:294
- The method references
wrappedItems
before it is declared. Move the declarationITaskItem[] wrappedItems = (ITaskItem[])_wrappedParameter;
above this line sowrappedItems
is in scope when computinglength
.
int length = wrappedItems?.Length ?? 0;
I'm waiting for pipelines to be working, then I will fire up an experimental insertion. |
@ccastanedaucf please resolve the conflicts |
@SimaTian exp insertion is ok, ptal |
This one is waiting for me. Last week I failed insertion several times - not due to this PR but due to my general struggle with insertion process. |
Finally a working insertion. LGTM: |
Context
Reattempt of #11638
This broke VS tests due to a
ToList()
not triggering before crossing an AppDomain boundary:Error:
Although the above AppDomain check is used in multiple
ITaskItem
implementations, the out-of-proc TaskHost hits the case where the object was created in the default AppDomain during deserialization, but is copying to anITaskItem
in a separate AppDomain. Therefore, the condition never triggers.Changes Made
This replaces the check with
RemotingServices.IsTransparentProxy(_)
, which matches howProjectItemInstance.TaskItem
handles this scenario.Testing
Added UTs for
TaskParameterTaskItem.CopyMetadataTo(_)
across AppDomains in both directions.