akkadotnet / akka.net

Canonical actor model implementation for .NET with local + distributed actors in C# and F#.
http://getakka.net
Other
4.69k stars 1.04k forks source link

Unify immutable collections #1676

Closed Horusiath closed 8 years ago

Horusiath commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

I'd definitely recommend it.

Aaronontheweb commented 8 years ago

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 commented 8 years ago

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

praneetloke commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

@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.

Danthar commented 8 years ago

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

ZoolWay commented 7 years ago

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