Avoid temporary list allocations and specify list capacities in TaskItem #7055

drewnoakes · 2021-11-22T06:01:57Z

Context

Noticed while browsing code.

Changes Made

Avoid creating temporary List<> objects in several cases
Specify List<> capacities to avoid temporaries during construction and wasted space in result
(minor) Use a named argument to resolve ambiguity
(minor) Replace as type checks with direct type conversion

Testing

Unit tests.

Notes

May be easier to review each commit separately.

src/Build/Instance/ProjectItemInstance.cs

src/Utilities/TaskItem.cs

Forgind

It isn't actually relevant to your PR, but I noticed a couple instances of github.com/microsoft/msbuild later in this file. Clean-up of that optional.

src/Build/Instance/ProjectItemInstance.cs

Forgind · 2021-11-22T15:21:11Z

src/Build/Instance/ProjectItemInstance.cs

@@ -929,7 +929,12 @@ public ICollection MetadataNames
            {
                get
                {
-                    List<string> names = new List<string>((List<string>)CustomMetadataNames);
+                    List<string> names = new List<string>(capacity: MetadataCount);


Note: this is only really an improvement if MetadataCount is very fast—which it currently might not be.

Also, is there some convenient method like:
return MetadataCollection.Count.Union(FileUtilities.ItemSpecModifiers.All);
?

All is an array, so I don't think so, but thought I'd check.

MetadataCount has been updated to be much faster. Please take another look and see if you feel this needs further work.

I said Count but meant Keys.

return MetadataCollection.Keys.Concat(FileUtilities.ItemSpecModifiers.All);
?

That's possible, though we'd need to convert the IEnumerable<> to an ICollection . Calling ToList or ToArray is going to result in potentially multiple redundant allocations, as the resulting collection size is not known from the enumerator produced by Concat.

By allocating a list up-front with the needed capacity, that overhead is avoided and no waste occurs in the backing arrays.

It would be possible to make this allocate one fewer object by using string[] directly, rather than List<string>.

src/Build/Instance/ProjectItemInstance.cs

drewnoakes · 2021-11-24T11:19:13Z

I noticed a couple instances of github.com/microsoft/msbuild later in this file. Clean-up of that optional.

Fixed in e9eb4f6. There are 267 other such URLs though.

ladipro

I noticed a couple instances of github.com/microsoft/msbuild later in this file. Clean-up of that optional.

Fixed in e9eb4f6. There are 267 other such URLs though.

Oops, I'll file a PR to fix the rest. Thank you!

src/Build/Instance/ProjectItemInstance.cs

Forgind · 2021-11-24T18:10:49Z

src/Build/Instance/ProjectItemInstance.cs

@@ -929,7 +929,12 @@ public ICollection MetadataNames
            {
                get
                {
-                    List<string> names = new List<string>((List<string>)CustomMetadataNames);
+                    List<string> names = new List<string>(capacity: MetadataCount);


I said Count but meant Keys.

return MetadataCollection.Keys.Concat(FileUtilities.ItemSpecModifiers.All);
?

@Forgind

### Context As @Forgind and @drewnoakes pointed out, we still have links to [github.com/microsoft/msbuild](https://github.com/microsoft/msbuild) in the tree. ### Changes Made Replaced [github.com/microsoft/msbuild](https://github.com/microsoft/msbuild) with [github.com/dotnet/msbuild](https://github.com/dotnet/msbuild). Except for the two occurrences fixed in #7055. ### Testing Build, CI

src/Build/Instance/ProjectItemInstance.cs

These duplicated statements were probably intended to validate that the equality implementation is commutative.

The previous code confused me. I thought the comment was the variable name, which would suggest the instance would be immutable. In fact, the opposite is true. Using a named argument avoids that confusion.

The use of `as` suggests some kind of runtime type checking is involved, and that it may fail. In fact, the type is statically known to implement the interface, so direct assignment is sufficient. There should be no runtime change here as the compiler appears to optimise the check away. I just find this form of the code clearer.

The repo was moved from the 'microsoft' org to the 'dotnet' org.

Required rewriting some of ProjectItemInstance.TaskItem.Equals.

There was only one usage. As the property is not cheap to call, inline it for further optimisation opportunities.

Internally it optimises for ICollection<>, reserving enough capacity in one hit, then using CopyTo to copy data.

drewnoakes · 2021-12-02T08:46:36Z

I've force-pushed an update that's rebased on main, drops those commits which did not consider overriding metadata, added test coverage for metadata on the task item, and have optimised what seems safe and worth doing, based on API usage.

The next step will be to reduce churn of temporary values in immutable collections when copying sets of metadata around by adding new batch operations. I'll hold off until this and #7085 to land in whatever form they may.

src/Build/Instance/ProjectItemInstance.cs

src/Build.UnitTests/Instance/TaskItem_Tests.cs

src/Build/Instance/ProjectItemInstance.cs

ladipro reviewed Nov 22, 2021

View reviewed changes

src/Build/Instance/ProjectItemInstance.cs Outdated Show resolved Hide resolved

src/Utilities/TaskItem.cs Outdated Show resolved Hide resolved

ladipro added the performance label Nov 22, 2021

Forgind reviewed Nov 22, 2021

View reviewed changes

ladipro approved these changes Nov 24, 2021

View reviewed changes

ladipro mentioned this pull request Nov 24, 2021

Fix github URLs microsoft/msbuild -> dotnet/msbuild #7083

Merged

Forgind reviewed Nov 24, 2021

View reviewed changes

Forgind requested changes Nov 29, 2021

View reviewed changes

src/Build/Instance/ProjectItemInstance.cs Outdated Show resolved Hide resolved

drewnoakes added 13 commits December 1, 2021 15:08

Fix bidirectional equality tests

4801e5e

These duplicated statements were probably intended to validate that the equality implementation is commutative.

Validate build success

3633c1d

Add a unit test for TaskItem metadata presentation

095a62f

Use named argument

9f4538b

The previous code confused me. I thought the comment was the variable name, which would suggest the instance would be immutable. In fact, the opposite is true. Using a named argument avoids that confusion.

Update github.com URL

2677de3

The repo was moved from the 'microsoft' org to the 'dotnet' org.

Strengthen TaskItem metadata test

ba55978

Reserve correct List<> capacity

d75c1d6

Remove CustomMetadataCount property

cd74460

Required rewriting some of ProjectItemInstance.TaskItem.Equals.

Inline CustomMetadataNames

f26fb96

There was only one usage. As the property is not cheap to call, inline it for further optimisation opportunities.

Reserve correct list capacity

21fe445

Use List<>.AddRange

3aea062

Internally it optimises for ICollection<>, reserving enough capacity in one hit, then using CopyTo to copy data.

Remove unused constructor

eab716d

drewnoakes force-pushed the dev/drnoakes/task-item-list-allocations branch from fdc7bf7 to eab716d Compare December 2, 2021 08:43

ladipro approved these changes Dec 3, 2021

View reviewed changes

src/Build/Instance/ProjectItemInstance.cs Outdated Show resolved Hide resolved

rainersigwald approved these changes Dec 3, 2021

View reviewed changes

src/Build.UnitTests/Instance/TaskItem_Tests.cs Show resolved Hide resolved

src/Build.UnitTests/Instance/TaskItem_Tests.cs Outdated Show resolved Hide resolved

Forgind approved these changes Dec 3, 2021

View reviewed changes

src/Build/Instance/ProjectItemInstance.cs Show resolved Hide resolved

src/Build/Instance/ProjectItemInstance.cs Show resolved Hide resolved

src/Build/Instance/ProjectItemInstance.cs Show resolved Hide resolved

rainersigwald added this to the VS 17.1 milestone Dec 6, 2021

drewnoakes added 2 commits December 7, 2021 12:44

Fix typo

ef21b3a

Return early if counts don't match

5c9d8cc

drewnoakes added 2 commits December 7, 2021 12:45

Add explanatory comment for equality algorithm

609a7b3

Merge branch 'main' into dev/drnoakes/task-item-list-allocations

f7cdd1a

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Dec 7, 2021

rainersigwald merged commit a70ee30 into dotnet:main Dec 7, 2021

drewnoakes deleted the dev/drnoakes/task-item-list-allocations branch December 9, 2021 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid temporary list allocations and specify list capacities in TaskItem #7055

Avoid temporary list allocations and specify list capacities in TaskItem #7055

drewnoakes commented Nov 22, 2021

Forgind left a comment

Forgind Nov 22, 2021

drewnoakes Nov 24, 2021

Forgind Nov 24, 2021

drewnoakes Nov 25, 2021

drewnoakes commented Nov 24, 2021

ladipro left a comment

Forgind Nov 24, 2021

drewnoakes commented Dec 2, 2021 •

edited

Avoid temporary list allocations and specify list capacities in TaskItem #7055

Avoid temporary list allocations and specify list capacities in TaskItem #7055

Conversation

drewnoakes commented Nov 22, 2021

Context

Changes Made

Testing

Notes

Forgind left a comment

Choose a reason for hiding this comment

Forgind Nov 22, 2021

Choose a reason for hiding this comment

drewnoakes Nov 24, 2021

Choose a reason for hiding this comment

Forgind Nov 24, 2021

Choose a reason for hiding this comment

drewnoakes Nov 25, 2021

Choose a reason for hiding this comment

drewnoakes commented Nov 24, 2021

ladipro left a comment

Choose a reason for hiding this comment

Forgind Nov 24, 2021

Choose a reason for hiding this comment

drewnoakes commented Dec 2, 2021 • edited

drewnoakes commented Dec 2, 2021 •

edited