{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/corefx/issues/2643","labels_url":"https://api.github.com/repos/dotnet/corefx/issues/2643/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/corefx/issues/2643/comments","events_url":"https://api.github.com/repos/dotnet/corefx/issues/2643/events","html_url":"https://github.com/dotnet/corefx/pull/2643","id":99348099,"number":2643,"title":"Use CopyTo where possible in InsertRange","user":{"login":"jamesqo","id":9159214,"avatar_url":"https://avatars.githubusercontent.com/u/9159214?v=3","gravatar_id":"","url":"https://api.github.com/users/jamesqo","html_url":"https://github.com/jamesqo","followers_url":"https://api.github.com/users/jamesqo/followers","following_url":"https://api.github.com/users/jamesqo/following{/other_user}","gists_url":"https://api.github.com/users/jamesqo/gists{/gist_id}","starred_url":"https://api.github.com/users/jamesqo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jamesqo/subscriptions","organizations_url":"https://api.github.com/users/jamesqo/orgs","repos_url":"https://api.github.com/users/jamesqo/repos","events_url":"https://api.github.com/users/jamesqo/events{/privacy}","received_events_url":"https://api.github.com/users/jamesqo/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/dotnet/corefx/labels/cla-already-signed","name":"cla-already-signed","color":"009800"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":36,"created_at":"2015-08-06T04:01:53Z","updated_at":"2015-09-06T14:13:31Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dotnet/corefx/pulls/2643","html_url":"https://github.com/dotnet/corefx/pull/2643","diff_url":"https://github.com/dotnet/corefx/pull/2643.diff","patch_url":"https://github.com/dotnet/corefx/pull/2643.patch"},"body":"This has been separated from PR #2397: Optimizations for ImmutableArray.\r\n\r\nThe logic has been changed to use `CopyTo` for enumerables that impl `ICollection` and `ICollection<T>`, which includes arrays. This also affects enumerables that don't inherit from them, since while getting the count of an `IEnumerable` we capture it into a `List<T>` (which impls `ICollection<T>`) to avoid enumerating over it more than once."},"comment":{"url":"https://api.github.com/repos/dotnet/corefx/issues/comments/138089040","html_url":"https://github.com/dotnet/corefx/pull/2643#issuecomment-138089040","issue_url":"https://api.github.com/repos/dotnet/corefx/issues/2643","id":138089040,"user":{"login":"AArnott","id":3548,"avatar_url":"https://avatars.githubusercontent.com/u/3548?v=3","gravatar_id":"","url":"https://api.github.com/users/AArnott","html_url":"https://github.com/AArnott","followers_url":"https://api.github.com/users/AArnott/followers","following_url":"https://api.github.com/users/AArnott/following{/other_user}","gists_url":"https://api.github.com/users/AArnott/gists{/gist_id}","starred_url":"https://api.github.com/users/AArnott/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AArnott/subscriptions","organizations_url":"https://api.github.com/users/AArnott/orgs","repos_url":"https://api.github.com/users/AArnott/repos","events_url":"https://api.github.com/users/AArnott/events{/privacy}","received_events_url":"https://api.github.com/users/AArnott/received_events","type":"User","site_admin":false},"created_at":"2015-09-06T14:13:31Z","updated_at":"2015-09-06T14:13:31Z","body":"The latest change doesn't eliminate the risk of exposing the inner array. I'm not saying that's the wrong design, I'm just calling it out. If the perf benefit is substantial enough, but we want to mitigate the risk, we might consider creating an intermediate array that we give out in order to use the `CopyTo` method on the source collection and then use `Array.Copy` to copy it again from the temporary collection to the collection we'll actually use for the new `ImmutableArray<T>`. That of course will eat away at least some of the pert benefit, and will cause a bit more GC pressure given we're allocating more than we were. That said, if we're eliminating allocating the `IEnumerator<T>` (which is of smaller, fixed size) then the additional object we're allocating for the temporary array is offset at least somewhat.\r\n\r\nI don't think the added complexity from this change is too severe. So if the perf wins are there, and useful to someone, I'm in favor."}}