Implement slicing APIs for ArraySegment<T> #9926

jamesqo · 2017-03-03T01:36:37Z

Progress towards https://github.com/dotnet/corefx/issues/10528.

I let Slice accept negative indices so the start of the ArraySegment<T> can backtrack to an earlier position in the array. Please let me know if you think this is a good idea or not (checking for it would involve some additional overhead).

/cc @jkotas, @KrzysztofCwalina

jamesqo · 2017-03-03T01:38:31Z

src/mscorlib/src/System/ArraySegment.cs

+        // another generic type in addition to ArraySegment<T> for new type parameters.
+        private static readonly T[] s_emptyArray = new T[0];
+
+        public static ArraySegment<T> Empty { get; } = new ArraySegment<T>(s_emptyArray);


Not sure if it would be better for perf to do new ArraySegment<T>(new T[0]) here and then return Empty._array in ToArray() to avoid another static field?

Single static field would be slightly better, I think.

Single static field would be slightly better, I think.

K, will remove s_emptyArray then (assuming by 'single static field' you mean only the Empty property).

Couldn't s_emptyArray be replaced by Array.Empty<T>?

jamesqo · 2017-03-03T01:38:51Z

@benaadams FYI

jkotas · 2017-03-03T01:59:02Z

src/mscorlib/src/System/ArraySegment.cs

@@ -106,12 +112,11 @@ public int Count
            }
        }

+        public ref T this[int index] => ref _array[_offset + index];


Note that this indexer will have poor performance characteristics on reference types for both reading and writing because of array covariance (it will have array covariance check for both). @KrzysztofCwalina Have you considered this during the API review?

I think this should be the classic get/set; forget about ref indexer here.

@jkotas I suggested it back in https://github.com/dotnet/corefx/issues/10528#issuecomment-265625210 and @KrzysztofCwalina approved of it. I thought it would be more flexible since you can do everything with a ref T (and more) that you can do with a regular indexer. If you say it is slower, however, I would be OK with having the latter.

jamesqo · 2017-03-04T02:08:35Z

@jkotas Thanks for reviewing. I've updated the PR in response to your comments.

jamesqo · 2017-03-04T02:44:07Z

Please hold on merging until I make corefx tests, actually. Thanks.

jamesqo · 2017-03-05T00:07:45Z

src/mscorlib/src/System/ArraySegment.cs

-            if (_array == null)
-                ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NullArray);
-            Contract.EndContractBlock();
+            get { return _array[_offset + index]; }


@jkotas, came up with a question while writing the corefx tests. Do you think argument validation should be added here to prevent people from accessing items that aren't within range of the segment?

Yes, I would think that the index should be validated here.

@jkotas I am surprised, I thought you would have said no. I think it could discourage people from using the new API since ArraySegment<T> is mostly used in perf-sensitive places. However, I suppose if we throw an exception here we could always decide to allow it in the future.

Do you have any example where ArraySegment is used in perf-sensitive place?

ArraySegment<T> is in the same category as List<T> in my mind - pretty convenient and lightweight, but still adds some overhead. I do not think that the perf-sensitive algorithms should be written against ArraySegment<T>. They should be written directly against array (or potentially Span<T> in future).

@jkotas Well, a quick GitHub search through Kestrel shows a lot of usages... https://github.com/aspnet/KestrelHttpServer/search?utf8=%E2%9C%93&q=ArraySegment. Here is one place where this API could make code more readable. I don't think we want people manually writing as.Array[as.Offset + index] to get better perf, because would defeat the whole point of this API, right?

Here is one place where this API could make code more readable.

Agree. However, this example is not performance critical. The extra argument validation is not a problem.

I don't think we want people manually writing as.Array[as.Offset + index] to get better perf

If you want to squeeze every last cycle out, you will probably write even something more convoluted. And that's perfectly fine. As I have said, the perf-sensitive algorithms should not be written against ArraySegment<T>.

I do not see a good reason why the ArraySegment indexer should deviate from framework design guidelines and ship argument validation.

