Batch metadata updates for better performance (attempt 2)#8240
Merged
rainersigwald merged 5 commits intodotnet:mainfrom Mar 28, 2023
Merged
Batch metadata updates for better performance (attempt 2)#8240rainersigwald merged 5 commits intodotnet:mainfrom
rainersigwald merged 5 commits intodotnet:mainfrom
Conversation
Member
rainersigwald
left a comment
There was a problem hiding this comment.
I have a vague recollection that when we first hit the bug, I tried a private build with a change much like this and it didn't work. However, I don't see how that could be possible so I suspect I set up the repro badly (like didn't copy over my private bits right or something).
Can you please add a regression-test case that uses a special name for the value as in #8153? Otherwise looks great!
Contributor
Author
|
Hi @rainersigwald , I've come up with such code: [Fact]
public void IsWellKnownAttributeValuePreserved()
{
ObjectModelHelpers.DeleteTempProjectDirectory();
ObjectModelHelpers.CreateFileInTempProjectDirectory("Myapp.proj", @"
<Project ToolsVersion=`msbuilddefaulttoolsversion` xmlns=`msbuildnamespace`>
<Target Name =`Repro`>
<CreateItem Include=`*.txt` AdditionalMetadata=`MyProperty=Identity`>
<Output TaskParameter=`Include` ItemName=`TestItem`/>
</CreateItem>
<Error Text=`@(TestItem)` Condition=""'%(MyProperty)' != 'Identity' ""/>
</Target>
</Project>
");
ObjectModelHelpers.CreateFileInTempProjectDirectory("Foo.txt", "foo");
MockLogger logger = new MockLogger(_testOutput);
ObjectModelHelpers.BuildTempProjectFileExpectSuccess("Myapp.proj", logger);
}I've confirmed that it would fail on #8098. |
…ange # Conflicts: # src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs
Forgind
approved these changes
Mar 28, 2023
This was referenced Apr 7, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improves dotnet/sdk#27738
It is another attempt to introduce changes proposed in #8098, which was reverted due to a bug.
Context
When profiling "design-time-builds" (see dotnet/sdk#27738) one method that stands out in the performance profiler is a method called GatherTaskOutputs. It copies the output of the executed task into a new ProjectItemInstance. Copying the output metadata involves creating and populating the ImmutableDicitionary in the newly created ProjectItemInstance. The copying process turns out to be a noticeably slow operation.
Changes Made
Instead of copying metadata properties one by one, all properties will be copied with a single SetItems operation.
According to BenchmarkDotNet using a single operation to populate the ImmutableDictionary is twice as fast.
Testing
Benchmarking using scenario from dotnet/sdk#27738 (comment).
before:
Time Elapsed 00:01:23.19
Time Elapsed 00:01:23.88
Time Elapsed 00:01:23.08
Time Elapsed 00:01:23.21
Time Elapsed 00:01:23.15
after:
Time Elapsed 00:01:20.66
Time Elapsed 00:01:20.98
Time Elapsed 00:01:20.91
Time Elapsed 00:01:20.97
Time Elapsed 00:01:20.89
Notes
Previous PR got reverted due to a bug in checking whether the copied metadata is one of the well-known attributes (we were checking
value, but we should check thekeyinstead). I've tried to add a test that validates that we filter out metadata items that have a well-known name, to prevent any such mistake in the future. Unfortunately, it is a non-easy scenario to reproduce in a test. MSBuild's code prevents the creation of custom metadata with a reserved name (e.g. during project parsing or in theTaskItemclass). The only way to work around it seems to be to implement a customITaskItemclass, but that feels to be too much for a single test.