Introduce the IResettable type.

[geeknoid]

Introduce the IResettable interface.

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

New API to make object pools easier.

Description

Fixes #44901

[davidfowl]

cc @stephentoub Can we move this to the BCL?

[stephentoub]

Can we move this to the BCL?

What types in runtime would we expect to implement this?

Who else will consume it other than this pool?

This seems to lack some of the generality we'd need supported in order to use this in the places we've discussed wanting reset ability. For example, a wrapper type like StreamWriter or DeflateStream, where if you pool it you want to be able to swap in a new underlying Stream as part of reseting it. How would that be supported with this interface? It's also not clear to me what reseting means so as to be sufficiently unambiguous. For example, let's say I wanted to pool StringBuilders. Would reseting just Clear it? Or would it also shrink it? Would StringBuilder itself have to encode a policy about what size is ammenable to pooling? That's not something we'd feel comfortable doing in SB itself; that's a policy that should be left up to consumers.

[geeknoid]

The semantics are pretty clear in my mind. Reset the object so that it has the same semantic behavior/state as when it was initialized. StringBuilder just gets cleared, List just gets cleared, Dictionary just gets cleared. If you don't like that behavior, just use a custom policy. But in the 99% case, you'd avoid customers writing boilerplate policies for the sake of pooling lists and dictionaries. BTW, an internal project I'm very familiar with has done just that, we have policies for different collection types and it's just dumb boilerplate code that I'd rather we not have to write

The fact it doesn't cover DeflateStream shouldn't deter from the fact it is useful in all these other places. Don't let the perfect be the enemy of the good.

[geeknoid]

BTW, this comment became untrue when I revamped the implementation of DefaultObjectPool a little while ago.

[halter73]

#45251 for reference.

[stephentoub]

Don't let the perfect be the enemy of the good.

I don't believe I am. If anything, I'm letting the good be the enemy of the not good.

It's great that it's been successful for your project. Putting this into the core libraries means it needs to be successful for millions of developers; it's a much, much, much larger scale. You'll forgive my wanting to get that right and not remake mistakes of the past, e.g. ICloneable.

StringBuilder just gets cleared, List just gets cleared, Dictionary just gets cleared.

If that's all you want to do, each of those is a one-liner before returning: sb.Clear(), list.Clear(), d.Clear(). So instead of having that one line be written and be clear as to exactly what's going to happen when this is pooled, and having it be a consistent approach regardless of whether that's the desired policy or something more complicated, we end up hiding it behind an interface implementation that bakes in the policy and requires an extra interface dispatch on each return. We wouldn't implement IResettable on these core collection types to bake in that specific policy, so using these with the pool would also require allocating a wrapper object that baked in that policy.

It also seems like if the goal is to centralize the resetting logic for the pool, that's doable via a pooling policy passed to the pool's ctor. It can do whatever it would like as part of the bool Return that's invoked in the same situation as this TryReset would be, and it allows you to customize on a per pool basis what the resetting does without having to bake it into the actual type being pooled.

I just don't see the value yet. If we want to treat this as part of this particular object pooling library, fine. But to push this lower in the stack, it would need to be thought through more completely.

[stephentoub]

cc @stephentoub Can we move this to the BCL?

David just clarified his question offline to me, that he was asking whether this whole library could move from dotnet/aspnetcore to dotnet/runtime, with no other changes, to sit next to the rest of the Microsoft.Extensions libraries. I'm fine with that. It's not clear to me why we kept this one back when we did the rest of the repo move.

[davidfowl]

I agree with @stephentoub that this interface isn't something that should be implemented on BCL types. The semantics might feel clear for simple objects (List, StringBuilder), but to push this lower into the core, we'd need to understand the semantics with more complex objects. I'm not sure this is IDisposable ubiquitous... in its current shape.

[halter73]

One thing I didn't really consider previously is that a subset of pooled objects might be resettable because we are checking the runtime type for the interface rather than T like we would have if we approved a new PooledObjectPolicy<T> where T : IResettable as originally proposed. I think this is fine, but it's worth noting.

I'm also curious if this has any measurable impact on the microbenchmarks added by #45251. I suspect not, but it should be easy to check.

I see that this got merged a minute ago, but I leave my comments since I've already written them.

[halter73]

#45251 for reference.

[halter73]

Nit:

Suggested change

	var p = new DefaultObjectPool<Resettable>(new DefaultPooledObjectPolicy<Resettable>());
	var p = ObjectPool.Create<Resettable>();

[halter73]

Suggested change

	/// Defines a method to reset an object to its initial state.
	/// Defines a method to reset an object to its initial state. This is used by <see cref="DefaultPooledObjectPolicy{T}"/> if implemented.

While others are free to call it, I think it's worth pointing out that this is the only thing that calls it by default.

[xiaoyuvax]

The semantics are pretty clear in my mind. Reset the object so that it has the same semantic behavior/state as when it was initialized. StringBuilder just gets cleared, List just gets cleared, Dictionary just gets cleared. If you don't like that behavior, just use a custom policy. But in the 99% case, you'd avoid customers writing boilerplate policies for the sake of pooling lists and dictionaries.

from @stephentoub: It also seems like if the goal is to centralize the resetting logic for the pool...

How do programmers solve the overhead of a centralized Resetting? Resetting every resetable object is not performance friendly obviously, since these resetables (with different subsets of their properties) r used differently in different Use Cases, but problme is they share the same centralized Reuse Case, i.e. by simply resetting their properties of various types and possibly a heirachy/series of complicated references, as is another start of big mess and waste!

I developed a performance-friendlier idea(or much of a design pattern) as described here:
Isolate Reuse Cases instead of Resetting

beyond this resetting problem, there r still several other scenarios waiting to be carefully handled as to fully exert the potential of ObjectPool, such as:

1. cross-reference (more than one object references the same object to be returned/got but still in use, and it is subject to be altered, even after being pooled), which is hard to be detected, and is prone to error.
2. cross-reference for underlying objects (objects referenced by properties of a object which is to be returned/got r still referenced by other obejcts) would cause mess when these underlying objects r more worthy to be pooled/reused than the parent object.
3. Collection members instead of the Collection object itself is more worthy to be pooled and reused. (clearing the collection does not automatically resue hundreds or thousands of elemental objects it contains).
   ...
   would want a comprehensive and neater solution for all these scenarios, maybe a "renew" keyword at language level which reinitiate the object memory with default values at low level would solve this from its root perfectly.

in my opinion so far: Consistently and carefully mange isolated Reuse Cases at design time is better in many aspects than doing centralized Resetting at runtime.

[ghost]

Hi @xiaoyuvax. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.