Modify BinderFactory to use a shared pool instead of one per BinderFactory instance. #72725
Conversation
…ctory instance. This showed up as abou 0.7% of allocations on a profile I took on my machine where I typed about 20 characters in a large file.
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| @@ -55,16 +55,15 @@ public override bool Equals(object obj) | |||
| // In a typing scenario, GetBinder is regularly called with a non-zero position. | |||
| // This results in a lot of allocations of BinderFactoryVisitors. Pooling them | |||
| // reduces this churn to almost nothing. | |||
| private readonly ObjectPool<BinderFactoryVisitor> _binderFactoryVisitorPool; | |||
| private static readonly ObjectPool<BinderFactoryVisitor> s_binderFactoryVisitorPool | |||
| = new ObjectPool<BinderFactoryVisitor>(() => new BinderFactoryVisitor(), 64); | |||
| Binder result = visitor.Visit(node); | ||
| _binderFactoryVisitorPool.Free(visitor); | ||
| s_binderFactoryVisitorPool.Free(visitor); |
There was a problem hiding this comment.
Would it be useful to clear the instance (remove the this reference) before returning it to the pool? #Closed
There was a problem hiding this comment.
Sure, would it be preferable to clear out the BinderFactoryVisitor instance completely (CSharpSyntaxNode/Symbol/BinderFactory references), or are you just worried about the BinderFactory reference being kept about?
There was a problem hiding this comment.
I'd clear all that's passed in Initialize.
|
|
||
| internal BinderFactory(CSharpCompilation compilation, SyntaxTree syntaxTree, bool ignoreAccessibility) | ||
| { | ||
| _compilation = compilation; | ||
| _syntaxTree = syntaxTree; | ||
| _ignoreAccessibility = ignoreAccessibility; | ||
|
|
||
| _binderFactoryVisitorPool = new ObjectPool<BinderFactoryVisitor>(() => new BinderFactoryVisitor(this), 64); |
There was a problem hiding this comment.
One of the challenges with using all these static pools is that it's very hard to test. Our tests run in parallel in many cases and hence trying to test behavior like "pool is same going in and out" is basically impossible. One of the nice parts about the binder is BinderFactory is that it had the pool as an instance variable hence it was easy to test.
Could we consider instead
- Keep these as an instance member
- Take the pool as an optional parameter
- When it's
nullgrab from thestaticpool?
All this ambient authority is hard to manage at times in the compiler
There was a problem hiding this comment.
Sure, I can add a parameter to the ctor that takes in an optional pool, one thing to note is that it appears that would require increasing the visibility of BinderFactoryVisitor to internal. I'll wait to make that change until there is a consensus on the preferred approach here.
There was a problem hiding this comment.
@jaredpar If we clear the instances when they are free'd back to the pool, the risk of side-effects via global state is reduced. Would that mitigate your concerns?
There was a problem hiding this comment.
I guess I'm unsure of the need to complicate it here? What is the goal of such a test?
There was a problem hiding this comment.
Would that mitigate your concerns?
No. The concern is whether we're using the pool properly: basically putting objects back when we're done. That is very hard to test when the pool is an ambient authority.
What is the goal of such a test?
To make sure we're actually using the pool properly. Its' a functional part of the type, testing it is a reasonable action.
There was a problem hiding this comment.
4th commit adds the optional pool parameter to the constructor
|
Is this whole thing just a workaround for the fact that we don't have |
This showed up as abou 0.7% of allocations on a profile I took on my machine where I typed about 20 characters in a large file.