Skip to content

Stored the initial capacity of the ConcurrentDictionary for correctly sizing the backing array after clearing the collection. #108065

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.

Sign up for GitHub

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

Jump to bottom
Merged
eiriktsarpalis merged 2 commits into from
Oct 5, 2024
Merged

Stored the initial capacity of the ConcurrentDictionary for correctly sizing the backing array after clearing the collection. #108065

eiriktsarpalis merged 2 commits into from
Oct 5, 2024

Conversation

@koenigst
Copy link
Contributor

@koenigst koenigst commented Sep 20, 2024

  • Stored the capacity in the ctor.
  • Used the stored capacity in Clear().
  • No tests were added or changed as the capacity logic was only implicitly tested.

Fixes #107016

@koenigst
Stored the initial capacity of the ConcurrentDictionary for correctly…
1ae48ce 
… sizing the backing array after clearing the collection.

* Stored the capacity in the ctor.
* Used the stored capacity in Clear().

Fixes dotnet#107016
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 20, 2024
@build-analysis build-analysis bot mentioned this pull request Sep 20, 2024
Closed
3 tasks
eiriktsarpalis
eiriktsarpalis approved these changes Sep 20, 2024
View reviewed changes
Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

LGTM. @stephentoub any objections?

@eiriktsarpalis
Copy link
Member

eiriktsarpalis commented Sep 20, 2024

  • No tests were added or changed as the capacity logic was only implicitly tested.

There is precedent in us using reflection to check intentional properties of collections types. Not sure if warranted here, but bare minimum you should try to check if there is test coverage for passing a large-ish capacity parameter and then clearing the collection.

@build-analysis build-analysis bot mentioned this pull request Sep 20, 2024
Open
@koenigst
Copy link
Contributor Author

koenigst commented Sep 25, 2024

  • No tests were added or changed as the capacity logic was only implicitly tested.

There is precedent in us using reflection to check intentional properties of collections types. Not sure if warranted here, but bare minimum you should try to check if there is test coverage for passing a large-ish capacity parameter and then clearing the collection.

I added a test that checks the following with two custom capacities:

  • Initial table size.
  • Table growth when adding items.
  • Table size after clearing.

106 items might be a bit excessive for a test but the linked issue mentioned this size as being relevant to them.

@build-analysis build-analysis bot mentioned this pull request Sep 25, 2024
Open
stephentoub
stephentoub approved these changes Oct 5, 2024
View reviewed changes
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks

@eiriktsarpalis eiriktsarpalis merged commit 3bcaadb into dotnet:main Oct 5, 2024
sirntar pushed a commit to sirntar/runtime that referenced this pull request Oct 8, 2024
@koenigst @sirntar
Stored the initial capacity of the ConcurrentDictionary for correctly…
11e94f0 
… sizing the backing array after clearing the collection. (dotnet#108065)

* Stored the initial capacity of the ConcurrentDictionary for correctly sizing the backing array after clearing the collection.

* Stored the capacity in the ctor.
* Used the stored capacity in Clear().

Fixes dotnet#107016

* Added a test to check the capacity logic of the ConcurrentDictionary.
@github-actions github-actions bot locked and limited conversation to collaborators Nov 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoub stephentoub stephentoub approved these changes

@eiriktsarpalis eiriktsarpalis eiriktsarpalis approved these changes

Assignees

No one assigned

Labels

area-System.Collections community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[API Proposal]: ConcurrentDictionary.Clear(bool noResize)

3 participants

@koenigst @eiriktsarpalis @stephentoub