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

Search for opportunities to use Dictionary.Remove while enumerating without defensive copy #18598

Closed
roji opened this issue Oct 25, 2019 · 3 comments
Labels
area-perf closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. community-contribution type-enhancement
Milestone

Comments

@roji
Copy link
Member

roji commented Oct 25, 2019

.NET Core 3.0 added the possibility to safely remove entries from Dictionary while enumerating (dotnet/coreclr#18854). We may have places where we copy defensively because this scenario was not previously supported - we may be able to remove these copies for better perf.

@ajcvickers ajcvickers added this to the Backlog milestone Oct 28, 2019
@ajcvickers ajcvickers added the good first issue This issue should be relatively straightforward to fix. label Oct 28, 2019
@michalczerwinski
Copy link
Contributor

michalczerwinski commented Apr 23, 2021

I this something I can help with?

Sample finding:

RemoveDerivedEntityTypes.cs, line 240:

            foreach (var entityType in entityTypeDictionary.Where(e => e.Key.BaseType != null))
            {
                foreach (var otherEntityType in entityTypesWithDerivedTypes)
                {
                    if (toRemove.Contains(otherEntityType)
                        || otherEntityType.Equals(entityType))
                    {
                        continue;
                    }

                    if (otherEntityType.Key.IsAssignableFrom(entityType.Key))
                    {
                        toRemove.Add(entityType);
                        break;
                    }
                }
            }

            foreach (var entityType in toRemove)
            {
                entityTypeDictionary.Remove(entityType.Key);
            }

No need for toRemove collection, we can just simply remove in the main loop..

I think I managed to find all potential places for this change:

image

Also, in some cases we may want to use something along the lines of EnumerateForRemoval.

@roji
Copy link
Member Author

roji commented Apr 24, 2021

@michalczerwinski I think this is in TableSharingConcurrencyTokenConvention.cs, not RemoveDerivedEntityTypes.cs, no?

Yes, this does seem like a candidate for simplification/optimization. Note that it's only possible to reliably do this where we're handling a concrete Dictionary (as opposed to IDictionary).

@michalczerwinski
Copy link
Contributor

I thoroughly verified all cases and ended up with #24814
Not much, I know. Please let me know if you can see more potential patterns.

@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label May 6, 2021
@ajcvickers ajcvickers modified the milestones: Backlog, 6.0.0 May 6, 2021
@ajcvickers ajcvickers added community-contribution and removed good first issue This issue should be relatively straightforward to fix. labels May 6, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0, 6.0.0-preview5 May 27, 2021
@ajcvickers ajcvickers modified the milestones: 6.0.0-preview5, 6.0.0 Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-perf closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. community-contribution type-enhancement
Projects
None yet
Development

No branches or pull requests

3 participants