Cap FormattedLogValues cache #561
Conversation
|
|
||
| public FormattedLogValues(string format, params object[] values) | ||
| { | ||
| if (values?.Length != 0 && format != null) | ||
| { | ||
| _formatter = _formatters.GetOrAdd(format, f => new LogValuesFormatter(f)); | ||
| if (_formatters.Count >= MaxCachedFormatters) |
There was a problem hiding this comment.
ConcurrentDictionary.Count acquires all locks which makes this method slow we either need to do our own counter or switch to locking around List<T>
There was a problem hiding this comment.
You could also reorder the checks here to reduce the impact
There was a problem hiding this comment.
I mean currently the Count check would happen even after the cap is reached but if you were to get a successful hit you could avoid this check.
There was a problem hiding this comment.
What is the impact right now vs not having the check?
There was a problem hiding this comment.
So if we are over the limit we just create it? That seems bad, what's the cache eviction policy on this? Seems like frequently occurring formats should never be dropped.
There was a problem hiding this comment.
@niemyjski You could always use LoggerMessage.Define way for perf...Example, MVC usage below:
https://github.com/aspnet/Logging/blob/dev/src/Microsoft.Extensions.Logging.Abstractions/LoggerMessage.cs
https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreLoggerExtensions.cs
BrennanConroy
force-pushed
the
brecon/leak
branch
from
5e9d398 to
2c81564
Compare
|
🆙 📅 |
|
No tests? |
|
Ping |
| { | ||
| _formatter = _formatters.GetOrAdd(format, f => | ||
| { | ||
| Interlocked.Increment(ref _count); |
There was a problem hiding this comment.
There was a problem hiding this comment.
I don't think 100% correctness is required here, we are preferring performance over it.
BrennanConroy
force-pushed
the
brecon/leak
branch
from
ffc7d15 to
cc503aa
Compare
#516
@glennc @davidfowl
I tried a ConcurrentLru cache in the initial commit, but it looked like perf halved in that scenario.
Current thought is to just cap the cache to prevent memory leaks, and if perf is a concern then the
LoggerMessage.DefineScopepattern can be used.Will file an issue in docs to document that pattern: dotnet/AspNetCore.Docs#2810