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

CollectionsMarshal.GetValueRef(Dictionary) #49388

Merged
merged 1 commit into from
Mar 17, 2021

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Mar 9, 2021

Implement the api

namespace System.Runtime.InteropServices
{
    public static class CollectionsMarshal
    {
        public ref TValue GetValueRefOrNullRef<TKey, TValue>(Dictionary<TKey, TValue> dictionary, TKey key);
    }
}

Contributes to #27062

/cc @layomia

@ghost
Copy link

ghost commented Mar 9, 2021

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

Issue Details

Still needs doc comments and tests

Resolves #27062

Author: benaadams
Assignees: -
Labels:

area-System.Collections

Milestone: -

@stephentoub
Copy link
Member

Still needs doc comments and tests

Are there places outside of Dictionary we should be using the new method, too?

@benaadams
Copy link
Member Author

Are there places outside of Dictionary we should be using the new method, too?

For GetValueRef the big wins are for struct TValues; either using in place or mutating which don't look to be a common pattern in CoreLib.

However GetValueRefOrAddDefault does look like it would have more applicability removing a double lookup from TryGet+Add; assuming it was implemented more efficiently. So is probably worth looping back and doing that as well.

@jkotas
Copy link
Member

jkotas commented Mar 9, 2021

GetValueRefOrAddDefault does look like it would have more applicability removing a double lookup from TryGet+Add

It also needs to be something reasonably hot to make it worth the trouble.

@benaadams
Copy link
Member Author

It also needs to be something reasonably hot to make it worth the trouble.

While CoreLib does create dictionaries, it doesn't really do much modifying of them and alas my search-fu isn't good enough to check all the other libs; though I definitely have uses further downstream

Looks like a ref enumerator

ref struct RefEnumerator
{
    (TKey key, ref TValue) Current { get; }
    bool MoveNext();
}

would be helpful too, but small steps 😉

@benaadams benaadams force-pushed the CollectionsMarshal-Dictionary branch from 310ad9b to 66feb3c Compare March 10, 2021 03:15
@benaadams benaadams closed this Mar 10, 2021
@benaadams benaadams reopened this Mar 10, 2021
@benaadams benaadams force-pushed the CollectionsMarshal-Dictionary branch 3 times, most recently from b45aaa5 to 8d37543 Compare March 10, 2021 13:24
@benaadams benaadams marked this pull request as ready for review March 10, 2021 13:42
@stephentoub
Copy link
Member

Thanks, @benaadams.

@benaadams
Copy link
Member Author

runtime (Libraries Test Run checked coreclr windows x64 Debug) Failure is #48236 though that's closed, and looks like the vm pool died for the rest

@benaadams benaadams closed this Mar 12, 2021
@benaadams benaadams reopened this Mar 12, 2021
@benaadams benaadams closed this Mar 17, 2021
@benaadams benaadams reopened this Mar 17, 2021
@benaadams benaadams force-pushed the CollectionsMarshal-Dictionary branch from 76c7749 to 4b9cc40 Compare March 17, 2021 02:26
@benaadams
Copy link
Member Author

Rebased to reset CI

@benaadams
Copy link
Member Author

🥳

@stephentoub stephentoub merged commit 46127ea into dotnet:main Mar 17, 2021
@benaadams benaadams deleted the CollectionsMarshal-Dictionary branch March 17, 2021 10:50
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants