jtmueller / Collections.Pooled

Fast, low-allocation ports of List, Dictionary, HashSet, Stack, and Queue using ArrayPool and Span.
MIT License
547 stars 48 forks source link

Add ability to force clear of pooled data #14

Closed jtmueller closed 5 years ago

jtmueller commented 5 years ago

Is your feature request related to a problem? Please describe. Pooled collections have different behavior between .NET Core 2.1+ and .NET Standard 2.0. Because .NET Standard has no mechanism to easily determine if a value type contains a reference type, the .NET Standard version always clears values before returning arrays to the array pool.

The .NET Core 2.1+ version has an optimization that only clears values before returning arrays when those values are reference types or contain reference types. This helps performance, but some users might be concerned about leaking sensitive data to other code running in the same process, and might want even value-type data to be cleared before returning arrays to the pool.

Describe the solution you'd like Pooled collections should allow the user to specify an "always clear" behavior - at least in the .NET Core 2.1+ version.

dzmitry-lahoda commented 5 years ago

We use unamanaged structs stored in array in game loop. Unity is .NET Standard 2.0, and will be .NET Standard 2.1 in future. Not .NET Core 2.1. For this setting, never clear data is an option. So for us opposite is right - never clear. So even if there is no api to check if unmanaged, we want to set that via ctor of collection. So may there should be enumeration of options.

jtmueller commented 5 years ago

Right now the .NET Core version of Collections.Pooled will not clear structs unless they contain reference types, while the .NET Standard version will always clear everything (because it doesn't know if what it's storing might be a struct that contains reference types).

So if your code is running on .NET Core 2.1+, you should be good to go already. If, however, you're running on .NET Framework or an older version of .NET Core then your structs will be needlessly cleared, at a small performance penalty.

I take your point about using an enum with Auto/Always/Never cases, but there's a risk: if someone sets Never for a collection that holds reference types, or value types that contain reference types, those reference types would be kept alive by the ArrayPool for an indefinite amount of time - the GC can't clean them up if the ArrayPool is still holding onto those references.

So given that the Never option is really only useful for code referencing the .NET Standard 2.0 version of Collections.Pooled, and that it carries a risk of hurting performance if used incorrectly, how important do you think this option is?

dzmitry-lahoda commented 5 years ago

We use Collections.Pooled as source code. So we may modify it. Anyway, these collections are already better for us existing in corefx (and we cannot just use unity collections as we need same on server).

jtmueller commented 5 years ago

@dzmitry-lahoda I've got this mostly implemented in the "features/clear-control" branch if you want to have a look to see if this would meet your needs. I'm in the process of re-running all the benchmarks before I finalize it. The new setting is a constructor parameter only, because it hurt performance too much to dynamically check on every call when it was a property. This exploded the number of constructor overloads, but I don't see an easy way around that.

dzmitry-lahoda commented 5 years ago

thanks, this will work for us:)