Copy directly to the internal buffer in List.InsertRange#6892
Copy directly to the internal buffer in List.InsertRange#6892vancem merged 1 commit intodotnet:masterfrom
Conversation
|
Never thought about such approach of getting access to the internal array :) |
I do not think so. There are other subtle behavior changes in addition to the one you have mentioned. |
Out of curiosity, what are they? If this is not going to be accepted as-is, perhaps I can detect whether a new buffer was allocated, then use the old buffer as an intermediary the |
E,g, CopyTo method can be indirectly accessing the list that the items are copied into. I do not think it is worth it to change anything here. These are slow rarely used convenience methods. |
|
@jkotas Ok, then. |
|
@jkotas TBH, I disagree with your opinion that this is a "rarely used convenience method." collection.Concat(enumerable).ToList();There will be a duplicate copy of I think it would be worth optimizing this at least if we can fit the collection into the old buffer, and then use that as an intermediary to copy. |
|
Also a sidenote: the List constructor directly copies to |
|
The constructor case is a bit different because of the same caller owns both the construction and the enumerable. I have looked at the history: The extra copy was not in the |
|
Reopening as per the decision in #7161. 🎉 |
|
LGTM |
|
@jamesqo Could you please port the change to CoreRT List as well (it lives in corefx)? |
List.InsertRangecurrently allocates an intermediary array to copy the items into, then copies it into the List's buffer. This changes it to copy directly into the List's buffer.I realize that this was probably intentionally done in case a bad
ICollectionimplementation held on to the array inCopyTo. However, List is already mutable as-is, so the only thing this would let such an implementation do is modify the array's contents without changing_version.@jkotas Do you think this is a change worth making? I understand it could cause problems with such a faulty implementation, but for other collections (esp. large ones) it seems worth it.
also cc'ing: @omariom @mikedn