dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.39k stars 4.75k forks source link

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

Closed benaadams closed 1 month ago

benaadams commented 2 months ago

Background and motivation

ConcurrentDictionary<TKey, TValue> allows setting the initial size; however when you call .Clear() it resets it to size 31

This is a problem if your expected size is 1 million; and it now goes through very many increasingly expensive resizes getting back to its previous size

API Proposal

namespace System.Collections.Generic;

public class ConcurrentDictionary<TKey, TValue>
{
    public void Clear(bool noResize);
}

If noResize is true it should use the current length of _tables._buckets.Length rather than HashHelpers.GetPrime(DefaultCapacity) when recreating the arrays

https://github.com/dotnet/runtime/blob/9a31a5b47aa7484bec71ba28aa59712c7898b902/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs#L723-L726

API Usage

// Fancy the value
var c = new ConcurrentDictionary<long, long>(concurrencyLevel: Environment.ProcessorCount * 16, capacity: 4_000_000);

c.Clear(noResize: true);

Alternative Designs

No response

Risks

No response

dotnet-policy-service[bot] commented 2 months ago

Tagging subscribers to this area: @dotnet/area-system-collections See info in area-owners.md if you want to be subscribed.

eiriktsarpalis commented 2 months ago

Could we entertain the possibility of making this the default behaviour for Clear? Dictionary.Clear doesn't resize either and we use TrimExcess if there's an explicit need to downsize.

benaadams commented 2 months ago

Could we entertain the possibility of making this the default behaviour for Clear? Dictionary.Clear doesn't resize either and we use TrimExcess if there's an explicit need to downsize.

I'd be happy with that

eiriktsarpalis commented 2 months ago

We would probably need to add corresponding ConcurrentDictionary.TrimExcess(...) methods to unblock people relying on the current behavior. Paging @stephentoub @jkotas in case there are any objections.

karakasa commented 2 months ago

Will the breaking change actually break anything other than GC timing?

stephentoub commented 2 months ago

it now goes through very many increasingly expensive resizes getting back to its previous size

I believe it'd be ~14 to get up to 1,000,000.

Could we entertain the possibility of making this the default behaviour for Clear? Dictionary.Clear doesn't resize either and we use TrimExcess if there's an explicit need to downsize.

ConcurrentDictionary is different. It has to allocate a new array as part of Clear, which is not the case for Dictionary, and why it prefers to allocate one of the default size rather than the current size, since it has no information to suggest that the capacity after clearing will get to be anywhere close to where it was. For the same reason, I'm not a fan of "noResize"; that would imply it's somehow able to keep the current capacity, but it's not.

I'd be ok considering storing the initial user provided capacity if one was provided and having Clear prefer to use that. But I don't think we should just have Clear always allocate a new array of length equal to the current array's length.

eiriktsarpalis commented 2 months ago

I'd be ok considering storing the initial user provided capacity if one was provided and having Clear prefer to use that.

@benaadams would this address your use case?

benaadams commented 2 months ago

I'd be ok considering storing the initial user provided capacity if one was provided and having Clear prefer to use that.

@benaadams would this address your use case?

Yes

Auxy6858 commented 1 month ago

Could we entertain the possibility of making this the default behaviour for Clear? Dictionary.Clear doesn't resize either and we use TrimExcess if there's an explicit need to downsize.

I'd be happy with that

Could this not cause issues with legacy software if implemented?

stephentoub commented 1 month ago

Could we entertain the possibility of making this the default behaviour for Clear? Dictionary.Clear doesn't resize either and we use TrimExcess if there's an explicit need to downsize.

I'd be happy with that

Could this not cause issues with legacy software if implemented?

What are your concerns?

Auxy6858 commented 1 month ago

Could we entertain the possibility of making this the default behaviour for Clear? Dictionary.Clear doesn't resize either and we use TrimExcess if there's an explicit need to downsize.

I'd be happy with that

Could this not cause issues with legacy software if implemented?

What are your concerns?

Would the new version of the method take a different amount of arguments or would it still work with no arguments?

stephentoub commented 1 month ago

Could we entertain the possibility of making this the default behaviour for Clear? Dictionary.Clear doesn't resize either and we use TrimExcess if there's an explicit need to downsize.

I'd be happy with that

Could this not cause issues with legacy software if implemented?

What are your concerns?

Would the new version of the method take a different amount of arguments or would it still work with no arguments?

The signature of the existing method doesn't change at all. And no new method gets added.

Auxy6858 commented 1 month ago

Ah, no worries then.