Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Add CollectionsMarshal.GetValueRefOrAddDefault#54611

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@Sergio0694
Copy link
Contributor

@Sergio0694Sergio0694 commentedJun 23, 2021•
edited
Loading

Contributes to#27062

namespaceSystem.Runtime.InteropServices{publicstaticclassCollectionsMarshal{publicstaticrefTValue?GetValueRefOrAddDefault<TKey,TValue>(Dictionary<TKey,TValue>dictionary,TKeykey,outboolexists)whereTKey:notnull;}}

NOTE: I've marked the returnedTValue as an uncostrained nullable value, because ifTValue is a reference type the returned reference will point tonull. Value types are not affected by this, ie. there is noNullable<T> involved here at all.

@ghost
Copy link

Note regarding thenew-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

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

Issue Details

Contributes to#27062

namespaceSystem.Runtime.InteropServices{publicstaticrefTValue?GetValueRefOrAddDefault<TKey,TValue>(Dictionary<TKey,TValue>dictionary,TKeykey,outboolexists)whereTKey:notnull;}}

NOTE: I've marked the returnedTValue as an uncostrained nullable value, because ifTValue is a reference type the returned reference will point tonull. Value types are not affected by this, ie. there is noNullable<T> involved here at all.

Author:Sergio0694
Assignees:-
Labels:

area-System.Collections,area-System.Runtime.InteropServices,new-api-needs-documentation

Milestone:-

@Sergio0694
Copy link
ContributorAuthor

@jkotas Could you clarify what the overhead of this new method would be, and whether it would be a blocker for this PR? This would be a very neat feature to have, and the API itself has been approved by API review too - if it's just the current implementation that's an issue would any of the proposed approaches help? As in, moving the code to a diferent spot, etc.

Thanks! 🙂

@jkotas
Copy link
Member

It makes every Dictionary instantiation more expensive, in particular with AOT.

@Sergio0694
Copy link
ContributorAuthor

I see. If the issue is specifically with the overhead of dictionary instantiations, would moving the code to a separate type (eg. a nested type within dictionary) solve that? That would only ever be loaded when the method is actually used, right? 🤔

@jkotas
Copy link
Member

would moving the code to a separate type (eg. a nested type within dictionary) solve that

Yes, it would solve that.

Sergio0694 reacted with hooray emojiSergio0694 reacted with heart emoji

@Sergio0694Sergio0694 marked this pull request as ready for reviewJune 25, 2021 08:47
@Sergio0694Sergio0694force-pushed thecollectionsmarshal-GetValueRefOrAddDefault branch from8b9e772 to31d7120CompareJune 25, 2021 13:47
@Sergio0694Sergio0694force-pushed thecollectionsmarshal-GetValueRefOrAddDefault branch from31d7120 to99523baCompareJuly 8, 2021 15:04
@Sergio0694
Copy link
ContributorAuthor

Test failures should be unrelated, and the changes in99523ba addresses Jan's concerns on additional overhead being added toDictionary<TKey, TValue> instantiations (moved all the new code to a separate type). Should be good to review now 😄

@Sergio0694Sergio0694force-pushed thecollectionsmarshal-GetValueRefOrAddDefault branch from99523ba to5367083CompareJuly 13, 2021 16:03
Copy link
Member

@GrabYourPitchforksGrabYourPitchforks left a comment

Choose a reason for hiding this comment

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

@Sergio0694 Thanks for this! Can you also add a unit test for the randomized hash logic? I think the appropriate place to add this would be in the filehttps://github.com/dotnet/runtime/blob/main/src/libraries/System.Collections/tests/Generic/Dictionary/HashCollisionScenarios/OutOfBoundsRegression.cs.

There's no need to modifyRunDictionaryTest (line 93 of that file) to run aCollectionsMarshal test for every possible combination; that's probably overkill. Instead, what you can do is immediatelybefore line 93, insert a manual call that looks something like:

RunCollectionTestCommon(()=>newDictionary<string,object>(StringComparison.Ordinal),(dictionary,key)=>CollectionsMarshal.GetRef(dictionary,key,out_)=null,(dictionary,key)=>dictionary.ContainsKey(key),    dictionary=>dictionary.Comparer,expectedInternalComparerTypeBeforeCollisionThreshold:nonRandomizedOrdinalComparerType,expectedPublicComparerBeforeCollisionThreshold:StringComparer.Ordinal,expectedInternalComparerTypeAfterCollisionThreshold:randomizedOrdinalComparerType);

In a nutshell, this utilizes the existing test infrastructure to generate a whole bunch of collisions, but the delegate you provide will force it to use your newCollectionsMarshal method instead ofAdd. This will get regression test coverage for string collision scenarios in this method.

@Sergio0694Sergio0694force-pushed thecollectionsmarshal-GetValueRefOrAddDefault branch from5367083 to1d7e386CompareJuly 14, 2021 11:35
Copy link
Member

@eiriktsarpaliseiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Other than minor feedback, LGTM. Thanks!

Sergio0694 reacted with hooray emojiSergio0694 reacted with rocket emoji
@GrabYourPitchforks
Copy link
Member

A reminder for the future - try to avoid rebase & force-push after reviews have started. Makes it a little more complicated for us since we can't rely on the GitHub web UI and instead we need to generate incremental diffs manually.

@Sergio0694
Copy link
ContributorAuthor

Oh, I'm sorry, I didn't realize that. Will definitely avoid doing that again in the future! 😅

Copy link
Member

@GrabYourPitchforksGrabYourPitchforks left a comment

Choose a reason for hiding this comment

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

Thanks so much! :D

Sergio0694 reacted with hooray emojiSergio0694 reacted with heart emoji
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.

Reviewers

@jkotasjkotasjkotas left review comments

@GrabYourPitchforksGrabYourPitchforksGrabYourPitchforks approved these changes

@eiriktsarpaliseiriktsarpaliseiriktsarpalis approved these changes

@stephentoubstephentoubAwaiting requested review from stephentoub

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Developers can use CollectionsMarshal ref accessors for Dictionary<TKey, TValue>

5 participants

@Sergio0694@jkotas@GrabYourPitchforks@stephentoub@eiriktsarpalis
[8]ページ先頭
©2009-2025 Movatter.jp