Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify immutable collections #1676

Closed
Horusiath opened this issue Jan 27, 2016 · 13 comments
Closed

Unify immutable collections #1676

Horusiath opened this issue Jan 27, 2016 · 13 comments

Comments

@Horusiath
Copy link
Contributor

At the present moment we have our own implementation of immutable collections - reason behind that was that Akka.NET predates System.Collections.Immutable package. However right now a lot of core plugins uses the latter or even mix of both.

This is an idea to move direct dependency on System.Collections.Immutable into Akka.dll, refactor internals to use this library and obsolete Akka.Util.Internal.Collections. NBench tests should give us some insights into how this change would affect an overall performance.

@akkadotnet/developers I hope we're on the same page here.

/cc #1175

@rogeralsing
Copy link
Contributor

Agree, we should also try to get rid of the custom AVL tree. someone pointed out that the Dictionary in Immutable collections are also AVL trees, so it would be safer to use that IMO.

@mattnischan
Copy link
Contributor

This is correct. ImmutableDictionary in System.Collections.Immutable (formerly Microsoft.Bcl.Immutable) is an immutable AVL tree.

Microsoft did release these in late 2013, though, so I think this was just an oversight rather than Akka.Net predating it. There a some good blog posts here and here.

@praneetloke
Copy link

Hey guys, on that note, System.Collections.Immutable's latest version 1.1.37 seems to add a dependency on .net 5. This is preventing me from being able to add packages like akka.cluster.tools on top of akka.cluster in VS2013. Akka.cluster seems to require system.Collections.immutable >= 1.1.37. It complains that it needs nuget client 3.0 to install akka.cluster.tools, which is not available for 2013 and below. So one would have to upgrade to vs2015 to use those additions.

I can submit another issue for this but this thread seemed related. Please let me know otherwise and I'll open a new issue for a separate discussion about this.

@mattnischan
Copy link
Contributor

Yeah, don't use 1.1.37. I don't know what versioning they're smoking, but it sure isn't SemVer. We found the same issue on our production code where we are using System.Collections.Immutable, so we reverted to 1.1.36 as a dependency. That doesn't pull in the .Net nuget deps.

@Aaronontheweb
Copy link
Member

@praneetloke @mattnischan

Yeah, don't use 1.1.37. I don't know what versioning they're smoking, but it sure isn't SemVer. We found the same issue on our production code where we are using System.Collections.Immutable, so we reverted to 1.1.36 as a dependency. That doesn't pull in the .Net nuget deps.

Ugh, that sucks - so should we be doing a revert on the Akka.Cluster dependencies then? We'd need to do the same for Cluster.Tools and Sharding too.

@mattnischan
Copy link
Contributor

I'd definitely recommend it.

@Aaronontheweb
Copy link
Member

cc @Horusiath Bartosz, you onboard with downgrading our version of System.Collections.Immutable? Based on what @mattnischan and @praneetloke have reported it sounds like that would be a good idea.

@MrTortoise
Copy link

ill pick this up. Pretty much the kind of task made for me with this cold!

@praneetloke
Copy link

@MrTortoise Any way that the version downgrade be treated as a separate issue and be tackled sooner? I realize that standardizing on using the immutable collections package is what this thread is about.

@MrTortoise
Copy link

ill do the nuget downgrade and issue a pull request first - but i have to get everything set up and tests running etc still. Last time i tried things were using packit and it didn't go well! Am intending to do it tonight. In meantime can you use a binding redirect?

@praneetloke
Copy link

@MrTortoise let me try that. I'll report back.

EDIT: Actually, I can't use a binding redirect because VS won't even let me install Akka.Cluster.Tools and .Sharding on top of Akka.Cluster because of the version dependency.

MrTortoise added a commit to MrTortoise/akka.net that referenced this issue Feb 6, 2016
MrTortoise added a commit to MrTortoise/akka.net that referenced this issue Feb 9, 2016
…ollections.immutable relatives.

Have depended upon current implementations enumerable over values collections being in ascending order following a load of tests on the collections i performed elsewhere.
This is not explicitly documented - but does follow the standard 'walk tree' behavior so is perhaps dependable.
@Danthar
Copy link
Member

Danthar commented Mar 2, 2016

Done by @MrTortoise rebased by @Aaronontheweb . Net result. Issue solved \0/

@ZoolWay
Copy link

ZoolWay commented Dec 6, 2016

Should this be reconsidered as System.Collections.Immutable 1.3.0 targets .NET 4.5 like 1.1.36 does?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants