{"action":"created","issue":{"url":"https://api.github.com/repos/NetTopologySuite/NetTopologySuite/issues/73","labels_url":"https://api.github.com/repos/NetTopologySuite/NetTopologySuite/issues/73/labels{/name}","comments_url":"https://api.github.com/repos/NetTopologySuite/NetTopologySuite/issues/73/comments","events_url":"https://api.github.com/repos/NetTopologySuite/NetTopologySuite/issues/73/events","html_url":"https://github.com/NetTopologySuite/NetTopologySuite/issues/73","id":128166360,"number":73,"title":"[Question / Discussion] PowerCollections in the Full .NET Framework","user":{"login":"airbreather","id":3792815,"avatar_url":"https://avatars.githubusercontent.com/u/3792815?v=3","gravatar_id":"","url":"https://api.github.com/users/airbreather","html_url":"https://github.com/airbreather","followers_url":"https://api.github.com/users/airbreather/followers","following_url":"https://api.github.com/users/airbreather/following{/other_user}","gists_url":"https://api.github.com/users/airbreather/gists{/gist_id}","starred_url":"https://api.github.com/users/airbreather/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/airbreather/subscriptions","organizations_url":"https://api.github.com/users/airbreather/orgs","repos_url":"https://api.github.com/users/airbreather/repos","events_url":"https://api.github.com/users/airbreather/events{/privacy}","received_events_url":"https://api.github.com/users/airbreather/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":1,"created_at":"2016-01-22T14:25:01Z","updated_at":"2016-01-22T15:04:07Z","closed_at":null,"body":"In every case but `BigList<T>`, the full .NET Framework provides a direct built-in replacement for PowerCollections types used in NTS.  Some of them ([SortedDictionary&lt;TKey, TValue&gt;](https://msdn.microsoft.com/en-us/library/f7fta44c.aspx)) only require .NET 2.0+.  Others ([HashSet&lt;T&gt;](https://msdn.microsoft.com/en-us/library/bb359438.aspx)) require .NET 3.5+.\r\n\r\nIn one case, replacing the implementation of a method from PowerCollections's `OrderedDictionary<TKey, TValue>` with [something](https://gist.github.com/airbreather/8b8544f9c4babcc490ba) based on the [implementation](https://github.com/dotnet/corefx/blob/cd235dbf9d06d64b06643007de13307bba5b92be/src/System.Collections/src/System/Collections/Generic/SortedSet.cs#L209-L244) from the built-in `SortedDictionary<TKey, TValue>` took a significant chunk off the time from an operation.  Specifically, it eliminated between 13%-14% of the total time spent in `Geometry.Union(Geometry)` from `CascadedPolygonUnion`.  I did not go in-depth to see if there were other places where we get penalized for using PowerCollections instead of a built-in.\r\n\r\nI wonder if it would be appropriate to revisit whether or not to include PowerCollections as a dependency in targets where there's a built-in equivalent for everything?  Built-in types tend to be highly optimized.  Specifically:\r\n\r\n1. For [QuadEdgeSubdivision](https://github.com/NetTopologySuite/NetTopologySuite/blob/0f56e478f91a0a40e4b75203ade55908990fc4ce/NetTopologySuite/Triangulate/QuadEdge/QuadEdgeSubdivision.cs), replace `ISet` with `HashSetQ` and change the `#if NET40` to `#if NET35` (`ISet` shows up once, in a private method signature that's guaranteed to always have a `HashSetQ`, so this is not a breaking change).\r\n2. For anything that uses `OrderedDictionary<TKey, TValue>`, use `SortedDictionary<TKey, TValue>` instead, `#if NET20`.\r\n3. For anything that uses `OrderedSet<T>`, use `SortedSet<T>` instead, `#if NET20`.\r\n4. For anything that uses `Set<T>`, use `HashSet<T>` instead, `#if NET35`.\r\n5. For anything that uses `BigList<T>`, re-evaluate whether or not it's actually better than `List<T>`.\r\n    1. For the [usage](https://github.com/NetTopologySuite/NetTopologySuite/blob/0f56e478f91a0a40e4b75203ade55908990fc4ce/NetTopologySuite/Index/Strtree/AbstractSTRtree.cs#L111-L114) in `AbstractSTRtree`, I think it's almost certainly a mistake to pick this over `List<T>`.  The `List<T>` constructor that would be used here is optimized for `ICollection<T>`-typed arguments (which this is), so initializing it would not require the array copies that `BigList<T>` exists to save us from.\r\n    2. This leaves just the [usage](https://github.com/NetTopologySuite/NetTopologySuite/blob/0f56e478f91a0a40e4b75203ade55908990fc4ce/NetTopologySuite/Planargraph/DirectedEdgeStar.cs#L16) in the `PlanarGraph` version of `DirectedEdgeStar`.\r\n        1. Do you have measurements that show that using `BigList<T>` here tends to result in favorable performance characteristics, as compared to `List<T>`?\r\n        2. If so, can you please point me to them and a list of steps that can be used to reproduce?\r\n        3. A polygonization for a [much more complicated line string](https://gist.github.com/airbreather/f3a63abc8464fda34d6b) than what I could find in the test repositories is still blazingly fast.  Running it 5000 times with a dramatically increased sampling rate in the profiler from what I usually like to use indicates **no** visible difference between `List<T>` and `BigList<T>` when I change it.  I expect that I'm not using something complicated enough to see the difference, though.\r\n\r\nI did not look at the tests.\r\n\r\nTo summarize: on targets that have both `NET20` and `NET35` defined, PowerCollections is redundant and sometimes significantly slower.  What are your thoughts about just conditionally replacing the implementations with the built-ins and removing the reference?"},"comment":{"url":"https://api.github.com/repos/NetTopologySuite/NetTopologySuite/issues/comments/173943868","html_url":"https://github.com/NetTopologySuite/NetTopologySuite/issues/73#issuecomment-173943868","issue_url":"https://api.github.com/repos/NetTopologySuite/NetTopologySuite/issues/73","id":173943868,"user":{"login":"DGuidi","id":216627,"avatar_url":"https://avatars.githubusercontent.com/u/216627?v=3","gravatar_id":"","url":"https://api.github.com/users/DGuidi","html_url":"https://github.com/DGuidi","followers_url":"https://api.github.com/users/DGuidi/followers","following_url":"https://api.github.com/users/DGuidi/following{/other_user}","gists_url":"https://api.github.com/users/DGuidi/gists{/gist_id}","starred_url":"https://api.github.com/users/DGuidi/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DGuidi/subscriptions","organizations_url":"https://api.github.com/users/DGuidi/orgs","repos_url":"https://api.github.com/users/DGuidi/repos","events_url":"https://api.github.com/users/DGuidi/events{/privacy}","received_events_url":"https://api.github.com/users/DGuidi/received_events","type":"User","site_admin":false},"created_at":"2016-01-22T15:04:06Z","updated_at":"2016-01-22T15:04:06Z","body":"> What are your thoughts about just conditionally replacing the implementations with the built-ins and removing the reference?\r\n\r\nI'm unable to retrieve some refs now, but some tim ago I've tried to replace PowerCollections with .NET BCL stuff and I seen that some tests are failing, due to a different internal behavior..."}}