Stop allocating new delegates in WriterDelegate.Dispose #4771
Conversation
Every call to Dispose is allocating one or two delegates. They're not unique to this instance; we can let the compiler cache them for us.
| _addLineInfoDelegate = (_addLineInfoDelegate != null) | ||
| ? new XamlLineInfoAddDelegate(ThrowBecauseWriterIsClosed2) | ||
| : null; | ||
| _addDelegate = delegate { throw new XamlException(SR.Get(SRID.WriterIsClosed)); }; |
There was a problem hiding this comment.
Do we even need to set _addDelegate and _addLineInfoDelegate ? ThrowIsDisposed should already be called before the use of _addDelegate or _addLineInfoDelegate.
There was a problem hiding this comment.
I don't know. I'm just trying to maintain behavior just in case. 😄
There was a problem hiding this comment.
I still think this PR is valid because it's faster than the existing code, which was already executed even if the new delegate is not invoked. A follow-up PR could be open to remove this "dead code" with more thorough testing to make sure that the delegates are never executed after dispose. I only said this over a quick glance over the code but they might be invoked.
There was a problem hiding this comment.
I still think this PR is valid
It is 😄
ghost
locked as resolved and limited conversation to collaborators
Description
Every call to Dispose is allocating one or two delegates. They're not unique to this instance; we can let the compiler cache them for us.
Customer Impact
Less allocation, less GC impact.
Regression
No
Testing
Just CI
Risk
Minimal. It's just changing one form of delegate creation to another.