Maybe I'm wrong here, but I guess all of the changes that cast GetChildrenOfType to ICollection do nothing here, because you cast you collection to ICollection and so the consumer of this code iterating through ICollection interface won't call a special GetEnumerator method that returns your struct, but rather will call a method on the interface that still would box the iterator.

Bah, I think you're right. This would only save on the OfTypeIterators. Maybe still substantial enough to be worth it, but it's less compelling now. Might need to actually compare and gather data.

For public consumers of these types, you can fix this with a bit of a hack which we use around the tree in situations where we can't change the public API.

Basically if you make the underlying ICollection a known public type, external consumers (like CPS, AnyCode, etc) can cast to and sniff for said type and avoid the boxing by calling the direct enumerator.

Gotcha. I'm not sure we can make it a known public type very easily though since the existing impl is lazy. It's effectively a linked list "view" on the underlying Xml object model, and while the enumerator may box, it's not knowable whether allocating the full collection would be beneficial or not.

Consider a naive consumer who just enumerates. With these changes, it would allocate the collection (just a wrapper) and box the enumerator. If we returned a well-known collection (let's say List), we'd have to enumerate the full collection and populate the whole collection, which takes more memory due to the underlying storage (array).

I guess the only benefits you're getting is here. But I'm curious if you can avoid the collection allocation here as well by having a property 'ChildrenEnumerator' that will return your struct enumerator?

Renamed ChildrenInternal -> ChildrenEnumerable for clarity

lol, due to the hidden copy that C# performs. :)

I don't know if MSBuild has the ability to ban APIs but I would consider banning usage of Children inside of MSBuild itself to avoid accidentally opt'ing into the boxing again. Can't obsolete because that would affect external consumers.

Hmm, interesting idea. It seems to work except I'm unsure how to handle this for UTs since it's a internal member and not all UTs have visibility. I could InternalsVisibleTo, but not sure if that's desirable here or not (MSBuild team feel free to express an opinion)

Note to self in case it's decided to do this:

P:Microsoft.Build.Construction.ProjectElementContainer.Children;Use ChildrenEnumerable instead to avoid boxing

We can skip the ban enforcement in UTs. I'll push a commit with that shortly.

We can skip the ban enforcement in UTs. I'll push a commit with that shortly.