Add CollectionsMarshal

[benaadams]

Resolves https://github.com/dotnet/corefx/issues/31597

[tannergooding]

To be "safe" and match what other mutating APIs do, AsSpan() needs to increment the version count.

AsReadOnlySpan does not have this requirement.

[jkotas]

Is it really needed to increment the version stamp? This is unsafe API. You have to know what you are doing.

[tannergooding]

Is there a way for users to ensure the version count is incremented without us doing it here?

The assumption is that, even if you are using this API for perf benefits, you still want to do the "correct" thing with regards to other usages (such as fulfilling the IEnumerable<T> contract List<T> is currently exposing).

Its a trivial operation to increment the version count once and makes it more correct. For the case where you aren't mutating you can just get the ReadOnlySpan<T>. If you really want to silently mutate and avoid the version increment, you can then use MemoryMarshal to get a writeable span from the ROSpan.

[benaadams]

Doesn't mimic List's behaviour; it should increment the version count on each change rather than just obtaining the Span window (which then may not change anything). That can't be done; also it would defeat the purpose of the Api.

That said if you are using the Span and enumerating the List at the same time; would be an unfortunate approach as enumerating over the Span will be much faster (as it can remove the bounds checks and skip the enumerator creation) and you already have that.

[tannergooding]

Doesn't mimic List's behaviour; it should increment the version count on each change rather than just obtaining the Span window (which then may not change anything).

Fair enough. The only downside is that it seems not ideal that users can't easily ensure "correct" versioning even if using the Span<T> APIs

[GrabYourPitchforks]

The reason we had suggested adding AsReadOnlySpan was that it wouldn't bump the internal version count on List<T>. If we're not going to bump the version count anyway, can we get away with having only AsSpan?

[benaadams]

The reason we had suggested adding AsReadOnlySpan was that it wouldn't bump the internal version count on List.

The version count wouldn't really make any difference unless you are calling .AsSpan inside the enumeration; if you do it outside it wouldn't have much effect; and if doing it inside it would be super inefficient.

[benaadams]

can we get away with having only AsSpan?

Dropped the .AsReadOnlySpan

[danmoseley]

Is it worth a comment that these fields are internal only for the use of CollectionsMarshal and to avoid further growing the generated code for List? It's not something we normally do.

[tannergooding]

Thanks @benaadams!

Are you going to put up the corresponding CoreFX reference assembly change?

[ahsonkhan]

nit: We should get in the habit of adding docs for parameters and return values (effectively all the xml fields that docs require). Something to consider going forward for new API additions (of course this applies to all future PRs and not just this one since we'd like to emphasize adding docs right in time and not accrue tech debt).

For example, see the following (which has param name and return value docs):
https://github.com/dotnet/dotnet-api-docs/blob/137fdd4f501ec90359833ad280064dbde43af7d3/xml/System.Collections/IList.xml#L213-L234
https://docs.microsoft.com/en-us/dotnet/api/system.collections.generic.icollection-1.contains?view=netframework-4.8

See the following for an example:
https://github.com/dotnet/corefx/blob/90f7cc01418c059f496528beeca77c5f5549e6fc/src/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.cs#L412-L432

cc @mairaw, @carlossanlop

Is there a way for us to detect and highlight the missing gaps in docs right in PRs to make it easier/discover-able for contributors?

[benaadams]

Are you going to put up the corresponding CoreFX reference assembly change?

dotnet/corefx#41306

[ahsonkhan]

We need null argument validation here, don't we?

[benaadams]

When writing the tests; I realised I'd prefer this arrangement

public static Span<T> AsSpan<T>(List<T>? list)
    => new Span<T>(list?._items, 0, list?._size ?? 0);

Not sure how people would feel about that?

[tannergooding]

is that better than list is null ? Span<T>.Empty : new Span<T>(list._items, 0, list._size)?

[tannergooding]

Seems like the latter would have less null checks/etc

[ahsonkhan]

It's easier (at least for me) to reason about the code if written how @tannergooding suggested. I also would use default instead of Span<T>.Empty

public static Span<T> AsSpan<T>(List<T>? list)
    => list is null ? default : new Span<T>(list._items, 0, list._size);

[tannergooding]

I believe we prefer Span<T>.Empty over default based on the existing usages

[ahsonkhan]

It's easier to see call sites of Span<T>.Empty than usage of default, but either works.

We have default being used in similar domain here:

coreclr/src/System.Private.CoreLib/shared/System/ArraySegment.cs

Line 170 in 163b30c

public static implicit operator ArraySegment<T>(T[] array) => array != null ? new ArraySegment<T>(array) : default;

There are only a few hits of Span<T>.Empty:
https://source.dot.net/#System.Private.CoreLib/shared/System/Span.cs,a02fdd2de3235cda,references

Although, a lot more for ReadOnlySpan<T>.Empty:
https://source.dot.net/#System.Private.CoreLib/shared/System/ReadOnlySpan.cs,47744f8b41c9226f,references