Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Add CollectionsMarshal #26867

Merged
merged 3 commits into from
Sep 25, 2019
Merged

Add CollectionsMarshal #26867

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7faa40
Add CollectionsMarshal
benaadams Sep 24, 2019
143c599
Feedback
benaadams Sep 24, 2019
fa9345a
Feedback
benaadams Sep 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/System.Private.CoreLib/shared/System.Private.CoreLib.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,7 @@
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ClassInterfaceAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ClassInterfaceType.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CoClassAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\CollectionsMarshal.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ComDefaultInterfaceAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ComEventInterfaceAttribute.cs" />
<Compile Include="$(MSBuildThisFileDirectory)System\Runtime\InteropServices\ComImportAttribute.cs" />
Expand Down
2 changes: 2 additions & 0 deletions src/System.Private.CoreLib/shared/System/Collections/Generic/List.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ public T this[int index]
}
}

internal Span<T> AsSpan() => new Span<T>(_items, 0, _size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

AsReadOnlySpan does not have this requirement.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

benaadams marked this conversation as resolved.
Show resolved Hide resolved

private static bool IsCompatibleObject(object? value)
{
// Non-null values are fine. Only accept nulls if T is a class or Nullable<U>.
Expand Down
25 changes: 25 additions & 0 deletions src/System.Private.CoreLib/shared/System/Runtime/InteropServices/CollectionsMarshal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;

namespace System.Runtime.InteropServices
{
/// <summary>
/// An unsafe class that provides a set of methods to access the underlying data representations of collections.
/// </summary>
public static class CollectionsMarshal
{
/// <summary>
/// Get a <see cref="Span{T}"/> view over a <see cref="List{T}"/>'s data.
/// Items should not be added or removed from the <see cref="List{T}"/> while the <see cref="Span{T}"/> is in use.
/// </summary>
Copy link
Member

@ahsonkhan ahsonkhan Sep 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

public static Span<T> AsSpan<T>(List<T> list) => list.AsSpan();
/// <summary>
/// Get a <see cref="ReadOnlySpan{T}"/> view over a <see cref="List{T}"/>'s data.
/// Items should not be added or removed from the <see cref="List{T}"/> while the <see cref="ReadOnlySpan{T}"/> is in use.
/// </summary>
public static ReadOnlySpan<T> AsReadOnlySpan<T>(List<T> list) => list.AsSpan();
}
}