Batch metadata updates for better performance#8098
Conversation
rainersigwald
left a comment
There was a problem hiding this comment.
This looks awesome, thanks!
Forgind
left a comment
There was a problem hiding this comment.
Your solution looks pretty big, so I could see where this would have benefits, and I'm assuming the test case you used in Benchmark.NET was similarly big. Can you try with something small? I'm wondering if this change might hurt in that case, since it adds Linq and KeyValuePair allocations.
My BenchamrkDotNet tests didn't cover extra allocations from Linq and KeyValuePair allocations, just pure comparison of My gut feeling is that for small solutions the extra allocations are not noticeable and for large solutions the benefit of improved performance of ImmutableDictionary is much greater than the cost of extra allocations. |
|
Looks like more variance after, but that might just be a product of small sample size. Thanks for checking that! |
|
"license/cla" status is still pending, what do I need to do to make it go green? |
We are bothering people internally, but it's going frustratingly slowly. Sorry! |
This reverts commit 3bcada9.
| _directMetadata ??= new CopyOnWritePropertyDictionary<ProjectMetadataInstance>(); | ||
|
|
||
| var metadata = items | ||
| .Where(item => !FileUtilities.ItemSpecModifiers.IsDerivableItemSpecModifier(item.Value)) |
There was a problem hiding this comment.
It should have been item.Key instead of item.Value - this mistake introduced the problem reported in #8153. I'm working on covering this scenario with unit tests.
Improves dotnet/sdk#27738 It is another attempt to introduce changes proposed in #8098, which was reverted due to a bug.
Improves dotnet/sdk#27738
Context
I was looking for a reason why "design-time-builds" are slow (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 theImmutableDicitionaryin the newly createdProjectItemInstance. 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.18
Time Elapsed 00:01:23.36
Time Elapsed 00:01:23.91
Time Elapsed 00:01:23.18
Time Elapsed 00:01:23.25
after:
Time Elapsed 00:01:20.87
Time Elapsed 00:01:20.21
Time Elapsed 00:01:20.71
Time Elapsed 00:01:20.49
Time Elapsed 00:01:20.60
This change improves performance of the tested scenario by 3-4%!
Notes