Avoid delegate allocation in TraceSourceLoggerProvider.GetOrAddTraceSource #71134
Conversation
…ource Every call to GetOrAddTraceSource was allocating a new delegate instance.
ghost
added
the
|
Tagging subscribers to this area: @dotnet/area-extensions-logging Issue DetailsEvery call to GetOrAddTraceSource was allocating a new delegate instance.
|
danmoseley
left a comment
danmoseley
left a comment
There was a problem hiding this comment.
Nit, maybe long enough to have some curlies.
| _sources.TryGetValue(name, out DiagnosticsTraceSource? source) ? source : | ||
| _sources.GetOrAdd(name, InitializeTraceSource(name)); |
There was a problem hiding this comment.
It feels like this is such a perf trap ... 99.9% of devs would write the code as it was originally.
There was a problem hiding this comment.
If InitializeTraceSource was static, it'd be ok (as of C# 11 and its caching of method group delegates). But as an instance method, the compiler is forced to allocate a new delegate. That's the nature of implicit delegate conversions.
There was a problem hiding this comment.
The other option here is:
_sources.GetOrAdd(name, static (name, self) => self.InitializeTraceSource(name), this)There was a problem hiding this comment.
It feels like this is such a perf trap
It's been this way for a very, very long time. Since most code won't have an observable perf impact of a delegate allocation in a call like this, it hasn't been too big a deal for most of that time (IMO of course).
There was a problem hiding this comment.
The other option here is
This library has a netstandard2.0 target.
4 participants
Every call to GetOrAddTraceSource was allocating a new delegate instance.