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

Avoid allocations due to use of Keys/Values instead of GetEnumerator #19126

Merged
merged 3 commits into from
May 8, 2017

Conversation

sharwell
Copy link
Member

@sharwell sharwell commented Apr 29, 2017

ImmutableDictionary<TKey, TValue> provides a non-allocating implementation of GetEnumerator(), but does not offer non-allocating implementations of the Keys or Values properties (dotnet/corefx#249 and linked issues). This pull request adds a Deconstruct extension method for KeyValuePair<TKey, TValue>, leveraging a new C# feature to provide almost identical use-site code for these properties without actually incurring the allocation overhead.

📝 This pull request builds on the change in #19125 to avoid merge conflicts (the first commit in the branch overlaps) (edit: no longer applies).

Ask Mode

Customer scenario

No observable changes.

  • Use GetEnumerator() instead of Keys where applicable
  • Use GetEnumerator() instead of Values where applicable
  • Add a Deconstruct extension method so the above changes can be implemented with minimal changes to the actual code structure

Bugs this fixes:

N/A

Workarounds, if any

None needed.

Risk

Low (no behavior changes).

Performance impact

Performance is improved by eliminating unnecessary allocations.

Is this a regression from a previous update?

No.

Root cause analysis:

Missed during code review?

How was the bug found?

Internal code review.

@sharwell
Copy link
Member Author

sharwell commented May 1, 2017

@dotnet/roslyn-compiler for review (let me know if anyone else should be added)

Please note the comment regarding the first commit overlapping with #19125.

{
isSuppressed = diagnosticOptions[diag.Id] == ReportDiagnostic.Suppress;
Copy link
Member

Choose a reason for hiding this comment

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

We really sohuld have an analyzer fro that pattern

Copy link
Member Author

Choose a reason for hiding this comment

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

We really should.

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@gafter gafter left a comment

Choose a reason for hiding this comment

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

LGTM

@sharwell sharwell changed the base branch from master to dev16 May 3, 2017 16:11
@sharwell sharwell changed the base branch from dev16 to master May 3, 2017 16:11
@sharwell sharwell modified the milestones: 15.later, 15.3 May 8, 2017
@sharwell
Copy link
Member Author

sharwell commented May 8, 2017

@MeiChin-Tsai for ask mode (at the request of @jaredpar)

@MeiChin-Tsai
Copy link

approved.

@sharwell sharwell merged commit 4011656 into dotnet:master May 8, 2017
@sharwell sharwell deleted the enumerator branch May 8, 2017 18:54
@jcouv
Copy link
Member

jcouv commented May 9, 2017

Nice

@sharwell
Copy link
Member Author

sharwell commented May 9, 2017

@jcouv I think this is my favorite use of C# 7 to date. It totally resolved something I previously found rather frustrating. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants