Several changes to reduce allocations #11273
Conversation
|
Related to #11160 |
| /// </example> | ||
| /// <typeparam name="T">The item type that is enumerated.</typeparam> | ||
| /// <param name="collection">The collections that will be enumerated.</param> | ||
| /// <returns>The enumerator for the collection.</returns> |
There was a problem hiding this comment.
If foreach can be optimised this way - shouldn't it be something that should be part of compiler/runtime? Are we possible missing on any already existing opportunity there? @stephentoub?
Or is this bringing benefit only on NetFx? (in which case we migh possibly want to conditionaly compile it only for NetFx?)
There was a problem hiding this comment.
This appears to be trying to manually special-case a few specific collection types by maintaining a discriminated union of all of their strongly-typed struct enumerators. I would not be surprised if this actually makes things more expensive in various situations, especially on .NET Core where dynamic PGO (on by default as of .NET 8) will already do such special-casing and handle devirtualizing and inlining the enumerator dispatches. In .NET 10 the enumerator allocation itself will also frequently be avoided thanks to stack allocation / escape analysis work currently being done by @AndyAyersMS.
There was a problem hiding this comment.
The intent with this helper is to selectively apply it in situations where the allocations are significant, so I'd be hesitant to unconditionally apply it for reasons that @stephentoub mentions. Anecdotally, I've applied this change in a couple of places and reverted the change because I saw an increase in CPU cost that wasn't offset by the reduction in allocations.
There was a problem hiding this comment.
Gotcha.
Sounds helpful on NetFX. We should probably measure impact on dotnet build (core) though
There was a problem hiding this comment.
handle devirtualizing and inlining the enumerator dispatches
Just to reinterate @Erarndt here, MSBuild uses workstation GC, and is often blocked from burning CPU because its stuck behind a single threaded GC performing a Gen0/Gen1 cleaning up these enumerators. While long term we want Server GC for this process which will clean up these allocations with ease, it often better within MSBuild burn additional CPU walking these collections across many threads to avoid the allocation, then it is to be block all threads behind whatever thread triggered the workstation GC. This will be applicable for both .NET Core and .NET Framework, until .NET Core does automatic escape analysis to avoid the allocation.
|
Is the plan to split this PR into several smaller ones still in place please? |
…rndt/reduceAllocations
| foreach (T property in (ICollection<T>)_properties) | ||
| ICollection<T> propertiesCollection = (ICollection<T>)_properties; | ||
| List<TResult> result = new(propertiesCollection.Count); | ||
| if (_properties is RetrievableValuedEntryHashSet<T> hashSet) |
There was a problem hiding this comment.
Please add a comment that RetrievableValuedEntryHashSet is prefered here since it has custome Enumerator implementation returning struct. Otherwise this change might easily get reverted by possible future refactorings
| @@ -522,7 +520,7 @@ private void RegisterResolversManifests(ElementLocation location) | |||
| // The collections are never modified after this point. | |||
| // So I've made them ReadOnly | |||
| _specificResolversManifestsRegistry = specificResolversManifestsRegistry.AsReadOnly(); | |||
There was a problem hiding this comment.
Why this AsReadOnly() is left and next one is removed?
Fixes #
Context
There is an appreciable amount of time all of the MSBuild process nodes spend doing GC during a build. Taking steps to reduce allocations. This PR contains a mix of approaches that reduce allocations in several areas.
Changes Made
Testing
Notes