Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ref accessors for List<T> #54999

Closed
kzrnm opened this issue Jul 1, 2021 · 4 comments
Closed

Add ref accessors for List<T> #54999

kzrnm opened this issue Jul 1, 2021 · 4 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections untriaged New issue has not been triaged by the area owner

Comments

@kzrnm
Copy link
Contributor

kzrnm commented Jul 1, 2021

Background and Motivation

Add ref accessors for List<T> same as Dictionary<TKey, TValue>. #27062.

Motivation

Updating struct causes struct copies. It may be expensive.

struct LargeStruct
{
    // Other members...
    
    public int MyInt { get; set; }

    // Other members...
}

LargeStruct value= list[index]; // struct copy
value.MyInt++;
list[index] = value; // another struct copy

Proposed API

namespace System.Runtime.InteropServices
{
    public static class CollectionsMarshal
    {
        /// <summary>
        ///   Gets a reference to the value at the given <paramref name="index"/>.
        /// </summary>
        public static ref T GetValueRef<T>(List<T> list, int index);
    }
}

Usage Examples

ref LargeStruct value = ref CollectionsMarshal.GetValueRef(list, index); // avoid struct copy
value.MyInt++;

Alternative Designs

It can also be resolved using CollectionsMarshal.AsSpan, but it doesn't correspond to API of Dictionary<TKey, TValue>.
I think this is non-intuitive.

// List<T> use AsSpan
ref var listRef = ref CollectionsMarshal.AsSpan(list)[index];

// Dictionary<TKey, TValue> use ref accessor
ref var dictRef = ref CollectionsMarshal.GetValueRef(dictionary, key);

Risks

@kzrnm kzrnm added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 1, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels Jul 1, 2021
@ghost
Copy link

ghost commented Jul 1, 2021

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

Add ref accessors for List<T> same as Dictionary<TKey, TValue>. #27062.

Motivation

Updating struct causes struct copies. It may be expensive.

struct LargeStruct
{
    // Other members...
    
    public int MyInt { get; set; }

    // Other members...
}

LargeStruct value= list[index]; // struct copy
value.MyInt++;
list[index] = value; // another struct copy

Proposed API

namespace System.Runtime.InteropServices
{
    public static class CollectionsMarshal
    {
        /// <summary>
        ///   Gets a reference to the value at the given <paramref name="index"/>.
        /// </summary>
        public static ref T GetValueRef<T>(List<T> list, int index);
    }
}

Usage Examples

ref LargeStruct value = ref CollectionsMarshal.GetValueRef(list, index); // avoid struct copy
value.MyInt++;

Alternative Designs

It can also be resolved using CollectionsMarshal.AsSpan, but it doesn't correspond to API of Dictionary<TKey, TValue>.
I think this is non-intuitive.

// List<T> use AsSpan
ref var listRef = ref CollectionsMarshal.AsSpan(list)[index];

// Dictionary<TKey, TValue> use ref accessor
ref var dictRef = ref CollectionsMarshal.GetValueRef(dictionary, key).MyInt++;

Risks

Author: naminodarie
Assignees: -
Labels:

api-suggestion, area-System.Collections, untriaged

Milestone: -

@huoyaoyuan
Copy link
Member

It can also be resolved using CollectionsMarshal.AsSpan, but it doesn't correspond to API of Dictionary<TKey, TValue>.
I think this is non-intuitive.

I think AsSpan is already a perfect solution. Considering boundary check, checking span boundary is somehow more institutive than Unsafe.IsNullRef.

The list and dictionary methods lives in the same class CollectionMarshal, so there should be no difficulty of discovery.

@eiriktsarpalis
Copy link
Member

I think AsSpan is already a perfect solution. Considering boundary check, checking span boundary is somehow more institutive than Unsafe.IsNullRef.

The list and dictionary methods lives in the same class CollectionMarshal, so there should be no difficulty of discovery.

Both great points. Unlike Dictionary I don't think there is much benefit in exposing ref accessors for List. I will close this issue, but we can always revisit should we discover a compelling reason to include it.

@kzrnm
Copy link
Contributor Author

kzrnm commented Jul 1, 2021

Thanks for reviewing!

@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

3 participants