@jkotas OK, not disagreeing. If someone finds the extra overhead to be a problem maybe they can submit a PR here to use Unsafe to circumvent the extra check. Or failing that, we can always decide to allow negative indices later.

KrzysztofCwalina · 2017-03-06T16:54:10Z

src/mscorlib/src/System/ArraySegment.cs

@@ -145,6 +167,34 @@ public bool Equals(ArraySegment<T> obj)
            return obj._array == _array && obj._offset == _offset && obj._count == _count;
        }

+        public ArraySegment<T> Slice(int index)


C# team is considering adding slicing syntax. It's not yet clear what would libraries have to do to support it. It could be Slice(int) method (just like here) or Slice(Range) method.

When this C# syntax ships, it would be great if ArraySegment supported it.

@jaredpar, @MadsTorgersen, any word on the syntax? Shoudl we just assume the Range solution? Should we add Range struct to the platform?

@KrzysztofCwalina Is this blocking this PR? In the case they decide to add a Range struct we can just add another overload in addition to these two.

I don't think it's blocking.

jamesqo · 2017-03-13T21:29:40Z

@jkotas Responded to your feedback. You can merge if you think this is good to go.

jkotas · 2017-03-14T01:19:53Z

src/mscorlib/src/System/ArraySegment.cs

+        {
+            ThrowInvalidOperationIfDefault();
+            destination.ThrowInvalidOperationIfDefault();
+            System.Array.Copy(_array, _offset, destination._array, destination._offset, _count);


This can write beyond limits of the destination

@jkotas Good catch. Fixed

jkotas · 2017-03-14T01:21:10Z

src/mscorlib/src/System/ArraySegment.cs

+        public ArraySegment<T> Slice(int index)
+        {
+            ThrowInvalidOperationIfDefault();
+            // Note: `index` is allowed to be negative so the start of this ArraySegment can be moved backwards.


Was the negative index for slicing discussed anywhere?

@KrzysztofCwalina Do you have an opinion for allowing negative indices for slicing?

@terrajobst Do you have an opinion on this either? (This is blocking the APIs from being added.)

@jkotas @KrzysztofCwalina how well do negative bounds work for our optimized bounds checking pattern?

if ((uint)index >= (uint)_count) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);

I think we want to make sure indexing is blazing fast, so I don't think we want to allow negative indices as it would cause additional branches. In slicing it might be OK performance wise, but would feel inconsistent if I can't do it for regular indexing.

It's also a pattern that doesn't exist in the BCL. I don't think we should introduce it here.

I think we want to make sure indexing is blazing fast

Plain array is blazing fast. ArraySegment is not. It has extra overhead by design.

It's also a pattern that doesn't exist in the BCL. I don't think we should introduce it here.

Agree.

Updated PR to disallow negative indices.

stephentoub · 2017-04-07T12:17:45Z

src/mscorlib/src/System/ArraySegment.cs

@@ -106,12 +110,31 @@ public int Count
            }
        }

-        public Enumerator GetEnumerator()
+        public T this[int index]


Have we already discussed the potential for this returning ref T instead of T? e.g.

public ref T this[int index] { get { if ((uint)index >= (uint)_count) ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index); return ref _array[_offset + index]; } }

?

+1

This is why ref returns were invented. Definitely should be used here because there are no compat issues to consider on this new API.

ref T returning indexer will not work well with array covariance. The rest of the ArraySegment is compatible with array covariance, so it will look odd.

String[] arr = new String[] { ... }; ArraySegment<object> segment = new ArraySegment(arr); Object elem = segment[0]; // Throws exception if the indexer returns `ref T`

Arrays.....

stephentoub

Thanks

jamesqo added 2 commits March 2, 2017 20:32

Add slicing APIs to ArraySegment

2c034d4

Implement Slice

2d26de8

dnfclas added the cla-already-signed label Mar 3, 2017

jamesqo commented Mar 3, 2017

View reviewed changes

karelz added the area-System.Collections label Mar 3, 2017

jkotas reviewed Mar 3, 2017

