Fix #14961 and tweak perf in ImmutableArray #15296
Conversation
danmoseley
requested review from
AArnott,
mellinoe,
ianhays and
Priya91
and removed request for
mellinoe
| @@ -106,6 +106,10 @@ internal static int GetCount<T>(ref IEnumerable<T> sequence) | |||
| /// </remarks> | |||
| internal static bool TryCopyTo<T>(this IEnumerable<T> sequence, T[] array, int arrayIndex) | |||
| { | |||
| Debug.Assert(sequence != null); | |||
| Debug.Assert(array != null); | |||
| Debug.Assert(arrayIndex >= 0 && arrayIndex < array.Length); | |||
There was a problem hiding this comment.
Are we certain this last assert is correct? I seem to recall some APIs allowing arrayIndex == array.Length when the number of elements to be copied was 0.
Also, it appears this method simply returned false when sequence was null before. Now it will throw NullReferenceException. Are we sure no callers pass a null 1st argument (perhaps as a result of an as cast that fails)?
There was a problem hiding this comment.
Also, it appears this method simply returned false when sequence was null before. Now it will throw NullReferenceException. Are we sure no callers pass a null 1st argument (perhaps as a result of an as cast that fails)?
@AArnott I double-checked and there are no callers doing that.
Are we certain this last assert is correct? I seem to recall some APIs allowing arrayIndex == array.Length when the number of elements to be copied was 0.
I did that intentionally because I thought this method was only being called in scenarios where count > 0. However, I just realized I introduced a usage in this PR where count can be 0. I'll change this to <=.
There was a problem hiding this comment.
Do we have a missing test case then to exercise the count==0 case?
There was a problem hiding this comment.
@AArnott Just added one.
| if (array == null) | ||
| { | ||
| throw new InvalidOperationException(SR.InvalidOperationOnDefaultArray); | ||
| } |
There was a problem hiding this comment.
It looks like you just manually inlined a method here. Why? I see you removed the method from the interface but I don't see why. You haven't avoided any boxing at this point anyway because the boxing already took place.
There was a problem hiding this comment.
@AArnott Interface calls are relatively expensive, and this one was in a commonly-used codepath and trivial to remove. Plus, it let us delete code. I'll see if I can post some stats on how/if this improves perf. (edit: It's actually hard for me to do so because I've run into a bunch of issues trying to build corefx ever since https://github.com/dotnet/corefx/issues/15135. If you'd really like to see perf stats though I can wait until my build issues get resolved.)
There was a problem hiding this comment.
No need to get numbers for me. Avoiding interface dispatch sounds worthy. Thanks.
| if (array == null) | ||
| { | ||
| throw new InvalidOperationException(SR.InvalidOperationOnDefaultArray); | ||
| } | ||
|
|
||
| // immutableArray.Array must not be null at this point, and we know it's an |
There was a problem hiding this comment.
Do you want to update the comment so it discusses the array variable instead of immutableArray.Array, now that the code beneath it deals with array?
|
Merged, @ianhays please review post-mortem if you want to ... |
Follow-up for #14798 and #13158. Specializes
InsertRangefor arrays, usesTryCopyToinImmutableArray.Builder, removes some unnecessary boxing inRemove/Replace, makes exceptions consistent inSetItem/Replace/Remove, and reenables corresponding tests.Fixes #14961, https://github.com/dotnet/corefx/issues/13695