Skip to content

Use Builder in ImmutableDictionary.SetItems extension#12402

Merged
AR-May merged 4 commits intodotnet:mainfrom
ccastanedaucf:dev/chcasta/perf-setitems
Oct 1, 2025
Merged

Use Builder in ImmutableDictionary.SetItems extension#12402
AR-May merged 4 commits intodotnet:mainfrom
ccastanedaucf:dev/chcasta/perf-setitems

Conversation

@ccastanedaucf
Copy link
Contributor

@ccastanedaucf ccastanedaucf commented Aug 22, 2025

Context

Small change to avoid allocating a generated enumerable. Using a Builder here is otherwise identical in perf to SetItems, as both hit the same paths to allocate a new instance + modified subtree nodes. ~34MB diff below.

Before
image

After
image

Copilot AI review requested due to automatic review settings August 22, 2025 21:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This change optimizes memory allocation in the SetItems extension method for ImmutableDictionary<string, string> by using ImmutableDictionary.Builder instead of a generator-based approach. The modification eliminates the intermediate enumerable allocation while maintaining the same validation logic and null-to-empty-string conversion behavior.

Key Changes

  • Replaced ValidateItems generator method with direct Builder operations
  • Eliminated intermediate enumerable allocation, saving ~34MB according to profiling data
  • Maintained identical functionality for key validation and null value handling

@ccastanedaucf ccastanedaucf changed the title Use ImmutableDictionary.Builder in SetItems extension Use Builder in ImmutableDictionary.SetItems extension Aug 22, 2025
@ghost ghost assigned AR-May Sep 2, 2025
Copy link
Member

@AR-May AR-May left a comment

Choose a reason for hiding this comment

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

Looks good to me. I will make exp insertion and look at numbers.

Copy link
Member

@surayya-MS surayya-MS left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@AR-May
Copy link
Member

AR-May commented Oct 1, 2025

Ok, exp insertion came up showing improvements. merging the PR.
There were some weird regressions, but they are for sure unrelated, that for kitten to investigate.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants