Enumerate over dictionary keys and values without allocations

[sharwell]

This pull request offers an alternative to #147. It eliminates the allocations when evaluating the Keys and Values properties of ImmutableDictionary<TKey, TValue>, but does so without increasing the allocated size of an ImmutableDictionary<TKey, TValue> instance.

The documentation associated with the members created by this pull request do not currently meet my own quality standards, but I sent the pull request before going through that part in detail in order to get early feedback. If the community believes this approach is the desired implementation for this functionality, I will go back through the documentation in detail.

[nguerrera]

Unfortunately, this is a breaking change that would prevent applications built against the stable 1.0 immutable collections from running against the new version. We preserve binary compatibility between stable package versions and therefore I don't think we can make this transformation.

[sharwell]

Thanks for taking a look. I wasn't sure what breaking changes policy was in place since no stable releases of System.Collections.Immutable have appeared on NuGet yet. I see now that the changes are also evaluated against the stable release of Microsoft.Bcl.Immutable.

Tomorrow I'll update this pull request to revert the change to this property and instead expose the KeyCollection and ValueCollection constructors, which allows users to use instances of these types without incurring allocations. Use of the Keys and Values properties will require a boxing allocation but should be slightly more efficient overall than the current iterator implementation.

[pdelvo]

You could just change the return type to IEnumerable<TKey/TValue> and it should be binary compatible

[nguerrera]

@pdelvo In other words revert this part of the change as @sharwell suggested he'd do next? :)

I don't think the extra code for the key and value enumeration is worth it if won't save on boxing for existing code. Consumers needing maximum efficiency can already get it with just one extra line of code:

Before:

foreach (var key in dictionary.Keys) {
   ...
}

After:

foreach (var pair in dictionary) {
    var key = pair.Key;
    ...
}

As such, I suggest we leave things as they are.

[AArnott]

I agree with @nguerrera. This would be significantly more code without avoiding the allocation. Efficiency/perf improvements need a real world driver to justify added code/complexity that more than pays for the cost of maintaining/shipping the added code. I don't see that here.

I appreciate your intent @sharwell, and I look forward to more of your contributions in the future.

[sharwell]

I think @nguerrera and @AArnott pointed out the real problem here and with #147 and didn't realize it. See #249. 😄 I created an issue rather than a pull request because I'm not sure what wording to use with respect to the caveat I listed.

[sharwell]

@nguerrera @AArnott I'm working on a library to [eventually] automate detection of this kind of API problem by comparing a current build to a previous release obtained from NuGet during the testing phase. I'd love it if you could add issues to the repository describing the types of changes you would consider "breaking changes".

https://github.com/rackerlabs/dotnet-compatibility

[nguerrera]

@sharwell Very cool. We actually have a similar tool that we've written internally. I will ask if we can open source it or at least share the full set of rules we implement. We do have documentation around what we consider breaking here:

https://github.com/dotnet/corefx/wiki/breaking-changes
https://github.com/dotnet/corefx/wiki/breaking-change-definitions
https://github.com/dotnet/corefx/wiki/breaking-change-rules

[sharwell]

@nguerrera That would be awesome. One of the advantages of the one I'm working on is it doesn't require the ability to resolve referenced assemblies (it takes advantage of the System.Reflection.Metadata package for this). Does your tool do something similar in this regard?

[nguerrera]

@sharwell Sweet. I love to see more System.Reflection.Metadata adoption. :)

We currently use CCI: http://ccimetadata.codeplex.com/

Note that there will be cases where you'll need to resolve references: e.g. to validate that a change of base type is OK because the old base type is still an ancestor.

[nguerrera]

@sharwell I'm also working on a better signature reading API that will help you clean up some code and handle some of the cases marked TODO more easily. Expect it to show up in January.

[sharwell]

@nguerrera I'm not seeing the issues for that 😑

On a serious note, I'd be interested in reviewing the API and possibly helping with the implementation and documentation. I've written a metadata reader from scratch and used them in many applications.

Edit: A placeholder #262 has been added now.

[nguerrera]

@sharwell Sounds great. This API will be targeted at a 1.1 release of System.Reflection.Metadata and we'll have plenty of time to iterate on the design, docs, and implementation. Help will be most appreciated. I'm swamped with other things from now through January, but I'll get the initial implementation and a mini-spec for review up in the new year.