Avoid iterator/list allocations when parsing projects #2577
Conversation
For each project and for every container constructor in a project (ItemGroup, ImportGroup, etc) we were allocating a List<XmlElementWithLocation> and a XmlChildEnumerator. In large solutions with large import graphs this was adding up to a non-trivial amount of allocations (1.7% of all allocations during evaluation in one trace). These with a struct-based enumerable/enumerator that manually walks the child elements.
|
This is the allocations in one trace I looked at:
|
|
@dotnet-bot test Windows_NT Build for CoreCLR please |
|
Here are the numbers:
|
| } | ||
| } | ||
|
|
||
| public void Reset() |
There was a problem hiding this comment.
Do you need to have a Reset method if you're not implementing the interfaces? Is Reset part of the pattern that foreach looks for?
|
|
||
| public XmlElementChildIterator GetEnumerator() | ||
| { | ||
| return this; |
There was a problem hiding this comment.
This may be a standard pattern, but is it correct to have the enumerable and the enumerator be the same object? It seems like the contract would be that you can call GetEnumerator() multiple times on the same enumerable and you should get different enumerators / iterators with different state.
There was a problem hiding this comment.
It's a pattern used by LINQ- http://index/?query=Enumerable.Select&rightProject=System.Core&file=System%5CLinq%5CEnumerable.cs&line=90. I was a little worried about increasing the size to handle this case (because it will never be hit), but I noticed they use a "state" field to track this and I might do the same thing.
Compiler doesn't require a enumerator to have Reset.
We were throwing when <Import>, <UsingTask>, etc had children, but we were picking the whitespace/comment (which were allowed) instead of the first illegal children.
|
@dsplaisted I addressed your comments, changes:
|
|
tag @Pilchie |
For each project and for every container construct in a project (ItemGroup, ImportGroup, etc) we were allocating a
List<XmlElementWithLocation>and aXmlChildEnumerator. In large solutions with large import graphs this was adding up to a non-trivial amount of allocations (1.7% of all allocations during evaluation in one trace).Replaces these with a struct-based enumerable/enumerator that manually walks the child elements.