View reviewed changes

karelz assigned ianhays, jamesqo, KrzysztofCwalina and safern Mar 3, 2017

Respond to PR feedback

8a5c436

jamesqo commented Mar 5, 2017

View reviewed changes

KrzysztofCwalina reviewed Mar 6, 2017

View reviewed changes

jamesqo mentioned this pull request Mar 12, 2017

Add tests for ArraySegment slicing APIs and GetEnumerator dotnet/corefx#17033

Merged

Respond to PR feedback

d9935bc

jkotas reviewed Mar 14, 2017

View reviewed changes

benaadams mentioned this pull request Mar 17, 2017

Question: Why do we have an extension method on ArraySegment that returns ArraySegment? dotnet/corefxlab#1326

Closed

Respond to PR feedback

c6775c8

stephentoub reviewed Apr 7, 2017

View reviewed changes

jamesqo added 3 commits April 8, 2017 23:13

Merge branch 'aseg.slice' of github.com:jamesqo/coreclr into aseg.slice

726c86f

Disallow negative indices in Slice()

1e9c6e5

Remove redundant explicit implementation

e870fc0

stephentoub approved these changes Apr 9, 2017

View reviewed changes

jkotas merged commit 40ecbb7 into dotnet:master Apr 10, 2017

jamesqo deleted the aseg.slice branch April 10, 2017 17:29

jamesqo mentioned this pull request Apr 18, 2017

[Port from CoreCLR] Implement slicing APIs for ArraySegment<T> dotnet/corert#3364

Merged

karelz modified the milestone: 2.0.0 Aug 28, 2017

MindyStivers mentioned this pull request Jan 31, 2020

Implicit operator throws when checking ArraySegment != null or ArraySegment == null dotnet/runtime#8949

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement slicing APIs for ArraySegment<T> #9926

Implement slicing APIs for ArraySegment<T> #9926

jamesqo commented Mar 3, 2017

jamesqo Mar 3, 2017 •

edited

Loading

jkotas Mar 3, 2017

jamesqo Mar 3, 2017

nil4 Mar 17, 2017

jamesqo commented Mar 3, 2017

jkotas Mar 3, 2017

jamesqo Mar 3, 2017

jamesqo commented Mar 4, 2017

jamesqo commented Mar 4, 2017

jamesqo Mar 5, 2017 •

edited

Loading

jkotas Mar 5, 2017

jamesqo Mar 6, 2017

jkotas Mar 6, 2017

jamesqo Mar 12, 2017 •

edited

Loading

jkotas Mar 13, 2017 •

edited

Loading

jamesqo Mar 13, 2017 •

edited

Loading

KrzysztofCwalina Mar 6, 2017

jamesqo Mar 12, 2017

KrzysztofCwalina Mar 13, 2017

jamesqo commented Mar 13, 2017

jkotas Mar 14, 2017

jamesqo Mar 21, 2017

jkotas Mar 14, 2017

jamesqo Mar 21, 2017

jamesqo Mar 30, 2017

terrajobst Apr 7, 2017

jkotas Apr 7, 2017

jamesqo Apr 9, 2017

stephentoub Apr 7, 2017

jaredpar Apr 7, 2017

jkotas Apr 7, 2017 •

edited

Loading

stephentoub Apr 7, 2017

davidfowl Apr 7, 2017

stephentoub left a comment

Implement slicing APIs for ArraySegment<T> #9926

Implement slicing APIs for ArraySegment<T> #9926

Conversation

jamesqo commented Mar 3, 2017

jamesqo Mar 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo commented Mar 4, 2017

jamesqo commented Mar 4, 2017

jamesqo Mar 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

jkotas Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

jamesqo Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

jamesqo Mar 3, 2017 •

edited

Loading

jamesqo Mar 5, 2017 •

edited

Loading

jamesqo Mar 12, 2017 •

edited

Loading

jkotas Mar 13, 2017 •

edited

Loading

jamesqo Mar 13, 2017 •

edited

Loading

jkotas Apr 7, 2017 •

edited

Loading