Skip to content

Reduce allocations in PropertyDictionary #11807

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Erarndt
Copy link
Contributor

@Erarndt Erarndt commented May 7, 2025

Fixes #

Context

Changes Made

Testing

Notes

@Copilot Copilot AI review requested due to automatic review settings May 7, 2025 22:56
Copy link
Contributor

@Copilot 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 PR optimizes the PropertyDictionary by reducing allocations during property filtering. Key changes include preallocating the result list with the known collection count and using a struct enumerator when possible for performance benefits.

Comment on lines 556 to 562
ICollection<T> propertiesCollection = (ICollection<T>)_properties;
List<TResult> result = new(propertiesCollection.Count);

// PERF: Prefer using struct enumerators from the concrete types to avoid allocations.
// RetrievableValuedEntryHashSet implements a struct enumerator.
if (_properties is RetrievableValuedEntryHashSet<T> hashSet)
{
Copy link
Preview

Copilot AI May 7, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider using 'propertiesCollection' for the type check (i.e. 'if (propertiesCollection is RetrievableValuedEntryHashSet hashSet)') to avoid redundant casting of _properties.

Suggested change
ICollection<T> propertiesCollection = (ICollection<T>)_properties;
List<TResult> result = new(propertiesCollection.Count);
// PERF: Prefer using struct enumerators from the concrete types to avoid allocations.
// RetrievableValuedEntryHashSet implements a struct enumerator.
if (_properties is RetrievableValuedEntryHashSet<T> hashSet)
{
if (_properties is RetrievableValuedEntryHashSet<T> hashSet)
{
List<TResult> result = new(hashSet.Count);
// PERF: Prefer using struct enumerators from the concrete types to avoid allocations.
// RetrievableValuedEntryHashSet implements a struct enumerator.

Copilot uses AI. Check for mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

This is weird or incomplete I think.
Is there any merit to the idea though?

Copy link
Member

@SimaTian SimaTian 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, just please take a look at the Copilot suggestion if it makes sense. If not, just dismiss and we can move on.

}
else
{
foreach (T property in propertiesCollection)
Copy link
Member

Choose a reason for hiding this comment

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

Is this branch used in practice? Should we add a Debug.Assert or something that it not become hot again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see one path where this would be used. There's one path that passes a ImmutableValuedElementCollectionConverter to the constructor which would hit the "else" path. I can check on how much that happens.

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.

3 participants